[MenuList] Allow including disabled items in keyboard navigation #19967
Conversation
|
I'm inclined to agree here (especially |
eps1lon
added
accessibility
eps1lon
changed the title
Details of bundle changes.Comparing: 6d62842...0c0a22e Details of page changes
|
|
thanks @eps1lon I have modified the tests to check for the modified behavior. |
There was a problem hiding this comment.
- What visual feedback should we provide when a disabled item is focused? There is no feedback right now.
- Should the behavior be the default one? In the WAI link, it seems to be referenced as something common, but does it mean it should be the default? For instance, on macOS, disabled items in a menu aren't focusable. The same goes for Google spreadsheets/docs.
- Should the Autocomplete behave identically? It doesn't right now, it could be inconsistent.
A few responses: -I don't believe based on the wcag specification that any other visual styling is required for disabled & focused elements. That would be a library specific choice for MUI. Happy to update this PR as necessary though. |
@scottander I have a hard time finding UI libraries to benchmark against. Do you have some good sources to benchmark? I only have found weak signals. The following go against it: The following go in favor of it: I wonder, should we make a distinction between a "menu bar" and a "dropdown menu"?
In any case, I'm in favor of making it configurable. As for the default value, I have no strong point of view. @eps1lon any preference? |
|
@oliviertassinari I do not have any more good examples of libraries following this pattern, although I will say that when trying to develop products that are accessible and do follow s3 specs closely enough to meet WCAG compliance it is generally not enough to follow what other software is doing, as accessibility is generally an after though (if that). I will add this as a configurable option in the props and add some more tests to cover. Any preference on the name? "enableFocusForDisabledItems" is what I am currently thinking. @eps1lon could you weigh in on the default value? |
| @@ -356,71 +356,65 @@ describe('<MenuList> integration', () => { | |||
|
|
|||
| fireEvent.keyDown(document.activeElement, { key: 'ArrowUp' }); | |||
|
|
|||
| expect(menuitems[3]).to.have.focus; | |||
| expect(menuitems[2]).to.have.focus; | |||
| }); | |||
|
|
|||
| it('should stay on a single item if it is the only focusable one', () => { | |||
There was a problem hiding this comment.
To me this used to test that focus won’t move to non-focusable items that are at the end of the list. The new test tests that a single item keeps focus on arrow down.
There was a problem hiding this comment.
with the new prop i can leave this test as it was.
|
I think the default should be to allow focus on disabled items and follow the guidelines. |
|
@joshwooding @oliviertassinari is there anything else I need to do- to move this PR along? Thanks for the help. |
There was a problem hiding this comment.
Thanks for pushing the effort further. I think that we still need to explore the following:
- We need visual feedback on the focused disabled items.
- The WAI-ARIA explains the pros and cons very well (even for screen readers only). Now, If we take into account that allowing the focus on disabled items harms the experience of none screen readers users, I think that the pragmatic choice is to optimize for the many, hence keep the current behavior as default. But I would need to benchmark on Windows to get a strong point of view on this. So far, it's only based on my experience on the products I use daily (macOS, Gsuite). Feature Request: Skip keyboard navigation of disabled options JedWatson/react-select#3902 is interesting too.
- I think that the behavior should be consistent, hence taking the Autocomplete component into account here.
oliviertassinari
added
PR: needs revision
|
I guess the question is: Is the presence of this element important enough that it should be perceivable even when disabled? The answer, obviously, depends on the context. And I guess it can only be answered on the app side. So making it configurable is the best option. Reakit has a |
docs/pages/api-docs/menu-list.md
Outdated
| @@ -31,6 +31,7 @@ the focus is placed inside the component it is fully keyboard accessible. | |||
| | <span class="prop-name">autoFocusItem</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, will focus the first menuitem if `variant="menu"` or selected item if `variant="selectedMenu"` | | |||
| | <span class="prop-name">children</span> | <span class="prop-type">node</span> | | MenuList contents, normally `MenuItem`s. | | |||
| | <span class="prop-name">disableListWrap</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the menu items will not wrap focus. | | |||
| | <span class="prop-name">enableFocusForDisabledItems</span> | <span class="prop-type">bool</span> | <span class="prop-default">true</span> | If `true`, will allow focus on disabled items. | | |||
There was a problem hiding this comment.
For boolean props we want have false as the default value to enable boolean shorthands like <MenuList disableFocusForDisabledItems />. See https://material-ui.com/guides/api/#property-naming
There was a problem hiding this comment.
seems like the consensus is to have this turned off by default so I just updated the current prop default.
Thanks @oliviertassinari, |
|
It is also worth thinking that the decision of opt-in vs opt-out affects all the above listed components too. |
|
@oliviertassinari I believe i have addressed the points you raised if you have a chance can you review the PR again? |
|
...bump @oliviertassinari or @joshwooding or @eps1lon |
oliviertassinari
removed
the
PR: needs revision
github-actions
bot
added
the
PR: out-of-date
oliviertassinari
force-pushed
the
menu-list-wcag
branch
3 times, most recently
from
f6b3352 to
e94923f
Compare
oliviertassinari
force-pushed
the
menu-list-wcag
branch
from
7fdaa1e to
0c0a22e
Compare
oliviertassinari
added
PR: ready to ship
and removed
PR: out-of-date
|
@scottander Thanks |
|
No problem and thanks for the help getting this one over the finish line @oliviertassinari |
eps1lon
changed the title
Fixes bug introduced by mui#19967
Small change to make menulist inline with WCAG specifications. (https://www.w3.org/TR/wai-aria-practices/#kbd_disabled_controls)
Disabled items should not be skipped in the menu lists during keyboard navigation as a visually impaired user will not be correctly notified via screen readers.