Skip to content
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

fix(a11y): key manager preventing arrow key events with modifier keys #13503

Merged
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
57 changes: 56 additions & 1 deletion src/cdk/a11y/key-manager/list-key-manager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

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

});
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
3 changes: 2 additions & 1 deletion src/lib/list/selection-list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
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