Skip to content

fix(menu): always focus first menu item #9383

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

Merged
merged 1 commit into from
Jan 23, 2018
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
16 changes: 14 additions & 2 deletions src/cdk/a11y/focus-key-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand All @@ -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<T> extends ListKeyManager<FocusableOption & T> {
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.
Expand All @@ -27,7 +39,7 @@ export class FocusKeyManager<T> extends ListKeyManager<FocusableOption & T> {
super.setActiveItem(index);

if (this.activeItem) {
this.activeItem.focus();
this.activeItem.focus(this._origin);
}
}
}
18 changes: 17 additions & 1 deletion src/cdk/a11y/list-key-manager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
}

Expand Down Expand Up @@ -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', () => {
Expand Down
3 changes: 2 additions & 1 deletion src/lib/menu/_menu-theme.scss
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
14 changes: 8 additions & 6 deletions src/lib/menu/menu-directive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down Expand Up @@ -229,16 +230,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);
Expand Down
26 changes: 22 additions & 4 deletions src/lib/menu/menu-item.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment here like

// We want to show the focus style for menu items only when the focus was not caused by
// a mouse or touch interaction. 

?

// 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();
}

Expand Down
2 changes: 2 additions & 0 deletions src/lib/menu/menu-module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -22,6 +23,7 @@ import {MatRippleModule} from '@angular/material/core';
CommonModule,
MatRippleModule,
MatCommonModule,
A11yModule,
],
exports: [MatMenu, MatMenuItem, MatMenuTrigger, MatCommonModule],
declarations: [MatMenu, MatMenuItem, MatMenuTrigger],
Expand Down
3 changes: 2 additions & 1 deletion src/lib/menu/menu-panel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand All @@ -22,7 +23,7 @@ export interface MatMenuPanel {
close: EventEmitter<void | 'click' | 'keydown'>;
parentMenu?: MatMenuPanel | undefined;
direction?: Direction;
focusFirstItem: () => void;
focusFirstItem: (origin?: FocusOrigin) => void;
resetActiveItem: () => void;
setPositionClasses: (x: MenuPositionX, y: MenuPositionY) => void;
setElevation?(depth: number): void;
Expand Down
14 changes: 1 addition & 13 deletions src/lib/menu/menu-trigger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down
18 changes: 11 additions & 7 deletions src/lib/menu/menu.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -1068,6 +1066,7 @@ describe('MatMenu', () => {

instance.alternateTrigger.openMenu();
fixture.detectChanges();
tick(500);

lastMenu = overlay.querySelector('.mat-menu-panel') as HTMLElement;

Expand Down Expand Up @@ -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;
Expand All @@ -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');
Expand Down Expand Up @@ -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');
}));

Expand Down