Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DataGrid] Fix VoiceOver reading the column name twice #14482

Merged
merged 5 commits into from
Sep 11, 2024
+10 −11
Merged

[DataGrid] Fix VoiceOver reading the column name twice #14482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8eaaa96
remove aria-label for the default column header since the screen read…
arminmeh Sep 4, 2024
70703f0
Put back aria label for the details panel toggle header item
arminmeh Sep 5, 2024
1ad6a1c
Remove aria-label from the string header title
arminmeh Sep 5, 2024
16ad8e7
Adjust test
arminmeh Sep 5, 2024
ae67fd4
destruct params
arminmeh Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/x-data-grid-pro/src/hooks/features/detailPanel/gridDetailPanelToggleColDef.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,5 @@ export const GRID_DETAIL_PANEL_TOGGLE_COL_DEF: GridColDef = {
return expandedRowIds.includes(rowId);
},
renderCell: (params) => <GridDetailPanelToggleCell {...params} />,
renderHeader: () => null,
renderHeader: ({ colDef }) => <div aria-label={colDef.headerName} />,
};
4 changes: 3 additions & 1 deletion packages/x-data-grid/src/components/columnHeaders/GridColumnHeaderTitle.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ const ColumnHeaderInnerTitle = React.forwardRef<
HTMLDivElement,
React.HTMLAttributes<HTMLDivElement>
>(function ColumnHeaderInnerTitle(props, ref) {
const { className, ...other } = props;
// Tooltip adds aria-label to the props, which is not needed since the children prop is a string
// See https://github.com/mui/mui-x/pull/14482
const { className, 'aria-label': ariaLabel, ...other } = props;
const rootProps = useGridRootProps();
const classes = useUtilityClasses(rootProps);

Expand Down
1 change: 0 additions & 1 deletion packages/x-data-grid/src/components/columnHeaders/GridGenericColumnHeaderItem.tsx
View file
Open in desktop
Copy link
Member

@MBilalShafi MBilalShafi Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be unrelated to this PR but Safari [Version 17.5 (19618.2.12.11.6)] reads normal cells twice too (used to do for header cells too but fixed with this PR)

voiceover-safari.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this is what I saw as well
since it is not mentioned in the original issue, I have opened a separate PR to address this

Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@ const GridGenericColumnHeaderItem = React.forwardRef(function GridGenericColumnH
tabIndex={tabIndex}
aria-colindex={colIndex + 1}
aria-sort={ariaSort}
aria-label={headerComponent == null ? label : undefined}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into this issue some time ago, but couldn't make it work properly.
Even with this change, it doesn't seem to work as expected – see the recording:

Screen.Recording.2024-09-04.at.19.24.04.mov

Any idea why it starts reading the column name twice?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which keys are you using to navigate the header items? 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Control + Option + arrow keys

Copy link
Contributor Author

@arminmeh arminmeh Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the reason
it looks like the Tooltip component is adding another aria-label on the wrapped component

mui-x/packages/x-data-grid/src/components/columnHeaders/GridColumnHeaderTitle.tsx

Lines 77 to 85 in cf441ec

<rootProps.slots.baseTooltip
title={description || tooltip}
{...rootProps.slotProps?.baseTooltip}
>
<ColumnHeaderInnerTitle onMouseOver={handleMouseOver} ref={titleRef}>
{label}
</ColumnHeaderInnerTitle>
</rootProps.slots.baseTooltip>
);

we don't pass it here, but consoling other in

mui-x/packages/x-data-grid/src/components/columnHeaders/GridColumnHeaderTitle.tsx

Lines 34 to 50 in cf441ec

const ColumnHeaderInnerTitle = React.forwardRef<
HTMLDivElement,
React.HTMLAttributes<HTMLDivElement>
>(function ColumnHeaderInnerTitle(props, ref) {
const { className, ...other } = props;
const rootProps = useGridRootProps();
const classes = useUtilityClasses(rootProps);
return (
<GridColumnHeaderTitleRoot
ref={ref}
className={clsx(classes.root, className)}
ownerState={rootProps}
{...other}
/>
);
});

shows aria-label prop

For string content, aria-label should not be attached to the element. For now, I will filter it out in our component, but I will make a PR to the component to fix it there as well (Tooltip can't predict what the content will be. Solution might be not to add aria attribute and leave it to devs to attach it to their custom component, but that would be a breaking change, so I will leave it as is)

Update
aria-label should still be removed, but that didn't help. What I have found out is that this is actually Chrome issue. I did the test in the plain table as suggested and confirmed that the same occurs there. In Safari it works fine.

I think that the voice over wants to connect header name with the cell when you use Control + Option + arrow keys. If you navigate through content you will notice that it reads out the data plus the column name which is great in that scenario.

Anyway, I think that we can ignore that issue and have at least regular arrow key navigation fixed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Control + Option + arrow keys

Ah makes sense, I tested with the arrow keys only, not the virtual cursor.

I think that the voice over wants to connect header name with the cell when you use Control + Option + arrow keys. If you navigate through content you will notice that it reads out the data plus the column name which is great in that scenario.

Strange bug.

Anyway, I think that we can ignore that issue and have at least regular arrow key navigation fixed

I tested again with the latest changes and it's working as expected with arrow keys at least 👍

{...other}
>
<div
Expand Down
14 changes: 6 additions & 8 deletions packages/x-data-grid/src/tests/columnGrouping.DataGrid.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,18 +363,16 @@ describe('<DataGrid /> - Column grouping', () => {
fireEvent.click(screen.getByRole('button', { name: /Update columns/ }));

const row1Headers = document.querySelectorAll<HTMLElement>(
'[aria-rowindex="1"] [role="columnheader"]',
'[aria-rowindex="1"] [role="columnheader"] .MuiDataGrid-columnHeaderTitle',
);
const row2Headers = document.querySelectorAll<HTMLElement>(
'[aria-rowindex="2"] [role="columnheader"]',
'[aria-rowindex="2"] [role="columnheader"] .MuiDataGrid-columnHeaderTitle',
);

expect(
Array.from(row1Headers).map((header) => header.getAttribute('aria-label')),
).to.deep.equal(['Group']);
expect(
Array.from(row2Headers).map((header) => header.getAttribute('aria-label')),
).to.deep.equal(['field_1']);
expect(Array.from(row1Headers).map((header) => header.textContent)).to.deep.equal(['Group']);
expect(Array.from(row2Headers).map((header) => header.textContent)).to.deep.equal([
'field_1',
]);
});
});

Expand Down