Navigation block: Do not toggle aria-expanded on hover when the overlay menu is opened #52170
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
luisherranz
added
[Focus] Accessibility (a11y)
luisherranz
commented
Jun 30, 2023
| @@ -103,7 +103,7 @@ function block_core_navigation_add_directives_to_submenu( $w, $block_attributes | |||
| ) ) { | |||
| // Add directives to the parent `<li>`. | |||
| $w->set_attribute( 'data-wp-interactive', true ); | |||
| $w->set_attribute( 'data-wp-context', '{ "core": { "navigation": { "isMenuOpen": { "click": false, "hover": false }, "overlay": false } } }' ); | |||
| $w->set_attribute( 'data-wp-context', '{ "core": { "navigation": { "submenuOpenedBy": {}, "type": "submenu" } } }' ); | |||
There was a problem hiding this comment.
The latest version of deepsignal includes an ownKey trap, so we don't need to initialize "click" and "hover" anymore.
Comment on lines
-739
to
+743
| '<button aria-haspopup="true" %3$s class="%6$s" data-micromodal-trigger="%1$s" %11$s>%9$s</button> | ||
| '<button aria-haspopup="true" %3$s class="%6$s" %11$s>%9$s</button> | ||
| <div class="%5$s" style="%7$s" id="%1$s" %12$s> | ||
| <div class="wp-block-navigation__responsive-close" tabindex="-1" data-micromodal-close> | ||
| <div class="wp-block-navigation__responsive-close" tabindex="-1"> | ||
| <div class="wp-block-navigation__responsive-dialog" aria-label="%8$s" %13$s> | ||
| <button %4$s data-micromodal-close class="wp-block-navigation__responsive-container-close" %14$s>%10$s</button> | ||
| <button %4$s class="wp-block-navigation__responsive-container-close" %14$s>%10$s</button> |
There was a problem hiding this comment.
I removed some micromodal leftovers.
There was a problem hiding this comment.
Those attributes were added back in this pull request because in WordPress 6.3, without Gutenberg installed, we decided to keep the old implementation, which needs them. I assume that, as we intend to include it in the next release 🤞 , should we revert the whole pull request?
There was a problem hiding this comment.
Oh yeah, we're pass the 6.3 feature freeze, so feel free to remove anything from the old implementation that is not needed anymore.
Comment on lines
-95
to
+122
| closeMenuOnHover( args ) { | ||
| closeMenu( args, 'hover' ); | ||
| closeMenuOnHover( store ) { | ||
| closeMenu( store, 'hover' ); | ||
| }, | ||
| openMenuOnClick( args ) { | ||
| openMenu( args, 'click' ); | ||
| openMenuOnClick( store ) { | ||
| openMenu( store, 'click' ); | ||
| }, | ||
| closeMenuOnClick( args ) { | ||
| closeMenu( args, 'click' ); | ||
| closeMenuOnClick( store ) { | ||
| closeMenu( store, 'click' ); |
There was a problem hiding this comment.
I renamed args to store everywhere because it feels more natural.
Comment on lines
-48
to
+46
| store( { | ||
| wpStore( { |
There was a problem hiding this comment.
I renamed this to wpStore to be able to use store instead of args in the functions.
| // Check if the menu is still open or not. | ||
| if ( ! selectors.core.navigation.isMenuOpen( { context } ) ) { | ||
| if ( ! selectors.core.navigation.isMenuOpen( store ) ) { |
There was a problem hiding this comment.
I started passing store instead of {context} to the selector. I think it's safer to do it that way by default.
| if ( | ||
| context.core.navigation.overlay && | ||
| ( event.key === 'Tab' || event.keyCode === 9 ) |
There was a problem hiding this comment.
I removed the keyCodes because they are deprecated and not needed in modern browsers: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode
Comment on lines
-8
to
-15
| 'area[href]', | ||
| 'input:not([disabled]):not([type="hidden"]):not([aria-hidden])', | ||
| 'select:not([disabled]):not([aria-hidden])', | ||
| 'textarea:not([disabled]):not([aria-hidden])', | ||
| 'button:not([disabled]):not([aria-hidden])', | ||
| 'iframe', | ||
| 'object', | ||
| 'embed', |
There was a problem hiding this comment.
I removed the elements that are unnecessary for the menu. If in the future we need them, we can add them back.
There was a problem hiding this comment.
If I remember correctly, we added all the focusable elements in case other blocks, like the image block, copy and paste that logic and they need it in this case. Maybe we can abstract it somehow (in the interactivity package?) if we see this is a common use case?
Anyway, right now it makes sense to keep only the necessary ones 👍
Comment on lines
+108
to
+111
| // Only open on hover if the overlay is closed. | ||
| Object.values( | ||
| navigation.overlayOpenedBy || {} | ||
| ).filter( Boolean ).length === 0 |
There was a problem hiding this comment.
This is the main change of this PR, where we check if the parent overlay is opened or not.
There was a problem hiding this comment.
Would it be safe to include a global variable to check if the overlay is opened? I can imagine other use cases where it could be handy to check store.state.core.navigation.overlayMenuOpened, or something similar.
There was a problem hiding this comment.
Sure, but I would only add it when there's a real need for it 🙂
|
Size Change: +187 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 3f0530a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5422620048
|
SantosGuillamot
approved these changes
Jul 5, 2023
There was a problem hiding this comment.
I've tested and it seems to be working as you describe 🙂 Apart from some minor comments I left, I have one question:
What should be the value of aria-expanded when the overlay menu is opened? Right now, it is false, but maybe it shouldn't exist at all? I don't know that much about accessibility but, if the submenu is always opened, it seems weird to me to have an aria-expanded="false" attribute (although I believe it is how it works with the previous implementation).
| ? 'dialog' | ||
| : '', | ||
| : ''; | ||
| }, | ||
| isMenuOpen: ( { context } ) => |
There was a problem hiding this comment.
Shouldn't we use store here for consistency?
There was a problem hiding this comment.
Oh, I'm using store only when the function is using at least one selector 🙂
| : 'submenuOpenedBy' | ||
| ] | ||
| ).filter( Boolean ).length > 0, | ||
| menuOpenBy: ( { context } ) => |
There was a problem hiding this comment.
Same as above: Shouldn't we use store here for consistency?
I don't know, maybe. But I think we can merge this PR, and if it turns out that it's better to remove it, do it in another PR. |
|
Thanks, Mario! I'm going to merge this now 🙂 |
luisherranz
deleted the
fix/navigation-block-remove-aria-expanded-on-overlay
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Do not toggle
aria-expandedon hover of the submenus when the overlay menu is opened.Why?
Because the submenus are expanded by defualt when the overlay menu is opened.
How?
I divided
isMenuOpeninto two different properties (overlayOpenedByandsubmenuOpenedBy) so it's possible to know if the parent overlay is opened or not in the children submenu. I also changed theoverlayboolean totype = "overlay"ortype = "submenu"to make it more explicit.I also updated
deepsignaland removed the leftovers in theblock-library'spackage.json.Testing Instructions
aria-expandedattribute of the submenus turnstrueon hoveraria-expandedattribute of the submenus doesn't turntrueon hoverScreenshots or screencast
Before
Screen.Capture.on.2023-06-30.at.13-51-49.mp4
After
Screen.Capture.on.2023-06-30.at.13-49-01.mp4