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

[Menulist] Add autoFocusItem for initial focus control #17571

Merged
merged 6 commits into from
Oct 1, 2019

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Sep 25, 2019

Closes #17539

<MenuList autoFocusItem /> will focus the selected or first menuitem when mounting or changing the prop. It's used to manage focus in https://material-ui.com/components/menus/#menulist-composition.

@eps1lon eps1lon added accessibility a11y package: material-ui Specific to @mui/material labels Sep 25, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Sep 25, 2019

@material-ui/core: parsed: +0.11% , gzip: +0.10%

Details of bundle changes.

Comparing: f25c643...7552485

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +0.11% 🔺 +0.10% 🔺 332,485 332,867 90,719 90,809
@material-ui/core/Paper 0.00% 0.00% 68,882 68,882 20,527 20,527
@material-ui/core/Paper.esm 0.00% 0.00% 62,319 62,319 19,264 19,264
@material-ui/core/Popper 0.00% 0.00% 28,405 28,405 10,172 10,172
@material-ui/core/Textarea 0.00% 0.00% 5,082 5,082 2,136 2,136
@material-ui/core/TrapFocus 0.00% 0.00% 3,766 3,766 1,596 1,596
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 16,348 16,348 5,818 5,818
@material-ui/core/useMediaQuery 0.00% 0.00% 2,488 2,488 1,050 1,050
@material-ui/lab 0.00% 0.00% 143,352 143,352 43,792 43,792
@material-ui/styles 0.00% 0.00% 51,641 51,641 15,353 15,353
@material-ui/system 0.00% 0.00% 15,581 15,581 4,341 4,341
Button 0.00% 0.00% 78,719 78,719 24,031 24,031
Modal 0.00% 0.00% 14,542 14,542 5,113 5,113
Portal 0.00% 0.00% 2,907 2,907 1,318 1,318
Rating 0.00% 0.00% 70,185 70,185 21,915 21,915
Slider 0.00% 0.00% 75,326 75,326 23,248 23,248
colorManipulator 0.00% 0.00% 3,835 3,835 1,519 1,519
docs.landing 0.00% 0.00% 54,267 54,267 14,344 14,344
docs.main +0.07% 🔺 +0.09% 🔺 587,126 587,508 187,881 188,051
packages/material-ui/build/umd/material-ui.production.min.js +0.12% 🔺 +0.10% 🔺 303,304 303,678 87,264 87,348

Generated by 🚫 dangerJS against 7552485

@ianschmitz
Copy link
Contributor

Depending on the outcome of the discussion in #17539, this PR would ideally implement a proper roving tabindex in MenuList to be a11y compliant. Currently the tabindex is forced to 0 on the first item, and is missing the roving behavior when using the arrow keys. This means when navigating away and back to the list the focused item is not returned to the previously focused item.

@ianschmitz
Copy link
Contributor

Here's a workaround we applied to MenuList that seems to work fairly well:

import { MenuListProps as MUIMenuListProps } from "@material-ui/core/MenuList";
import React from "react";
import List from "../List";

// TODO: Remove wrapper and export `MUIMenuList` directly once a11y issues are resolved:
// https://github.com/mui-org/material-ui/issues/16644
const MenuList: React.FC<MUIMenuListProps> = ({ children, ...other }) => {
    const menuListRef = React.useRef<HTMLUListElement>(null);

    const handleKeyDown = (event: React.KeyboardEvent<any>): void => {
        // See https://www.w3.org/TR/wai-aria-practices-1.1/#TreeView.
        const nodes = Array.from(menuListRef.current!.children) as HTMLElement[];
        let index = nodes.findIndex(el => el.contains(document.activeElement));
        if (event.key === "ArrowDown") {
            if (++index >= nodes.length) {
                index = 0;
            }
            nodes[index].focus();
            event.preventDefault();
        } else if (event.key === "ArrowUp") {
            if (--index < 0) {
                index = nodes.length - 1;
            }
            nodes[index].focus();
            event.preventDefault();
        } else if (event.key === "Home") {
            nodes[0].focus();
            event.preventDefault();
        } else if (event.key === "End") {
            nodes[nodes.length - 1].focus();
            event.preventDefault();
        }
    };

    const handleFocus = (): void => {
        // Implement a "roving index" so that only the current item (and its
        // secondary actions) are focusable via Tab and Shift+Tab.
        const nodes = Array.from(menuListRef.current!.children) as HTMLElement[];
        for (const node of nodes) {
            node.tabIndex = node.contains(document.activeElement) ? 0 : -1;
        }
    };

    React.useLayoutEffect(() => {
        if (menuListRef.current) {
            (menuListRef.current.firstChild as HTMLElement).tabIndex = 0;
        }
    }, []);

    return (
        <List {...other} onFocus={handleFocus} onKeyDown={handleKeyDown} ref={menuListRef}>
            {children}
        </List>
    );
};

export * from "@material-ui/core/MenuList";

export default MenuList;

@eps1lon
Copy link
Member Author

eps1lon commented Sep 27, 2019

this PR would ideally implement a proper roving tabindex in MenuList to be a11y complian

Please be careful with the wording here. You assume it implements listbox which it doesn't. This was never the intention. It was only meant for customizing the Menu:

For answering those needs, we expose a MenuList component that you can compose, with Popper in this example.

A menu does not need a roving tabindex.

This is not to say we will not work on a Listbox solution. We need to find out where we would need this first and decide on a design. I only want to prevent false claims about a11y compliance if they are based on missing roving tab index

@ianschmitz
Copy link
Contributor

Fair enough! If you guys are still open to a keyboard accessible list component that would be great. Perhaps the docs should better clarify the intention?

@eps1lon
Copy link
Member Author

eps1lon commented Sep 29, 2019

Fair enough! If you guys are still open to a keyboard accessible list component that would be great. Perhaps the docs should better clarify the intention?

I'll add a comment that this component is intended to be used as a role="menu".

I still don't understand what you mean by "accessible list component". If you place focus manually on one of the items then it does implement keyboard navigation according to https://www.w3.org/TR/wai-aria-practices/examples/menu-button/menu-button-links.html.

@eps1lon eps1lon marked this pull request as ready for review September 29, 2019 13:54
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 29, 2019

I still don't understand what you mean by "accessible list component".

@ianschmitz Me neither. It would be great if you could link to a section in the WAI-ARIA about the feature you are looking for.

Notice that React seems to work on similar problems, under the React-interactions, formerly react-ui effort (a name they save for later, based on facebook/react#16842).

Also notice that I had to implement a list box with non-DOM focus (active descendant) for the autocomplete, ComboBox, MultiSelect effort #17037.

@ianschmitz
Copy link
Contributor

ianschmitz commented Sep 30, 2019

I still don't understand what you mean by "accessible list component".

I think listbox is the closest to what I was describing. Here's a demo: https://codesandbox.io/s/material-demo-b1z97. However in the case of the left navigation menu, the tree component may actually be desirable if you have nested menus. The left navigation bar in MUI's docs would be a great example of that.

Sorry i haven't been doing a great job explaining myself 😉

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

@eps1lon
Copy link
Member Author

eps1lon commented Oct 1, 2019

The left navigation bar in MUI's docs would be a great example of that.

💯

I think we should start with a separate component for a listbox first (because it requires persisting the active item) and then see if it makes sense to deduplicate some logic.

@eps1lon eps1lon merged commit 656a92a into mui:master Oct 1, 2019
@eps1lon eps1lon deleted the feat/MenuList/keyboard-a11y branch October 1, 2019 08:28
@rommelmamedov
Copy link
Contributor

After updating React version it gives this warning

Screen Shot 2019-10-28 at 12 12 11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MenuList is not accessible
6 participants