Only mark nav menu as dialog when open #36617
Conversation
vcanales
force-pushed
the
fix/nav-menu-should-not-always-be-dialog
branch
from
54a0377 to
09ffa07
Compare
|
Size Change: +145 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
vcanales
force-pushed
the
fix/nav-menu-should-not-always-be-dialog
branch
from
332af9a to
348f33e
Compare
vcanales
added
[Block] Navigation
|
Thanks for working on this. For now this PR appears to fix the issue where the navigation menu reports itself as being a dialog, even when it isn't. This happens when, for example, the navigation is set to collapse into a button to open a dialog on mobile, but show the fully expanded menu on desktop. |
vcanales
force-pushed
the
fix/nav-menu-should-not-always-be-dialog
branch
from
348f33e to
79b4156
Compare
|
@vcanales Thanks for the PR. This is almost working perfectly, just a couple final suggestions.
This really doesn't need aria-expanded because focus is being placed in a dialog and the content outside of the dialog is non-interactive. Also, the label needs to be updated that way users know what this button does.
For the Close button:
Let's update that label and remove aria-expanded because this button simply closes the dialog. It doesn't in theory collapse anything. This is a dialog, not an accordion/tabs module/similar.
I am okay with keeping aria-haspopup because it can be used with dialog triggers.
Also, it seems there is an unused aria-labelledby attribute on the dialog div.
Can you add some description text such as a heading 1 right after the Close button? E.g.
The other option would be removing aria-labelledby and replacing it with aria-label.
The whole point of implementing a label is to add context for screen readers. Otherwise, when the dialog opens, users hear "dialog" instead of "Menu dialog" which adds context. Can these be implemented? Thanks. |
|
Thank you so much for the exceptionally detailed and highly actionable feedback, Alex. |
vcanales
changed the title
|
@alexstine Thanks for your thorough review! I've addressed your concerns, and will be following up with another PR tackling some issues that the Open on Click Submenus seem to have when the overlay is open (aria-expanded can be toggled, but they just remain open). I would appreciate one more review, and approval. |
tellthemachines
added
the
Backport to WP 6.7 Beta/RC
|
@vcanales It seems like aria-expanded is now gone up until you click Open navigation menu. Then it switches to This alternate PR is working on submenus, it is possible this issue may be fixed? #36631 Thanks. |
@alexstine if you're referring to the the issue with the open on click submenus @vcanales mentioned, that isn't fixed in #36631. Ideally, to fix that we should not only remove |
There was a problem hiding this comment.
Thanks for working on this! Just a few comments and questions below.
Edit: when testing locally, the positioning of the URL input for nav link blocks is at the top left of the page, instead of right by the block:
This happens both in the site and the post editor. There's also a warning in the console: Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method. at URLInput. Not sure if this is related to the changes in this branch or not; rebasing trunk might help if not.
| const dialogProps = { | ||
| className: 'wp-block-navigation__responsive-dialog', | ||
| 'aria-labelledby': `${ modalId }-title`, | ||
| ...( isOpen && { role: 'dialog', 'aria-modal': true } ), |
There was a problem hiding this comment.
If we're adding these attributes dynamically only when the modal is open, do we still need the separate set of markup for when the nav is not in overlay mode? For the front end, could we toggle them in the view script instead?
|
|
||
| &:not(.always-shown) { | ||
| @include break-small { | ||
| display: block; |
There was a problem hiding this comment.
If we want the flex layout settings to reflect correctly in the nav contents, this class has to duplicate the responsive container content styles here.
|
@tellthemachines I'm okay with the submenus keeping aria-expanded if they can be expanded/collapsed (open/closed). I just don't think it's needed on the main menu trigger as that opens in a dialog. Sections inside a dialog is technically a toggle, so aria-expanded is okay. |
vcanales
force-pushed
the
fix/nav-menu-should-not-always-be-dialog
branch
from
1cebb1e to
d596aed
Compare
|
I just left a comment on #36600 (comment), that I also noticed an issue with the dialog + VoiceOver. This PR seems to fix that issue too (thought I haven't tested any edge cases). I noticed this broke the horizontal styling - my menu has a Page List, a Submenu, and a Custom Link, and before the PR they all line up on a single line, but after, the Page List is on its own line.
|
This is likely caused by the new container being unstyled. |
|
I tried to address the feedback in #36617 (comment) and https://github.com/WordPress/gutenberg/pull/36617/files#r755734687 (thank you @tellthemachines). It isn't completely clear to me how best to test this, but I think I got it right. I would appreciate a sanity check, so we can land this one. |
| <div class="%5$s" style="%7$s" id="modal-%1$s"> | ||
| <div class="wp-block-navigation__responsive-close" tabindex="-1" data-micromodal-close> | ||
| <div class="wp-block-navigation__responsive-dialog" role="dialog" aria-modal="true" aria-labelledby="modal-%1$s-title" > | ||
| <div class="wp-block-navigation__responsive-dialog" role="dialog" aria-modal="true" aria-label="Navigation Menu" > |
There was a problem hiding this comment.
This text change is missing the translation function.
|
Thanks @jasmussen ! The alignment is much better, but I think it still needs I'm still seeing the issue with the URL input rendering at the top left of the screen, and when that happens the block toolbar also disappears (only if it's inline, if it's set to top toolbar it still shows). I'm wondering if that might be due to the two sets of markup? |
Redundant as screen readers such as NVDA append "Menu" to these buttons.
|
I'll take a look. One challenge I'm seeing at the moment is that in this PR I can no longer select menu items. Trunk works: In this branch, the block toolbar doesn't appear: I suspect it's the added wrapper that's intercepting a pointer event. I'll push a rebase, then see what I can do. |
jasmussen
force-pushed
the
fix/nav-menu-should-not-always-be-dialog
branch
from
7bfa903 to
a5f76f5
Compare
|
Yeah, I'd like to try adding the dialog dynamically in the front end because if it works, we won't need the two sets of markup. Hopefully I have some time to look into it tomorrow. |
|
The selection issue is a bit gnarly, and I don't think I can fix that in CSS only: It does seem like the best solution might be a different approach to this one, which minimizes the markup changes in the editor. I'd love to help in any way I can here. |
|
Ok, I created #37434 as an alternative without markup changes (apart from adding/removing attributes, of course). Feedback and testing appreciated! Ideally we should get this in before Beta 4 early next week. |
|
Should we remove this from WP 5.9 Must-Haves project now that #37434 is merged? |
|
Yup, this can be closed now. Thanks everyone for your collaboration! |
tellthemachines
removed
the
Backport to WP 6.7 Beta/RC
priethor
added
the
[Focus] Accessibility (a11y)
Description
This PR addresses part of the problem described in #36600, where an incorrectly labeled menu was being identified as a dialog, when in fact it was an unopened menu. This caused screen readers such as NVDA to read out the word "dialog" in places where it shouldn't have.
I would really appreciate feedback on how we can further address the problems described on the issue.
How has this been tested?
Navigating through a Navigation Menu which is not collapsed reads:
"Navigation landmark with N items", and then reads the text for the first link on the list.
Submenu arrows read "Button Collapsed", pressing this would open a submenu. This can be improved by more accurately describing the button with something like "Button, Collapsed Submenu".
Navigating to a button to open the Overlay Menu reads:
"Navigation landmark, open menu button, collapsed submenu".
After pressing enter on this button, screen reader reads:
"Expanded, Dialog, List, List with N items", and then it reads the text on each item.
The menu can be navigated using the Tab key.