[DataGrid] Fix VoiceOver reading the column name twice

[arminmeh]

Fixes #9954

aria-label was only added if the default header component was used (GridColumnHeaderTitle).
That component adds label as its content which is read by the voice over, so the aria-label is not needed at all.

[mui-bot]

Deploy preview: https://deploy-preview-14482--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against ae67fd4

[KenanYusuf]

Tested with VoiceOver and it is working as expected.

[cherniavskii]

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?

[KenanYusuf]

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

[cherniavskii]

Control + Option + arrow keys

[arminmeh]

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

[KenanYusuf]

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 👍

[arminmeh]

Before, the condition headerComponent == null ? label : undefined in GridGenericColumnHeaderItem was ensuring that the aria label is added to this column without actually rendering anything.
Now the column definition holds that logic in itself

[cherniavskii]

How about we do this internally instead?

diff --git a/packages/x-data-grid-pro/src/hooks/features/detailPanel/gridDetailPanelToggleColDef.tsx b/packages/x-data-grid-pro/src/hooks/features/detailPanel/gridDetailPanelToggleColDef.tsx
index 0fc2a55a78..ce3a18b882 100644
--- a/packages/x-data-grid-pro/src/hooks/features/detailPanel/gridDetailPanelToggleColDef.tsx
+++ b/packages/x-data-grid-pro/src/hooks/features/detailPanel/gridDetailPanelToggleColDef.tsx
@@ -29,5 +29,5 @@ export const GRID_DETAIL_PANEL_TOGGLE_COL_DEF: GridColDef = {
     return expandedRowIds.includes(rowId);
   },
   renderCell: (params) => <GridDetailPanelToggleCell {...params} />,
-  renderHeader: (params) => <div aria-label={params.colDef.headerName} />,
+  renderHeader: () => null,
 };
diff --git a/packages/x-data-grid/src/components/columnHeaders/GridGenericColumnHeaderItem.tsx b/packages/x-data-grid/src/components/columnHeaders/GridGenericColumnHeaderItem.tsx
index 412bb46be9..91ecd13dc5 100644
--- a/packages/x-data-grid/src/components/columnHeaders/GridGenericColumnHeaderItem.tsx
+++ b/packages/x-data-grid/src/components/columnHeaders/GridGenericColumnHeaderItem.tsx
@@ -120,7 +120,7 @@ const GridGenericColumnHeaderItem = React.forwardRef(function GridGenericColumnH
         <div className={classes.titleContainer} role="presentation">
           <div className={classes.titleContainerContent}>
             {headerComponent !== undefined ? (
-              headerComponent
+              headerComponent || <div aria-label={label} />
             ) : (
               <GridColumnHeaderTitle label={label} description={description} columnWidth={width} />
             )}

[arminmeh]

I would rather keep it with the definition that actually needs it and keep the generic component clean from it.
Otherwise we have this (for me) weird algorithm where we say:

• If you don't provide a component a fallback will be rendered
• Else If your component is null another fallback will be rendered
• Else your component is rendered

Searching the code, I didn't find any other place where null is returned (there is another definition with ' ', but that will not end up in the same code branch)

Do you think that someone relies on the current logic and that the things will break?

[cherniavskii]

I was mostly considering compatibility here, but also a better default – no need to know this trick to make the column header accessible.
The empty column header is a rare case though.
I don't have a strong opinion on this one, I'm OK with your approach 👍🏻

[MBilalShafi]

Looks great on Firefox and Chrome with Arrow keys. 🎉
Found a small issue on Safari, feel free to handle it separately if it's unrelated.

[MBilalShafi]

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

[arminmeh]

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