From e05f93981aa1e966f265ec1c9691c04af63eb773 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 29 Sep 2017 22:11:22 +0200 Subject: [PATCH] fix(selection-list): restore focus if active item is destroyed (#7125) * 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. --- src/lib/list/selection-list.spec.ts | 50 ++++++++++------- src/lib/list/selection-list.ts | 87 +++++++++-------------------- 2 files changed, 55 insertions(+), 82 deletions(-) diff --git a/src/lib/list/selection-list.spec.ts b/src/lib/list/selection-list.spec.ts index 0c692695d4a3..3fa335190d87 100644 --- a/src/lib/list/selection-list.spec.ts +++ b/src/lib/list/selection-list.spec.ts @@ -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'; @@ -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', () => { @@ -144,12 +149,9 @@ describe('MatSelectionList', () => { createKeyboardEvent('keydown', SPACE, testListItem); let selectList = selectionList.injector.get(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(); @@ -157,16 +159,26 @@ describe('MatSelectionList', () => { 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); @@ -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); @@ -432,11 +441,12 @@ describe('MatSelectionList', () => { Sent Mail - + Drafts `}) class SelectionListWithListOptions { + showLastOption: boolean = true; } @Component({template: ` diff --git a/src/lib/list/selection-list.ts b/src/lib/list/selection-list.ts index f476fa95cd9a..80fe6de4eab8 100644 --- a/src/lib/list/selection-list.ts +++ b/src/lib/list/selection-list.ts @@ -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, @@ -38,8 +37,6 @@ import { mixinDisabled, mixinDisableRipple, } from '@angular/material/core'; -import {merge} from 'rxjs/observable/merge'; -import {Subscription} from 'rxjs/Subscription'; /** @docs-private */ @@ -55,8 +52,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 @@ -70,10 +65,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()', }, @@ -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(); - /** Emitted when the option is selected. */ @Output() selectChange = new EventEmitter(); /** Emitted when the option is deselected. */ @Output() deselected = new EventEmitter(); - /** Emitted when the option is destroyed. */ - @Output() destroyed = new EventEmitter(); - constructor(private _renderer: Renderer2, private _element: ElementRef, private _changeDetector: ChangeDetectorRef, @@ -144,7 +134,7 @@ export class MatListOption extends _MatListOptionMixinBase } ngOnDestroy(): void { - this.destroyed.emit({option: this}); + this.selectionList._removeOptionFromList(this); } /** Toggles the selection state of the option. */ @@ -157,7 +147,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. */ @@ -173,11 +162,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. */ @@ -208,17 +193,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; @@ -238,14 +217,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. */ @@ -271,36 +242,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. */ @@ -338,4 +296,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); + } }