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.

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

Fix List Filter Menu is not accessible #6967

Merged
merged 3 commits into from
Dec 15, 2021
Merged

Fix List Filter Menu is not accessible #6967

merged 3 commits into from
Dec 15, 2021

Conversation

WiXSL
Copy link
Contributor

@WiXSL WiXSL commented Dec 9, 2021

Fixes #6964

@vercel vercel bot temporarily deployed to Preview – react-admin-storybook December 9, 2021 23:31 Inactive
@vercel vercel bot temporarily deployed to Preview – react-admin December 9, 2021 23:31 Inactive
@WiXSL WiXSL added the RFR Ready For Review label Dec 10, 2021
Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

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

While you're on this file, would you mind adding the missing aria-haspopup="true" on the Button element?

<FilterButtonMenuItem
key={filterElement.props.source}
filter={filterElement}
resource={resource}
onShow={handleShow}
autoFocus={index === 0}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't get why this is necessary to fix the keyboard navigation. Can you explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't get it to work otherwise, I'm forcing the focus on the first menu item, do you have a suggestion with aria attributes?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No but I fail to find a difference between our code and the MUI v4 demos for the menu

Copy link
Contributor Author

@WiXSL WiXSL Dec 10, 2021

Choose a reason for hiding this comment

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

No but I fail to find a difference between our code and the MUI v4 demos for the menu

What I've noticed is that they don't have a forwardRef MenuItem inside a Menu in their examples.
The keyboard navigation works out of the box without this, but of course, we need it because of the use of FilterButtonMenuItem.

For example, this works (without the use of FilterButtonMenuItem):

// FilterButton.jsx
...
<Menu
    open={open}
    anchorEl={anchorEl.current}
    onClose={handleRequestClose}
>
    {hiddenFilters.map((filterElement: JSX.Element, index) => (
        <MenuItem
            key={filterElement.props.source}
            data-key={filterElement.props.source}
            data-default-value={filterElement.props.defaultValue}
            onClick={() =>
                handleShow({
                    source: filterElement.props.source,
                    defaultValue: filterElement.props.defaultValue,
                })
            }
        >
            <FieldTitle
                label={filterElement.props.label}
                source={filterElement.props.source}
                resource={resource}
            />
        </MenuItem>
    ))}
</Menu>
...

Of course, it would be a BC.
Now, why does it work when not using a forwardRef ?, I don't know.

@vercel vercel bot temporarily deployed to Preview – react-admin December 10, 2021 10:52 Inactive
@vercel vercel bot temporarily deployed to Preview – react-admin-storybook December 10, 2021 10:52 Inactive
@fzaninotto fzaninotto merged commit 583f7df into master Dec 15, 2021
@fzaninotto fzaninotto deleted the fix-list-keyboard branch December 15, 2021 13:16
@fzaninotto fzaninotto added this to the 3.19.4 milestone Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

List Filter Menu is not accessible
3 participants