Skip to content

feat(menu): add keyboard events and accessibility #937

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

feat(menu): add keyboard events and accessibility #937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 13 additions & 0 deletions e2e/components/menu/menu-page.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ export class MenuPage {

menu() { return element(by.css('.md-menu')); }

start() { return element(by.id('start')); }

trigger() { return element(by.id('trigger')); }

triggerTwo() { return element(by.id('trigger-two')); }
Expand All @@ -32,6 +34,17 @@ export class MenuPage {

combinedMenu() { return element(by.css('.md-menu.combined')); }

// TODO(kara): move to common testing utility
pressKey(key: any): void {
browser.actions().sendKeys(key).perform();
}

// TODO(kara): move to common testing utility
expectFocusOn(el: ElementFinder): void {
expect(browser.driver.switchTo().activeElement().getInnerHtml())
.toBe(el.getInnerHtml());
}

expectMenuPresent(expected: boolean) {
return browser.isElementPresent(by.css('.md-menu')).then((isPresent) => {
expect(isPresent).toBe(expected);
Expand Down
137 changes: 134 additions & 3 deletions e2e/components/menu/menu.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ describe('menu', () => {
page.trigger().click();
Copy link
Member

Choose a reason for hiding this comment

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

Have you found it generally more difficult to add unit tests for the menu than e2e tests? The e2e tests are good at capturing the "real" behavior, but they take much longer to run (and are flakier), so covering as much of the behavior as possible in unit tests will generally make maintenance much easier later.


page.expectMenuPresent(true);
expect(page.menu().getText()).toEqual("One\nTwo\nThree");
expect(page.menu().getText()).toEqual("One\nTwo\nThree\nFour");
});

it('should close menu when area outside menu is clicked', () => {
Expand Down Expand Up @@ -45,14 +45,14 @@ describe('menu', () => {

it('should support multiple triggers opening the same menu', () => {
page.triggerTwo().click();
expect(page.menu().getText()).toEqual("One\nTwo\nThree");
expect(page.menu().getText()).toEqual("One\nTwo\nThree\nFour");
page.expectMenuAlignedWith(page.menu(), 'trigger-two');

page.body().click();
page.expectMenuPresent(false);

page.trigger().click();
expect(page.menu().getText()).toEqual("One\nTwo\nThree");
expect(page.menu().getText()).toEqual("One\nTwo\nThree\nFour");
page.expectMenuAlignedWith(page.menu(), 'trigger');

page.body().click();
Expand All @@ -66,6 +66,137 @@ describe('menu', () => {
});
});

describe('keyboard events', () => {
beforeEach(() => {
// click start button to avoid tabbing past navigation
page.start().click();
page.pressKey(protractor.Key.TAB);
});

it('should auto-focus the first item when opened with keyboard', () => {
page.pressKey(protractor.Key.ENTER);
page.expectFocusOn(page.items(0));
});

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

it('should focus subsequent items when down arrow is pressed', () => {
page.pressKey(protractor.Key.ENTER);
page.pressKey(protractor.Key.DOWN);
page.expectFocusOn(page.items(1));
});

it('should focus previous items when up arrow is pressed', () => {
page.pressKey(protractor.Key.ENTER);
page.pressKey(protractor.Key.DOWN);
page.pressKey(protractor.Key.UP);
page.expectFocusOn(page.items(0));
});

it('should focus subsequent items when tab is pressed', () => {
page.pressKey(protractor.Key.ENTER);
page.pressKey(protractor.Key.TAB);
page.expectFocusOn(page.items(1));
});

it('should focus previous items when shift-tab is pressed', () => {
page.pressKey(protractor.Key.ENTER);
page.pressKey(protractor.Key.TAB);
// need a protractor "chord" to hit shift-tab simultaneously
page.pressKey(protractor.Key.chord(protractor.Key.SHIFT, protractor.Key.TAB));
page.expectFocusOn(page.items(0));
});

it('should handle a mix of tabs and arrow presses', () => {
page.pressKey(protractor.Key.ENTER);
page.pressKey(protractor.Key.TAB);
page.pressKey(protractor.Key.UP);
page.expectFocusOn(page.items(0));

page.pressKey(protractor.Key.DOWN);
page.pressKey(protractor.Key.chord(protractor.Key.SHIFT, protractor.Key.TAB));
page.expectFocusOn(page.items(0));
});

it('should skip disabled items using arrow keys', () => {
page.pressKey(protractor.Key.ENTER);
page.pressKey(protractor.Key.DOWN);
page.pressKey(protractor.Key.DOWN);
page.expectFocusOn(page.items(3));

page.pressKey(protractor.Key.UP);
page.expectFocusOn(page.items(1));
});

it('should skip disabled items using tabs', () => {
page.pressKey(protractor.Key.ENTER);
page.pressKey(protractor.Key.TAB);
page.pressKey(protractor.Key.TAB);
page.expectFocusOn(page.items(3));

page.pressKey(protractor.Key.chord(protractor.Key.SHIFT, protractor.Key.TAB));
page.expectFocusOn(page.items(1));
});

it('should close the menu when tabbing past items', () => {
page.pressKey(protractor.Key.ENTER);
page.pressKey(protractor.Key.TAB);
page.pressKey(protractor.Key.TAB);
page.pressKey(protractor.Key.TAB);
page.expectMenuPresent(false);

page.start().click();
page.pressKey(protractor.Key.TAB);
page.pressKey(protractor.Key.ENTER);
page.pressKey(protractor.Key.chord(protractor.Key.SHIFT, protractor.Key.TAB));
page.expectMenuPresent(false);
});

it('should close the menu when arrow keying past items', () => {
page.pressKey(protractor.Key.ENTER);
page.pressKey(protractor.Key.DOWN);
page.pressKey(protractor.Key.DOWN);
page.pressKey(protractor.Key.DOWN);
page.expectMenuPresent(false);

page.start().click();
page.pressKey(protractor.Key.TAB);
page.pressKey(protractor.Key.ENTER);
page.pressKey(protractor.Key.UP);
page.expectMenuPresent(false);
});

it('should focus before and after trigger when tabbing past items', () => {
page.pressKey(protractor.Key.ENTER);
page.pressKey(protractor.Key.TAB);
page.pressKey(protractor.Key.TAB);
page.pressKey(protractor.Key.TAB);
page.expectFocusOn(page.triggerTwo());

// navigate back to trigger
page.pressKey(protractor.Key.chord(protractor.Key.SHIFT, protractor.Key.TAB));
page.pressKey(protractor.Key.ENTER);

page.pressKey(protractor.Key.chord(protractor.Key.SHIFT, protractor.Key.TAB));
page.expectFocusOn(page.start());
});

it('should focus on trigger when arrow keying past items', () => {
page.pressKey(protractor.Key.ENTER);
page.pressKey(protractor.Key.DOWN);
page.pressKey(protractor.Key.DOWN);
page.pressKey(protractor.Key.DOWN);
page.expectFocusOn(page.trigger());

page.pressKey(protractor.Key.ENTER);
page.pressKey(protractor.Key.UP);
page.expectFocusOn(page.trigger());
});
});

describe('position - ', () => {

it('should default menu alignment to "after below" when not set', () => {
Expand Down
9 changes: 6 additions & 3 deletions src/components/menu/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

### Not yet implemented

- Keyboard events: up arrow, down arrow, enter
- `prevent-close` option, to turn off automatic menu close when clicking outside the menu
- Custom offset support
- Menu groupings (which menus are allowed to open together)
Expand Down Expand Up @@ -136,7 +135,12 @@ Output:
### Accessibility

The menu adds `role="menu"` to the main menu element and `role="menuitem"` to each menu item. It
also adds `aria-hasPopup="true"` to the trigger element.
also adds `aria-hasPopup="true"` to the trigger element.

#### Keyboard events:
- <kbd>DOWN_ARROW</kbd> or <kbd>TAB</kbd>: Focus next menu item
- <kbd>UP_ARROW</kbd> or <kbd>SHIFT_TAB</kbd>: Focus previous menu item
- <kbd>ENTER</kbd>: Select focused item

### Menu attributes

Expand Down Expand Up @@ -165,4 +169,3 @@ also adds `aria-hasPopup="true"` to the trigger element.
| `destroyMenu()` | `Promise<void>` | Destroys the menu overlay completely.



77 changes: 72 additions & 5 deletions src/components/menu/menu-directive.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,21 @@
// TODO(kara): keyboard events for menu navigation
// TODO(kara): prevent-close functionality

import {
Attribute,
Component,
ContentChildren,
EventEmitter,
Input,
Output,
QueryList,
TemplateRef,
ViewChild,
ViewEncapsulation
} from '@angular/core';
import {MenuPositionX, MenuPositionY} from './menu-positions';
import {MdMenuInvalidPositionX, MdMenuInvalidPositionY} from './menu-errors';
import {MdMenuItem} from './menu-item';
import {UP_ARROW, DOWN_ARROW, TAB} from '@angular2-material/core/keyboard/keycodes';

@Component({
moduleId: module.id,
Expand All @@ -25,6 +28,7 @@ import {MdMenuInvalidPositionX, MdMenuInvalidPositionY} from './menu-errors';
})
export class MdMenu {
_showClickCatcher: boolean = false;
private _focusedItemIndex: number = 0;

// config object to be passed into the menu's ngClass
_classList: Object;
Expand All @@ -33,6 +37,7 @@ export class MdMenu {
positionY: MenuPositionY = 'below';

@ViewChild(TemplateRef) templateRef: TemplateRef<any>;
@ContentChildren(MdMenuItem) items: QueryList<MdMenuItem>;

constructor(@Attribute('x-position') posX: MenuPositionX,
@Attribute('y-position') posY: MenuPositionY) {
Expand Down Expand Up @@ -65,6 +70,72 @@ export class MdMenu {
this._showClickCatcher = bool;
}

/**
* 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.
* TODO: internal
*/
_focusFirstItem() { this.items.first.focus(); }
Copy link
Member

Choose a reason for hiding this comment

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

I'd make the function description something like

/**
 * Focus the first item in the menu. This method is intended to be used by the menu trigger
 * to focus the first item when the menu is opened by the ENTER key.
 * TODO: internal
 */


// TODO(kara): update this when (keydown.downArrow) testability is fixed
// TODO: internal
_handleKeydown(event: KeyboardEvent): void {
if (event.keyCode === DOWN_ARROW) {
this._focusNextItem();
} else if (event.keyCode === UP_ARROW) {
this._focusPreviousItem();
} else if (event.keyCode === TAB) {
this._handleTabKeypress(event.shiftKey);
}
}

/**
* When the tab key is pressed (changing focus natively), this function syncs up
* the focusedItemIndex to match the currently focused item. If the shift key is
* also pressed, we know that the focus has changed to the previous tabindex. If not,
* focus has changed to the next tabindex.
*/
private _handleTabKeypress(shiftPressed: boolean): void {
shiftPressed ? this._updateFocusedItemIndex(-1) : this._updateFocusedItemIndex(1);
}

/**
* This emits a close event to which the trigger is subscribed. When emitted, the
* trigger will close the menu.
*/
private _emitCloseEvent(): void {
this._focusedItemIndex = 0;
this.close.emit(null);
}

// When focus would shift past the start or end of the menu, close the menu.
private _closeIfFocusLeavesMenu(): void {
if (this._focusedItemIndex >= this.items.length || this._focusedItemIndex < 0) {
this._emitCloseEvent();
Copy link
Member

Choose a reason for hiding this comment

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

Why do you emit a close event here? AFAICT, it doesn't actually close the menu, does it?

Copy link
Member

Choose a reason for hiding this comment

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

If you end up removing this emit, I think you can omit this function entirely and simply prevent the focus from ever going out-of-bounds in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does close the menu. If someone tries to tab or arrow past the menu items in either direction, the menu should close. The trigger is subscribed to the "close" event. When it's emitted from the menu, the trigger will close it.

}
}

private _focusNextItem(): void {
this._updateFocusedItemIndex(1);
this.items.toArray()[this._focusedItemIndex].focus();
}

private _focusPreviousItem(): void {
this._updateFocusedItemIndex(-1);
this.items.toArray()[this._focusedItemIndex].focus();
}

private _updateFocusedItemIndex(delta: number, itemArray: MdMenuItem[] = this.items.toArray()) {
this._focusedItemIndex += delta;
this._closeIfFocusLeavesMenu();

// skip all disabled menu items recursively until an active one
// is reached or the menu closes for overreaching bounds
while (itemArray[this._focusedItemIndex].disabled) {
this._updateFocusedItemIndex(delta, itemArray);
}
}

private _setPositionX(pos: MenuPositionX): void {
if ( pos !== 'before' && pos !== 'after') {
throw new MdMenuInvalidPositionX();
Expand All @@ -78,8 +149,4 @@ export class MdMenu {
}
this.positionY = pos;
}

private _emitCloseEvent(): void {
this.close.emit(null);
}
}
Loading