Skip to content

Commit

Permalink
fix(selection-list): restore focus if active item is destroyed
Browse files Browse the repository at this point in the history 
* Removes the `destroyed` and `onFocus` observables and instead just calls methods on the injected `MdSelectionList` instance.

* Fixes that the active item is not updating if the active item is being destroyed.
  • Loading branch information
@devversion
devversion committed Sep 29, 2017
1 parent 53c42a4 commit 1335a0c
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 82 deletions.
50 changes: 30 additions & 20 deletions src/lib/list/selection-list.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {DOWN_ARROW, SPACE, UP_ARROW} from '@angular/cdk/keycodes';
import {Platform} from '@angular/cdk/platform';
import {createKeyboardEvent} from '@angular/cdk/testing';
import {createKeyboardEvent, dispatchFakeEvent} from '@angular/cdk/testing';
import {Component, DebugElement} from '@angular/core';
import {async, ComponentFixture, inject, TestBed} from '@angular/core/testing';
import {By} from '@angular/platform-browser';
Expand Down Expand Up @@ -28,24 +28,29 @@ describe('MatSelectionList', () => {
TestBed.compileComponents();
}));


beforeEach(async(() => {
fixture = TestBed.createComponent(SelectionListWithListOptions);
fixture.detectChanges();

listOption = fixture.debugElement.queryAll(By.directive(MatListOption));
listItemEl = fixture.debugElement.query(By.css('.mat-list-item'));
selectionList = fixture.debugElement.query(By.directive(MatSelectionList));
fixture.detectChanges();
}));

it('should add and remove focus class on focus/blur', () => {
expect(listItemEl.nativeElement.classList).not.toContain('mat-list-item-focus');
// Use the second list item, because the first one is always disabled.
const listItem = listOption[1].nativeElement;

listOption[0].componentInstance._handleFocus();
expect(listItem.classList).not.toContain('mat-list-item-focus');

dispatchFakeEvent(listItem, 'focus');
fixture.detectChanges();
expect(listItemEl.nativeElement.className).toContain('mat-list-item-focus');
expect(listItem.className).toContain('mat-list-item-focus');

listOption[0].componentInstance._handleBlur();
dispatchFakeEvent(listItem, 'blur');
fixture.detectChanges();
expect(listItemEl.nativeElement.className).not.toContain('mat-list-item-focus');
expect(listItem.className).not.toContain('mat-list-item-focus');
});

it('should be able to set a value on a list option', () => {
Expand Down Expand Up @@ -144,29 +149,36 @@ describe('MatSelectionList', () => {
createKeyboardEvent('keydown', SPACE, testListItem);
let selectList =
selectionList.injector.get<MatSelectionList>(MatSelectionList).selectedOptions;
let options = selectionList.componentInstance.options;
let array = options.toArray();
let focusItem = array[1];
expect(selectList.selected.length).toBe(0);

focusItem.focus();
dispatchFakeEvent(testListItem, 'focus');
selectionList.componentInstance._keydown(SPACE_EVENT);

fixture.detectChanges();

expect(selectList.selected.length).toBe(1);
});

it('should restore focus if active option is destroyed', () => {
const manager = selectionList.componentInstance._keyManager;

listOption[3].componentInstance._handleFocus();

expect(manager.activeItemIndex).toBe(3);

fixture.componentInstance.showLastOption = false;
fixture.detectChanges();

expect(manager.activeItemIndex).toBe(2);
});

it('should focus previous item when press UP ARROW', () => {
let testListItem = listOption[2].nativeElement as HTMLElement;
let UP_EVENT: KeyboardEvent =
createKeyboardEvent('keydown', UP_ARROW, testListItem);
let options = selectionList.componentInstance.options;
let array = options.toArray();
let focusItem = array[2];
let manager = selectionList.componentInstance._keyManager;

focusItem.focus();
dispatchFakeEvent(listOption[2].nativeElement, 'focus');
expect(manager.activeItemIndex).toEqual(2);

selectionList.componentInstance._keydown(UP_EVENT);
Expand All @@ -180,12 +192,9 @@ describe('MatSelectionList', () => {
let testListItem = listOption[2].nativeElement as HTMLElement;
let DOWN_EVENT: KeyboardEvent =
createKeyboardEvent('keydown', DOWN_ARROW, testListItem);
let options = selectionList.componentInstance.options;
let array = options.toArray();
let focusItem = array[2];
let manager = selectionList.componentInstance._keyManager;

focusItem.focus();
dispatchFakeEvent(listOption[2].nativeElement, 'focus');
expect(manager.activeItemIndex).toEqual(2);

selectionList.componentInstance._keydown(DOWN_EVENT);
Expand Down Expand Up @@ -404,11 +413,12 @@ describe('MatSelectionList', () => {
<mat-list-option checkboxPosition="before" value="sent-mail">
Sent Mail
</mat-list-option>
<mat-list-option checkboxPosition="before" value="drafts">
<mat-list-option checkboxPosition="before" value="drafts" *ngIf="showLastOption">
Drafts
</mat-list-option>
</mat-selection-list>`})
class SelectionListWithListOptions {
showLastOption: boolean = true;
}

@Component({template: `
Expand Down
87 changes: 25 additions & 62 deletions src/lib/list/selection-list.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {FocusableOption, FocusKeyManager} from '@angular/cdk/a11y';
import {coerceBooleanProperty} from '@angular/cdk/coercion';
import {SelectionModel} from '@angular/cdk/collections';
import {SPACE} from '@angular/cdk/keycodes';
import {RxChain, startWith, switchMap} from '@angular/cdk/rxjs';
import {
AfterContentInit,
ChangeDetectionStrategy,
Expand All @@ -37,8 +36,6 @@ import {
mixinDisabled,
mixinDisableRipple,
} from '@angular/material/core';
import {merge} from 'rxjs/observable/merge';
import {Subscription} from 'rxjs/Subscription';


/** @docs-private */
Expand All @@ -54,8 +51,6 @@ export interface MatSelectionListOptionEvent {
option: MatListOption;
}

const FOCUSED_STYLE: string = 'mat-list-item-focus';

/**
* Component for list-options of selection-list. Each list-option can automatically
* generate a checkbox and can put current item into the selectionModel of selection-list
Expand All @@ -69,10 +64,11 @@ const FOCUSED_STYLE: string = 'mat-list-item-focus';
'role': 'option',
'class': 'mat-list-item mat-list-option',
'(focus)': '_handleFocus()',
'(blur)': '_handleBlur()',
'(blur)': '_hasFocus = false',
'(click)': '_handleClick()',
'tabindex': '-1',
'[class.mat-list-item-disabled]': 'disabled',
'[class.mat-list-item-focus]': '_hasFocus',
'[attr.aria-selected]': 'selected.toString()',
'[attr.aria-disabled]': 'disabled.toString()',
},
Expand Down Expand Up @@ -109,18 +105,12 @@ export class MatListOption extends _MatListOptionMixinBase
get selected() { return this._selected; }
set selected(value: boolean) { this._selected = coerceBooleanProperty(value); }

/** Emitted when the option is focused. */
onFocus = new EventEmitter<MatSelectionListOptionEvent>();

/** Emitted when the option is selected. */
@Output() selectChange = new EventEmitter<MatSelectionListOptionEvent>();

/** Emitted when the option is deselected. */
@Output() deselected = new EventEmitter<MatSelectionListOptionEvent>();

/** Emitted when the option is destroyed. */
@Output() destroyed = new EventEmitter<MatSelectionListOptionEvent>();

constructor(private _renderer: Renderer2,
private _element: ElementRef,
private _changeDetector: ChangeDetectorRef,
Expand All @@ -138,7 +128,7 @@ export class MatListOption extends _MatListOptionMixinBase
}

ngOnDestroy(): void {
this.destroyed.emit({option: this});
this.selectionList._removeOptionFromList(this);
}

/** Toggles the selection state of the option. */
Expand All @@ -151,7 +141,6 @@ export class MatListOption extends _MatListOptionMixinBase
/** Allows for programmatic focusing of the option. */
focus(): void {
this._element.nativeElement.focus();
this.onFocus.emit({option: this});
}

/** Whether this list item should show a ripple effect when clicked. */
Expand All @@ -167,11 +156,7 @@ export class MatListOption extends _MatListOptionMixinBase

_handleFocus() {
this._hasFocus = true;
this._renderer.addClass(this._element.nativeElement, FOCUSED_STYLE);
}

_handleBlur() {
this._renderer.removeClass(this._element.nativeElement, FOCUSED_STYLE);
this.selectionList._setFocusedOption(this);
}

/** Retrieves the DOM element of the component host. */
Expand Down Expand Up @@ -202,17 +187,11 @@ export class MatListOption extends _MatListOptionMixinBase
changeDetection: ChangeDetectionStrategy.OnPush
})
export class MatSelectionList extends _MatSelectionListMixinBase
implements FocusableOption, CanDisable, CanDisableRipple, AfterContentInit, OnDestroy {
implements FocusableOption, CanDisable, CanDisableRipple, AfterContentInit {

/** Tab index for the selection-list. */
_tabIndex = 0;

/** Subscription to all list options' onFocus events */
private _optionFocusSubscription = Subscription.EMPTY;

/** Subscription to all list options' destroy events */
private _optionDestroyStream = Subscription.EMPTY;

/** The FocusKeyManager which handles focus. */
_keyManager: FocusKeyManager<MatListOption>;

Expand All @@ -232,14 +211,6 @@ export class MatSelectionList extends _MatSelectionListMixinBase
if (this.disabled) {
this._tabIndex = -1;
}

this._optionFocusSubscription = this._onFocusSubscription();
this._optionDestroyStream = this._onDestroySubscription();
}

ngOnDestroy(): void {
this._optionDestroyStream.unsubscribe();
this._optionFocusSubscription.unsubscribe();
}

/** Focus the selection-list. */
Expand All @@ -265,36 +236,23 @@ export class MatSelectionList extends _MatSelectionListMixinBase
});
}

/** Map all the options' destroy event subscriptions and merge them into one stream. */
private _onDestroySubscription(): Subscription {
return RxChain.from(this.options.changes)
.call(startWith, this.options)
.call(switchMap, (options: MatListOption[]) => {
return merge(...options.map(option => option.destroyed));
}).subscribe((e: MatSelectionListOptionEvent) => {
let optionIndex: number = this.options.toArray().indexOf(e.option);
if (e.option._hasFocus) {
// Check whether the option is the last item
if (optionIndex < this.options.length - 1) {
this._keyManager.setActiveItem(optionIndex);
} else if (optionIndex - 1 >= 0) {
this._keyManager.setActiveItem(optionIndex - 1);
}
}
e.option.destroyed.unsubscribe();
});
/** Sets the focused option of the selection-list. */
_setFocusedOption(option: MatListOption) {
this._keyManager.updateActiveItemIndex(this._getOptionIndex(option));
}

/** Map all the options' onFocus event subscriptions and merge them into one stream. */
private _onFocusSubscription(): Subscription {
return RxChain.from(this.options.changes)
.call(startWith, this.options)
.call(switchMap, (options: MatListOption[]) => {
return merge(...options.map(option => option.onFocus));
}).subscribe((e: MatSelectionListOptionEvent) => {
let optionIndex: number = this.options.toArray().indexOf(e.option);
this._keyManager.updateActiveItemIndex(optionIndex);
});
/** Removes an option from the selection list and updates the active item. */
_removeOptionFromList(option: MatListOption) {
if (option._hasFocus) {
const optionIndex = this._getOptionIndex(option);

// Check whether the option is the last item
if (optionIndex > 0) {
this._keyManager.setPreviousItemActive();
} else if (optionIndex === 0 && this.options.length > 1) {
this._keyManager.setNextItemActive();
}
}
}

/** Passes relevant key presses to our key manager. */
Expand Down Expand Up @@ -332,4 +290,9 @@ export class MatSelectionList extends _MatSelectionListMixinBase
private _isValidIndex(index: number): boolean {
return index >= 0 && index < this.options.length;
}

/** Returns the index of the specified list option. */
private _getOptionIndex(option: MatListOption): number {
return this.options.toArray().indexOf(option);
}
}

0 comments on commit 1335a0c

Please sign in to comment.