[DataGrid] Fix menu accessible #1105

DanailH · 2021-02-23T15:33:51Z

Fixes #881
Fixes #882

packages/grid/_modules_/grid/utils/material-ui-utils.ts

packages/grid/_modules_/grid/components/columnHeaders/ColumnHeaderMenuIcon.tsx

oliviertassinari · 2021-02-23T16:57:51Z

packages/grid/_modules_/grid/components/menu/GridMenu.tsx

          <Paper>
-            <ClickAwayListener onClickAway={onClickAway}>{children}</ClickAwayListener>
+            <ClickAwayListener onClickAway={onClickAway}>
+              <div>{children}</div>


I think that it should aways be a MenuList as a first child (can be coming from {children}). Why is it not working in HEAD?

It's weird, it works for the export and the density but it doesn't for the column menu, by not working I mean it throws a warning in the console.

The error is that the passed children need to be able to hold a ref. This fixes the issue for all cases we currently have and I can't see any regressions.

Ok, I had a look. Any weird behavior is an invitation for exploration :). The GridColumnMenu component isn't forwarding its ref, not the extra props to the MenuList component. It could be solved easily but, there a bunch of extra props (the GridBaseComponentProps) and it's not versatile.

Instead, I have pushed a commit to reorganize the composition order. Grow doesn't host any div, it needs to apply props to its children, same for ClickAwayListener. We save one DOM node.

In the process, I have found an improvement opportunity in the core, ClickAwayListener could forward its props to the children, it doesn't.

packages/grid/_modules_/grid/components/menu/columnMenu/GridColumnMenu.tsx

packages/grid/_modules_/grid/components/columnHeaders/ColumnHeaderMenuIcon.tsx

…aderMenuIcon.tsx Co-authored-by: Olivier Tassinari <[email protected]>

oliviertassinari

Things I have seen, we aren't too far:

There are two approved suggestions that were only partially applied in the changes.
As far as I know, Tab should prevent default, there is a native behavior (it scrolls the page before the focus moves back to where it should be) we want to disable. On the other hand, Escape has no native behavior hence we don't need to prevent default. It seems to be how the Menu in the core behaves. It makes me think of mui/material-ui#13470 (comment)
The focus isn't moving to the first item in the list. Related to mui/material-ui#17571.
I don't understand what we do with useId. Did you look at what the hook is doing and the requirement for the id to work correctly? In the current usage, using the hook or not changes nothing.

packages/grid/_modules_/grid/components/menu/GridMenu.tsx

oliviertassinari · 2021-02-24T19:36:31Z

packages/grid/_modules_/grid/components/menu/GridMenu.tsx

          <Paper>
-            <ClickAwayListener onClickAway={onClickAway}>{children}</ClickAwayListener>
+            <ClickAwayListener onClickAway={onClickAway}>
+              <div>{children}</div>


Ok, I had a look. Any weird behavior is an invitation for exploration :). The GridColumnMenu component isn't forwarding its ref, not the extra props to the MenuList component. It could be solved easily but, there a bunch of extra props (the GridBaseComponentProps) and it's not versatile.

Instead, I have pushed a commit to reorganize the composition order. Grow doesn't host any div, it needs to apply props to its children, same for ClickAwayListener. We save one DOM node.

In the process, I have found an improvement opportunity in the core, ClickAwayListener could forward its props to the children, it doesn't.

packages/grid/_modules_/grid/components/menu/columnMenu/GridColumnMenu.tsx

…lumnMenu.tsx Co-authored-by: Olivier Tassinari <[email protected]>

Co-authored-by: Olivier Tassinari <[email protected]>

DanailH · 2021-02-25T12:06:59Z

Things I have seen, we aren't too far:

There are two approved suggestions that were only partially applied in the changes.

As far as I know, Tab should prevent default, there is a native behavior (it scrolls the page before the focus moves back to where it should be) we want to disable. On the other hand, Escape has no native behavior hence we don't need to prevent default. It seems to be how the Menu in the core behaves. It makes me think of mui-org/material-ui#13470 (comment)

The focus isn't moving to the first item in the list. Related to mui-org/material-ui#17571.

I don't understand what we do with useId. Did you look at what the hook is doing and the requirement for the id to work correctly? In the current usage, using the hook or not changes nothing.

Alright, I pushed the changes requested.

A couple of things, I'll keep autoFocus for the GridColumnMenu because the autoFocusItem is not working due to the current structure (needs refactoring if we want to make it work). It's still accessible via the keyboard. The drawback is that the focus is not on the first element but on the wrapper and you need one arrow to click to select the first or last element in the menu.
Another thing was the ids for the columne menu and the 3dot button - went for the option of keeping them in the state for the time being (there was already a columnMenu section in the state that holds the field so I added the id and the labelledby as well).

packages/grid/_modules_/grid/components/toolbar/GridDensitySelector.tsx

…taGrid-column-menu-a11y

dtassone · 2021-02-25T15:26:47Z

packages/grid/_modules_/grid/models/api/columnMenuApi.ts

+   * @param labelledby
   */
-  showColumnMenu: (field: string) => void;
+  showColumnMenu: (field: string, id: string, labelledby: string) => void;


why do we need id and labelledBy to show the column menu? And how can consumers get them?

We need them so that the labelledby and the controlledby are hooked up correctly. I had to do it like this because they that the menu and the button are set up that is the only option. In terms of how users can get them -> there are in the state while the menu is opened.

ShowColumnMenu should only show the menu when one click on the 3 dots icon next to the column title.
Is that still the case?

there are in the state while the menu is opened.

Well showColumnMenu opens the menu so how can we know the labelledBy before we call the showColumnMenu. Chicken and egg situation? :)

I mean the existing functionality still works the same. Also, why do we need to know the ID beforehand it is for hooking up the accessibility, there are no functionalities or styles attached to it?

@dtassone can I merge this, it would be nice to include the fix in the upcoming release 🙃

how do you render the grid with the menu open. Imagine if the user want to put a custom component...

How is it harmful?

I don't think that would be an issue, they can still open it right, it's just that the id and the labelledBy won't be set

How is it harmful?

It could prevent us to improve the internal: e.g. this very discussion. I don't understand the use cases for a developer to programmatically open a column menu from outside the normal end-user flows. Wouldn't it be better to say a temporary NO and wait for a developer to come up with a valid use case?

they can still open it right, it's just that the id and the labelledBy won't be set

So the a11y would be broken?

I don't think that would be an issue, they can still open it right, it's just that the id and the labelledBy won't be set

why not make them optional?
Also why calling it id? Something like menuElementId or whatever a bit more specific so it's more understandable

Fix a11y related issues

9c99844

DanailH added type: bug It doesn't behave as expected. accessibility a11y scope: data grid Changes related to the data grid. labels Feb 23, 2021

DanailH requested review from dtassone and oliviertassinari February 23, 2021 15:33

DanailH self-assigned this Feb 23, 2021

DanailH commented Feb 23, 2021

View reviewed changes

packages/grid/_modules_/grid/utils/material-ui-utils.ts Outdated Show resolved Hide resolved

dtassone reviewed Feb 23, 2021

View reviewed changes

packages/grid/_modules_/grid/components/columnHeaders/ColumnHeaderMenuIcon.tsx Show resolved Hide resolved

oliviertassinari reviewed Feb 23, 2021

View reviewed changes

packages/grid/_modules_/grid/components/menu/columnMenu/GridColumnMenu.tsx Show resolved Hide resolved

DanailH added 2 commits February 24, 2021 10:17

Fix pr comments

8139ab4

Get latest changes

d13fdfa

DanailH requested review from dtassone and oliviertassinari February 24, 2021 09:23

oliviertassinari reviewed Feb 24, 2021

View reviewed changes

packages/grid/_modules_/grid/components/columnHeaders/ColumnHeaderMenuIcon.tsx Outdated Show resolved Hide resolved

DanailH and others added 2 commits February 24, 2021 16:35

Update packages/grid/_modules_/grid/components/columnHeaders/ColumnHe…

21480db

…aderMenuIcon.tsx Co-authored-by: Olivier Tassinari <[email protected]>

save the cost of one div

676033f

oliviertassinari requested changes Feb 24, 2021

View reviewed changes

DanailH and others added 3 commits February 25, 2021 10:37

Update packages/grid/_modules_/grid/components/menu/columnMenu/GridCo…

5bc221f

…lumnMenu.tsx Co-authored-by: Olivier Tassinari <[email protected]>

Update packages/grid/_modules_/grid/components/menu/GridMenu.tsx

3010282

Co-authored-by: Olivier Tassinari <[email protected]>

Address PR comments

020886c

DanailH requested a review from oliviertassinari February 25, 2021 12:07

oliviertassinari reviewed Feb 25, 2021

View reviewed changes

packages/grid/_modules_/grid/components/toolbar/GridDensitySelector.tsx Outdated Show resolved Hide resolved

oliviertassinari approved these changes Feb 25, 2021

View reviewed changes

DanailH added 2 commits February 25, 2021 14:06

Remove unneeded role

5b1cd4d

Merge branch 'master' of github.com:mui-org/material-ui-x into fix/Da…

646ebb0

…taGrid-column-menu-a11y

dtassone reviewed Feb 25, 2021

View reviewed changes

dtassone approved these changes Feb 26, 2021

View reviewed changes

DanailH merged commit 635dbb2 into mui:master Feb 26, 2021

oliviertassinari changed the title ~~[DataGrid] Make GridMenu accessible~~ [DataGrid] Fix menu accessible Feb 27, 2021

Uh oh!

[DataGrid] Fix menu accessible #1105

[DataGrid] Fix menu accessible #1105

Uh oh!

Conversation

DanailH commented Feb 23, 2021

Uh oh!

Uh oh!

Uh oh!

oliviertassinari Feb 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanailH Feb 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oliviertassinari left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oliviertassinari Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DanailH commented Feb 25, 2021

Uh oh!

Uh oh!

dtassone Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

DanailH Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtassone Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

DanailH Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanailH Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

dtassone Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

dtassone Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

DanailH Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtassone Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

oliviertassinari Feb 23, 2021 •

edited

Loading

DanailH Feb 23, 2021 •

edited

Loading

oliviertassinari Feb 24, 2021 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

oliviertassinari Feb 24, 2021 •

edited

Loading

DanailH Feb 25, 2021 •

edited

Loading

DanailH Feb 25, 2021 •

edited

Loading

oliviertassinari Feb 26, 2021 •

edited

Loading