Olivero narrow/mobile menu constrains tabbing in one direction only.

I feel like there's an existing issue to remove the custom tab code and use core's tabbing manager, but I can't find it

This should fix it!

Changes from comment #8 are remedied.

The code now dynamically queries the navigation for tabbable elements when tab is pressed.

I also moved the generation of the array of tabbable elements into its own function. It checks for all of the queries that are suggested, and also filters out elements that have visibility: hidden and display: none

In addition to the above, I changed the terminology to say "tabbable" instead of "focusable"

Forgot a period in my code comment.

Tugboat Preview

https://3191077-focus-trap-my31ah3yurgrqluspurwcaoflvidxst2.tugboat.qa/

Fixing the linting errors from the previous patch.

The last patch works as expected .
Before patch

After patch

It would great to add test coverage of this bug fix.

I discussed this with @nod and @lauriii and we were wondering why we're not leveraging the tabbable library that's part of core. I think we could consider refactoring to use that here - or maybe as part of follow-up. I'm not sure what makes more sense. Either way - having a test to prove we don't regress seems very worthwhile.

we were wondering why we're not leveraging the tabbable library that's part of core

The issue for that is at #3190242: Nav should use core/drupal.tabbingmanager instead of custom focus manager

One reason is that it currently relies on jQuery and jQueryUI, but there's some accessibility reasons in there too from @andrewmacpherson in the comments.

@mherchel thanks for linking that issue - good to see that in hand. We need to add tests here.

+++ b/core/themes/olivero/js/navigation.js
@@ -10,6 +10,20 @@
+  function getTabbableElements(el) {

FWIW this seems to be doing exactly what the tabbable library can do - see https://www.drupal.org/node/3186532

+++ b/core/themes/olivero/js/navigation.es6.js
@@ -10,6 +10,34 @@
+  /**
+   * Creates list of elements within an element that can be tabbed to via
+   * keyboard.
+   * @param {element} el - element from which to generate list.
+   *
+   * @return {array} array of tabbable elements.
+   */
+  function getTabbableElements(el) {
+    const tabbableElementSelector =
+      'button, [href], input, select, textarea, summary, audio[controls], video[controls], [tabindex]';
+    const negateElementSelector =
+      '[tabindex^="-"], [disabled], [type="hidden"]';
+    const tabbableElements = [];
+
+    el.querySelectorAll(tabbableElementSelector).forEach((el) => {
+      const elementStyle = window.getComputedStyle(el);
+      if (
+        !el.matches(negateElementSelector) &&
+        elementStyle.visibility !== 'hidden' &&
+        elementStyle.display !== 'none'
+      ) {
+        tabbableElements.push(el);
+      }
+    });
+
+    return tabbableElements;
+  }

Actually this can be entirely replaced by the tabbable library - which at least is tested JS code. We can leave focus trap part of the discussion to the other issue but there's no reason to have this code in Olivero.

From what I gather, #3113649: Remove drupal.tabbingmanager's jQueryUI dependency is not yet committed. The current tabbable library has jQueryUI dependency that we do not want to introduce.

I'll postpone this until that's committed.

@mherchel ah you're right we've still not added the tabbable library... we could still work on adding JS tests here and if #3113649: Remove drupal.tabbingmanager's jQueryUI dependency gets delay we could file a follow-up instead.

#3113649: Remove drupal.tabbingmanager's jQueryUI dependency is now committed.

Re-roll of #11 is attached.

Updating patch to use the new core tabbable library was super easy. Patch and interdiff attached.

Still need tests.

#3205434: Add Nightwatch Test Coverage for Olivero will make it possible to write a Nightwatch test to cover this issue.

Re-roll of #22

Now that #3205434: Add Nightwatch Test Coverage for Olivero is in, we can add tests!

I'm including

• test only patch (which should fail).
• interdiff
• Full patch (which should pass)

Latest Tugboat preview: https://3191077-focus-trap-2-ojzhptztqdzoz9kfnxhzodnlcph0eo37.tugboat.qa/

Needs reroll after #3173900: Refactor Olivero's JavaScript Drupal behaviors to use once() was committed. Working on it.

re-roll attached.

Updated tugboat: https://3191077-focus-trap-3-raoktoqiyjg8tbctjkbd9exrnnyfvqio.tugboat.qa/

Patch #27 looks good from a code perspective. I tested the patch and everything is working as expected. Users are now able to tab forward and backward in the tabbing order (see attached video screenshot). Marking as RTBC!

1. +++ b/core/themes/olivero/js/navigation.es6.js
   @@ -65,21 +65,27 @@
   +        const tabbableNavElements = tabbable.tabbable(props.navWrapper);
   

   The global tabbable should be only accessed in the IIFE that's wrapping this whole file.

2. +++ b/core/themes/olivero/js/navigation.es6.js
   @@ -104,34 +110,30 @@
   -      const navWrapperId = 'header-nav';
   -      const navWrapper = once(
   +      const headerId = 'header';
   +      const header = once(
   ...
   -        `#${navWrapperId}`,
   +        `#${headerId}`,
   ...
   +      const navWrapperId = 'header-nav';
   

   What are these changes?

3. +++ b/core/themes/olivero/js/navigation.es6.js
   @@ -104,34 +110,30 @@
   +        header.classList.add(`${headerId}-processed`);
   

   We probably don't need this anymore given that it's not referenced anywhere.

4. +++ b/core/themes/olivero/js/second-level-navigation.es6.js
   @@ -120,10 +120,9 @@
          );
   -      const state = button.getAttribute('aria-expanded') === 'true';
    
   -      if (state) {
   -        subNavsAreOpen = true;
   +      if (button) {
   +        subNavsAreOpen = button.getAttribute('aria-expanded') === 'true';
   

   How is this related to this issue?

The global tabbable should be only accessed in the IIFE that's wrapping this whole file.

Good call! Done.

What are these changes?

These changes are related to

We probably don't need this anymore given that it's not referenced anywhere.

Good catch. Done

How is this related to this issue?

I haven't worked on that in probably a month, but I'm guessing I saw a console error. I removed this from this patch.

regarding @lauriii's question "What are these changes?" in #29... he was asking why we're attaching the event listener to the header... and the answer is that we really don't need to. I'm going to create a new patch.

OK. Found out why I'm passing in the header prop. It's because the navigation button is not a child element of the navigation wrapper, which means that the keydown event would not fire.

I'm attaching a new patch with a more verbose comment.

The revised patch #32 looks good and addresses the feedback that @lauriii mentioned in #29 with the exception of line item (3), which @mherchel noted (#32) as something that we need and should not be removed.

RTBC +1

Patch #32 seems fine to me. I am able to tab forward and backward in the tabbing order.
RTBC +1

Committed f3674e9 and pushed to 9.2.x. Thanks!