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

feat(select): allow focusing items by typing #2907

Merged
merged 3 commits into from
Aug 8, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
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 src/cdk/a11y/activedescendant-key-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@
* found in the LICENSE file at https://angular.io/license
*/

import {ListKeyManager, CanDisable} from './list-key-manager';
import {ListKeyManager, ListKeyManagerItem} from './list-key-manager';

/**
* This is the interface for highlightable items (used by the ActiveDescendantKeyManager).
* Each item must know how to style itself as active or inactive and whether or not it is
* currently disabled.
*/
export interface Highlightable extends CanDisable {
export interface Highlightable extends ListKeyManagerItem {
setActiveStyles(): void;
setInactiveStyles(): void;
}
Expand Down
4 changes: 2 additions & 2 deletions src/cdk/a11y/focus-key-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@
*/

import {QueryList} from '@angular/core';
import {ListKeyManager, CanDisable} from './list-key-manager';
import {ListKeyManager, ListKeyManagerItem} from './list-key-manager';

/**
* This is the interface for focusable items (used by the FocusKeyManager).
* Each item must know how to focus itself and whether or not it is currently disabled.
*/
export interface Focusable extends CanDisable {
export interface Focusable extends ListKeyManagerItem {
focus(): void;
}

Expand Down
112 changes: 84 additions & 28 deletions src/cdk/a11y/list-key-manager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@ import {first} from '../rxjs/index';


class FakeFocusable {
constructor(private _label = '') { }
disabled = false;
focus() {}
getLabel() { return this._label; }
}

class FakeHighlightable {
Expand All @@ -25,6 +27,7 @@ class FakeQueryList<T> extends QueryList<T> {
toArray() {
return this.items;
}
get first() { return this.items[0]; }
}


Expand All @@ -43,7 +46,7 @@ describe('Key managers', () => {
downArrow: createKeyboardEvent('keydown', DOWN_ARROW),
upArrow: createKeyboardEvent('keydown', UP_ARROW),
tab: createKeyboardEvent('keydown', TAB),
unsupported: createKeyboardEvent('keydown', 65) // corresponds to the letter "a"
unsupported: createKeyboardEvent('keydown', 192) // corresponds to the tilde character (~)
};
});

Expand All @@ -52,7 +55,11 @@ describe('Key managers', () => {
let keyManager: ListKeyManager<FakeFocusable>;

beforeEach(() => {
itemList.items = [new FakeFocusable(), new FakeFocusable(), new FakeFocusable()];
itemList.items = [
new FakeFocusable('one'),
new FakeFocusable('two'),
new FakeFocusable('three')
];
keyManager = new ListKeyManager<FakeFocusable>(itemList);

// first item is already focused
Expand Down Expand Up @@ -383,6 +390,55 @@ describe('Key managers', () => {

});

describe('typeahead mode', () => {
const debounceInterval = 300;

beforeEach(() => {
keyManager.withTypeAhead(debounceInterval);
keyManager.setActiveItem(-1);
});

it('should debounce the input key presses', fakeAsync(() => {
keyManager.onKeydown(createKeyboardEvent('keydown', 79)); // types "o"
keyManager.onKeydown(createKeyboardEvent('keydown', 78)); // types "n"
keyManager.onKeydown(createKeyboardEvent('keydown', 69)); // types "e"

expect(keyManager.activeItem).not.toBe(itemList.items[0]);

tick(debounceInterval);

expect(keyManager.activeItem).toBe(itemList.items[0]);
}));

it('should focus the first item that starts with a letter', fakeAsync(() => {
keyManager.onKeydown(createKeyboardEvent('keydown', 84)); // types "t"

tick(debounceInterval);

expect(keyManager.activeItem).toBe(itemList.items[1]);
}));

it('should focus the first item that starts with sequence of letters', fakeAsync(() => {
keyManager.onKeydown(createKeyboardEvent('keydown', 84)); // types "t"
keyManager.onKeydown(createKeyboardEvent('keydown', 72)); // types "h"

tick(debounceInterval);

expect(keyManager.activeItem).toBe(itemList.items[2]);
}));

it('should cancel any pending timers if a navigation key is pressed', fakeAsync(() => {
keyManager.onKeydown(createKeyboardEvent('keydown', 84)); // types "t"
keyManager.onKeydown(createKeyboardEvent('keydown', 72)); // types "h"
keyManager.onKeydown(fakeKeyEvents.downArrow);

tick(debounceInterval);

expect(keyManager.activeItem).toBe(itemList.items[0]);
}));

});

});

describe('FocusKeyManager', () => {
Expand All @@ -400,40 +456,40 @@ describe('Key managers', () => {
spyOn(itemList.items[2], 'focus');
});

it('should focus subsequent items when down arrow is pressed', () => {
keyManager.onKeydown(fakeKeyEvents.downArrow);
it('should focus subsequent items when down arrow is pressed', () => {
keyManager.onKeydown(fakeKeyEvents.downArrow);

expect(itemList.items[0].focus).not.toHaveBeenCalled();
expect(itemList.items[1].focus).toHaveBeenCalledTimes(1);
expect(itemList.items[2].focus).not.toHaveBeenCalled();
expect(itemList.items[0].focus).not.toHaveBeenCalled();
expect(itemList.items[1].focus).toHaveBeenCalledTimes(1);
expect(itemList.items[2].focus).not.toHaveBeenCalled();

keyManager.onKeydown(fakeKeyEvents.downArrow);
expect(itemList.items[0].focus).not.toHaveBeenCalled();
expect(itemList.items[1].focus).toHaveBeenCalledTimes(1);
expect(itemList.items[2].focus).toHaveBeenCalledTimes(1);
});
keyManager.onKeydown(fakeKeyEvents.downArrow);
expect(itemList.items[0].focus).not.toHaveBeenCalled();
expect(itemList.items[1].focus).toHaveBeenCalledTimes(1);
expect(itemList.items[2].focus).toHaveBeenCalledTimes(1);
});

it('should focus previous items when up arrow is pressed', () => {
keyManager.onKeydown(fakeKeyEvents.downArrow);
it('should focus previous items when up arrow is pressed', () => {
keyManager.onKeydown(fakeKeyEvents.downArrow);

expect(itemList.items[0].focus).not.toHaveBeenCalled();
expect(itemList.items[1].focus).toHaveBeenCalledTimes(1);
expect(itemList.items[0].focus).not.toHaveBeenCalled();
expect(itemList.items[1].focus).toHaveBeenCalledTimes(1);

keyManager.onKeydown(fakeKeyEvents.upArrow);
keyManager.onKeydown(fakeKeyEvents.upArrow);

expect(itemList.items[0].focus).toHaveBeenCalledTimes(1);
expect(itemList.items[1].focus).toHaveBeenCalledTimes(1);
});
expect(itemList.items[0].focus).toHaveBeenCalledTimes(1);
expect(itemList.items[1].focus).toHaveBeenCalledTimes(1);
});

it('should allow setting the focused item without calling focus', () => {
expect(keyManager.activeItemIndex)
.toBe(0, `Expected first item of the list to be active.`);
it('should allow setting the focused item without calling focus', () => {
expect(keyManager.activeItemIndex)
.toBe(0, `Expected first item of the list to be active.`);

keyManager.updateActiveItemIndex(1);
expect(keyManager.activeItemIndex)
.toBe(1, `Expected activeItemIndex to update after calling updateActiveItemIndex().`);
expect(itemList.items[1].focus).not.toHaveBeenCalledTimes(1);
});
keyManager.updateActiveItemIndex(1);
expect(keyManager.activeItemIndex)
.toBe(1, `Expected activeItemIndex to update after calling updateActiveItemIndex().`);
expect(itemList.items[1].focus).not.toHaveBeenCalledTimes(1);
});

});

Expand Down
76 changes: 51 additions & 25 deletions src/cdk/a11y/list-key-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,42 +9,77 @@
import {QueryList} from '@angular/core';
import {Observable} from 'rxjs/Observable';
import {Subject} from 'rxjs/Subject';
import {UP_ARROW, DOWN_ARROW, TAB} from '@angular/cdk/keyboard';
import {Subscription} from 'rxjs/Subscription';
import {UP_ARROW, DOWN_ARROW, TAB, A, Z} from '@angular/cdk/keyboard';
import {RxChain, debounceTime, filter, map, doOperator} from '@angular/cdk/rxjs';

/**
* This interface is for items that can be disabled. The type passed into
* ListKeyManager must extend this interface.
* This interface is for items that can be passed to a ListKeyManager.
*/
export interface CanDisable {
export interface ListKeyManagerItem {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe Item -> Option? I think every time we use this it's for the listbox + option combo

disabled?: boolean;
getLabel?(): string;
}

/**
* 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.
*/
export class ListKeyManager<T extends CanDisable> {
private _activeItemIndex: number = -1;
export class ListKeyManager<T extends ListKeyManagerItem> {
private _activeItemIndex = -1;
private _activeItem: T;
private _tabOut = new Subject<void>();
private _wrap: boolean = false;
private _wrap = false;
private _pressedInputKeys: number[] = [];
Copy link
Member

Choose a reason for hiding this comment

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

Add description for _pressedInputKeys? This one isn't as immediately obvious as the other properties

private _nonNavigationKeyStream = new Subject<number>();
private _typeaheadSubscription: Subscription;

constructor(private _items: QueryList<T>) { }

/**
* Turns on wrapping mode, which ensures that the active item will wrap to
* the other end of list when there are no more items in the given direction.
*
* @returns The ListKeyManager that the method was called on.
*/
withWrap(): this {
this._wrap = true;
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.
*/
withTypeAhead(debounceInterval = 200): this {
if (this._items.length && this._items.some(item => typeof item.getLabel !== 'function')) {
throw Error('ListKeyManager items in typeahead mode must implement the `getLabel` method.');
}
Copy link
Member

Choose a reason for hiding this comment

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

Add test for this error?


if (this._typeaheadSubscription) {
this._typeaheadSubscription.unsubscribe();
}

this._typeaheadSubscription = RxChain.from(this._nonNavigationKeyStream)
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to add a description of how you're using _pressedInputKeys in this chain

.call(filter, keyCode => keyCode >= A && keyCode <= Z)
Copy link

@andrewbents andrewbents Aug 18, 2017

Choose a reason for hiding this comment

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

so, it only searches by letters? What if options contain other characters? Also, a lot of languages have more letters, so some of them don't fit in the A-Z range

Copy link
Member Author

@crisbeto crisbeto Aug 18, 2017

Choose a reason for hiding this comment

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

This checks the keyboard codes, not the actual letters. E.g. 68 will correspond to d in English and д in a Bulgarian phonetic keyboard. As for the languages with more letters, nothing has come up yet, but we can always update the range. This seems to be what Closure uses as well.

Choose a reason for hiding this comment

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

Russian has 6 letters outside this range, they are on [ ] ; ' , . keys. So, they won't be handled? Also I think non-alphabetic characters should be supported as well

Choose a reason for hiding this comment

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

What about numbers? Does it work with numeric values? And when should we expect it to be release?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've submitted #6543 that should handle more or less everything. It'll still fall back to A-Z and 0-9 for browsers that don't support KeyboardEvent.key (should be a very small percentage).

.call(doOperator, keyCode => this._pressedInputKeys.push(keyCode))
.call(debounceTime, debounceInterval)
.call(filter, () => this._pressedInputKeys.length > 0)
.call(map, () => String.fromCharCode(...this._pressedInputKeys))
.subscribe(inputString => {
const activeItemIndex = this._items.toArray().findIndex(item => {
Copy link
Member

Choose a reason for hiding this comment

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

IE11 doesn't have findIndex or startsWith

Copy link
Member Author

Choose a reason for hiding this comment

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

It gets added by core-js, we have a couple of other places where we use startsWith.

Copy link
Member

Choose a reason for hiding this comment

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

activeItemIndex -> matchedItemIndex?

return item.getLabel!().toUpperCase().trim().startsWith(inputString);
});

if (activeItemIndex > -1) {
this.setActiveItem(activeItemIndex);
}

this._pressedInputKeys = [];
});

return this;
}

/**
* Sets the active item to the item at the index specified.
*
* @param index The index of the item to be set as active.
*/
setActiveItem(index: number): void {
Expand All @@ -58,20 +93,12 @@ export class ListKeyManager<T extends CanDisable> {
*/
onKeydown(event: KeyboardEvent): void {
switch (event.keyCode) {
case DOWN_ARROW:
this.setNextItemActive();
break;
case UP_ARROW:
this.setPreviousItemActive();
break;
case TAB:
// Note that we shouldn't prevent the default action on tab.
this._tabOut.next();
return;
default:
return;
case DOWN_ARROW: this.setNextItemActive(); break;
case UP_ARROW: this.setPreviousItemActive(); break;
default: this._nonNavigationKeyStream.next(event.keyCode); return;
Copy link
Member

Choose a reason for hiding this comment

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

Add back comment about using return here to skip preventDefault?

}

this._pressedInputKeys = [];
event.preventDefault();
}

Expand Down Expand Up @@ -119,7 +146,7 @@ export class ListKeyManager<T extends CanDisable> {
* when focus is shifted off of the list.
*/
get tabOut(): Observable<void> {
return this._tabOut.asObservable();
return filter.call(this._nonNavigationKeyStream, keyCode => keyCode === TAB);
}

/**
Expand Down Expand Up @@ -173,5 +200,4 @@ export class ListKeyManager<T extends CanDisable> {
}
this.setActiveItem(index);
}

}
2 changes: 2 additions & 0 deletions src/cdk/keyboard/keycodes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,5 @@ export const TAB = 9;
export const ESCAPE = 27;
export const BACKSPACE = 8;
export const DELETE = 46;
export const A = 65;
export const Z = 90;
4 changes: 2 additions & 2 deletions src/lib/core/a11y/activedescendant-key-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@
* found in the LICENSE file at https://angular.io/license
*/

import {ListKeyManager, CanDisable} from './list-key-manager';
import {ListKeyManager, ListKeyManagerItem} from './list-key-manager';

/**
* This is the interface for highlightable items (used by the ActiveDescendantKeyManager).
* Each item must know how to style itself as active or inactive and whether or not it is
* currently disabled.
*/
export interface Highlightable extends CanDisable {
export interface Highlightable extends ListKeyManagerItem {
setActiveStyles(): void;
setInactiveStyles(): void;
}
Expand Down
15 changes: 4 additions & 11 deletions src/lib/core/a11y/focus-key-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,18 @@
* found in the LICENSE file at https://angular.io/license
*/

import {QueryList} from '@angular/core';
import {ListKeyManager, CanDisable} from './list-key-manager';
import {ListKeyManager, ListKeyManagerItem} from './list-key-manager';

/**
* This is the interface for focusable items (used by the FocusKeyManager).
* Each item must know how to focus itself and whether or not it is currently disabled.
* Each item must know how to focus itself, whether or not it is currently disabled
* and be able to supply it's label.
*/
export interface Focusable extends CanDisable {
export interface Focusable extends ListKeyManagerItem {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe make this something like FocusableOption now?

focus(): void;
}


export class FocusKeyManager extends ListKeyManager<Focusable> {

constructor(items: QueryList<Focusable>) {
super(items);
}

/**
* This method sets the active item to the item at the specified index.
* It also adds focuses the newly active item.
Expand All @@ -35,5 +29,4 @@ export class FocusKeyManager extends ListKeyManager<Focusable> {
this.activeItem.focus();
}
}

}
4 changes: 1 addition & 3 deletions src/lib/core/a11y/list-key-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,4 @@
* found in the LICENSE file at https://angular.io/license
*/

export {CanDisable, ListKeyManager} from '@angular/cdk/a11y';


export {ListKeyManagerItem, ListKeyManager} from '@angular/cdk/a11y';
Loading