From e34b89e85a1edae382d8f708994c16cde9442f40 Mon Sep 17 00:00:00 2001 From: Serhii Kulykov Date: Tue, 19 Mar 2024 10:11:24 +0200 Subject: [PATCH] fix: focus first non-disabled item if all items have tabindex -1 (#7230) --- .../a11y-base/src/keyboard-direction-mixin.js | 2 +- packages/a11y-base/src/list-mixin.js | 12 +++++-- packages/a11y-base/test/list-mixin.test.js | 34 +++++++++++++++++++ packages/context-menu/test/items.common.js | 28 +++++++++++++++ packages/menu-bar/test/sub-menu.test.js | 20 +++++++++++ 5 files changed, 93 insertions(+), 3 deletions(-) diff --git a/packages/a11y-base/src/keyboard-direction-mixin.js b/packages/a11y-base/src/keyboard-direction-mixin.js index eea006312d..4d043e606a 100644 --- a/packages/a11y-base/src/keyboard-direction-mixin.js +++ b/packages/a11y-base/src/keyboard-direction-mixin.js @@ -36,7 +36,7 @@ export const KeyboardDirectionMixin = (superclass) => if (Array.isArray(items)) { const idx = this._getAvailableIndex(items, 0, null, (item) => !isElementHidden(item)); if (idx >= 0) { - items[idx].focus(); + this._focus(idx); } } } diff --git a/packages/a11y-base/src/list-mixin.js b/packages/a11y-base/src/list-mixin.js index ba86ff4b27..70fab80f90 100644 --- a/packages/a11y-base/src/list-mixin.js +++ b/packages/a11y-base/src/list-mixin.js @@ -8,6 +8,7 @@ import { Debouncer } from '@vaadin/component-base/src/debounce.js'; import { getNormalizedScrollLeft, setNormalizedScrollLeft } from '@vaadin/component-base/src/dir-utils.js'; import { getFlattenedElements } from '@vaadin/component-base/src/dom-utils.js'; import { SlotObserver } from '@vaadin/component-base/src/slot-observer.js'; +import { isElementHidden } from './focus-utils.js'; import { KeyboardDirectionMixin } from './keyboard-direction-mixin.js'; /** @@ -117,8 +118,15 @@ export const ListMixin = (superClass) => if (this._observer) { this._observer.flush(); } - const firstItem = this.querySelector('[tabindex="0"]') || (this.items ? this.items[0] : null); - this._focusItem(firstItem); + + const items = Array.isArray(this.items) ? this.items : []; + const idx = this._getAvailableIndex(items, 0, null, (item) => item.tabIndex === 0 && !isElementHidden(item)); + if (idx >= 0) { + this._focus(idx); + } else { + // Call `KeyboardDirectionMixin` logic to focus first non-disabled item. + super.focus(); + } } /** @protected */ diff --git a/packages/a11y-base/test/list-mixin.test.js b/packages/a11y-base/test/list-mixin.test.js index 9204d4d59a..142190f0a9 100644 --- a/packages/a11y-base/test/list-mixin.test.js +++ b/packages/a11y-base/test/list-mixin.test.js @@ -288,6 +288,40 @@ const runTests = (defineHelper, baseMixin) => { list.focus(); expect(spy.calledOnce).to.be.true; }); + + it('should call focus() on the first non-disabled item if all items have tabindex -1', () => { + list.items.forEach((item) => { + item.tabIndex = -1; + }); + const spy = sinon.spy(list.items[2], 'focus'); + list.focus(); + expect(spy.calledOnce).to.be.true; + }); + + it('should call focus() on the first non-disabled visible item if all items have tabindex -1', () => { + list.items.forEach((item) => { + item.tabIndex = -1; + }); + list.items[2].setAttribute('hidden', ''); + const spy = sinon.spy(list.items[3], 'focus'); + list.focus(); + expect(spy.calledOnce).to.be.true; + }); + + it('should change tabindex from -1 to 0 on first non-disabled item when focusing it', () => { + list.items.forEach((item) => { + item.tabIndex = -1; + }); + list.focus(); + [-1, -1, 0, -1].forEach((val, idx) => expect(list.items[idx].tabIndex).to.equal(val)); + }); + + it('should change tabindex from 0 to -1 on items with tabindex 0 other than focused one', () => { + list.items[2].tabIndex = 0; + list.items[3].tabIndex = 0; + list.focus(); + [-1, -1, 0, -1].forEach((val, idx) => expect(list.items[idx].tabIndex).to.equal(val)); + }); }); describe('tabIndex when all the items are disabled', () => { diff --git a/packages/context-menu/test/items.common.js b/packages/context-menu/test/items.common.js index 6148764bf0..8c97bbd67e 100644 --- a/packages/context-menu/test/items.common.js +++ b/packages/context-menu/test/items.common.js @@ -414,6 +414,34 @@ describe('items', () => { expect(items[0].hasAttribute('focus-ring')).to.be.true; }); + it('should focus first non-disabled item after re-opening when using components', async () => { + subMenu.close(); + rootOverlay.focus(); + + rootMenu.items[3].children[0].disabled = true; + + const rootItem = getMenuItems(rootMenu)[3]; + + // Open the sub-menu with item components + await openMenu(rootItem); + const subMenu2 = getSubMenu(rootMenu); + const items = getMenuItems(subMenu2); + + // Arrow Down to focus next item + items[1].focus(); + arrowDownKeyDown(items[1]); + expect(items[2].hasAttribute('focus-ring')).to.be.true; + + // Arrow Left to close the sub-menu + arrowLeftKeyDown(items[2]); + await nextFrame(); + expect(rootItem.hasAttribute('focus-ring')).to.be.true; + + // Re-open sub-menu and check focus + await openMenu(rootItem); + expect(items[1].hasAttribute('focus-ring')).to.be.true; + }); + it('should have modeless sub menus', () => { const rootItemRect = getMenuItems(rootMenu)[0].getBoundingClientRect(); const element = document.elementFromPoint(rootItemRect.left, rootItemRect.top); diff --git a/packages/menu-bar/test/sub-menu.test.js b/packages/menu-bar/test/sub-menu.test.js index 7ffe2d6d74..82d1f0fcdf 100644 --- a/packages/menu-bar/test/sub-menu.test.js +++ b/packages/menu-bar/test/sub-menu.test.js @@ -58,6 +58,9 @@ describe('sub-menu', () => { { component: createComponent('Menu Item 3 2'), }, + { + component: createComponent('Menu Item 3 3'), + }, ], }, ]; @@ -181,6 +184,23 @@ describe('sub-menu', () => { expect(items[0].hasAttribute('focus-ring')).to.be.true; }); + it('should focus first non-disabled item after re-opening when using components', async () => { + menu.items[2].children[0].disabled = true; + + arrowDown(buttons[2]); + await nextRender(subMenu); + + const items = subMenuOverlay.querySelectorAll('vaadin-menu-bar-item'); + expect(items[1].hasAttribute('focus-ring')).to.be.true; + + // Close and re-open + esc(items[1]); + arrowDown(buttons[2]); + await nextRender(subMenu); + + expect(items[1].hasAttribute('focus-ring')).to.be.true; + }); + it('should close sub-menu on first item arrow up', async () => { arrowDown(buttons[0]); await oneEvent(subMenu, 'opened-changed');