Merge pull request #234 from OpenSourceOrg/fix/t51eng-2081-review-findings-develop

Fix accessibility and robustness findings from the branch review (develop)

Author: mlaetitia

themes/osi/assets/css/editor-style.css.map

@@ -1 +1 @@
-<?php return array('dependencies' => array(), 'version' => '0bfa516281fef59f8671');
+<?php return array('dependencies' => array(), 'version' => 'f179507f6e77a9763a06');

themes/osi/assets/js/build/theme.asset.php

@@ -29,6 +29,12 @@ if ( header && megaItems.length ) {
 		);
 	};
 
+	const dismiss = ( item ) => {
+		closeItem( item );
+		item.classList.add( 'is-dismissed' );
+		syncHeader();
+	};
+
 	const closeItem = ( item ) => {
 		item.classList.remove( 'is-open' );
 		const trigger = triggerOf( item );
@@ -45,14 +51,14 @@ if ( header && megaItems.length ) {
 	megaItems.forEach( ( item ) => {
 		const trigger = triggerOf( item );
 		let closeTimer = null;
+		let suppressOpen = false;
 
 		if ( trigger ) {
-			trigger.setAttribute( 'aria-haspopup', 'true' );
 			trigger.setAttribute( 'aria-expanded', 'false' );
 		}
 
 		const open = () => {
-			if ( ! desktopNav.matches ) {
+			if ( suppressOpen || ! desktopNav.matches ) {
 				return;
 			}
 			window.clearTimeout( closeTimer );
@@ -96,19 +102,25 @@ if ( header && megaItems.length ) {
 		}
 
 		item.addEventListener( 'mouseenter', open );
-		item.addEventListener( 'mouseleave', () => close( false ) );
+		item.addEventListener( 'mouseleave', () => {
+			item.classList.remove( 'is-dismissed' );
+			close( false );
+		} );
 		item.addEventListener( 'focusin', open );
 		item.addEventListener( 'focusout', ( event ) => {
 			if ( ! item.contains( event.relatedTarget ) ) {
 				close( false );
 			}
 		} );
+		// returning focus to the trigger fires focusin, which would reopen the panel
 		item.addEventListener( 'keydown', ( event ) => {
 			if ( 'Escape' === event.key && item.classList.contains( 'is-open' ) ) {
-				close( true );
+				suppressOpen = true;
+				dismiss( item );
 				if ( trigger ) {
 					trigger.focus();
 				}
+				suppressOpen = false;
 			}
 		} );
 	} );
@@ -122,6 +134,16 @@ if ( header && megaItems.length ) {
 		}
 	} );
 
+	// a panel opened by hover holds focus nowhere, so Escape has to be caught globally
+	document.addEventListener( 'keydown', ( event ) => {
+		if (
+			'Escape' === event.key &&
+			document.querySelector( '.nav-main--menu > .menu-item.megamenu.is-open' )
+		) {
+			megaItems.forEach( dismiss );
+		}
+	} );
+
 	desktopNav.addEventListener( 'change', ( event ) => {
 		if ( ! event.matches ) {
 			closeAll();

themes/osi/assets/js/build/theme.js

@@ -46,6 +46,7 @@ $maxPadding: 48px; // duplicates var(--wp--custom--spacing--max-padding)
 
 // header dimensions
 $headerInnerHeight: 125px; // .header--inner fixed height; admin-bar offsets come from core's --wp-admin--admin-bar--height
+$mobileRowHeight: 34px; // mobile accordion row: the caret's 44px tap target centres its glyph on this
 $midPadding: 32px; // duplicates var(--wp--custom--spacing--mid-padding);
 $smallPadding: 16px; // duplicates var(--wp--custom--spacing--small-padding);
 

themes/osi/assets/js/src/theme/mega-menu.js

@@ -182,7 +182,8 @@
     .header-main.is-nav-open {
         @include nav-open-layers;
     }
-    .header-main:has(.menu-item.megamenu:hover) {
+    // :not(.is-dismissed) so Escape clears the veil while the pointer is still on the item
+    .header-main:has(.menu-item.megamenu:hover:not(.is-dismissed)) {
         @include nav-open-layers;
     }
     .header-main-small {

themes/osi/assets/scss/_1_settings.breakpoints.scss

@@ -4,8 +4,10 @@ $chevronDown: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/sv
   padding-right: 30px;
 }
 
-.nav-main--menu, .nav-mobile--menu {
-  ul.sub-menu a[href='#'] {
+// desktop only: on mobile these rows are the accordion trigger, and mobile-menu-toggle.js
+// needs the click. It calls preventDefault(), so the empty href never navigates.
+@media only screen and (min-width: #{$break-nav}) {
+  .nav-main--menu ul.sub-menu a[href='#'] {
     pointer-events: none;
     text-decoration: none;
   }
@@ -59,7 +61,7 @@ $chevronDown: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/sv
   }
 
   .megamenu-featured--heading {
-    color: $brandColor1;
+    color: $brandColor1_Dk;
     display: block;
     text-transform: uppercase;
   }
@@ -77,7 +79,7 @@ $chevronDown: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/sv
   }
 
   a.megamenu-featured--more {
-    color: $brandColor1;
+    color: $brandColor1_Dk;
     display: inline-block;
     font-weight: $baseWeightBold;
     padding: 0;
@@ -87,7 +89,7 @@ $chevronDown: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/sv
     }
 
     &:hover, &:focus {
-      color: $brandColor1_Dk;
+      color: $interactColor_Dk;
       text-decoration: underline;
     }
   }
@@ -159,12 +161,12 @@ $chevronDown: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/sv
       .menu-item.current-menu-item > a,
       .menu-item.current-menu-parent > a,
       .menu-item.current-menu-ancestor > a {
-        color: $brandColor1;
+        color: $brandColor1_Dk;
         text-decoration: none;
       }
 
       .menu-item.tab-active > a {
-        color: $brandColor1;
+        color: $brandColor1_Dk;
         font-weight: 700;
       }
 
@@ -201,25 +203,34 @@ $chevronDown: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/sv
           transition: all .3s;
 
           &:after {
-              color: $brandColor1;
+              color: $brandColor1_Dk;
           }
         }
 
+        // the 44px tap target starts at the row's top edge, so centre the glyph on the
+        // row height: 50% on the button would follow the expanded submenu down the page
         &:after {
           color: inherit;
           content: '\25BC';
           font-size: 12px;
-          line-height: 44px;
+          line-height: $mobileRowHeight;
           padding: 0;
           transform: none;
         }
       }
 
       .menu-toggle-active:after {
-        color: $brandColor1;
+        color: $brandColor1_Dk;
         transform: rotate(180deg);
       }
 
+      // the generic sub-menu link hover (brand-links) outranks the shared card rule,
+      // so the darker hover has to be set at this depth to win
+      .megamenu-featured a.megamenu-featured--more:hover,
+      .megamenu-featured a.megamenu-featured--more:focus {
+        color: $interactColor_Dk;
+      }
+
       ul.sub-menu {
         margin-left: 0;
         margin-bottom: 20px;
@@ -426,7 +437,7 @@ $chevronDown: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/sv
           margin-bottom: 1.25rem;
 
           .megamenu-eyebrow {
-            color: $brandColor1;
+            color: $brandColor1_Dk;
             display: block;
             font-size: .85rem;
             font-weight: $baseWeightBold;
@@ -520,6 +531,11 @@ $chevronDown: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/sv
           .megamenu-featured--more {
             margin-top: .75rem;
             padding: 0;
+
+            // as on mobile: the generic sub-menu link hover outranks the shared rule
+            &:hover, &:focus {
+              color: $interactColor_Dk;
+            }
           }
         }
       }
@@ -531,10 +547,17 @@ $chevronDown: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/sv
       }
 
       // separate rule, as above: an unparseable :has() would drop the JS one too
-      .header-main:has(.menu-item.megamenu:hover) & > ul.sub-menu {
+      .header-main:has(.menu-item.megamenu:hover:not(.is-dismissed)) & > ul.sub-menu {
         transition: none;
       }
 
+      // Escape sets .is-dismissed: without this the pointer still satisfies :hover below
+      // and the panel stays up, since JS cannot clear a CSS hover state
+      &.is-dismissed:hover > ul.sub-menu {
+        opacity: 0;
+        visibility: hidden;
+      }
+
       // keyboard open is JS-driven (.is-open); :focus-within here would defeat Escape
       &:hover > ul.sub-menu,
       &.is-open > ul.sub-menu {

themes/osi/assets/scss/_6_components.header.scss

@@ -176,7 +176,7 @@ footer {
 				color: $Ndarkest;
 
 				&:hover, &:focus {
-					color: $brandColor1;
+					color: $brandColor1_Dk;
 				}
 			}
 		}
@@ -195,7 +195,7 @@ footer {
 	}
 
 	.nav-main .nav-main--cta {
-		background-color: $brandColor1;
+		background-color: $brandColor1_Dk;
 		border-radius: 999px;
 		bottom: 24px;
 		color: $Nwhite;
@@ -216,7 +216,7 @@ footer {
 		z-index: 5;
 
 		&:hover, &:focus {
-			background-color: $brandColor1_Dk;
+			background-color: $interactColor_Dk;
 			color: $Nwhite;
 			text-decoration: none;
 		}

themes/osi/assets/scss/_6_components.navigation--subnav.scss

@@ -375,7 +375,7 @@ function osi_supporters_shortcode_renderer( array $args = array() ): string {
  *
  * @return array
  */
-function osi_megamenu_placeholder_link_atts( array $atts, WP_Post $item, stdClass $args, int $depth ): array {
+function osi_megamenu_placeholder_link_atts( $atts, $item = null, $args = null, $depth = 0 ) { // phpcs:ignore Squiz.Commenting.FunctionComment.ScalarTypeHintMissing,Squiz.Commenting.FunctionComment.TypeHintMissing -- other walkers re-apply this filter with fewer arguments; typed params would fatal.
 	if ( 0 < $depth && isset( $args->theme_location, $atts['href'] ) && 'primary_navigation' === $args->theme_location && '#' === $atts['href'] ) {
 		$atts['tabindex'] = '-1';
 	}