This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new f66843ef0c7 fix(plugin-chart-table): stop narrowing sticky
header/footer width (#43937)
f66843ef0c7 is described below
commit f66843ef0c77ae195b3dcc6a36b89ce38b11a8e1
Author: Joe Li <[email protected]>
AuthorDate: Wed Sep 9 09:47:27 2026 -0700
fix(plugin-chart-table): stop narrowing sticky header/footer width (#43937)
---
.../src/DataTable/hooks/useSticky.tsx | 35 +++-
.../test/DataTable/hooks/useSticky.test.tsx | 205 +++++++++++++++++++++
.../test/DataTable/utils/getScrollBarSize.test.ts | 6 +-
3 files changed, 238 insertions(+), 8 deletions(-)
diff --git
a/superset-frontend/plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx
b/superset-frontend/plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx
index 65231bc652d..eee392e0c0e 100644
---
a/superset-frontend/plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx
+++
b/superset-frontend/plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx
@@ -16,6 +16,8 @@
* specific language governing permissions and limitations
* under the License.
*/
+
+/** @jsxImportSource @emotion/react */
import {
Children,
cloneElement,
@@ -286,9 +288,28 @@ function StickyWrap({
</colgroup>
);
- const headerContainerWidth = hasVerticalScroll
- ? maxWidth - scrollBarSize
- : maxWidth;
+ // Below, `width: maxWidth` is applied unconditionally (never reduced by
+ // subtracting a separately-measured scrollbar width, unlike this file's
+ // previous `maxWidth - scrollBarSize`). That's the load-bearing part of
+ // this fix: the shared colgroup (computed from the sizer below, whose
+ // own clientWidth can only ever be <= maxWidth) can never need more
+ // width than that, so a header/footer wrapper that's never narrowed
+ // below maxWidth can never clip it, regardless of whether any
+ // JS-measured scrollbar size agrees with what the sizer/body actually
+ // reserve in a given browser.
+ //
+ // `scrollbarGutter`/`scrollBarStyles` below are a separate, secondary
+ // measure -- matching an actual clip boundary is not what they're for
+ // (an `overflow: hidden` box's clip boundary sits at its real
+ // border-box edge regardless of `scrollbar-gutter`, which only affects
+ // what `clientWidth` reports). They keep header/footer's reported
+ // `clientWidth` consistent with body's so that, when both a vertical
+ // and a horizontal scrollbar are present, the horizontal `scrollLeft`
+ // synced from body (see `onScroll` below) reveals the same slice of the
+ // row in header/footer as is actually visible in body.
+ const headerFooterGutter: CSSProperties = {
+ scrollbarGutter: hasVerticalScroll ? 'stable' : undefined,
+ };
headerTable = (
<div
@@ -296,9 +317,11 @@ function StickyWrap({
ref={scrollHeaderRef}
style={{
overflow: 'hidden',
- width: headerContainerWidth,
+ width: maxWidth,
boxSizing: 'border-box',
+ ...headerFooterGutter,
}}
+ css={scrollBarStyles}
role="presentation"
>
{cloneElement(
@@ -317,9 +340,11 @@ function StickyWrap({
ref={scrollFooterRef}
style={{
overflow: 'hidden',
- width: headerContainerWidth,
+ width: maxWidth,
boxSizing: 'border-box',
+ ...headerFooterGutter,
}}
+ css={scrollBarStyles}
role="presentation"
>
{cloneElement(
diff --git
a/superset-frontend/plugins/plugin-chart-table/test/DataTable/hooks/useSticky.test.tsx
b/superset-frontend/plugins/plugin-chart-table/test/DataTable/hooks/useSticky.test.tsx
new file mode 100644
index 00000000000..d75eca12fc4
--- /dev/null
+++
b/superset-frontend/plugins/plugin-chart-table/test/DataTable/hooks/useSticky.test.tsx
@@ -0,0 +1,205 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useCallback } from 'react';
+import { useTable, Column } from 'react-table';
+import { render } from '@superset-ui/core/spec';
+import useSticky from '../../../src/DataTable/hooks/useSticky';
+
+// A value distinguishable from any real scrollbar width, so the width
+// assertions below can detect whether header/footer's wrapper width was
+// computed by subtracting this JS-measured probe from `maxWidth` (the old,
+// removed `maxWidth - scrollBarSize` behavior) rather than always being the
+// unconditional `maxWidth` the fix uses. If that subtraction is ever
+// reintroduced, header/footer's `style.width` would read
+// `${MAX_WIDTH - MOCKED_SCROLLBAR_PROBE_SIZE}px`, an unmistakably wrong
+// value given how large this mock is.
+const MOCKED_SCROLLBAR_PROBE_SIZE = 42;
+
+jest.mock('../../../src/DataTable/utils/getScrollBarSize', () => ({
+ __esModule: true,
+ CUSTOM_SCROLLBAR_SIZE: 8,
+ default: () => 0,
+ getCustomScrollBarSize: () => MOCKED_SCROLLBAR_PROBE_SIZE,
+}));
+
+const MAX_WIDTH = 300;
+const MAX_HEIGHT = 120; // small enough that the mocked content forces a
vertical scroll
+
+const TOTAL_HEADER_HEIGHT = 30;
+const TOTAL_FOOTER_HEIGHT = 30;
+// Larger than `MAX_HEIGHT - TOTAL_HEADER_HEIGHT - TOTAL_FOOTER_HEIGHT`, so the
+// sticky layout effect computes `hasVerticalScroll: true`.
+const FULL_TABLE_HEIGHT = 400;
+
+function mockMeasurements() {
+ jest
+ .spyOn(HTMLElement.prototype, 'clientHeight', 'get')
+ .mockImplementation(function mockClientHeight(this: HTMLElement) {
+ if (this.tagName === 'THEAD') return TOTAL_HEADER_HEIGHT;
+ if (this.tagName === 'TFOOT') return TOTAL_FOOTER_HEIGHT;
+ if (this.tagName === 'TABLE') return FULL_TABLE_HEIGHT;
+ return 0;
+ });
+ jest
+ .spyOn(HTMLElement.prototype, 'getBoundingClientRect')
+ .mockImplementation(function mockRect(this: HTMLElement) {
+ const width = this.tagName === 'TH' ? 60 : 0;
+ return {
+ width,
+ height: 0,
+ top: 0,
+ left: 0,
+ right: width,
+ bottom: 0,
+ x: 0,
+ y: 0,
+ toJSON: () => {},
+ } as DOMRect;
+ });
+}
+
+type Row = { category: string; amount: string };
+
+const columns: Column<Row>[] = [
+ { Header: 'Category', accessor: 'category' },
+ { Header: 'SUM(amount)', accessor: 'amount' },
+];
+
+const data: Row[] = Array.from({ length: 8 }, (_, i) => ({
+ category: `Category ${i}`,
+ amount: `${1234567.891234 + i}`,
+}));
+
+function StickyTableHarness() {
+ const getTableSize = useCallback(
+ () => ({ width: MAX_WIDTH, height: MAX_HEIGHT }),
+ [],
+ );
+ const { getTableProps, headerGroups, rows, prepareRow, wrapStickyTable } =
+ useTable<Row>(
+ {
+ columns,
+ data,
+ getTableSize,
+ },
+ useSticky,
+ );
+
+ const renderTable = () => (
+ <table {...getTableProps()}>
+ <thead>
+ {headerGroups.map(hg => (
+ <tr {...hg.getHeaderGroupProps()} key={hg.id}>
+ {hg.headers.map(col => (
+ <th {...col.getHeaderProps()} key={col.id}>
+ {col.render('Header')}
+ </th>
+ ))}
+ </tr>
+ ))}
+ </thead>
+ <tbody>
+ {rows.map(row => {
+ prepareRow(row);
+ return (
+ <tr {...row.getRowProps()} key={row.id}>
+ {row.cells.map(cell => (
+ <td {...cell.getCellProps()} key={cell.column.id}>
+ {cell.render('Cell')}
+ </td>
+ ))}
+ </tr>
+ );
+ })}
+ </tbody>
+ <tfoot>
+ <tr key="footer">
+ <th>Summary</th>
+ <td>
+ <strong>14814904.694808</strong>
+ </td>
+ </tr>
+ </tfoot>
+ </table>
+ );
+
+ return <div data-test="sticky-root">{wrapStickyTable(renderTable)}</div>;
+}
+
+test('sticky header/footer width matches the body, independent of the
scrollbar-size probe', () => {
+ mockMeasurements();
+
+ const { container } = render(<StickyTableHarness />);
+
+ const root = container.querySelector('[data-test="sticky-root"] > div');
+ expect(root).not.toBeNull();
+ const [headerDiv, bodyDiv, footerDiv] = Array.from(
+ root!.children,
+ ) as HTMLDivElement[];
+
+ expect(bodyDiv.style.width).toBe(`${MAX_WIDTH}px`);
+
+ // This is the load-bearing assertion for the reported bug. Before the fix
+ // these read `${MAX_WIDTH - MOCKED_SCROLLBAR_PROBE_SIZE}px` (258px) --
+ // genuinely narrower than the body, from a real CSS `width` subtraction
+ // (`maxWidth - scrollBarSize`), not just a smaller reported `clientWidth`.
+ // A wrapper that's actually narrower than the shared, fixed-layout
+ // colgroup it has to display gets genuinely clipped by its own
+ // `overflow: hidden` (verified with real hit-testing in a real browser --
+ // this is not true of the `scrollbarGutter` assertions below). The fix
+ // makes header/footer always exactly `maxWidth`, which the colgroup
+ // (bounded by the sizer's `clientWidth`, itself bounded by `maxWidth`)
+ // can never exceed.
+ expect(headerDiv.style.width).toBe(`${MAX_WIDTH}px`);
+ expect(footerDiv.style.width).toBe(`${MAX_WIDTH}px`);
+
+ // Secondary, not itself load-bearing for preventing clipping: real
+ // hit-testing shows `scrollbar-gutter` on an `overflow: hidden` box
+ // changes what `clientWidth` reports without moving where it actually
+ // clips, so this doesn't guard against the reported bug by itself. It's
+ // asserted because header/footer's reported `clientWidth` still needs to
+ // match body's `clientWidth` for their programmatically
+ // synced `scrollLeft` (see `onScroll` in `useSticky.tsx`) to reveal the
+ // same slice of the row body actually shows, when a horizontal scrollbar
+ // is present alongside a vertical one.
+ expect(headerDiv.style.scrollbarGutter).toBe(bodyDiv.style.scrollbarGutter);
+ expect(footerDiv.style.scrollbarGutter).toBe(bodyDiv.style.scrollbarGutter);
+ expect(bodyDiv.style.scrollbarGutter).toBe('stable');
+
+ // Pin the `css={scrollBarStyles}` addition to header/footer directly (part
+ // of the same secondary consistency measure as the `scrollbarGutter`
+ // assertions above, not the clipping fix). This component carries
+ // `/** @jsxImportSource @emotion/react */`, which makes
+ // Babel route its `css` prop through Emotion's jsx runtime instead of
+ // passing `css` straight through as an inert DOM attribute (the default in
+ // this repo's Jest/Babel setup, which -- unlike the webpack/SWC build --
+ // doesn't set `importSource: '@emotion/react'` globally). With the pragma
+ // in place, an applied `css` prop is observable as a real, non-empty
+ // className, so this assertion actually fails without the fix instead of
+ // passing regardless of whether `scrollBarStyles` is wired up.
+ //
+ // Before `css={scrollBarStyles}` was added to header/footer, they had no
+ // emotion-generated class at all (`className === ''`) while the body kept
+ // its own -- so this fails pre-fix and passes post-fix.
+ expect(headerDiv.className).not.toBe('');
+ expect(headerDiv.className).toBe(bodyDiv.className);
+ expect(footerDiv.className).toBe(bodyDiv.className);
+
+ jest.restoreAllMocks();
+});
diff --git
a/superset-frontend/plugins/plugin-chart-table/test/DataTable/utils/getScrollBarSize.test.ts
b/superset-frontend/plugins/plugin-chart-table/test/DataTable/utils/getScrollBarSize.test.ts
index 7ad2d83b800..8284d6bb292 100644
---
a/superset-frontend/plugins/plugin-chart-table/test/DataTable/utils/getScrollBarSize.test.ts
+++
b/superset-frontend/plugins/plugin-chart-table/test/DataTable/utils/getScrollBarSize.test.ts
@@ -45,8 +45,8 @@ test('getCustomScrollBarSize measures the probe using the
shared custom scrollba
});
test('CUSTOM_SCROLLBAR_SIZE matches the custom scrollbar width rendered in the
sticky table', () => {
- // useSticky.tsx's scrollBarStyles must stay in sync with this constant so
- // the sticky header's shrink amount always matches the body's real
- // scrollbar width.
+ // useSticky.tsx's scrollBarStyles sets `::-webkit-scrollbar { width: ... }`
+ // from this constant, so it must stay in sync with it or the real
+ // scrollbar body/sizer render won't match what this constant claims.
expect(CUSTOM_SCROLLBAR_SIZE).toBe(8);
});
