From 843061777d4d5ef4bc9ee7614381d5d593330005 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Tue, 23 Jan 2018 16:46:50 +0100 Subject: [PATCH] fix(menu): always focus first menu item (#9383) * Switches to always focusing the first menu item, even when opening by mouse. The focus styling will only apply when focused by keyboard. * Adds the ability to set the focus origin through the `FocusKeyManager`. Fixes #9252. --- src/cdk/a11y/focus-key-manager.ts | 16 ++++++++++++++-- src/cdk/a11y/list-key-manager.spec.ts | 18 +++++++++++++++++- src/lib/menu/_menu-theme.scss | 3 ++- src/lib/menu/menu-directive.ts | 14 ++++++++------ src/lib/menu/menu-item.ts | 26 ++++++++++++++++++++++---- src/lib/menu/menu-module.ts | 2 ++ src/lib/menu/menu-panel.ts | 3 ++- src/lib/menu/menu-trigger.ts | 14 +------------- src/lib/menu/menu.spec.ts | 18 +++++++++++------- 9 files changed, 79 insertions(+), 35 deletions(-) diff --git a/src/cdk/a11y/focus-key-manager.ts b/src/cdk/a11y/focus-key-manager.ts index 154bfcea2dc3..d4d6ee00b731 100644 --- a/src/cdk/a11y/focus-key-manager.ts +++ b/src/cdk/a11y/focus-key-manager.ts @@ -7,6 +7,7 @@ */ import {ListKeyManager, ListKeyManagerOption} from './list-key-manager'; +import {FocusOrigin} from './focus-monitor'; /** * This is the interface for focusable items (used by the FocusKeyManager). @@ -15,10 +16,21 @@ import {ListKeyManager, ListKeyManagerOption} from './list-key-manager'; */ export interface FocusableOption extends ListKeyManagerOption { /** Focuses the `FocusableOption`. */ - focus(): void; + focus(origin?: FocusOrigin): void; } export class FocusKeyManager extends ListKeyManager { + private _origin: FocusOrigin = 'program'; + + /** + * Sets the focus origin that will be passed in to the items for any subsequent `focus` calls. + * @param origin Focus origin to be used when focusing items. + */ + setFocusOrigin(origin: FocusOrigin): this { + this._origin = origin; + return this; + } + /** * This method sets the active item to the item at the specified index. * It also adds focuses the newly active item. @@ -27,7 +39,7 @@ export class FocusKeyManager extends ListKeyManager { super.setActiveItem(index); if (this.activeItem) { - this.activeItem.focus(); + this.activeItem.focus(this._origin); } } } diff --git a/src/cdk/a11y/list-key-manager.spec.ts b/src/cdk/a11y/list-key-manager.spec.ts index 507a7cb0d513..a1bdc2934226 100644 --- a/src/cdk/a11y/list-key-manager.spec.ts +++ b/src/cdk/a11y/list-key-manager.spec.ts @@ -6,12 +6,13 @@ import {createKeyboardEvent} from '../testing/event-objects'; import {ActiveDescendantKeyManager} from './activedescendant-key-manager'; import {FocusKeyManager} from './focus-key-manager'; import {ListKeyManager} from './list-key-manager'; +import {FocusOrigin} from './focus-monitor'; class FakeFocusable { constructor(private _label = '') { } disabled = false; - focus() {} + focus(_focusOrigin?: FocusOrigin) {} getLabel() { return this._label; } } @@ -626,6 +627,21 @@ describe('Key managers', () => { expect(itemList.items[1].focus).not.toHaveBeenCalledTimes(1); }); + it('should be able to set the focus origin', () => { + keyManager.setFocusOrigin('mouse'); + + keyManager.onKeydown(fakeKeyEvents.downArrow); + expect(itemList.items[1].focus).toHaveBeenCalledWith('mouse'); + + keyManager.onKeydown(fakeKeyEvents.downArrow); + expect(itemList.items[2].focus).toHaveBeenCalledWith('mouse'); + + keyManager.setFocusOrigin('keyboard'); + + keyManager.onKeydown(fakeKeyEvents.upArrow); + expect(itemList.items[1].focus).toHaveBeenCalledWith('keyboard'); + }); + }); describe('ActiveDescendantKeyManager', () => { diff --git a/src/lib/menu/_menu-theme.scss b/src/lib/menu/_menu-theme.scss index 53053c5dcdc0..667a4f5991ca 100644 --- a/src/lib/menu/_menu-theme.scss +++ b/src/lib/menu/_menu-theme.scss @@ -26,7 +26,8 @@ } .mat-menu-item:hover, - .mat-menu-item:focus, + .mat-menu-item.cdk-program-focused, + .mat-menu-item.cdk-keyboard-focused, .mat-menu-item-highlighted { &:not([disabled]) { background: mat-color($background, 'hover'); diff --git a/src/lib/menu/menu-directive.ts b/src/lib/menu/menu-directive.ts index e11a75d8a8d0..7b04f1559595 100644 --- a/src/lib/menu/menu-directive.ts +++ b/src/lib/menu/menu-directive.ts @@ -40,6 +40,7 @@ import {MatMenuItem} from './menu-item'; import {MatMenuPanel} from './menu-panel'; import {MenuPositionX, MenuPositionY} from './menu-positions'; import {coerceBooleanProperty} from '@angular/cdk/coercion'; +import {FocusOrigin} from '@angular/cdk/a11y'; /** Default `mat-menu` options that can be overridden. */ @@ -227,16 +228,17 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnDestroy { } /** - * Focus the first item in the menu. This method is used by the menu trigger - * to focus the first item when the menu is opened by the ENTER key. + * Focus the first item in the menu. + * @param origin Action from which the focus originated. Used to set the correct styling. */ - focusFirstItem(): void { - this._keyManager.setFirstItemActive(); + focusFirstItem(origin: FocusOrigin = 'program'): void { + // TODO(crisbeto): make the origin required when doing breaking changes. + this._keyManager.setFocusOrigin(origin).setFirstItemActive(); } /** - * Resets the active item in the menu. This is used when the menu is opened by mouse, - * allowing the user to start from the first option when pressing the down arrow. + * Resets the active item in the menu. This is used when the menu is opened, allowing + * the user to start from the first option when pressing the down arrow. */ resetActiveItem() { this._keyManager.setActiveItem(-1); diff --git a/src/lib/menu/menu-item.ts b/src/lib/menu/menu-item.ts index 1049052772ba..188ae27d8a02 100644 --- a/src/lib/menu/menu-item.ts +++ b/src/lib/menu/menu-item.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {FocusableOption} from '@angular/cdk/a11y'; +import {FocusableOption, FocusMonitor, FocusOrigin} from '@angular/cdk/a11y'; import { ChangeDetectionStrategy, Component, @@ -64,16 +64,34 @@ export class MatMenuItem extends _MatMenuItemMixinBase /** Whether the menu item acts as a trigger for a sub-menu. */ _triggersSubmenu: boolean = false; - constructor(private _elementRef: ElementRef) { + constructor( + private _elementRef: ElementRef, + // TODO(crisbeto): switch to a required param when doing breaking changes. + private _focusMonitor?: FocusMonitor) { super(); + + if (_focusMonitor) { + // Start monitoring the element so it gets the appropriate focused classes. We want + // to show the focus style for menu items only when the focus was not caused by a + // mouse or touch interaction. + _focusMonitor.monitor(this._getHostElement(), false); + } } /** Focuses the menu item. */ - focus(): void { - this._getHostElement().focus(); + focus(origin: FocusOrigin = 'program'): void { + if (this._focusMonitor) { + this._focusMonitor.focusVia(this._getHostElement(), origin); + } else { + this._getHostElement().focus(); + } } ngOnDestroy() { + if (this._focusMonitor) { + this._focusMonitor.stopMonitoring(this._getHostElement()); + } + this._hovered.complete(); } diff --git a/src/lib/menu/menu-module.ts b/src/lib/menu/menu-module.ts index 19827a471397..f44a8af8226f 100644 --- a/src/lib/menu/menu-module.ts +++ b/src/lib/menu/menu-module.ts @@ -14,6 +14,7 @@ import {MatMenu, MAT_MENU_DEFAULT_OPTIONS} from './menu-directive'; import {MatMenuItem} from './menu-item'; import {MatMenuTrigger, MAT_MENU_SCROLL_STRATEGY_PROVIDER} from './menu-trigger'; import {MatRippleModule} from '@angular/material/core'; +import {A11yModule} from '@angular/cdk/a11y'; @NgModule({ @@ -22,6 +23,7 @@ import {MatRippleModule} from '@angular/material/core'; CommonModule, MatRippleModule, MatCommonModule, + A11yModule, ], exports: [MatMenu, MatMenuItem, MatMenuTrigger, MatCommonModule], declarations: [MatMenu, MatMenuItem, MatMenuTrigger], diff --git a/src/lib/menu/menu-panel.ts b/src/lib/menu/menu-panel.ts index 6a7e47b0ba63..7ed72c66c74b 100644 --- a/src/lib/menu/menu-panel.ts +++ b/src/lib/menu/menu-panel.ts @@ -9,6 +9,7 @@ import {EventEmitter, TemplateRef} from '@angular/core'; import {MenuPositionX, MenuPositionY} from './menu-positions'; import {Direction} from '@angular/cdk/bidi'; +import {FocusOrigin} from '@angular/cdk/a11y'; /** * Interface for a custom menu panel that can be used with `matMenuTriggerFor`. @@ -22,7 +23,7 @@ export interface MatMenuPanel { close: EventEmitter; parentMenu?: MatMenuPanel | undefined; direction?: Direction; - focusFirstItem: () => void; + focusFirstItem: (origin?: FocusOrigin) => void; resetActiveItem: () => void; setPositionClasses: (x: MenuPositionX, y: MenuPositionY) => void; setElevation?(depth: number): void; diff --git a/src/lib/menu/menu-trigger.ts b/src/lib/menu/menu-trigger.ts index e277b51cf774..bc8ba4840476 100644 --- a/src/lib/menu/menu-trigger.ts +++ b/src/lib/menu/menu-trigger.ts @@ -234,19 +234,7 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy { this.menu.direction = this.dir; this._setMenuElevation(); this._setIsMenuOpen(true); - - // If the menu was opened by mouse, we focus the root node, which allows for the keyboard - // interactions to work. Otherwise, if the menu was opened by keyboard, we focus the first item. - if (this._openedByMouse) { - let rootNode = this._overlayRef!.overlayElement.firstElementChild as HTMLElement; - - if (rootNode) { - this.menu.resetActiveItem(); - rootNode.focus(); - } - } else { - this.menu.focusFirstItem(); - } + this.menu.focusFirstItem(this._openedByMouse ? 'mouse' : 'program'); } /** Updates the menu elevation based on the amount of parent menus that it has. */ diff --git a/src/lib/menu/menu.spec.ts b/src/lib/menu/menu.spec.ts index 8fdaffa64048..31159c30c377 100644 --- a/src/lib/menu/menu.spec.ts +++ b/src/lib/menu/menu.spec.ts @@ -230,22 +230,20 @@ describe('MatMenu', () => { expect(fixture.componentInstance.items.last.getLabel()).toBe('Item with an icon'); }); - it('should focus the menu panel root node when it was opened by mouse', () => { + it('should set the proper focus origin when opening by mouse', fakeAsync(() => { const fixture = TestBed.createComponent(SimpleMenu); - fixture.detectChanges(); + spyOn(fixture.componentInstance.items.first, 'focus').and.callThrough(); const triggerEl = fixture.componentInstance.triggerEl.nativeElement; dispatchFakeEvent(triggerEl, 'mousedown'); triggerEl.click(); fixture.detectChanges(); + tick(500); - const panel = overlayContainerElement.querySelector('.mat-menu-panel')!; - - expect(panel).toBeTruthy('Expected the panel to be rendered.'); - expect(document.activeElement).toBe(panel, 'Expected the panel to be focused.'); - }); + expect(fixture.componentInstance.items.first.focus).toHaveBeenCalledWith('mouse'); + })); it('should close the menu when using the CloseScrollStrategy', fakeAsync(() => { const scrolledSubject = new Subject(); @@ -1068,6 +1066,7 @@ describe('MatMenu', () => { instance.alternateTrigger.openMenu(); fixture.detectChanges(); + tick(500); lastMenu = overlay.querySelector('.mat-menu-panel') as HTMLElement; @@ -1121,6 +1120,7 @@ describe('MatMenu', () => { compileTestComponent(); instance.rootTriggerEl.nativeElement.click(); fixture.detectChanges(); + tick(500); expect(overlay.querySelectorAll('.mat-menu-panel').length).toBe(1, 'Expected one open menu'); instance.showLazy = true; @@ -1130,6 +1130,8 @@ describe('MatMenu', () => { dispatchMouseEvent(lazyTrigger, 'mouseenter'); fixture.detectChanges(); + tick(500); + expect(lazyTrigger.classList) .toContain('mat-menu-item-highlighted', 'Expected the trigger to be highlighted'); expect(overlay.querySelectorAll('.mat-menu-panel').length).toBe(2, 'Expected two open menus'); @@ -1157,10 +1159,12 @@ describe('MatMenu', () => { repeaterFixture.componentInstance.rootTriggerEl.nativeElement.click(); repeaterFixture.detectChanges(); + tick(500); expect(overlay.querySelectorAll('.mat-menu-panel').length).toBe(1, 'Expected one open menu'); dispatchMouseEvent(overlay.querySelector('.level-one-trigger')!, 'mouseenter'); repeaterFixture.detectChanges(); + tick(500); expect(overlay.querySelectorAll('.mat-menu-panel').length).toBe(2, 'Expected two open menus'); }));