Fix accessibility issues navigation block experiment (#50786)

* Add toggle functionality to submenu buttons

* Add support for true/false enumerated attributes

* Move `focusout` to the parent `<li>`

* Follow preact logic on enumerated attributes

* Add priorities to directives

* Move `handleMenuKeydown` to parent element

* Fix coding standards

lib/experimental/interactivity-api/blocks.php

@@ -164,22 +164,18 @@ function gutenberg_block_core_navigation_add_directives_to_markup( $block_conten
  * <li
  *   class="has-child"
  *   data-wp-context='{ "core": { "navigation": { "isMenuOpen": false, "overlay": false } } }'
+ *   data-wp-effect="effects.core.navigation.initMenu"
+ *   data-wp-on.keydown="actions.core.navigation.handleMenuKeydown"
+ *   data-wp-on.focusout="actions.core.navigation.handleMenuFocusout"
  * >
  *   <button
  *     class="wp-block-navigation-submenu__toggle"
  *     data-wp-on.click="actions.core.navigation.openMenu"
  *     data-wp-bind.aria-expanded="context.core.navigation.isMenuOpen"
- *     data-wp-on.keydown="actions.core.navigation.handleMenuKeydown"
- *     data-wp-on.focusout="actions.core.navigation.handleMenuFocusout"
  *   >
  *   </button>
  *   <span>Title</span>
- *   <ul
- *     class="wp-block-navigation__submenu-container"
- *     data-wp-effect="effects.core.navigation.initMenu"
- *     data-wp-on.focusout="actions.core.navigation.handleMenuFocusout"
- *     data-wp-on.keydown="actions.core.navigation.handleMenuKeydown"
- *   >
+ *   <ul class="wp-block-navigation__submenu-container">
  *     SUBMENU ITEMS
  *   </ul>
  * </li>
@@ -197,6 +193,9 @@ function gutenberg_block_core_navigation_add_directives_to_submenu( $w ) {
 	) ) {
 		// Add directives to the parent `<li>`.
 		$w->set_attribute( 'data-wp-context', '{ "core": { "navigation": { "isMenuOpen": false, "overlay": false } } }' );
+		$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' );
 
 		// Add directives to the toggle submenu button.
 		if ( $w->next_tag(
@@ -205,23 +204,10 @@ function gutenberg_block_core_navigation_add_directives_to_submenu( $w ) {
 				'class_name' => 'wp-block-navigation-submenu__toggle',
 			)
 		) ) {
-			$w->set_attribute( 'data-wp-on.click', 'actions.core.navigation.openMenu' );
+			$w->set_attribute( 'data-wp-on.click', 'actions.core.navigation.toggleMenu' );
 			$w->set_attribute( 'data-wp-bind.aria-expanded', 'context.core.navigation.isMenuOpen' );
-			$w->set_attribute( 'data-wp-on.keydown', 'actions.core.navigation.handleMenuKeydown' );
-			$w->set_attribute( 'data-wp-on.focusout', 'actions.core.navigation.handleMenuFocusout' );
 		};
 
-		// Add directives to the `<ul>` containing the subitems.
-		if ( $w->next_tag(
-			array(
-				'tag_name'   => 'UL',
-				'class_name' => 'wp-block-navigation__submenu-container',
-			)
-		) ) {
-			$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' );
-		};
 		// Iterate through subitems if exist.
 		gutenberg_block_core_navigation_add_directives_to_submenu( $w );
 	}

packages/block-library/src/navigation/interactivity.js

@@ -86,6 +86,13 @@ store( {
 						}
 					}
 				},
+				toggleMenu: ( { context, actions, ref } ) => {
+					if ( context.core.navigation.isMenuOpen ) {
+						actions.core.navigation.closeMenu( { context } );
+					} else {
+						actions.core.navigation.openMenu( { context, ref } );
+					}
+				},
 				handleMenuKeydown: ( { actions, context, event } ) => {
 					if ( context.core.navigation.isMenuOpen ) {
 						// If Escape close the menu

packages/block-library/src/utils/interactivity/directives.js

@@ -49,7 +49,8 @@ export default () => {
 			}, [ context, inheritedValue ] );
 
 			return <Provider value={ value }>{ children }</Provider>;
-		}
+		},
+		{ priority: 5 }
 	);
 
 	// data-wp-effect.[name]
@@ -140,16 +141,22 @@ export default () => {
 					} );
 					element.props[ attribute ] = result;
 
+					// This seems necessary because Preact doesn't change the attributes
+					// on the hydration, so we have to do it manually. It doesn't need
+					// deps because it only needs to do it the first time.
 					useEffect( () => {
-						// This seems necessary because Preact doesn't change the attributes
-						// on the hydration, so we have to do it manually. It doesn't need
-						// deps because it only needs to do it the first time.
-						if ( result === false ) {
+						// aria- and data- attributes have no boolean representation.
+						// A `false` value is different from the attribute not being
+						// present, so we can't remove it.
+						// We follow Preact's logic: https://github.com/preactjs/preact/blob/ea49f7a0f9d1ff2c98c0bdd66aa0cbc583055246/src/diff/props.js#L131C24-L136
+						if ( result === false && attribute[ 4 ] !== '-' ) {
 							element.ref.current.removeAttribute( attribute );
 						} else {
 							element.ref.current.setAttribute(
 								attribute,
-								result === true ? '' : result
+								result === true && attribute[ 4 ] !== '-'
+									? ''
+									: result
 							);
 						}
 					}, [] );

packages/block-library/src/utils/interactivity/hooks.js

@@ -1,7 +1,7 @@
 /**
  * External dependencies
  */
-import { h, options, createContext } from 'preact';
+import { h, options, createContext, cloneElement } from 'preact';
 import { useRef, useMemo } from 'preact/hooks';
 /**
  * Internal dependencies
@@ -13,8 +13,10 @@ const context = createContext( {} );
 
 // WordPress Directives.
 const directiveMap = {};
-export const directive = ( name, cb ) => {
+const directivePriorities = {};
+export const directive = ( name, cb, { priority = 10 } = {} ) => {
 	directiveMap[ name ] = cb;
+	directivePriorities[ name ] = priority;
 };
 
 // Resolve the path to some property of the store object.
@@ -43,12 +45,77 @@ const getEvaluate =
 		return hasNegationOperator ? ! returnValue : returnValue;
 	};
 
+// Separate directives by priority. The resulting array contains objects
+// of directives grouped by same priority, and sorted in ascending order.
+const usePriorityLevels = ( directives ) =>
+	useMemo( () => {
+		const byPriority = Object.entries( directives ).reduce(
+			( acc, [ name, values ] ) => {
+				const priority = directivePriorities[ name ];
+				if ( ! acc[ priority ] ) acc[ priority ] = {};
+				acc[ priority ][ name ] = values;
+
+				return acc;
+			},
+			{}
+		);
+
+		return Object.entries( byPriority )
+			.sort( ( [ p1 ], [ p2 ] ) => p1 - p2 )
+			.map( ( [ , obj ] ) => obj );
+	}, [ directives ] );
+
 // Directive wrapper.
 const Directive = ( { type, directives, props: originalProps } ) => {
 	const ref = useRef( null );
 	const element = h( type, { ...originalProps, ref } );
-	const props = { ...originalProps, children: element };
 	const evaluate = useMemo( () => getEvaluate( { ref } ), [] );
+
+	// Add wrappers recursively for each priority level.
+	const byPriorityLevel = usePriorityLevels( directives );
+	return (
+		<RecursivePriorityLevel
+			directives={ byPriorityLevel }
+			element={ element }
+			evaluate={ evaluate }
+			originalProps={ originalProps }
+		/>
+	);
+};
+
+// Priority level wrapper.
+const RecursivePriorityLevel = ( {
+	directives: [ directives, ...rest ],
+	element,
+	evaluate,
+	originalProps,
+} ) => {
+	// This element needs to be a fresh copy so we are not modifying an already
+	// rendered element with Preact's internal properties initialized. This
+	// prevents an error with changes in `element.props.children` not being
+	// reflected in `element.__k`.
+	element = cloneElement( element );
+
+	// Recursively render the wrapper for the next priority level.
+	//
+	// Note that, even though we're instantiating a vnode with a
+	// `RecursivePriorityLevel` here, its render function will not be executed
+	// just yet. Actually, it will be delayed until the current render function
+	// has finished. That ensures directives in the current priorty level have
+	// run (and thus modified the passed `element`) before the next level.
+	const children =
+		rest.length > 0 ? (
+			<RecursivePriorityLevel
+				directives={ rest }
+				element={ element }
+				evaluate={ evaluate }
+				originalProps={ originalProps }
+			/>
+		) : (
+			element
+		);
+
+	const props = { ...originalProps, children };
 	const directiveArgs = { directives, props, element, context, evaluate };
 
 	for ( const d in directives ) {