sadpandajoe commented on code in PR #43917: URL: https://github.com/apache/superset/pull/43917#discussion_r4153447573
########## superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/Styles.test.tsx: ########## @@ -0,0 +1,282 @@ +/** + * 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 '@testing-library/jest-dom'; +import { render } from '@testing-library/react'; +import { supersetTheme } from '@apache-superset/core/theme'; +import PivotTableChart from '../../src/PivotTableChart'; +import transformProps from '../../src/plugin/transformProps'; +import testData from '../testData'; +import { ProviderWrapper } from '../testHelpers'; + +test('sticky-positions the row-label column and its corner header cell(s) so they stay visible while scrolling', () => { + const transformedProps = { + ...transformProps(testData.withoutColTotals), + margin: 32, + legacy_order_by: null, + order_desc: false, + }; + const { container } = render( + ProviderWrapper({ + children: <PivotTableChart {...transformedProps} />, + }), + ); + + const rowLabelCell = container.querySelector('tbody th.pvtRowLabel'); + expect(rowLabelCell).toBeInTheDocument(); + const rowLabelStyle = getComputedStyle(rowLabelCell as Element); + expect(rowLabelStyle.position).toBe('sticky'); + expect(rowLabelStyle.left).toBe('0px'); + + // The corner cells above the frozen row-label column (the column + // attribute name cell in each column-header row and the row-attribute + // name cell in the row-header row) must stick on both axes and paint + // over the column labels that scroll underneath them. Asserted as two + // separate non-empty groups -- rather than one merged list -- so that if + // either selector stops matching (e.g. `pvtCornerLabel` is dropped from + // the column-attribute name cell), that group's absence fails the test + // instead of silently leaving only the other group's cells checked. + const columnCornerCells = container.querySelectorAll( + 'thead th.pvtCornerLabel', + ); + const rowHeaderCornerCells = container.querySelectorAll( + 'thead tr.pvtRowHeaderRow th.pvtAxisLabel', + ); + expect(columnCornerCells.length).toBeGreaterThan(0); + expect(rowHeaderCornerCells.length).toBeGreaterThan(0); + const cornerCells = [...columnCornerCells, ...rowHeaderCornerCells]; + // The frozen block in each column-header row has to cover the same + // columns as the frozen row label below it (its own column plus the + // padding column), otherwise the uncovered strip shows column labels + // scrolling through the corner. + const rowLabelSpan = (rowLabelCell as HTMLTableCellElement).colSpan; + expect(rowLabelSpan).toBe(2); + columnCornerCells.forEach(cell => { + expect((cell as HTMLTableCellElement).colSpan).toBe(rowLabelSpan); Review Comment: With one row and one column dimension, removing the merged row-attribute header’s `colSpan={2}` would leave part of the frozen body-label block uncovered, but these assertions would still pass because only `columnCornerCells` have their span checked. Could this rendered-chart test also assert that the row-header corner cell spans the same two columns as the body label? ########## superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/Styles.test.tsx: ########## @@ -0,0 +1,282 @@ +/** + * 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 '@testing-library/jest-dom'; +import { render } from '@testing-library/react'; +import { supersetTheme } from '@apache-superset/core/theme'; +import PivotTableChart from '../../src/PivotTableChart'; +import transformProps from '../../src/plugin/transformProps'; +import testData from '../testData'; +import { ProviderWrapper } from '../testHelpers'; + +test('sticky-positions the row-label column and its corner header cell(s) so they stay visible while scrolling', () => { + const transformedProps = { + ...transformProps(testData.withoutColTotals), + margin: 32, + legacy_order_by: null, + order_desc: false, + }; + const { container } = render( + ProviderWrapper({ + children: <PivotTableChart {...transformedProps} />, + }), + ); + + const rowLabelCell = container.querySelector('tbody th.pvtRowLabel'); + expect(rowLabelCell).toBeInTheDocument(); + const rowLabelStyle = getComputedStyle(rowLabelCell as Element); + expect(rowLabelStyle.position).toBe('sticky'); + expect(rowLabelStyle.left).toBe('0px'); + + // The corner cells above the frozen row-label column (the column + // attribute name cell in each column-header row and the row-attribute + // name cell in the row-header row) must stick on both axes and paint + // over the column labels that scroll underneath them. Asserted as two + // separate non-empty groups -- rather than one merged list -- so that if + // either selector stops matching (e.g. `pvtCornerLabel` is dropped from + // the column-attribute name cell), that group's absence fails the test + // instead of silently leaving only the other group's cells checked. + const columnCornerCells = container.querySelectorAll( + 'thead th.pvtCornerLabel', + ); + const rowHeaderCornerCells = container.querySelectorAll( + 'thead tr.pvtRowHeaderRow th.pvtAxisLabel', + ); + expect(columnCornerCells.length).toBeGreaterThan(0); + expect(rowHeaderCornerCells.length).toBeGreaterThan(0); + const cornerCells = [...columnCornerCells, ...rowHeaderCornerCells]; + // The frozen block in each column-header row has to cover the same + // columns as the frozen row label below it (its own column plus the + // padding column), otherwise the uncovered strip shows column labels + // scrolling through the corner. + const rowLabelSpan = (rowLabelCell as HTMLTableCellElement).colSpan; + expect(rowLabelSpan).toBe(2); + columnCornerCells.forEach(cell => { + expect((cell as HTMLTableCellElement).colSpan).toBe(rowLabelSpan); + }); + cornerCells.forEach(cell => { + const style = getComputedStyle(cell); + expect(style.position).toBe('sticky'); + expect(style.top).toBe('0px'); + expect(style.left).toBe('0px'); + // Assert the declared value, not just a positive number: with no + // z-index rule applied, jsdom's getComputedStyle resolves to '' and + // Number('') is 0, so a bare `> 0` check would still pass. + expect(style.zIndex).toBe('1'); + }); + + // The sticky thead is its own stacking context, so the corner cell's + // z-index can't outrank the row labels by itself. The thead as a whole + // has to sit above the frozen row-label column. + const thead = container.querySelector('thead'); + expect(thead).toBeInTheDocument(); + expect(getComputedStyle(thead as Element).zIndex).toBe('2'); + expect(rowLabelStyle.zIndex).toBe('1'); +}); + +test('keeps the sticky totals row above the frozen row-label column', () => { + const transformedProps = { + ...transformProps(testData.withColTotals), + margin: 32, + legacy_order_by: null, + order_desc: false, + }; + const { container } = render( + ProviderWrapper({ + children: <PivotTableChart {...transformedProps} />, + }), + ); + + const rowLabelCell = container.querySelector('tbody th.pvtRowLabel'); + const totalsRow = container.querySelector('tbody tr.pvtRowTotals'); + expect(rowLabelCell).toBeInTheDocument(); + expect(totalsRow).toBeInTheDocument(); + expect(getComputedStyle(totalsRow as Element).zIndex).toBe('2'); + expect(getComputedStyle(rowLabelCell as Element).zIndex).toBe('1'); + + // The totals row's leading label freezes at the left edge alongside the + // body row labels, and sits above the totals values in its own row. + const totalsLabel = totalsRow?.querySelector('th.pvtRowTotalLabel'); + expect(totalsLabel).toBeInTheDocument(); + const totalsLabelStyle = getComputedStyle(totalsLabel as Element); + expect(totalsLabelStyle.position).toBe('sticky'); + expect(totalsLabelStyle.left).toBe('0px'); + // Assert the declared value rather than `> 0`: with no z-index rule + // applied, jsdom resolves getComputedStyle(...).zIndex to '', and + // Number('') is 0, so a bare positivity check would still pass. + expect(totalsLabelStyle.zIndex).toBe('1'); + expect((totalsLabel as HTMLTableCellElement).colSpan).toBe( + (rowLabelCell as HTMLTableCellElement).colSpan, + ); +}); + +test('lets the active (cross-filter) highlight win over the frozen row-label background', () => { + const transformedProps = { + ...transformProps(testData.withoutColTotals), + margin: 32, + legacy_order_by: null, + order_desc: false, + }; + const { container } = render( + ProviderWrapper({ + children: <PivotTableChart {...transformedProps} />, + }), + ); + + // Computed colors come back normalized (rgb()), so run the theme tokens + // through the same normalization before comparing. + const normalizeColor = (color: string) => { + const probe = document.createElement('div'); + probe.style.backgroundColor = color; + return probe.style.backgroundColor; + }; + + const rowLabelCell = container.querySelector('tbody th.pvtRowLabel'); + expect(rowLabelCell).toBeInTheDocument(); + expect(getComputedStyle(rowLabelCell as Element).backgroundColor).toBe( + normalizeColor(supersetTheme.colorBgBase), + ); + + rowLabelCell?.classList.add('active'); + expect(getComputedStyle(rowLabelCell as Element).backgroundColor).toBe( + normalizeColor(supersetTheme.colorPrimaryBg), + ); +}); + +test('does not freeze any header cell when the pivot has column dimensions but no row dimensions', () => { + const transformedProps = { + ...transformProps(testData.columnsOnly), + margin: 32, + legacy_order_by: null, + order_desc: false, + }; + const { container } = render( + ProviderWrapper({ + children: <PivotTableChart {...transformedProps} />, + }), + ); + + // Without row dimensions there is no row-header row, so the leading + // cell of the last header row is the column attribute name. It must + // scroll with its column rather than being treated as a corner cell. + expect(container.querySelector('thead tr.pvtRowHeaderRow')).toBeNull(); + const headerCells = container.querySelectorAll('thead th'); + expect(headerCells.length).toBeGreaterThan(0); + headerCells.forEach(cell => { + expect(getComputedStyle(cell).position).not.toBe('sticky'); + }); +}); + +test('does not freeze the row-label column or its corner cell(s) with more than one row dimension', () => { + // With multiple row attributes, every row-label and corner cell would + // freeze at the same left: 0 edge and stack on top of one another, so + // freezing is scoped to the single-row-dimension case until a + // per-column offset fast-follow lands. Uses a fixture with both multiple + // row dimensions and a column dimension (unlike groupedRowsWithoutColTotals, + // which has no column dimension) so the pvtCornerLabel group below is + // actually non-empty and its non-sticky assertion is exercised, rather + // than passing vacuously. + const transformedProps = { + ...transformProps(testData.groupedRowsWithColumnDim), + margin: 32, + legacy_order_by: null, + order_desc: false, + }; + const { container } = render( + ProviderWrapper({ + children: <PivotTableChart {...transformedProps} />, + }), + ); + + const rowLabelCells = container.querySelectorAll('tbody th.pvtRowLabel'); + expect(rowLabelCells.length).toBeGreaterThan(0); + rowLabelCells.forEach(cell => { + expect(getComputedStyle(cell).position).not.toBe('sticky'); + }); + + // Asserted as two separate non-empty groups, mirroring the + // sticky-positioning test above, so that if either selector stops + // matching, that group's absence fails the test instead of silently + // leaving only the other group's cells checked. + const columnCornerCells = container.querySelectorAll( + 'thead th.pvtCornerLabel', + ); + const rowHeaderCornerCells = container.querySelectorAll( + 'thead tr.pvtRowHeaderRow th.pvtAxisLabel', + ); + expect(columnCornerCells.length).toBeGreaterThan(0); + expect(rowHeaderCornerCells.length).toBeGreaterThan(0); + [...columnCornerCells, ...rowHeaderCornerCells].forEach(cell => { + expect(getComputedStyle(cell).position).not.toBe('sticky'); + }); +}); + +test('does not leak sticky positioning onto the totals label from its row with more than one row dimension', () => { + // th.pvtRowTotalLabel's direct parent is tr.pvtRowTotals, which is + // unconditionally sticky (it freezes to the bottom edge regardless of + // row-dimension count). The non-sticky branch here must resolve to an + // explicit 'static' rather than 'inherit', or the label would pick up + // the row's own sticky position and stay pinned at left: 0 anyway. + const transformedProps = { + ...transformProps(testData.groupedRowsWithColTotals), + margin: 32, + legacy_order_by: null, + order_desc: false, + }; + const { container } = render( + ProviderWrapper({ + children: <PivotTableChart {...transformedProps} />, + }), + ); + + const totalsLabel = container.querySelector( + 'tbody tr.pvtRowTotals th.pvtRowTotalLabel', + ); + expect(totalsLabel).toBeInTheDocument(); + expect(getComputedStyle(totalsLabel as Element).position).toBe('static'); +}); + +test('does not sticky-position the row-label column or corner cell(s) in dashboard edit mode', () => { + // TableRenderers detects dashboard edit mode by looking for this class + // on the document, rather than via a prop. + const editingMarker = document.createElement('div'); + editingMarker.className = 'dashboard--editing'; + document.body.appendChild(editingMarker); + + try { + const transformedProps = { + ...transformProps(testData.withoutColTotals), + margin: 32, + legacy_order_by: null, + order_desc: false, + }; + const { container } = render( + ProviderWrapper({ + children: <PivotTableChart {...transformedProps} />, + }), + ); + + const rowLabelCell = container.querySelector('tbody th.pvtRowLabel'); + expect(rowLabelCell).toBeInTheDocument(); + expect(getComputedStyle(rowLabelCell as Element).position).not.toBe( Review Comment: This edit-mode test only checks the body-label guard, so a regression in either independent corner-cell or totals-label guard would leave an orphan frozen cell during editing without failing the test. Could it render the totals-enabled fixture and assert both non-empty corner groups are non-sticky and the totals label is `static`? ########## superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx: ########## @@ -911,8 +901,21 @@ export function TableRenderer(props: TableRendererProps) { subArrow = attrIdx + 1 < maxColVisible! ? arrowExpanded : arrowCollapsed; } + // With row dimensions, the corner block above the frozen row labels + // is rowAttrs.length + 1 columns wide: the row-attribute columns plus + // the padding column that renderTableRow folds into the last row + // label. Span the column-attribute name across that whole block so a + // single sticky cell freezes it, rather than a rowAttrs-wide spacer + // that leaves the name's own column scrolling through the corner. + const hasRowAttrs = settingsRowAttrs.length !== 0; const attrNameCell = ( - <th key="label" className="pvtAxisLabel"> + <th + key="label" + className={ + hasRowAttrs ? 'pvtAxisLabel pvtCornerLabel' : 'pvtAxisLabel' + } + colSpan={hasRowAttrs ? settingsRowAttrs.length + 1 : undefined} Review Comment: The body-cell example was already possible before this change, but the merge newly places the “Country” axis-name header under the spanning “Year” header; `role="grid"` still preserves native header roles and spans ([ARIA in HTML](https://www.w3.org/TR/html-aria/#el-th)). Could this merge avoid that unrelated header association without requiring a broader grid accessibility rewrite? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
