From 69a1576253597e9bb1eafec31538c00191169611 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Tue, 7 Jul 2020 19:18:30 +0200 Subject: [PATCH] fix(list): don't redirect focus to first option on mouse focus We have some logic that redirects focus to the first option whenever the list receives focus. The main target for this is tabbing into the list, but because we were using the `focus` event, it was also happening for mouse focus which can cause the page to jump around for long lists. These changes make it so that we only move it on keyboard and programmatic focus. Fixes #18948. --- src/material/list/BUILD.bazel | 1 + src/material/list/selection-list.spec.ts | 27 ++++++++++++++- src/material/list/selection-list.ts | 42 +++++++++++++---------- tools/public_api_guard/material/list.d.ts | 5 ++- 4 files changed, 52 insertions(+), 23 deletions(-) diff --git a/src/material/list/BUILD.bazel b/src/material/list/BUILD.bazel index 0e1abf9a3fee..ca60c4f45eea 100644 --- a/src/material/list/BUILD.bazel +++ b/src/material/list/BUILD.bazel @@ -58,6 +58,7 @@ ng_test_library( ), deps = [ ":list", + "//src/cdk/a11y", "//src/cdk/keycodes", "//src/cdk/testing/private", "//src/material/core", diff --git a/src/material/list/selection-list.spec.ts b/src/material/list/selection-list.spec.ts index 51326f144cae..7c18ab1bcf8e 100644 --- a/src/material/list/selection-list.spec.ts +++ b/src/material/list/selection-list.spec.ts @@ -13,7 +13,15 @@ import { QueryList, ViewChildren, } from '@angular/core'; -import {async, ComponentFixture, fakeAsync, TestBed, tick, flush} from '@angular/core/testing'; +import { + async, + ComponentFixture, + fakeAsync, + TestBed, + tick, + flush, + inject, +} from '@angular/core/testing'; import {MatRipple, defaultRippleAnimationConfig, ThemePalette} from '@angular/material/core'; import {By} from '@angular/platform-browser'; import { @@ -23,6 +31,7 @@ import { MatSelectionListChange } from './index'; import {FormControl, FormsModule, NgModel, ReactiveFormsModule} from '@angular/forms'; +import {FocusMonitor} from '@angular/cdk/a11y'; describe('MatSelectionList without forms', () => { describe('with list option', () => { @@ -304,6 +313,21 @@ describe('MatSelectionList without forms', () => { expect(listOptions[0].componentInstance.focus).toHaveBeenCalled(); }); + it('should not move focus to the first item if focus originated from a mouse interaction', + fakeAsync(inject([FocusMonitor], (focusMonitor: FocusMonitor) => { + spyOn(listOptions[0].componentInstance, 'focus').and.callThrough(); + + const manager = selectionList.componentInstance._keyManager; + expect(manager.activeItemIndex).toBe(-1); + + focusMonitor.focusVia(selectionList.nativeElement, 'mouse'); + fixture.detectChanges(); + flush(); + + expect(manager.activeItemIndex).toBe(-1); + expect(listOptions[0].componentInstance.focus).not.toHaveBeenCalled(); + }))); + it('should focus the previously focused option when the list takes focus a second time', () => { spyOn(listOptions[1].componentInstance, 'focus').and.callThrough(); @@ -743,6 +767,7 @@ describe('MatSelectionList without forms', () => { dispatchMouseEvent(rippleTarget, 'mousedown'); dispatchMouseEvent(rippleTarget, 'mouseup'); + flush(); expect(rippleTarget.querySelectorAll('.mat-ripple-element').length) .toBe(0, 'Expected no ripples after list ripples are disabled.'); diff --git a/src/material/list/selection-list.ts b/src/material/list/selection-list.ts index 35179047f201..f689fae86829 100644 --- a/src/material/list/selection-list.ts +++ b/src/material/list/selection-list.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {FocusableOption, FocusKeyManager} from '@angular/cdk/a11y'; +import {FocusableOption, FocusKeyManager, FocusMonitor} from '@angular/cdk/a11y'; import {BooleanInput, coerceBooleanProperty} from '@angular/cdk/coercion'; import {SelectionModel} from '@angular/cdk/collections'; import { @@ -319,7 +319,6 @@ export class MatListOption extends _MatListOptionMixinBase implements AfterConte host: { 'role': 'listbox', 'class': 'mat-selection-list mat-list-base', - '(focus)': '_onFocus()', '(keydown)': '_keydown($event)', '[attr.aria-multiselectable]': 'multiple', '[attr.aria-disabled]': 'disabled.toString()', @@ -417,7 +416,9 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements CanD constructor(private _element: ElementRef, // @breaking-change 11.0.0 Remove `tabIndex` parameter. @Attribute('tabindex') tabIndex: string, - private _changeDetector: ChangeDetectorRef) { + private _changeDetector: ChangeDetectorRef, + // @breaking-change 11.0.0 `_focusMonitor` parameter to become required. + private _focusMonitor?: FocusMonitor) { super(); } @@ -460,6 +461,23 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements CanD } } }); + + // @breaking-change 11.0.0 Remove null assertion once _focusMonitor is required. + this._focusMonitor?.monitor(this._element) + .pipe(takeUntil(this._destroyed)) + .subscribe(origin => { + if (origin === 'keyboard' || origin === 'program') { + const activeIndex = this._keyManager.activeItemIndex; + + if (!activeIndex || activeIndex === -1) { + // If there is no active index, set focus to the first option. + this._keyManager.setFirstItemActive(); + } else { + // Otherwise, set focus to the active option. + this._keyManager.setActiveItem(activeIndex); + } + } + }); } ngOnChanges(changes: SimpleChanges) { @@ -473,6 +491,8 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements CanD } ngOnDestroy() { + // @breaking-change 11.0.0 Remove null assertion once _focusMonitor is required. + this._focusMonitor?.stopMonitoring(this._element); this._destroyed.next(); this._destroyed.complete(); this._isDestroyed = true; @@ -575,22 +595,6 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements CanD this.selectionChange.emit(new MatSelectionListChange(this, option)); } - /** - * When the selection list is focused, we want to move focus to an option within the list. Do this - * by setting the appropriate option to be active. - */ - _onFocus(): void { - const activeIndex = this._keyManager.activeItemIndex; - - if (!activeIndex || (activeIndex === -1)) { - // If there is no active index, set focus to the first option. - this._keyManager.setFirstItemActive(); - } else { - // Otherwise, set focus to the active option. - this._keyManager.setActiveItem(activeIndex); - } - } - /** Implemented as part of ControlValueAccessor. */ writeValue(values: string[]): void { this._value = values; diff --git a/tools/public_api_guard/material/list.d.ts b/tools/public_api_guard/material/list.d.ts index 14c317e3ab1d..cac2e9d90cd4 100644 --- a/tools/public_api_guard/material/list.d.ts +++ b/tools/public_api_guard/material/list.d.ts @@ -115,10 +115,9 @@ export declare class MatSelectionList extends _MatSelectionListMixinBase impleme selectedOptions: SelectionModel; readonly selectionChange: EventEmitter; tabIndex: number; - constructor(_element: ElementRef, tabIndex: string, _changeDetector: ChangeDetectorRef); + constructor(_element: ElementRef, tabIndex: string, _changeDetector: ChangeDetectorRef, _focusMonitor?: FocusMonitor | undefined); _emitChangeEvent(option: MatListOption): void; _keydown(event: KeyboardEvent): void; - _onFocus(): void; _removeOptionFromList(option: MatListOption): MatListOption | null; _reportValueChange(): void; _setFocusedOption(option: MatListOption): void; @@ -136,7 +135,7 @@ export declare class MatSelectionList extends _MatSelectionListMixinBase impleme static ngAcceptInputType_disabled: BooleanInput; static ngAcceptInputType_multiple: BooleanInput; static ɵcmp: i0.ɵɵComponentDefWithMeta; - static ɵfac: i0.ɵɵFactoryDef; + static ɵfac: i0.ɵɵFactoryDef; } export declare class MatSelectionListChange {