From 165d5311cd42d0525995e12f9b3ee6055cafeb18 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Thu, 1 Jun 2023 17:40:32 +0200 Subject: [PATCH 1/5] fix: TableHeaderCell should not render button when not sortable Using the approach with the smallest impact to API and slot rendering. The slot rendered a native `button` always, but with role="presentation" when it was not interactive. The slot always renders a `div` but with ARIA button semantics when it is interactive Fixes #27633 --- .../react-table/etc/react-table.api.md | 12 +--------- .../react-components/react-table/package.json | 2 ++ .../DataGridHeaderCell.test.tsx.snap | 7 ++---- .../TableHeaderCell/TableHeaderCell.test.tsx | 18 +++++++++++---- .../TableHeaderCell/TableHeaderCell.types.ts | 3 +-- .../TableHeaderCell.test.tsx.snap | 7 ++---- .../TableHeaderCell/useTableHeaderCell.tsx | 22 +++++++------------ 7 files changed, 30 insertions(+), 41 deletions(-) diff --git a/packages/react-components/react-table/etc/react-table.api.md b/packages/react-components/react-table/etc/react-table.api.md index da972c37b1bea..a69d17c3a88ad 100644 --- a/packages/react-components/react-table/etc/react-table.api.md +++ b/packages/react-components/react-table/etc/react-table.api.md @@ -6,18 +6,8 @@ /// -import { ARIAButtonSlotProps } from '@fluentui/react-aria'; -import type { AvatarSize } from '@fluentui/react-avatar'; -import type { Checkbox } from '@fluentui/react-checkbox'; -import type { CheckboxProps } from '@fluentui/react-checkbox'; -import type { ComponentProps } from '@fluentui/react-utilities'; -import type { ComponentState } from '@fluentui/react-utilities'; -import type { ForwardRefComponent } from '@fluentui/react-utilities'; -import type { Radio } from '@fluentui/react-radio'; import * as React_2 from 'react'; import { ReactNode } from 'react'; -import type { Slot } from '@fluentui/react-utilities'; -import type { SlotClassNames } from '@fluentui/react-utilities'; // @public (undocumented) export type CellRenderFunction = (column: TableColumnDefinition, dataGridContextValue: DataGridContextValue) => React_2.ReactNode; @@ -402,7 +392,7 @@ export type TableHeaderCellProps = ComponentProps> export type TableHeaderCellSlots = { root: Slot<'th', 'div'>; sortIcon: Slot<'span'>; - button: NonNullable>; + button: NonNullable>; aside: Slot<'span'>; }; diff --git a/packages/react-components/react-table/package.json b/packages/react-components/react-table/package.json index 4764cf11e4af3..9fa0644ed8388 100644 --- a/packages/react-components/react-table/package.json +++ b/packages/react-components/react-table/package.json @@ -16,6 +16,8 @@ "bundle-size": "bundle-size measure", "clean": "just-scripts clean", "code-style": "just-scripts code-style", + "e2e": "cypress run --component", + "e2e:local": "cypress open --component", "just": "just-scripts", "lint": "just-scripts lint", "test": "jest --passWithNoTests", diff --git a/packages/react-components/react-table/src/components/DataGridHeaderCell/__snapshots__/DataGridHeaderCell.test.tsx.snap b/packages/react-components/react-table/src/components/DataGridHeaderCell/__snapshots__/DataGridHeaderCell.test.tsx.snap index ddebf4fc22d05..280eef7736e6a 100644 --- a/packages/react-components/react-table/src/components/DataGridHeaderCell/__snapshots__/DataGridHeaderCell.test.tsx.snap +++ b/packages/react-components/react-table/src/components/DataGridHeaderCell/__snapshots__/DataGridHeaderCell.test.tsx.snap @@ -7,14 +7,11 @@ exports[`DataGridHeaderCell renders a default state 1`] = ` role="columnheader" tabindex="0" > - + `; diff --git a/packages/react-components/react-table/src/components/TableHeaderCell/TableHeaderCell.test.tsx b/packages/react-components/react-table/src/components/TableHeaderCell/TableHeaderCell.test.tsx index c6b91c9a198f7..8dd02d432f961 100644 --- a/packages/react-components/react-table/src/components/TableHeaderCell/TableHeaderCell.test.tsx +++ b/packages/react-components/react-table/src/components/TableHeaderCell/TableHeaderCell.test.tsx @@ -65,16 +65,26 @@ describe('TableHeaderCell', () => { expect(container.querySelector('svg')).toBe(null); }); + it('should have interactive button when sortable', () => { + const { getByRole } = render( + + Cell + , + ); + + const button = getByRole('button'); + expect(button?.getAttribute('tabindex')).toEqual('0'); + }); + it('should not have interactive button when not sortable', () => { - const { container } = render( + const { queryByRole } = render( Cell , ); - const button = container.querySelector('button'); - expect(button?.getAttribute('role')).toEqual('presentation'); - expect(button?.getAttribute('tabindex')).toEqual('-1'); + const button = queryByRole('button'); + expect(button).toBeNull(); }); it.each(['ascending', 'descending'])( diff --git a/packages/react-components/react-table/src/components/TableHeaderCell/TableHeaderCell.types.ts b/packages/react-components/react-table/src/components/TableHeaderCell/TableHeaderCell.types.ts index 229587688e78c..8cb1f2f198dec 100644 --- a/packages/react-components/react-table/src/components/TableHeaderCell/TableHeaderCell.types.ts +++ b/packages/react-components/react-table/src/components/TableHeaderCell/TableHeaderCell.types.ts @@ -1,5 +1,4 @@ import type { ComponentProps, ComponentState, Slot } from '@fluentui/react-utilities'; -import { ARIAButtonSlotProps } from '@fluentui/react-aria'; import { SortDirection, TableContextValue } from '../Table/Table.types'; export type TableHeaderCellSlots = { @@ -10,7 +9,7 @@ export type TableHeaderCellSlots = { /** * Button handles correct narration and interactions for sorting; */ - button: NonNullable>; + button: NonNullable>; /** * aside content for anything that should be after main content of the table header cell */ diff --git a/packages/react-components/react-table/src/components/TableHeaderCell/__snapshots__/TableHeaderCell.test.tsx.snap b/packages/react-components/react-table/src/components/TableHeaderCell/__snapshots__/TableHeaderCell.test.tsx.snap index 6cc1897b4105b..d5c1452a7088d 100644 --- a/packages/react-components/react-table/src/components/TableHeaderCell/__snapshots__/TableHeaderCell.test.tsx.snap +++ b/packages/react-components/react-table/src/components/TableHeaderCell/__snapshots__/TableHeaderCell.test.tsx.snap @@ -5,14 +5,11 @@ exports[`TableHeaderCell renders a default state 1`] = ` - + `; diff --git a/packages/react-components/react-table/src/components/TableHeaderCell/useTableHeaderCell.tsx b/packages/react-components/react-table/src/components/TableHeaderCell/useTableHeaderCell.tsx index 1cf13bfe072a7..41f9bb5a31abe 100644 --- a/packages/react-components/react-table/src/components/TableHeaderCell/useTableHeaderCell.tsx +++ b/packages/react-components/react-table/src/components/TableHeaderCell/useTableHeaderCell.tsx @@ -4,7 +4,7 @@ import { useFocusWithin } from '@fluentui/react-tabster'; import { ArrowUpRegular, ArrowDownRegular } from '@fluentui/react-icons'; import type { TableHeaderCellProps, TableHeaderCellState } from './TableHeaderCell.types'; import { useTableContext } from '../../contexts/tableContext'; -import { useARIAButtonShorthand } from '@fluentui/react-aria'; +import { useARIAButtonProps } from '@fluentui/react-aria'; const sortIcons = { ascending: , @@ -27,10 +27,15 @@ export const useTableHeaderCell_unstable = ( const { noNativeElements, sortable } = useTableContext(); const rootComponent = props.as ?? noNativeElements ? 'div' : 'th'; + + // The button slot should only have button semantics if the header is sortable + const buttonSlot = resolveShorthand(props.button, { required: true }); + const ariaButtonSlot = useARIAButtonProps('div', buttonSlot); + return { components: { root: rootComponent, - button: 'button', + button: 'div', sortIcon: 'span', aside: 'span', }, @@ -45,18 +50,7 @@ export const useTableHeaderCell_unstable = ( required: !!props.sortDirection, defaultProps: { children: props.sortDirection ? sortIcons[props.sortDirection] : undefined }, }), - button: useARIAButtonShorthand(props.button, { - required: true, - defaultProps: { - role: 'presentation', - tabIndex: -1, - type: 'button', - ...(sortable && { - role: undefined, - tabIndex: undefined, - }), - }, - }), + button: sortable ? ariaButtonSlot : buttonSlot, sortable, noNativeElements, }; From 1ad50dbd6c8a43db9446126249f3bdde3d51d576 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Thu, 1 Jun 2023 17:45:17 +0200 Subject: [PATCH 2/5] changefile --- ...react-table-a86be917-7fe4-41a7-a5aa-19d22c2aa1f8.json | 7 +++++++ .../react-components/react-table/etc/react-table.api.md | 9 +++++++++ 2 files changed, 16 insertions(+) create mode 100644 change/@fluentui-react-table-a86be917-7fe4-41a7-a5aa-19d22c2aa1f8.json diff --git a/change/@fluentui-react-table-a86be917-7fe4-41a7-a5aa-19d22c2aa1f8.json b/change/@fluentui-react-table-a86be917-7fe4-41a7-a5aa-19d22c2aa1f8.json new file mode 100644 index 0000000000000..863bb42a6e8e5 --- /dev/null +++ b/change/@fluentui-react-table-a86be917-7fe4-41a7-a5aa-19d22c2aa1f8.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "fix: TableHeaderCell should not render button when not sortable", + "packageName": "@fluentui/react-table", + "email": "lingfangao@hotmail.com", + "dependentChangeType": "patch" +} diff --git a/packages/react-components/react-table/etc/react-table.api.md b/packages/react-components/react-table/etc/react-table.api.md index a69d17c3a88ad..12ee3456135d3 100644 --- a/packages/react-components/react-table/etc/react-table.api.md +++ b/packages/react-components/react-table/etc/react-table.api.md @@ -6,8 +6,17 @@ /// +import type { AvatarSize } from '@fluentui/react-avatar'; +import type { Checkbox } from '@fluentui/react-checkbox'; +import type { CheckboxProps } from '@fluentui/react-checkbox'; +import type { ComponentProps } from '@fluentui/react-utilities'; +import type { ComponentState } from '@fluentui/react-utilities'; +import type { ForwardRefComponent } from '@fluentui/react-utilities'; +import type { Radio } from '@fluentui/react-radio'; import * as React_2 from 'react'; import { ReactNode } from 'react'; +import type { Slot } from '@fluentui/react-utilities'; +import type { SlotClassNames } from '@fluentui/react-utilities'; // @public (undocumented) export type CellRenderFunction = (column: TableColumnDefinition, dataGridContextValue: DataGridContextValue) => React_2.ReactNode; From a8772a3bf22c227fe5051c794a3c6f63d79b6436 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Thu, 1 Jun 2023 19:30:14 +0200 Subject: [PATCH 3/5] update to font weight --- .../TableHeaderCell/useTableHeaderCellStyles.styles.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/react-components/react-table/src/components/TableHeaderCell/useTableHeaderCellStyles.styles.ts b/packages/react-components/react-table/src/components/TableHeaderCell/useTableHeaderCellStyles.styles.ts index ac2deb4a7e86b..f0f99fca4495e 100644 --- a/packages/react-components/react-table/src/components/TableHeaderCell/useTableHeaderCellStyles.styles.ts +++ b/packages/react-components/react-table/src/components/TableHeaderCell/useTableHeaderCellStyles.styles.ts @@ -33,6 +33,7 @@ const useFlexLayoutStyles = makeStyles({ */ const useStyles = makeStyles({ root: { + fontWeight: tokens.fontWeightRegular, ...shorthands.padding('0px', tokens.spacingHorizontalS), ...createCustomFocusIndicatorStyle( { From f9ad4ffb9abd42eec4c1a2321785b931402a9857 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Mon, 19 Jun 2023 15:42:09 +0200 Subject: [PATCH 4/5] render div by default instead --- .../react-table/etc/react-table.api.md | 3 ++- .../DataGridHeaderCell.test.tsx.snap | 1 + .../TableHeaderCell/TableHeaderCell.types.ts | 4 +++- .../__snapshots__/TableHeaderCell.test.tsx.snap | 1 + .../TableHeaderCell/useTableHeaderCell.tsx | 17 +++++++++++------ .../useTableHeaderCellStyles.styles.ts | 1 + 6 files changed, 19 insertions(+), 8 deletions(-) diff --git a/packages/react-components/react-table/etc/react-table.api.md b/packages/react-components/react-table/etc/react-table.api.md index 944aa64566772..bd22ea064cb0a 100644 --- a/packages/react-components/react-table/etc/react-table.api.md +++ b/packages/react-components/react-table/etc/react-table.api.md @@ -6,6 +6,7 @@ /// +import { ARIAButtonSlotProps } from '@fluentui/react-aria'; import type { AvatarSize } from '@fluentui/react-avatar'; import type { Checkbox } from '@fluentui/react-checkbox'; import type { CheckboxProps } from '@fluentui/react-checkbox'; @@ -407,7 +408,7 @@ export type TableHeaderCellProps = ComponentProps> export type TableHeaderCellSlots = { root: Slot<'th', 'div'>; sortIcon: Slot<'span'>; - button: NonNullable>; + button: NonNullable; aside: Slot<'span'>; }; diff --git a/packages/react-components/react-table/src/components/DataGridHeaderCell/__snapshots__/DataGridHeaderCell.test.tsx.snap b/packages/react-components/react-table/src/components/DataGridHeaderCell/__snapshots__/DataGridHeaderCell.test.tsx.snap index 280eef7736e6a..bc66c6cec1c3a 100644 --- a/packages/react-components/react-table/src/components/DataGridHeaderCell/__snapshots__/DataGridHeaderCell.test.tsx.snap +++ b/packages/react-components/react-table/src/components/DataGridHeaderCell/__snapshots__/DataGridHeaderCell.test.tsx.snap @@ -9,6 +9,7 @@ exports[`DataGridHeaderCell renders a default state 1`] = ` > diff --git a/packages/react-components/react-table/src/components/TableHeaderCell/TableHeaderCell.types.ts b/packages/react-components/react-table/src/components/TableHeaderCell/TableHeaderCell.types.ts index 8cb1f2f198dec..b71d9e377d144 100644 --- a/packages/react-components/react-table/src/components/TableHeaderCell/TableHeaderCell.types.ts +++ b/packages/react-components/react-table/src/components/TableHeaderCell/TableHeaderCell.types.ts @@ -1,4 +1,5 @@ import type { ComponentProps, ComponentState, Slot } from '@fluentui/react-utilities'; +import { ARIAButtonSlotProps } from '@fluentui/react-aria'; import { SortDirection, TableContextValue } from '../Table/Table.types'; export type TableHeaderCellSlots = { @@ -9,7 +10,8 @@ export type TableHeaderCellSlots = { /** * Button handles correct narration and interactions for sorting; */ - button: NonNullable>; + button: NonNullable; + /** * aside content for anything that should be after main content of the table header cell */ diff --git a/packages/react-components/react-table/src/components/TableHeaderCell/__snapshots__/TableHeaderCell.test.tsx.snap b/packages/react-components/react-table/src/components/TableHeaderCell/__snapshots__/TableHeaderCell.test.tsx.snap index d5c1452a7088d..3c62b58246e7d 100644 --- a/packages/react-components/react-table/src/components/TableHeaderCell/__snapshots__/TableHeaderCell.test.tsx.snap +++ b/packages/react-components/react-table/src/components/TableHeaderCell/__snapshots__/TableHeaderCell.test.tsx.snap @@ -7,6 +7,7 @@ exports[`TableHeaderCell renders a default state 1`] = ` > diff --git a/packages/react-components/react-table/src/components/TableHeaderCell/useTableHeaderCell.tsx b/packages/react-components/react-table/src/components/TableHeaderCell/useTableHeaderCell.tsx index 41f9bb5a31abe..beeb43575c6d7 100644 --- a/packages/react-components/react-table/src/components/TableHeaderCell/useTableHeaderCell.tsx +++ b/packages/react-components/react-table/src/components/TableHeaderCell/useTableHeaderCell.tsx @@ -2,9 +2,9 @@ import * as React from 'react'; import { getNativeElementProps, resolveShorthand, useMergedRefs } from '@fluentui/react-utilities'; import { useFocusWithin } from '@fluentui/react-tabster'; import { ArrowUpRegular, ArrowDownRegular } from '@fluentui/react-icons'; +import { useARIAButtonShorthand } from '@fluentui/react-aria'; import type { TableHeaderCellProps, TableHeaderCellState } from './TableHeaderCell.types'; import { useTableContext } from '../../contexts/tableContext'; -import { useARIAButtonProps } from '@fluentui/react-aria'; const sortIcons = { ascending: , @@ -28,10 +28,6 @@ export const useTableHeaderCell_unstable = ( const rootComponent = props.as ?? noNativeElements ? 'div' : 'th'; - // The button slot should only have button semantics if the header is sortable - const buttonSlot = resolveShorthand(props.button, { required: true }); - const ariaButtonSlot = useARIAButtonProps('div', buttonSlot); - return { components: { root: rootComponent, @@ -50,7 +46,16 @@ export const useTableHeaderCell_unstable = ( required: !!props.sortDirection, defaultProps: { children: props.sortDirection ? sortIcons[props.sortDirection] : undefined }, }), - button: sortable ? ariaButtonSlot : buttonSlot, + button: useARIAButtonShorthand(props.button, { + required: true, + defaultProps: { + as: 'div', + ...(!sortable && { + role: 'presentation', + tabIndex: undefined, + }), + }, + }), sortable, noNativeElements, }; diff --git a/packages/react-components/react-table/src/components/TableHeaderCell/useTableHeaderCellStyles.styles.ts b/packages/react-components/react-table/src/components/TableHeaderCell/useTableHeaderCellStyles.styles.ts index f0f99fca4495e..e680f7c39fa69 100644 --- a/packages/react-components/react-table/src/components/TableHeaderCell/useTableHeaderCellStyles.styles.ts +++ b/packages/react-components/react-table/src/components/TableHeaderCell/useTableHeaderCellStyles.styles.ts @@ -115,6 +115,7 @@ export const useTableHeaderCellStyles_unstable = (state: TableHeaderCellState): state.noNativeElements ? layoutStyles.flex.root : layoutStyles.table.root, state.root.className, ); + state.button.className = mergeClasses( tableHeaderCellClassNames.button, styles.resetButton, From ffc1fd115febf68480f10526dfc6f64702d8b556 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Mon, 19 Jun 2023 18:42:18 +0200 Subject: [PATCH 5/5] revert --- packages/react-components/react-table/etc/react-table.api.md | 2 +- .../src/components/TableHeaderCell/TableHeaderCell.types.ts | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/react-components/react-table/etc/react-table.api.md b/packages/react-components/react-table/etc/react-table.api.md index bd22ea064cb0a..dbcfd5534d1e0 100644 --- a/packages/react-components/react-table/etc/react-table.api.md +++ b/packages/react-components/react-table/etc/react-table.api.md @@ -408,7 +408,7 @@ export type TableHeaderCellProps = ComponentProps> export type TableHeaderCellSlots = { root: Slot<'th', 'div'>; sortIcon: Slot<'span'>; - button: NonNullable; + button: NonNullable>; aside: Slot<'span'>; }; diff --git a/packages/react-components/react-table/src/components/TableHeaderCell/TableHeaderCell.types.ts b/packages/react-components/react-table/src/components/TableHeaderCell/TableHeaderCell.types.ts index b71d9e377d144..229587688e78c 100644 --- a/packages/react-components/react-table/src/components/TableHeaderCell/TableHeaderCell.types.ts +++ b/packages/react-components/react-table/src/components/TableHeaderCell/TableHeaderCell.types.ts @@ -10,8 +10,7 @@ export type TableHeaderCellSlots = { /** * Button handles correct narration and interactions for sorting; */ - button: NonNullable; - + button: NonNullable>; /** * aside content for anything that should be after main content of the table header cell */