Navigation block: Set correct aria-expanded on hover #50953
Conversation
luisherranz
added
[Focus] Accessibility (a11y)
luisherranz
requested review from
spacedmonkey and
tellthemachines
as code owners
luisherranz
added
the
Backport to Gutenberg RC
|
Size Change: -17.9 kB (-1%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 07b41f9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5120886073
|
|
Thanks a lot for the pull request, Luis 🙂 I have some comments that I believe should be addressed (video at the end of the comment with a detailed explanation):
Video explaining it in more detail: https://www.loom.com/share/4a551b225738429997a611bc2237403b |
|
Cool. Thanks Mario, those make sense. I'll make the corrections as soon as I can 🙂 |
|
This should be ready for review. I made a video to explain how the new 
|
There was a problem hiding this comment.
I tested it, and everything seems to be working great. Really interesting implementation, thanks for taking care of it 🙂
I just left a small comment on the roleAttribute selector.
Apart from that, when both "Open on click" and "Show arrow" settings are off, the submenu is also opened on hover and we are not adding the aria-expanded attribute. On the other hand, there is no button with those settings, so I am not sure if it is needed. Additionally, I'd like to explore the possibility of not sending the JavaScript files in this particular case because they are not needed (apart from the aria-expanded on hover which I am not sure).
That's a good question. I don't know either. Should cc: @jeryj @alexstine
Sure. We can explore that in a subsequent PR. If the |
Sure, I was just mentioning that because, if "Open on click" and "Show arrow" are disabled, we would be sending the JavaScript files just to add the |
luisherranz
removed
the
Backport to Gutenberg RC
|
@luisherranz I am not sure what you mean by your question above. Please try to help me understand? If there are no submenus, then JS should not be required as toggling attributes will not be required. Do keep in mind, on smaller viewports, there is likely still a submenu trigger that will need |
|
When the option "Show arrows" is set to But when the option "Show arrows" is set to The question is: is this correct? Or should we add the @SantosGuillamot feel free to correct me if I didn't get this correctly. |
|
@luisherranz If show arrows is off, is it possible to still have submenus? If yes, the trigger button should always remain as that's how screen readers open the submenus. |
As of today, yes. You can choose:
When those options are chosen, the submenus open on mouse hover or on focus. There is no Interestingly, that option is not only bad for accessibility but also for mobile because there is no way to hover without navigating to the parent link. That's usually not a problem because people usually have the overlay switched on in mobile, but it's a problem if you have it turned off or for tablets. The funny thing is that "Show arrows" is always forced to be From my limited knowledge, it looks like it should be the other way around: "Show arrows" should be forced to be |
|
It seems like the logic of not forcing arrows (and therefore missing the This is the original PR from @tellthemachines. It has some conversations about accessibility. @tellthemachines, would you mind letting us know your opinion about this? Thanks 🙂 |
|
@luisherranz Yeah, we should always have buttons to open the submenu. Hover or focus on the parent is not enough. I am not sure what was discussed back then but it needs to change now. Even wordpress.org itself was changed to always have submenu triggers as I complained about it here. WordPress/wporg-mu-plugins#207 Allowing users this much control is damaging. Thanks. |
The PR description includes this statement:
|
|
Hey folks, as this is a somewhat unrelated and merged PR, I've opened a new issue to continue this conversation: |
* Set correct aria-expanded on hover * Store both `click` and `hover` in `isMenuOpen` * Fix nav menu * Remove menuOpenedOn debugger * Add comments and fix example HTML * Fix PHP lint * Just in case openSubmenusOnClick exists but it's false * Switch to isMenuOpen selector in roleAttribute
What?
It sets the correct
aria-expandedfor the button of the submenus when the mouse is over it.Why?
Because previously it was only working for the keyboard, but as stated by @jeryj in Slack, some users may use both a screen reader and a mouse.
How?
I added
mouseoverandmouseoutdirectives to the<li>element.I also created a new
contextproperty calledisHoveringto indicate that the mouse is over the submenu (lielement), because in that case, theisMenuOpenproperty should not turnfalseif the user clicks the button, uses the keyboard, or moves the focus away.Testing Instructions
You can use WordPress Playground to test this out: https://playground.wordpress.net/?gutenberg-pr=50953
enterto "toggle the menu", the state of expanded should remain (because the mouse is hovering and prevents it from closing)Screenshots or screencast
hlAcWl.mp4