Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[menu-bar] Add possibility to disable roving tabindex #7525

Merged
merged 17 commits into from
Aug 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/menu-bar/src/vaadin-menu-bar-mixin.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,13 @@ export declare class MenuBarMixinClass {
*/
reverseCollapse: boolean | null | undefined;

/**
* If true, the top-level menu items is traversable by tab
* instead of arrow keys (i.e. disabling roving tabindex)
* @attr {boolean} tab-navigation
*/
tabNavigation: boolean | null | undefined;

/**
* Closes the current submenu.
*/
Expand Down
62 changes: 58 additions & 4 deletions packages/menu-bar/src/vaadin-menu-bar-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
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 { ControllerMixin } from '@vaadin/component-base/src/controller-mixin.js';
import { ResizeMixin } from '@vaadin/component-base/src/resize-mixin.js';
Expand Down Expand Up @@ -144,6 +144,15 @@ export const MenuBarMixin = (superClass) =>
type: Boolean,
},

/**
* If true, the top-level menu items is traversable by tab
* instead of arrow keys (i.e. disabling roving tabindex)
* @attr {boolean} tab-navigation
*/
tabNavigation: {
type: Boolean,
},

/**
* @type {boolean}
* @protected
Expand Down Expand Up @@ -173,6 +182,7 @@ export const MenuBarMixin = (superClass) =>
'__i18nChanged(i18n, _overflow)',
'_menuItemsChanged(items, _overflow, _container)',
'_reverseCollapseChanged(reverseCollapse, _overflow, _container)',
'_tabNavigationChanged(tabNavigation, _overflow, _container)',
];
}

Expand Down Expand Up @@ -349,6 +359,22 @@ export const MenuBarMixin = (superClass) =>
}
}

/** @private */
_tabNavigationChanged(tabNavigation, overflow, container) {
if (overflow && container) {
const target = this.querySelector('[tabindex="0"]');
this._buttons.forEach((btn) => {
if (target) {
this._setTabindex(btn, btn === target);
jcgueriaud1 marked this conversation as resolved.
Show resolved Hide resolved
} else {
this._setTabindex(btn, false);
}
btn.setAttribute('role', tabNavigation ? 'button' : 'menuitem');
});
}
this.setAttribute('role', tabNavigation ? 'group' : 'menubar');
}

/** @private */
__hasOverflowChanged(hasOverflow, overflow) {
if (overflow) {
Expand Down Expand Up @@ -540,7 +566,7 @@ export const MenuBarMixin = (superClass) =>

/** @protected */
_initButtonAttrs(button) {
button.setAttribute('role', 'menuitem');
button.setAttribute('role', this.tabNavigation ? 'button' : 'menuitem');
jcgueriaud1 marked this conversation as resolved.
Show resolved Hide resolved

if (button === this._overflow || (button.item && button.item.children)) {
button.setAttribute('aria-haspopup', 'true');
Expand Down Expand Up @@ -667,7 +693,11 @@ export const MenuBarMixin = (superClass) =>

/** @protected */
_setTabindex(button, focused) {
button.setAttribute('tabindex', focused ? '0' : '-1');
if (this.tabNavigation && !button.disabled) {
button.setAttribute('tabindex', '0');
} else {
button.setAttribute('tabindex', focused ? '0' : '-1');
}
}

/**
Expand Down Expand Up @@ -715,7 +745,12 @@ export const MenuBarMixin = (superClass) =>
*/
_setFocused(focused) {
if (focused) {
const target = this.querySelector('[tabindex="0"]');
let target = this.querySelector('[tabindex="0"]');
if (this.tabNavigation) {
jcgueriaud1 marked this conversation as resolved.
Show resolved Hide resolved
// Switch submenu on menu button Tab / Shift Tab
target = this.querySelector('[focused]');
this.__switchSubMenu(target);
}
if (target) {
this._buttons.forEach((btn) => {
this._setTabindex(btn, btn === target);
Expand Down Expand Up @@ -839,6 +874,25 @@ export const MenuBarMixin = (superClass) =>
// Prevent ArrowLeft from being handled in context-menu
e.stopImmediatePropagation();
this._onKeyDown(e);
} else if (e.keyCode === 9 && this.tabNavigation) {
// Switch opened submenu on submenu item Tab / Shift Tab
const items = this._getItems() || [];
const currentIdx = items.indexOf(this.focused);
const increment = e.shiftKey ? -1 : 1;
let idx = currentIdx + increment;
idx = this._getAvailableIndex(items, idx, increment, (item) => !isElementHidden(item));
this.__switchSubMenu(items[idx]);
}
}
}

/** @private */
__switchSubMenu(target) {
const wasExpanded = this._expandedButton != null && this._expandedButton !== target;
if (wasExpanded) {
this._close();
if (target.item && target.item.children) {
this.__openSubMenu(target, true, { keepFocus: true });
}
}
}
Expand Down
37 changes: 37 additions & 0 deletions packages/menu-bar/test/a11y.common.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,43 @@ describe('a11y', () => {
});
});

it('should set role attribute on host element in tabNavigation', async () => {
menu.tabNavigation = true;
await nextRender(menu);
expect(menu.getAttribute('role')).to.equal('group');
});

it('should set role attribute on menu bar buttons in tabNavigation', async () => {
menu.tabNavigation = true;
await nextRender(menu);
buttons.forEach((btn) => {
expect(btn.getAttribute('role')).to.equal('button');
});
});

it('should update role attribute on menu bar buttons when changing items', async () => {
menu.items = [...menu.items, { text: 'New item' }];
await nextRender(menu);
menu._buttons.forEach((btn) => {
expect(btn.getAttribute('role')).to.equal('menuitem');
});
});

it('should update role attribute on menu bar buttons when changing items in tabNavigation', async () => {
menu.tabNavigation = true;
await nextRender(menu);
menu.items = [...menu.items, { text: 'New item' }];
await nextRender(menu);
menu._buttons.forEach((btn) => {
expect(btn.getAttribute('role')).to.equal('button');
});
menu.tabNavigation = false;
await nextRender(menu);
menu._buttons.forEach((btn) => {
expect(btn.getAttribute('role')).to.equal('menuitem');
});
});

it('should set aria-haspopup attribute on buttons with nested items', () => {
buttons.forEach((btn) => {
const hasPopup = btn === overflow || btn.item.children ? 'true' : null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ snapshots["menu-bar basic"] =
aria-expanded="false"
aria-haspopup="true"
role="menuitem"
tabindex="0"
tabindex="-1"
>
Reports
</vaadin-menu-bar-button>
Expand All @@ -31,7 +31,7 @@ snapshots["menu-bar basic"] =
class="help"
last-visible=""
role="menuitem"
tabindex="0"
tabindex="-1"
>
<vaadin-menu-bar-item aria-selected="false">
<strong>
Expand All @@ -46,7 +46,7 @@ snapshots["menu-bar basic"] =
hidden=""
role="menuitem"
slot="overflow"
tabindex="0"
tabindex="-1"
>
<div aria-hidden="true">
···
Expand Down
59 changes: 59 additions & 0 deletions packages/menu-bar/test/menu-bar.common.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
nextRender,
nextUpdate,
} from '@vaadin/testing-helpers';
import { sendKeys } from '@web/test-runner-commands';
import sinon from 'sinon';

describe('custom element definition', () => {
Expand Down Expand Up @@ -89,6 +90,29 @@ describe('root menu layout', () => {
});
});

it('should set tabindex to 0 when the button is not disabled in tab navigation', async () => {
menu.tabNavigation = true;
await nextUpdate(menu);
buttons.forEach((btn) => {
if (btn.disabled) {
expect(btn.getAttribute('tabindex')).to.equal('-1');
} else {
expect(btn.getAttribute('tabindex')).to.equal('0');
}
});
});

it('should reset tabindex after switching back from tab navigation', async () => {
menu.tabNavigation = true;
await nextUpdate(menu);
menu.tabNavigation = false;
await nextUpdate(menu);
expect(buttons[0].getAttribute('tabindex')).to.equal('0');
buttons.slice(1).forEach((btn) => {
expect(btn.getAttribute('tabindex')).to.equal('-1');
});
});

it('should not throw when changing items before the menu bar is attached', () => {
expect(() => {
const menuBar = document.createElement('vaadin-menu-bar');
Expand Down Expand Up @@ -206,6 +230,41 @@ describe('root menu layout', () => {
});
});

describe('tab navigation mode', () => {
beforeEach(() => {
menu.tabNavigation = true;
});

it('should move focus to next button on Tab keydown', async () => {
buttons[0].focus();
await sendKeys({ press: 'Tab' });
expect(buttons[1].hasAttribute('focused')).to.be.true;
});

it('should move focus to prev button on Shift Tab keydown', async () => {
buttons[1].focus();
await sendKeys({ down: 'Shift' });
await sendKeys({ press: 'Tab' });
await sendKeys({ up: 'Shift' });
expect(buttons[0].hasAttribute('focused')).to.be.true;
});

it('should move focus to fourth button if third is disabled on Tab keydown', async () => {
await updateItemsAndButtons();
buttons[1].focus();
await sendKeys({ press: 'Tab' });
expect(buttons[3].hasAttribute('focused')).to.be.true;
});

it('should not move the focus to the next button if tab navigation is disabled', async () => {
menu.tabNavigation = false;
menu.focus();
expect(document.activeElement).to.equal(buttons[0]);
await sendKeys({ press: 'Tab' });
expect(document.activeElement).to.not.equal(buttons[1]);
});
});

describe('updating items', () => {
it('should remove buttons when setting empty array', async () => {
menu.items = [];
Expand Down
16 changes: 16 additions & 0 deletions packages/menu-bar/test/overflow.common.js
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,22 @@ describe('overflow', () => {
await onceResized(menu);

expect(buttons[0].getAttribute('tabindex')).to.equal('0');
expect(buttons[1].getAttribute('tabindex')).to.equal('-1');
});

it('should set tabindex -1 on the overflow menu in tab navigation', async () => {
menu.tabNavigation = true;
buttons[0].focus();
arrowRight(buttons[0]);

expect(buttons[0].getAttribute('tabindex')).to.equal('0');
expect(buttons[1].getAttribute('tabindex')).to.equal('0');

menu.style.width = '150px';
await onceResized(menu);

expect(buttons[0].getAttribute('tabindex')).to.equal('0');
expect(buttons[1].getAttribute('tabindex')).to.equal('-1');
});

it('should set the aria-label of the overflow button according to the i18n of the menu bar', async () => {
Expand Down
35 changes: 35 additions & 0 deletions packages/menu-bar/test/sub-menu.common.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
touchend,
touchstart,
} from '@vaadin/testing-helpers';
import { sendKeys } from '@web/test-runner-commands';
import sinon from 'sinon';
import { setCancelSyntheticClickEvents } from '@polymer/polymer/lib/utils/settings.js';
import { isTouch } from '@vaadin/component-base/src/browser-utils.js';
Expand Down Expand Up @@ -261,6 +262,40 @@ describe('sub-menu', () => {
expect(buttons[1].hasAttribute('focus-ring')).to.be.true;
});

it('should switch menubar button with items and open submenu on Tab in tab navigation', async () => {
menu.tabNavigation = true;
menu.items = [...menu.items, { text: 'Menu Item 4', children: [{ text: 'Menu Item 4 1' }] }];
await nextUpdate(menu);
buttons = menu._buttons;
buttons[2].focus();
arrowDown(buttons[2]);
await oneEvent(subMenu, 'opened-changed');

await sendKeys({ press: 'Tab' });
await nextRender(subMenu);

expect(subMenu.opened).to.be.true;
expect(subMenu.listenOn).to.equal(buttons[3]);
});

it('should switch menubar button with items and open submenu on Shift Tab in tab navigation', async () => {
menu.tabNavigation = true;
menu.items = [...menu.items, { text: 'Menu Item 4', children: [{ text: 'Menu Item 4 1' }] }];
await nextUpdate(menu);
buttons = menu._buttons;
buttons[3].focus();
arrowDown(buttons[3]);
await oneEvent(subMenu, 'opened-changed');

await sendKeys({ down: 'Shift' });
await sendKeys({ press: 'Tab' });
await sendKeys({ up: 'Shift' });
await nextRender(subMenu);

expect(subMenu.opened).to.be.true;
expect(subMenu.listenOn).to.equal(buttons[2]);
});

it('should focus first item on arrow down after opened on arrow left', async () => {
arrowDown(buttons[0]);
await oneEvent(subMenu, 'opened-changed');
Expand Down
1 change: 1 addition & 0 deletions packages/menu-bar/test/typings/menu-bar.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const menu = document.createElement('vaadin-menu-bar');
const assertType = <TExpected>(actual: TExpected) => actual;

assertType<boolean | null | undefined>(menu.openOnHover);
assertType<boolean | null | undefined>(menu.tabNavigation);
assertType<MenuItem[]>(menu.items);
assertType<string>(menu.overlayClass);

Expand Down
Loading