fix(action menu): keyboard accessibility omnibus #5031
Conversation
🦋 Changeset detectedLatest commit: 645342d The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Branch previewReview the following VRT differencesWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
Lighthouse scores
What is this?Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on Transfer Size
Request Count
|
Pull Request Test Coverage Report for Build 13062627835Details
💛 - Coveralls |
…child WAI ARIA APG
…nikkimk/fix-menu-a11y
| @@ -95,12 +95,35 @@ Content assigned to the `value` slot will be placed at the end of the `<sp-menu- | |||
| An `<sp-menu-item>` can also accept content addressed to its `submenu` slot. Using the `<sp-menu>` element with this slot name the options will be surfaced in flyout menu that can be activated by hovering over the root menu item with your pointer or focusing the menu item and pressing the appropriate `ArrowRight` or `ArrowLeft` key based on text direction to move into the submenu. | |||
|
|
|||
| ```html | |||
| <sp-menu style="width: 200px;"> | |||
| <p> | |||
There was a problem hiding this comment.
Added a more detailed example to docs to see menu item with submenus.
| @@ -87,7 +92,7 @@ export class MenuGroup extends Menu { | |||
| <span class="header" ?hidden=${!this.headerElement}> | |||
| <slot name="header" @slotchange=${this.updateLabel}></slot> | |||
| </span> | |||
| <sp-menu ignore>${this.renderMenuItemSlot()}</sp-menu> | |||
| ${this.renderMenuItemSlot()} | |||
There was a problem hiding this comment.
A menu group should not have a nested menu.
| @property({ type: Boolean, reflect: true }) | ||
| public open = false; | ||
|
|
||
| public override click(): void { |
There was a problem hiding this comment.
This is the source of the issues where Voice Over users can't click menu items.
| @@ -489,10 +589,18 @@ export class MenuItem extends LikeAnchor( | |||
| this.active = false; | |||
| } | |||
|
|
|||
| public async openOverlay(): Promise<void> { | |||
| public async openOverlay(focus: boolean = false): Promise<void> { | |||
There was a problem hiding this comment.
When navigating via keyboard, opening the menu should also set focus on an item within a menu.
| @@ -518,6 +626,19 @@ export class MenuItem extends LikeAnchor( | |||
| this.updateAriaSelected(); | |||
| } | |||
|
|
|||
| protected override willUpdate(changes: PropertyValues<this>): void { | |||
There was a problem hiding this comment.
If focus is inside the submenu when it close, move focus back to its parent item.
| @@ -117,11 +117,59 @@ describe('Menu item', () => { | |||
| ).anchorElement.dispatchEvent(new FocusEvent('focus')); | |||
|
|
|||
| await elementUpdated(item); | |||
| expect(el === document.activeElement).to.be.true; | |||
|
|
|||
| expect(item === document.activeElement).to.be.true; | |||
There was a problem hiding this comment.
Menus should delegate focus to an item
| item.click(); | ||
|
|
||
| expect(clickTargetSpy.calledWith(anchorElement)).to.be.true; | ||
| }); | ||
| it('allows link click', async () => { |
There was a problem hiding this comment.
Added test to verify that menuitems with href were clickable.
| @@ -638,16 +630,17 @@ describe('Menu w/ groups [selects]', () => { | |||
| input.focus(); | |||
| expect(document.activeElement === input).to.be.true; | |||
| await sendKeys({ press: 'Shift+Tab' }); | |||
| expect(document.activeElement === groupA).to.be.true; | |||
| expect(document.activeElement === options[0]).to.be.true; | |||
There was a problem hiding this comment.
Groups should delegate focus to items
| } | ||
| }); | ||
| } | ||
| super.update(changes); | ||
| } | ||
|
|
||
| protected hasAccessibleLabel(): boolean { |
There was a problem hiding this comment.
Action Menu was throwing the no label warning for a test that used the label-only slot, so I decided to override Picker's accessible label check and warning with a warning specific to action menu.
| @@ -853,7 +886,7 @@ export class Picker extends PickerBase { | |||
| return; | |||
| } | |||
| if (code === 'ArrowUp' || code === 'ArrowDown') { | |||
| this.toggle(true); | |||
| this.keyboardOpen(); | |||
There was a problem hiding this comment.
Focus should be on menu item after ArrowDown opens menu
| acceptsEventCode(code: string): boolean { | ||
| if (code === 'End' || code === 'Home') { | ||
| acceptsEventKey(key: string): boolean { | ||
| if (key === 'End' || key === 'Home') { |
There was a problem hiding this comment.
Using key instead of code to allow for Numpad keys.
| @@ -64,19 +61,42 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { | |||
| return [menuStyles]; | |||
| } | |||
|
|
|||
| static override shadowRootOptions = { | |||
There was a problem hiding this comment.
Menu should delegate focus to item
| @@ -575,55 +558,34 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { | |||
| } | |||
| } | |||
|
|
|||
| protected navigateWithinMenu(event: KeyboardEvent): void { | |||
There was a problem hiding this comment.
RTI now handles this.
|
|
||
| protected navigateBetweenRelatedMenus(event: KeyboardEvent): void { | ||
| const { key } = event; | ||
| protected navigateBetweenRelatedMenus(event: MenuItemKeydownEvent): void { |
There was a problem hiding this comment.
Since menu items have focus, menuitems need to forward keyboard event info to manage this.
| return; | ||
| } | ||
| } | ||
| if (key === ' ' || key === 'Enter') { | ||
| const childItem = this.childItems[this.focusedItemIndex]; |
| this.navigateBetweenRelatedMenus(event); | ||
| } | ||
|
|
||
| public focusMenuItemByOffset(offset: number): MenuItem { |
| @@ -811,26 +699,6 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { | |||
| this.descendentOverlays = new Map<Overlay, Overlay>(); | |||
| } | |||
|
|
|||
| private forwardFocusVisibleToItem(item: MenuItem): void { | |||
There was a problem hiding this comment.
Item is now focusable so no need to forward
nikkimk
requested review from
caseyisonit,
jnurthen,
majornista and
Rajdeepc
and removed request for
Rajdeepc
|
This seems to have broken the preventDefault behaviour when pressing space on a menu item. The page now scrolls. |
|
Action menu items are not reading for screen readers.
Description
Action menu should be using a roving tabindex not
aria-activedescendantbecause of cross-root ARIA limitations as well as lack of iOS support.The
sp-menuthat action menu uses was refactored to use a roving tabindex, and the numpad keys fix that was made in action menu are now applied to the focus group controller which the roving tabindex controller uses.Related issue(s)
Motivation and context
VoiceOver could not read the menu items when navigated via keyboard because of the cross-root aria issues above. Using the same roving tabindex controller that other components in our repo use, allows us to ensure roving tabindex and keyboard navigation is accessible and consistent across all components.
How has this been tested?
selectsand menu groups should function like the menu groups in the Editor Menubar Example from the WAI ARIA APGDoes screenreader read menuitems? (resolves #4556 and without regression on #3751)
Can you use a screenreader to click a menuitem? (resolves #4997)
Does keyboard navigation of menuitems work as it should? (closes #4557)
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.