Skip to content

Commit

Permalink
fix(a11y): key manager preventing arrow key events with modifier keys
Browse files Browse the repository at this point in the history
* 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 angular#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 angular#13496.
  • Loading branch information
crisbeto committed Oct 9, 2018
1 parent 4b15b78 commit 49ad9e1
Show file tree
Hide file tree
Showing 5 changed files with 88 additions and 17 deletions.
55 changes: 55 additions & 0 deletions src/cdk/a11y/key-manager/list-key-manager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,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);
Expand Down Expand Up @@ -334,6 +344,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: 'altKey' | 'ctrlKey' | 'metaKey' | 'shiftKey') {
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();
}

});
Expand Down
35 changes: 23 additions & 12 deletions src/cdk/a11y/key-manager/list-key-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -42,6 +45,7 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
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
Expand Down Expand Up @@ -118,6 +122,15 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
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.
Expand Down Expand Up @@ -188,45 +201,43 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
*/
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:
this.tabOut.next();
return;

case DOWN_ARROW:
if (this._vertical) {
if (this._vertical && isModifierAllowed) {
this.setNextItemActive();
break;
} else {
return;
}

case UP_ARROW:
if (this._vertical) {
if (this._vertical && isModifierAllowed) {
this.setPreviousItemActive();
break;
} else {
return;
}

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;
Expand Down
9 changes: 6 additions & 3 deletions src/cdk/testing/event-objects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,14 @@ export function createTouchEvent(type: string, pageX = 0, pageY = 0) {
/** Dispatches a keydown event from an element. */
export function createKeyboardEvent(type: string, keyCode: number, target?: Element, key?: string) {
let event = document.createEvent('KeyboardEvent') as any;
// Firefox does not support `initKeyboardEvent`, but supports `initKeyEvent`.
let initEventFn = (event.initKeyEvent || event.initKeyboardEvent).bind(event);
let originalPreventDefault = event.preventDefault;

initEventFn(type, true, true, window, 0, 0, 0, 0, 0, keyCode);
// Firefox does not support `initKeyboardEvent`, but supports `initKeyEvent`.
if (event.initKeyEvent) {
event.initKeyEvent(type, true, true, window, 0, 0, 0, 0, 0, keyCode);
} else {
event.initKeyboardEvent(type, true, true, window, 0, key, 0, '', false);
}

// Webkit Browsers don't set the keyCode when calling the init function.
// See related bug https://bugs.webkit.org/show_bug.cgi?id=16735
Expand Down
3 changes: 2 additions & 1 deletion src/lib/list/selection-list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,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);
Expand Down
3 changes: 2 additions & 1 deletion src/lib/select/select.ts
Original file line number Diff line number Diff line change
Expand Up @@ -864,7 +864,8 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
this._keyManager = new ActiveDescendantKeyManager<MatOption>(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
Expand Down

0 comments on commit 49ad9e1

Please sign in to comment.