Skip to content

feat(list-key-manager): accept item references in setActiveItem #10029

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

Merged
merged 1 commit into from
Mar 1, 2018
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
19 changes: 15 additions & 4 deletions src/cdk/a11y/key-manager/activedescendant-key-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,22 @@ export interface Highlightable extends ListKeyManagerOption {
export class ActiveDescendantKeyManager<T> extends ListKeyManager<Highlightable & T> {

/**
* This method sets the active item to the item at the specified index.
* It also adds active styles to the newly active item and removes active
* styles from the previously active item.
* Sets the active item to the item at the specified index and adds the
* active styles to the newly active item. Also removes active styles
* from the previously active item.
* @param index Index of the item to be set as active.
*/
setActiveItem(index: number): void {
setActiveItem(index: number): void;

/**
* Sets the active item to the item to the specified one and adds the
* active styles to the it. Also removes active styles from the
* previously active item.
* @param item Item to be set as active.
*/
setActiveItem(item: T): void;
Copy link
Member

Choose a reason for hiding this comment

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

Technically T could be number. It would avoid the ambiguity if we just kept the separate methods (setActiveItem and setActiveItemIndex)

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 don't think so. The _items that are passed in through the constructor are a QueryList which can't contain numbers.

Copy link
Member

Choose a reason for hiding this comment

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

Is there ever a time when a component would want to use one or the other in a single call? Still seems like it would be more explicit to have both methods with index vs. item behaviors.

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 would be more explicit, but it adds a couple of extra methods to the API that do more or less the same. I don't think there's a case where a component would want to use both of them in the same call.

Copy link
Member

Choose a reason for hiding this comment

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

Ack, I don't feel that strongly. My only other comment would be that the implemention method should be item: T|number

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I was going for initially, but then TS seems to have a hard time figuring out the type when we pass the item to updateActiveItem. There were also some issues when trying to assign the active item inside the derived classes. I went with any since the implementation method signature won't show up in the type info anyway.


setActiveItem(index: any): void {
if (this.activeItem) {
this.activeItem.setInactiveStyles();
}
Expand Down
17 changes: 13 additions & 4 deletions src/cdk/a11y/key-manager/focus-key-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,20 @@ export class FocusKeyManager<T> extends ListKeyManager<FocusableOption & T> {
}

/**
* This method sets the active item to the item at the specified index.
* It also adds focuses the newly active item.
* Sets the active item to the item at the specified
* index and focuses the newly active item.
* @param index Index of the item to be set as active.
*/
setActiveItem(index: number): void {
super.setActiveItem(index);
setActiveItem(index: number): void;

/**
* Sets the active item to the item that is specified and focuses it.
* @param item Item to be set as active.
*/
setActiveItem(item: T): void;

setActiveItem(item: any): void {
super.setActiveItem(item);

if (this.activeItem) {
this.activeItem.focus(this._origin);
Expand Down
23 changes: 23 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 @@ -324,6 +324,29 @@ describe('Key managers', () => {
.toBe(1, `Expected activeItemIndex to be updated when setActiveItem() was called.`);
});

it('should be able to set the active item by reference', () => {
expect(keyManager.activeItemIndex)
.toBe(0, `Expected first item of the list to be active.`);

keyManager.setActiveItem(itemList.items[2]);
expect(keyManager.activeItemIndex)
.toBe(2, `Expected activeItemIndex to be updated.`);
});

it('should be able to set the active item without emitting an event', () => {
const spy = jasmine.createSpy('change spy');
const subscription = keyManager.change.subscribe(spy);

expect(keyManager.activeItemIndex).toBe(0);

keyManager.updateActiveItem(2);

expect(keyManager.activeItemIndex).toBe(2);
expect(spy).not.toHaveBeenCalled();

subscription.unsubscribe();
});

it('should expose the active item correctly', () => {
keyManager.onKeydown(fakeKeyEvents.downArrow);

Expand Down
41 changes: 35 additions & 6 deletions src/cdk/a11y/key-manager/list-key-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,14 +144,21 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
* 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 {
setActiveItem(index: number): void;

/**
* Sets the active item to the specified item.
* @param item The item to be set as active.
*/
setActiveItem(item: T): void;

setActiveItem(item: any): void {
const previousIndex = this._activeItemIndex;

this._activeItemIndex = index;
this._activeItem = this._items.toArray()[index];
this.updateActiveItem(item);

if (this._activeItemIndex !== previousIndex) {
this.change.next(index);
this.change.next(this._activeItemIndex);
}
}

Expand Down Expand Up @@ -246,12 +253,34 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
: this._setActiveItemByDelta(-1);
}

/**
* Allows setting the active without any other effects.
* @param index Index of the item to be set as active.
*/
updateActiveItem(index: number): void;

/**
* Allows setting the active item without any other effects.
* @param item Item to be set as active.
*/
updateActiveItem(item: T): void;

updateActiveItem(item: any): void {
const itemArray = this._items.toArray();
const index = typeof item === 'number' ? item : itemArray.indexOf(item);

this._activeItemIndex = index;
this._activeItem = itemArray[index];
}

/**
* Allows setting of the activeItemIndex without any other effects.
* @param index The new activeItemIndex.
* @deprecated Use `updateActiveItem` instead.
* @deletion-target 7.0.0
*/
updateActiveItemIndex(index: number) {
this._activeItemIndex = index;
updateActiveItemIndex(index: number): void {
this.updateActiveItem(index);
}

/**
Expand Down
7 changes: 2 additions & 5 deletions src/lib/chips/chip-list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -567,14 +567,11 @@ export class MatChipList extends _MatChipListMixinBase implements MatFormFieldCo
// Shift focus to the active item. Note that we shouldn't do this in multiple
// mode, because we don't know what chip the user interacted with last.
if (correspondingChip) {
const correspondingChipIndex = this.chips.toArray().indexOf(correspondingChip);

if (isUserInput) {
this._keyManager.setActiveItem(correspondingChipIndex);
this._keyManager.setActiveItem(correspondingChip);
} else {
this._keyManager.updateActiveItemIndex(correspondingChipIndex);
this._keyManager.updateActiveItem(correspondingChip);
}

}
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/lib/select/select.ts
Original file line number Diff line number Diff line change
Expand Up @@ -808,7 +808,7 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
// Shift focus to the active item. Note that we shouldn't do this in multiple
// mode, because we don't know what option the user interacted with last.
if (correspondingOption) {
this._keyManager.setActiveItem(this.options.toArray().indexOf(correspondingOption));
this._keyManager.setActiveItem(correspondingOption);
}
}

Expand Down Expand Up @@ -910,7 +910,7 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
this._selectionModel.toggle(option);
this.stateChanges.next();
wasSelected ? option.deselect() : option.select();
this._keyManager.setActiveItem(this._getOptionIndex(option)!);
this._keyManager.setActiveItem(option);
this._sortValues();
} else {
this._clearSelection(option.value == null ? undefined : option);
Expand Down Expand Up @@ -976,7 +976,7 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
if (this.empty) {
this._keyManager.setFirstItemActive();
} else {
this._keyManager.setActiveItem(this._getOptionIndex(this._selectionModel.selected[0])!);
this._keyManager.setActiveItem(this._selectionModel.selected[0]);
}
}
}
Expand Down