Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Navigation block: Do not toggle aria-expanded on hover when the overlay menu is opened #52170

Merged
merged 4 commits into from
Jul 5, 2023
Merged

Navigation block: Do not toggle aria-expanded on hover when the overlay menu is opened #52170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fb6d4fd
Fix aria expanded when overlay is open
luisherranz Jun 30, 2023
9578e46
Update deepsignal and remove old packages
luisherranz Jun 30, 2023
b5f06f2
Remove unnecessary initial context
luisherranz Jun 30, 2023
3f0530a
Rename OpenBy to OpenedBy
luisherranz Jun 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 16 additions & 18 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 0 additions & 4 deletions packages/block-library/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
],
"dependencies": {
"@babel/runtime": "^7.16.0",
"@preact/signals": "^1.1.3",
"@wordpress/a11y": "file:../a11y",
"@wordpress/api-fetch": "file:../api-fetch",
"@wordpress/autop": "file:../autop",
Expand Down Expand Up @@ -65,13 +64,10 @@
"change-case": "^4.1.2",
"classnames": "^2.3.1",
"colord": "^2.7.0",
"deepsignal": "^1.3.0",
"escape-html": "^1.0.3",
"fast-average-color": "^9.1.1",
"fast-deep-equal": "^3.1.3",
"memize": "^2.1.0",
"micromodal": "^0.4.10",
"preact": "^10.13.2",
"remove-accents": "^0.4.2",
"uuid": "^8.3.0"
},
Expand Down
10 changes: 5 additions & 5 deletions packages/block-library/src/navigation/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" } } }' );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest version of deepsignal includes an ownKey trap, so we don't need to initialize "click" and "hover" anymore.

$w->set_attribute( 'data-wp-effect', 'effects.core.navigation.initMenu' );
$w->set_attribute( 'data-wp-on--focusout', 'actions.core.navigation.handleMenuFocusout' );
$w->set_attribute( 'data-wp-on--keydown', 'actions.core.navigation.handleMenuKeydown' );
Expand Down Expand Up @@ -711,7 +711,7 @@ function render_block_core_navigation( $attributes, $content, $block ) {
if ( defined( 'IS_GUTENBERG_PLUGIN' ) && IS_GUTENBERG_PLUGIN && $should_load_view_script ) {
$nav_element_directives = '
data-wp-interactive
data-wp-context=\'{ "core": { "navigation": { "isMenuOpen": { "click": false, "hover": false }, "overlay": true, "roleAttribute": "" } } }\'
data-wp-context=\'{ "core": { "navigation": { "overlayOpenedBy": {}, "type": "overlay", "roleAttribute": "" } } }\'
';
$open_button_directives = '
data-wp-on--click="actions.core.navigation.openMenuOnClick"
Expand All @@ -736,11 +736,11 @@ function render_block_core_navigation( $attributes, $content, $block ) {
}

$responsive_container_markup = sprintf(
'<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>
Comment on lines -739 to +743
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed some micromodal leftovers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<div class="wp-block-navigation__responsive-container-content" id="%1$s-content">
%2$s
</div>
Expand Down
129 changes: 73 additions & 56 deletions packages/block-library/src/navigation/view.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,33 @@
/**
* WordPress dependencies
*/
import { store } from '@wordpress/interactivity';
import { store as wpStore } from '@wordpress/interactivity';

const focusableSelectors = [
'a[href]',
'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',
Comment on lines -8 to -15
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the elements that are unnecessary for the menu. If in the future we need them, we can add them back.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

'[contenteditable]',
'[tabindex]:not([tabindex^="-"])',
];

const openMenu = ( { context, ref }, menuOpenedOn ) => {
context.core.navigation.isMenuOpen[ menuOpenedOn ] = true;
const openMenu = ( store, menuOpenedOn ) => {
const { context, ref, selectors } = store;
selectors.core.navigation.menuOpenBy( store )[ menuOpenedOn ] = true;
context.core.navigation.previousFocus = ref;
if ( context.core.navigation.overlay ) {
if ( context.core.navigation.type === 'overlay' ) {
// Add a `has-modal-open` class to the <html> root.
document.documentElement.classList.add( 'has-modal-open' );
}
};

const closeMenu = ( { context, selectors }, menuClosedOn ) => {
context.core.navigation.isMenuOpen[ menuClosedOn ] = false;
const closeMenu = ( store, menuClosedOn ) => {
const { context, selectors } = store;
selectors.core.navigation.menuOpenBy( store )[ menuClosedOn ] = false;
// Check if the menu is still open or not.
if ( ! selectors.core.navigation.isMenuOpen( { context } ) ) {
if ( ! selectors.core.navigation.isMenuOpen( store ) ) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.modal?.contains(
window.document.activeElement
Expand All @@ -39,18 +37,19 @@ const closeMenu = ( { context, selectors }, menuClosedOn ) => {
}
context.core.navigation.modal = null;
context.core.navigation.previousFocus = null;
if ( context.core.navigation.overlay ) {
if ( context.core.navigation.type === 'overlay' ) {
document.documentElement.classList.remove( 'has-modal-open' );
}
}
};

store( {
wpStore( {
Comment on lines -48 to +46
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed this to wpStore to be able to use store instead of args in the functions.

effects: {
core: {
navigation: {
initMenu: ( { context, selectors, ref } ) => {
if ( selectors.core.navigation.isMenuOpen( { context } ) ) {
initMenu: ( store ) => {
const { context, selectors, ref } = store;
if ( selectors.core.navigation.isMenuOpen( store ) ) {
const focusableElements =
ref.querySelectorAll( focusableSelectors );
context.core.navigation.modal = ref;
Expand All @@ -60,8 +59,9 @@ store( {
focusableElements[ focusableElements.length - 1 ];
}
},
focusFirstElement: ( { context, selectors, ref } ) => {
if ( selectors.core.navigation.isMenuOpen( { context } ) ) {
focusFirstElement: ( store ) => {
const { selectors, ref } = store;
if ( selectors.core.navigation.isMenuOpen( store ) ) {
ref.querySelector(
'.wp-block-navigation-item > *:first-child'
).focus();
Expand All @@ -73,60 +73,77 @@ store( {
selectors: {
core: {
navigation: {
roleAttribute: ( { context, selectors } ) =>
context.core.navigation.overlay &&
selectors.core.navigation.isMenuOpen( { context } )
roleAttribute: ( store ) => {
const { context, selectors } = store;
return context.core.navigation.type === 'overlay' &&
selectors.core.navigation.isMenuOpen( store )
? 'dialog'
: '',
: '';
},
isMenuOpen: ( { context } ) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use store here for consistency?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I'm using store only when the function is using at least one selector 🙂

// The menu is opened if either `click` or `hover` is true.
Object.values( context.core.navigation.isMenuOpen ).filter(
Boolean
).length > 0,
// The menu is opened if either `click`, `hover` or `focus` is true.
Object.values(
context.core.navigation[
context.core.navigation.type === 'overlay'
? 'overlayOpenedBy'
: 'submenuOpenedBy'
]
).filter( Boolean ).length > 0,
menuOpenBy: ( { context } ) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above: Shouldn't we use store here for consistency?

context.core.navigation[
context.core.navigation.type === 'overlay'
? 'overlayOpenedBy'
: 'submenuOpenedBy'
],
},
},
},
actions: {
core: {
navigation: {
openMenuOnHover( args ) {
openMenu( args, 'hover' );
openMenuOnHover( store ) {
const { navigation } = store.context.core;
if (
navigation.type === 'submenu' &&
// Only open on hover if the overlay is closed.
Object.values(
navigation.overlayOpenedBy || {}
).filter( Boolean ).length === 0
Comment on lines +108 to +111
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main change of this PR, where we check if the parent overlay is opened or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but I would only add it when there's a real need for it 🙂

)
openMenu( store, 'hover' );
},
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' );
Comment on lines -95 to +122
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed args to store everywhere because it feels more natural.

},
toggleMenuOnClick: ( args ) => {
const { context } = args;
if ( context.core.navigation.isMenuOpen.click ) {
closeMenu( args, 'click' );
toggleMenuOnClick: ( store ) => {
const { selectors } = store;
if ( selectors.core.navigation.menuOpenBy( store ).click ) {
closeMenu( store, 'click' );
} else {
openMenu( args, 'click' );
openMenu( store, 'click' );
}
},
handleMenuKeydown: ( args ) => {
const { context, event } = args;
if ( context.core.navigation.isMenuOpen.click ) {
// If Escape close the menu
if (
event?.key === 'Escape' ||
event?.keyCode === 27
) {
closeMenu( args, 'click' );
handleMenuKeydown: ( store ) => {
const { context, selectors, event } = store;
if ( selectors.core.navigation.menuOpenBy( store ).click ) {
// If Escape close the menu.
if ( event?.key === 'Escape' ) {
closeMenu( store, 'click' );
return;
}

// Trap focus if it is an overlay (main menu)
// Trap focus if it is an overlay (main menu).
if (
context.core.navigation.overlay &&
( event.key === 'Tab' || event.keyCode === 9 )
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

context.core.navigation.type === 'overlay' &&
event.key === 'Tab'
) {
// If shift + tab it change the direction
// If shift + tab it change the direction.
if (
event.shiftKey &&
window.document.activeElement ===
Expand All @@ -146,21 +163,21 @@ store( {
}
}
},
handleMenuFocusout: ( args ) => {
const { context, event } = args;
handleMenuFocusout: ( store ) => {
const { context, selectors, event } = store;
// If focus is outside modal, and in the document, close menu
// event.target === The element losing focus
// event.relatedTarget === The element receiving focus (if any)
// When focusout is outsite the document,
// `window.document.activeElement` doesn't change
// `window.document.activeElement` doesn't change.
if (
context.core.navigation.isMenuOpen.click &&
selectors.core.navigation.menuOpenBy( store ).click &&
! context.core.navigation.modal?.contains(
event.relatedTarget
) &&
event.target !== window.document.activeElement
) {
closeMenu( args, 'click' );
closeMenu( store, 'click' );
}
},
},
Expand Down
2 changes: 1 addition & 1 deletion packages/interactivity/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
"react-native": "src/index",
"dependencies": {
"@preact/signals": "^1.1.3",
"deepsignal": "^1.3.0",
"deepsignal": "^1.3.3",
"preact": "^10.13.2"
},
"publishConfig": {
Expand Down