Skip to content

fix(menu): not handling keyboard events when opened by mouse #4843

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
Oct 10, 2017
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
4 changes: 2 additions & 2 deletions e2e/components/menu-e2e.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,9 @@ describe('menu', () => {
expectFocusOn(page.items(0));
});

it('should not focus the first item when opened with mouse', () => {
it('should focus the panel when opened by mouse', () => {
page.trigger().click();
expectFocusOn(page.trigger());
expectFocusOn(page.menu());
});

it('should focus subsequent items when down arrow is pressed', () => {
Expand Down
8 changes: 8 additions & 0 deletions src/lib/menu/menu-directive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,14 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnDestroy {
this._keyManager.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.
*/
resetActiveItem() {
this._keyManager.setActiveItem(-1);
}

/**
* It's necessary to set position-based classes to ensure the menu panel animation
* folds out from the correct direction.
Expand Down
1 change: 1 addition & 0 deletions src/lib/menu/menu-panel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export interface MatMenuPanel {
parentMenu?: MatMenuPanel | undefined;
direction?: Direction;
focusFirstItem: () => void;
resetActiveItem: () => void;
setPositionClasses: (x: MenuPositionX, y: MenuPositionY) => void;
setElevation?(depth: number): void;
}
14 changes: 10 additions & 4 deletions src/lib/menu/menu-trigger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,10 +225,16 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
this._setMenuElevation();
this._setIsMenuOpen(true);

// Should only set focus if opened via the keyboard, so keyboard users can
// can easily navigate menu items. According to spec, mouse users should not
// see the focus style.
if (!this._openedByMouse) {
// 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();
}
}
Expand Down
1 change: 1 addition & 0 deletions src/lib/menu/menu.html
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
(click)="close.emit('click')"
[@transformMenu]="_panelAnimationState"
(@transformMenu.done)="_onAnimationDone($event)"
tabindex="-1"
role="menu">
<div class="mat-menu-content" [@fadeInItems]="'showing'">
<ng-content></ng-content>
Expand Down
1 change: 1 addition & 0 deletions src/lib/menu/menu.scss
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ $mat-menu-submenu-indicator-size: 10px !default;
@include mat-menu-positions();
max-height: calc(100vh - #{$mat-menu-item-height});
border-radius: $mat-menu-border-radius;
outline: 0;

// Prevent the user from interacting while the panel is still animating.
// This avoids issues where the user could accidentally open a sub-menu,
Expand Down
33 changes: 19 additions & 14 deletions src/lib/menu/menu.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
dispatchEvent,
createKeyboardEvent,
createMouseEvent,
dispatchFakeEvent,
} from '@angular/cdk/testing';


Expand Down Expand Up @@ -191,6 +192,23 @@ 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', () => {
const fixture = TestBed.createComponent(SimpleMenu);

fixture.detectChanges();

const triggerEl = fixture.componentInstance.triggerEl.nativeElement;

dispatchFakeEvent(triggerEl, 'mousedown');
triggerEl.click();
fixture.detectChanges();

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.');
});

describe('positions', () => {
let fixture: ComponentFixture<PositionedMenu>;
let panel: HTMLElement;
Expand Down Expand Up @@ -811,20 +829,6 @@ describe('MatMenu', () => {
.toBe(true, 'Expected focus to be back inside the root menu');
});

it('should not shift focus to the sub-menu when it was opened by hover', () => {
compileTestComponent();
instance.rootTriggerEl.nativeElement.click();
fixture.detectChanges();

const levelOneTrigger = overlay.querySelector('#level-one-trigger')! as HTMLElement;

dispatchMouseEvent(levelOneTrigger, 'mouseenter');
fixture.detectChanges();

expect(overlay.querySelectorAll('.mat-menu-panel')[1].contains(document.activeElement))
.toBe(false, 'Expected focus to not be inside the nested menu');
});

it('should position the sub-menu to the right edge of the trigger in ltr', () => {
compileTestComponent();
instance.rootTriggerEl.nativeElement.style.position = 'fixed';
Expand Down Expand Up @@ -1179,6 +1183,7 @@ class CustomMenuPanel implements MatMenuPanel {
@ViewChild(TemplateRef) templateRef: TemplateRef<any>;
@Output() close = new EventEmitter<void | 'click' | 'keydown'>();
focusFirstItem = () => {};
resetActiveItem = () => {};
setPositionClasses = () => {};
}

Expand Down