Keyboard experience requires twice the effort in Menu

[anevins12]

Hey I'm new here,

I found that the menu which appears to the side of content takes twice as many tabs to navigate through it than it should. This is because the <li> of each menu link has a tabindex of 0, which puts it into the keyboard navigation.

May I ask why the <li> elements have this tabindex? If the reason is that they need to have focus in other scenarios then maybe we can change the 0 to -1. That removes them from the natural keyboard navigation but sitll makes them programmatically focusable.

I've attached a screenshot that outlines 3 of these links in the menu but this issue applies to all <li>s in the menu that surround link.

Happy to look into this issue after discussion.

[anevins12]

This looks like it's the default config coming form Material-UI Core:

ButtonBase.defaultProps = {
  centerRipple: false,
  component: 'button',
  disableRipple: false,
  disableTouchRipple: false,
  focusRipple: false,
  tabIndex: '0',
  type: 'button'
};

_drupal-admin-ui/nodemodules/@material-ui/core/ButtonBase/ButtonBase.js

Looks like we can override that.

[anevins12]

Note to self: Try to stop the <li> from rendering the button component: https://github.com/jsdrupal/drupal-admin-ui/blob/master/packages/admin-ui/src/components/06_wrappers/Default/Default.js

[anevins12]

Okay I kinda get why the Button component was used on the <li>. It's for styling purposes. Looks like we'll have to add some more CSS to the <li> instead - Expect CSS on the PR.

[anevins12]

As we'll be moving away from the design and CSS of Material UI in future, I'm intentionally leaving the CSS out of this Merge Request.

[martinfrances107]

@anevins12

Thank you for pointing this out ...

When I looked at your PR and saw that you removed the button property... it triggered something in the back of my subconscious.

That code is brittle .. well it is difficult in the context of material-ui to get the components into a satisfactory form.

I expect that as you are using a library that all the 'ally' issues would be taken into consideration and that all best practise would be baked into the patterns by default.

This is not the case and it is just finicky to get right.

You would mind if I expanded the drive behind your issue to fix a related problem?

If you run

REACT_APP_AXE=true yarn workspace @drupal/admin-ui start

and look at the console -- I see the return of a old screen reader issue. The button have to be semantically ensconced in a list structure so that the button will be correctly read aloud as a list

when I look at the ally report in the console I see that list is polluted by invalid elements

serious: <ul> and <ol> must only directly contain <li>, <script> or <template> elements https://dequeuniversity.com/rules/axe/3.2/list?application=axeAPI
index.js:72

I want to find time next week to sort this out. So I will try and add to your PR soon.

[anevins12]

That code is brittle .. well it is difficult in the context of material-ui to get the components into a satisfactory form.

Do you mean interaction wise? I think that's a real issue but it should be tackled on the Anchor tag level. We should not be repurposing other HTML for visual appearance.

I expect that as you are using a library that all the 'ally' issues would be taken into consideration and that all best practise would be baked into the patterns by default.

You mean us using Material UI? Actually yes Material UI does resolve this already, because they provide a pattern for a Button element. The problem is not in Material UI. It's an issue in our implemention of the Button, as we're using it for a <li>.

You would mind if I expanded the drive behind your issue to fix a related problem?

We are introducing unnecessary tabindex to the DOM. If you were to tab through the menu, you now have to exert twice the effort to get through the navigaiton because we have both the interactive <li> and the descendant <a> for each menu item.

and look at the console -- I see the return of a old screen reader issue. The button have to be semantically ensconced in a list structure so that the button will be correctly read aloud as a list

I don't have so much of an issue with that, but it's not predictable. List items are really good at being list items and provide real value to people using assistive technologies. It is very useful to know how many list items are in the menu. If you repurpose the list as something else, you lose that useful information.

The WordPress community have implemented <li> elements as buttons and have fallen short of accessibility requirements: https://core.trac.wordpress.org/ticket/47148

Let's not repeat this mistake and have to reverse engineer it afterwards.

[anevins12]

when I look at the ally report in the console I see that list is polluted by invalid elements

Interesting, this seems to be because the role of button takes over and AXE no longer views the list item as an <li>. Maybe this is what happens on the screen reader level too so it's probably expected behaviour. This error goes away when you remove the button repurpose.

[martinfrances107]

Thanks for the conversation ... valuable points .. I am not ignoring this ... I am just thinking about the next steps...

Many people see the future as bespoke components tailored for drupal's need - outside the material-ui framework. While I have only spoken about material-ui problems. I do find it a useful thing to extend. I am not expressing a opinion - I am just compiling a list of constraints.

Maybe a good judgement call would be to proceed with this PR -- and this fix the ally issues later in the development cycle. I would be ok with that.