From 4e8a4dfbe201c06670cd0c7c21c29cdf8d83dc65 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Tue, 9 Oct 2018 10:34:24 +0300 Subject: [PATCH] fix(a11y): key manager preventing arrow key events with modifier keys * Skips handling arrow key events that have a modifier key. Previously we would prevent the default action which can get in the way of some of the native keyboard shortcuts (e.g. marking all of the text using shift + up arrow). This has come up before in #11987, however these changes expand it to the key manager. * Adds an API that allows consumers to opt into having the key manager handle some modifier keys. This is an exception for some of our components where we have custom functionality for shift + arrow key. * Fixes the `metaKey` always being set to true on our fake keyboard events. Fixes #13496. --- .../a11y/key-manager/list-key-manager.spec.ts | 57 ++++++++++++++++++- src/cdk/a11y/key-manager/list-key-manager.ts | 35 ++++++++---- src/lib/list/selection-list.ts | 3 +- src/lib/select/select.ts | 3 +- 4 files changed, 83 insertions(+), 15 deletions(-) diff --git a/src/cdk/a11y/key-manager/list-key-manager.spec.ts b/src/cdk/a11y/key-manager/list-key-manager.spec.ts index 83966bbea41b..1c1ba2ce8022 100644 --- a/src/cdk/a11y/key-manager/list-key-manager.spec.ts +++ b/src/cdk/a11y/key-manager/list-key-manager.spec.ts @@ -5,7 +5,7 @@ import {fakeAsync, tick} from '@angular/core/testing'; import {createKeyboardEvent} from '@angular/cdk/testing'; import {ActiveDescendantKeyManager} from './activedescendant-key-manager'; import {FocusKeyManager} from './focus-key-manager'; -import {ListKeyManager} from './list-key-manager'; +import {ListKeyManager, ListKeyManagerModifierKey} from './list-key-manager'; import {FocusOrigin} from '../focus-monitor/focus-monitor'; import {Subject} from 'rxjs'; @@ -103,6 +103,16 @@ describe('Key managers', () => { expect(spy).toHaveBeenCalled(); }); + it('should emit tabOut when the tab key is pressed with a modifier', () => { + const spy = jasmine.createSpy('tabOut spy'); + keyManager.tabOut.pipe(take(1)).subscribe(spy); + + Object.defineProperty(fakeKeyEvents.tab, 'shiftKey', {get: () => true}); + keyManager.onKeydown(fakeKeyEvents.tab); + + expect(spy).toHaveBeenCalled(); + }); + it('should emit an event whenever the active item changes', () => { const spy = jasmine.createSpy('change spy'); const subscription = keyManager.change.subscribe(spy); @@ -338,6 +348,51 @@ describe('Key managers', () => { keyManager.onKeydown(this.prevKeyEvent); expect(this.prevKeyEvent.defaultPrevented).toBe(true); }); + + it('should not do anything for arrow keys if the alt key is held down', () => { + runModifierKeyTest(this, 'altKey'); + }); + + it('should not do anything for arrow keys if the control key is held down', () => { + runModifierKeyTest(this, 'ctrlKey'); + }); + + it('should not do anything for arrow keys if the meta key is held down', () => { + runModifierKeyTest(this, 'metaKey'); + }); + + it('should not do anything for arrow keys if the shift key is held down', () => { + runModifierKeyTest(this, 'shiftKey'); + }); + + } + + /** Runs the test that asserts that we handle modifier keys correctly. */ + function runModifierKeyTest(context: { + nextKeyEvent: KeyboardEvent, + prevKeyEvent: KeyboardEvent + }, modifier: ListKeyManagerModifierKey) { + const initialActiveIndex = keyManager.activeItemIndex; + const spy = jasmine.createSpy('change spy'); + const subscription = keyManager.change.subscribe(spy); + + expect(context.nextKeyEvent.defaultPrevented).toBe(false); + expect(context.prevKeyEvent.defaultPrevented).toBe(false); + + Object.defineProperty(context.nextKeyEvent, modifier, {get: () => true}); + Object.defineProperty(context.prevKeyEvent, modifier, {get: () => true}); + + keyManager.onKeydown(context.nextKeyEvent); + expect(context.nextKeyEvent.defaultPrevented).toBe(false); + expect(keyManager.activeItemIndex).toBe(initialActiveIndex); + expect(spy).not.toHaveBeenCalled(); + + keyManager.onKeydown(context.prevKeyEvent); + expect(context.prevKeyEvent.defaultPrevented).toBe(false); + expect(keyManager.activeItemIndex).toBe(initialActiveIndex); + expect(spy).not.toHaveBeenCalled(); + + subscription.unsubscribe(); } }); diff --git a/src/cdk/a11y/key-manager/list-key-manager.ts b/src/cdk/a11y/key-manager/list-key-manager.ts index 3e3e1e8f76e6..ed7b426c6762 100644 --- a/src/cdk/a11y/key-manager/list-key-manager.ts +++ b/src/cdk/a11y/key-manager/list-key-manager.ts @@ -30,6 +30,9 @@ export interface ListKeyManagerOption { getLabel?(): string; } +/** Modifier keys handled by the ListKeyManager. */ +export type ListKeyManagerModifierKey = 'altKey' | 'ctrlKey' | 'metaKey' | 'shiftKey'; + /** * This class manages keyboard events for selectable lists. If you pass it a query list * of items, it will set the active item correctly when arrow events occur. @@ -42,6 +45,7 @@ export class ListKeyManager { private _typeaheadSubscription = Subscription.EMPTY; private _vertical = true; private _horizontal: 'ltr' | 'rtl' | null; + private _allowedModifierKeys: ListKeyManagerModifierKey[] = []; /** * Predicate function that can be used to check whether an item should be skipped @@ -118,6 +122,15 @@ export class ListKeyManager { return this; } + /** + * Modifier keys which are allowed to be held down and whose default actions will be prevented + * as the user is pressing the arrow keys. Defaults to not allowing any modifier keys. + */ + withAllowedModifierKeys(keys: ListKeyManagerModifierKey[]): this { + this._allowedModifierKeys = keys; + return this; + } + /** * Turns on typeahead mode which allows users to set the active item by typing. * @param debounceInterval Time to wait after the last keystroke before setting the active item. @@ -188,6 +201,10 @@ export class ListKeyManager { */ onKeydown(event: KeyboardEvent): void { const keyCode = event.keyCode; + const modifiers: ListKeyManagerModifierKey[] = ['altKey', 'ctrlKey', 'metaKey', 'shiftKey']; + const isModifierAllowed = modifiers.every(modifier => { + return !event[modifier] || this._allowedModifierKeys.indexOf(modifier) > -1; + }); switch (keyCode) { case TAB: @@ -195,7 +212,7 @@ export class ListKeyManager { return; case DOWN_ARROW: - if (this._vertical) { + if (this._vertical && isModifierAllowed) { this.setNextItemActive(); break; } else { @@ -203,7 +220,7 @@ export class ListKeyManager { } case UP_ARROW: - if (this._vertical) { + if (this._vertical && isModifierAllowed) { this.setPreviousItemActive(); break; } else { @@ -211,22 +228,16 @@ export class ListKeyManager { } case RIGHT_ARROW: - if (this._horizontal === 'ltr') { - this.setNextItemActive(); - break; - } else if (this._horizontal === 'rtl') { - this.setPreviousItemActive(); + if (this._horizontal && isModifierAllowed) { + this._horizontal === 'rtl' ? this.setPreviousItemActive() : this.setNextItemActive(); break; } else { return; } case LEFT_ARROW: - if (this._horizontal === 'ltr') { - this.setPreviousItemActive(); - break; - } else if (this._horizontal === 'rtl') { - this.setNextItemActive(); + if (this._horizontal && isModifierAllowed) { + this._horizontal === 'rtl' ? this.setNextItemActive() : this.setPreviousItemActive(); break; } else { return; diff --git a/src/lib/list/selection-list.ts b/src/lib/list/selection-list.ts index e01f216eec1c..644ddc95826f 100644 --- a/src/lib/list/selection-list.ts +++ b/src/lib/list/selection-list.ts @@ -345,7 +345,8 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements Focu .withTypeAhead() // Allow disabled items to be focusable. For accessibility reasons, there must be a way for // screenreader users, that allows reading the different options of the list. - .skipPredicate(() => false); + .skipPredicate(() => false) + .withAllowedModifierKeys(['shiftKey']); if (this._tempValues) { this._setOptionsFromValues(this._tempValues); diff --git a/src/lib/select/select.ts b/src/lib/select/select.ts index 7a3bbd02d0f5..9cafab774f3a 100644 --- a/src/lib/select/select.ts +++ b/src/lib/select/select.ts @@ -864,7 +864,8 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit, this._keyManager = new ActiveDescendantKeyManager(this.options) .withTypeAhead() .withVerticalOrientation() - .withHorizontalOrientation(this._isRtl() ? 'rtl' : 'ltr'); + .withHorizontalOrientation(this._isRtl() ? 'rtl' : 'ltr') + .withAllowedModifierKeys(['shiftKey']); this._keyManager.tabOut.pipe(takeUntil(this._destroy)).subscribe(() => { // Restore focus to the trigger before closing. Ensures that the focus