fix: ensure menu-bar buttons always have correct tabindex (#10356)

Author: web-padawan

packages/a11y-base/src/keyboard-direction-mixin.js

@@ -44,15 +44,23 @@ export const KeyboardDirectionMixin = (superclass) =>
      * @override
      */
     focus(options) {
-      const items = this._getItems();
-      if (Array.isArray(items)) {
-        const idx = this._getAvailableIndex(items, 0, null, (item) => !isElementHidden(item));
-        if (idx >= 0) {
-          this._focus(idx, options);
-        }
+      const idx = this._getFocusableIndex();
+      if (idx >= 0) {
+        this._focus(idx, options);
       }
     }
 
+    /**
+     * Get the index of a first focusable item, if any.
+     *
+     * @return {Element[]}
+     * @protected
+     */
+    _getFocusableIndex() {
+      const items = this._getItems();
+      return Array.isArray(items) ? this._getAvailableIndex(items, 0, null, (item) => !isElementHidden(item)) : -1;
+    }
+
     /**
      * Get the list of items participating in keyboard navigation.
      * By default, it treats all the light DOM children as items.

packages/menu-bar/src/vaadin-menu-bar-mixin.js

@@ -8,7 +8,7 @@ import { Directive, directive } from 'lit/directive.js';
 import { ifDefined } from 'lit/directives/if-defined.js';
 import { DisabledMixin } from '@vaadin/a11y-base/src/disabled-mixin.js';
 import { FocusMixin } from '@vaadin/a11y-base/src/focus-mixin.js';
-import { isElementFocused, isKeyboardActive } from '@vaadin/a11y-base/src/focus-utils.js';
+import { isElementFocused, isElementHidden, isKeyboardActive } from '@vaadin/a11y-base/src/focus-utils.js';
 import { KeyboardDirectionMixin } from '@vaadin/a11y-base/src/keyboard-direction-mixin.js';
 import { microTask } from '@vaadin/component-base/src/async.js';
 import { Debouncer } from '@vaadin/component-base/src/debounce.js';
@@ -500,12 +500,6 @@ export const MenuBarMixin = (superClass) =>
 
         const items = buttons.filter((b) => !remaining.includes(b)).map((b) => b.item);
         this.__updateOverflow(items);
-
-        // Ensure there is at least one button with tabindex set to 0
-        // so that menu-bar is not skipped when navigating with Tab
-        if (remaining.length && !remaining.some((btn) => btn.getAttribute('tabindex') === '0')) {
-          this._setTabindex(remaining[remaining.length - 1], true);
-        }
       }
     }
 
@@ -536,13 +530,23 @@ export const MenuBarMixin = (superClass) =>
       const isSingleButton = newOverflowCount === buttons.length || (newOverflowCount === 0 && buttons.length === 1);
       this.toggleAttribute('has-single-button', isSingleButton);
 
+      // Collect visible buttons to detect if tabindex should be updated
+      const visibleButtons = buttons.filter((btn) => btn.style.visibility !== 'hidden');
+
+      if (!visibleButtons.length) {
+        // If all buttons except overflow are hidden, set tabindex on it
+        this._overflow.setAttribute('tabindex', '0');
+      } else if (!visibleButtons.some((btn) => btn.getAttribute('tabindex') === '0')) {
+        // Ensure there is at least one button with tabindex set to 0
+        // so that menu-bar is not skipped when navigating with Tab
+        this._setTabindex(visibleButtons[visibleButtons.length - 1], true);
+      }
+
       // Apply first/last visible attributes to the visible buttons
-      buttons
-        .filter((btn) => btn.style.visibility !== 'hidden')
-        .forEach((btn, index, visibleButtons) => {
-          btn.toggleAttribute('first-visible', index === 0);
-          btn.toggleAttribute('last-visible', !this._hasOverflow && index === visibleButtons.length - 1);
-        });
+      visibleButtons.forEach((btn, index, visibleButtons) => {
+        btn.toggleAttribute('first-visible', index === 0);
+        btn.toggleAttribute('last-visible', !this._hasOverflow && index === visibleButtons.length - 1);
+      });
     }
 
     /** @private */
@@ -761,8 +765,7 @@ export const MenuBarMixin = (superClass) =>
      */
     _setFocused(focused) {
       if (focused) {
-        const selector = this.tabNavigation ? '[focused]' : '[tabindex="0"]';
-        const target = this.querySelector(`vaadin-menu-bar-button${selector}`);
+        const target = this.__getFocusTarget();
         if (target) {
           this._buttons.forEach((btn) => {
             this._setTabindex(btn, btn === target);
@@ -776,6 +779,24 @@ export const MenuBarMixin = (superClass) =>
       }
     }
 
+    /** @private */
+    __getFocusTarget() {
+      // First, check if focus is moving to a visible button
+      let target = this._buttons.find((btn) => isElementFocused(btn));
+
+      if (!target) {
+        const selector = this.tabNavigation ? '[focused]' : '[tabindex="0"]';
+        // Next, check if there is a button that could be focused but is hidden
+        target = this.querySelector(`vaadin-menu-bar-button${selector}`);
+
+        if (isElementHidden(target)) {
+          target = this._buttons[this._getFocusableIndex()];
+        }
+      }
+
+      return target;
+    }
+
     /**
      * @param {!KeyboardEvent} event
      * @private

packages/menu-bar/test/keyboard-navigation.test.js

@@ -1,6 +1,6 @@
 import { expect } from '@vaadin/chai-plugins';
 import { sendKeys } from '@vaadin/test-runner-commands';
-import { fixtureSync, nextRender, nextUpdate } from '@vaadin/testing-helpers';
+import { fixtureSync, nextRender, nextResize, nextUpdate } from '@vaadin/testing-helpers';
 import sinon from 'sinon';
 import '../src/vaadin-menu-bar.js';
 
@@ -138,6 +138,82 @@ describe('keyboard navigation', () => {
           expect(buttons[3].hasAttribute('focused')).to.be.true;
         });
       });
+
+      describe('overflow', () => {
+        beforeEach(async () => {
+          menu.items = [{ text: 'Item Foo' }, { text: 'Item Bar' }];
+          await nextRender();
+          buttons = menu._buttons;
+        });
+
+        it('should move focus back to the overflow button on Shift + Tab after Tab', async () => {
+          // Hide all buttons except overflow
+          menu.style.width = '100px';
+          await nextResize(menu);
+
+          firstGlobalFocusable.focus();
+          await sendKeys({ press: 'Tab' });
+          expect(document.activeElement).to.equal(menu._overflow);
+
+          await sendKeys({ press: 'Tab' });
+          expect(document.activeElement).to.equal(lastGlobalFocusable);
+
+          await sendKeys({ press: 'Shift+Tab' });
+          expect(document.activeElement).to.equal(menu._overflow);
+        });
+
+        it('should move focus back to the overflow button on Tab after Shift + Tab', async () => {
+          // Show 1 button + overflow
+          menu.style.width = '120px';
+          await nextResize(menu);
+
+          lastGlobalFocusable.focus();
+          await sendKeys({ press: 'Shift+Tab' });
+          expect(document.activeElement).to.equal(menu._overflow);
+
+          await sendKeys({ press: 'Shift+Tab' });
+          expect(document.activeElement).to.equal(firstGlobalFocusable);
+
+          await sendKeys({ press: 'Tab' });
+          expect(document.activeElement).to.equal(menu._overflow);
+        });
+
+        it('should move focus back to the overflow button on Tab after button is hidden', async () => {
+          lastGlobalFocusable.focus();
+          await sendKeys({ press: 'Shift+Tab' });
+          expect(document.activeElement).to.equal(buttons[1]);
+
+          await sendKeys({ press: 'Shift+Tab' });
+          expect(document.activeElement).to.equal(firstGlobalFocusable);
+
+          // Hide all buttons except overflow
+          menu.style.width = '100px';
+          await nextResize(menu);
+
+          await sendKeys({ press: 'Tab' });
+          expect(document.activeElement).to.equal(menu._overflow);
+        });
+
+        it('should not skip buttons on Tab after overflow becomes hidden', async () => {
+          // Hide all buttons except overflow
+          menu.style.width = '100px';
+          await nextResize(menu);
+
+          firstGlobalFocusable.focus();
+          await sendKeys({ press: 'Tab' });
+          expect(document.activeElement).to.equal(menu._overflow);
+
+          await sendKeys({ press: 'Tab' });
+          expect(document.activeElement).to.equal(lastGlobalFocusable);
+
+          // Show all buttons and hide overflow
+          menu.style.width = '100%';
+          await nextResize(menu);
+
+          await sendKeys({ press: 'Shift+Tab' });
+          expect(document.activeElement).to.equal(buttons[1]);
+        });
+      });
     });
 
     describe('tab navigation mode', () => {

packages/menu-bar/test/menu-bar.test.js

@@ -1,5 +1,5 @@
 import { expect } from '@vaadin/chai-plugins';
-import { fixtureSync, focusin, nextRender, nextUpdate } from '@vaadin/testing-helpers';
+import { fixtureSync, nextRender, nextUpdate } from '@vaadin/testing-helpers';
 import '../src/vaadin-menu-bar.js';
 
 describe('custom element definition', () => {
@@ -71,7 +71,7 @@ describe('root menu layout', () => {
   });
 
   it('should set tabindex to -1 to all the buttons except first one', () => {
-    focusin(menu);
+    menu.focus();
     expect(buttons[0].getAttribute('tabindex')).to.equal('0');
     buttons.slice(1).forEach((btn) => {
       expect(btn.getAttribute('tabindex')).to.equal('-1');

test/integration/menu-bar-tooltip.test.js

@@ -6,7 +6,6 @@ import {
   escKeyDown,
   fire,
   fixtureSync,
-  focusin,
   focusout,
   mousedown,
   nextRender,
@@ -144,7 +143,7 @@ describe('menu-bar with tooltip', () => {
 
     it('should show tooltip on menu button keyboard focus', () => {
       tabKeyDown(document.body);
-      focusin(buttons[0]);
+      buttons[0].focus();
       expect(tooltip.opened).to.be.true;
     });
 
@@ -159,27 +158,27 @@ describe('menu-bar with tooltip', () => {
 
     it('should set tooltip target on menu button keyboard focus', () => {
       tabKeyDown(document.body);
-      focusin(buttons[0]);
+      buttons[0].focus();
       expect(tooltip.target).to.be.equal(buttons[0]);
     });
 
     it('should set tooltip context on menu button keyboard focus', () => {
       tabKeyDown(document.body);
-      focusin(buttons[0]);
+      buttons[0].focus();
       expect(tooltip.context).to.be.instanceOf(Object);
       expect(tooltip.context.item.text).to.equal('Edit');
     });
 
     it('should hide tooltip on menu-bar focusout', () => {
       tabKeyDown(document.body);
-      focusin(buttons[0]);
+      buttons[0].focus();
       focusout(menuBar);
       expect(tooltip.opened).to.be.false;
     });
 
     it('should hide tooltip on menuBar menu button content Esc', () => {
       tabKeyDown(document.body);
-      focusin(buttons[0]);
+      buttons[0].focus();
       escKeyDown(buttons[0]);
       expect(tooltip.opened).to.be.false;
     });
@@ -250,7 +249,7 @@ describe('menu-bar with tooltip', () => {
         arrowRight(buttons[0]);
         arrowRight(buttons[1]);
         tabKeyDown(document.body);
-        focusin(buttons[buttons.length - 1]);
+        menuBar._overflow.focus();
         expect(tooltip.opened).to.be.false;
       });
     });
@@ -327,7 +326,7 @@ describe('menu-bar with tooltip', () => {
         tooltip.focusDelay = 100;
 
         tabKeyDown(document.body);
-        focusin(buttons[0]);
+        buttons[0].focus();
         expect(tooltip.opened).to.be.false;
 
         clock.tick(100);
@@ -338,7 +337,7 @@ describe('menu-bar with tooltip', () => {
         tooltip.hideDelay = 100;
 
         tabKeyDown(document.body);
-        focusin(buttons[0]);
+        buttons[0].focus();
         clock.tick(DEFAULT_DELAY);
 
         escKeyDown(buttons[0]);