Skip to content

Commit

Permalink
fix(list): don't redirect focus to first option on mouse focus (#19889)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
crisbeto committed Aug 20, 2020
1 parent fc67ff3 commit 22eca83
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 23 deletions.
1 change: 1 addition & 0 deletions src/material/list/BUILD.bazel
Expand Up @@ -58,6 +58,7 @@ ng_test_library(
),
deps = [
":list",
"//src/cdk/a11y",
"//src/cdk/keycodes",
"//src/cdk/testing/private",
"//src/material/core",
Expand Down
27 changes: 26 additions & 1 deletion src/material/list/selection-list.spec.ts
Expand Up @@ -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 {
Expand All @@ -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', () => {
Expand Down Expand Up @@ -302,6 +311,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();

Expand Down Expand Up @@ -731,6 +755,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.');
Expand Down
42 changes: 23 additions & 19 deletions src/material/list/selection-list.ts
Expand Up @@ -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 {
Expand Down Expand Up @@ -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()',
Expand Down Expand Up @@ -417,7 +416,9 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements CanD
constructor(private _element: ElementRef<HTMLElement>,
// @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();
}

Expand Down Expand Up @@ -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) {
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
5 changes: 2 additions & 3 deletions tools/public_api_guard/material/list.d.ts
Expand Up @@ -115,10 +115,9 @@ export declare class MatSelectionList extends _MatSelectionListMixinBase impleme
selectedOptions: SelectionModel<MatListOption>;
readonly selectionChange: EventEmitter<MatSelectionListChange>;
tabIndex: number;
constructor(_element: ElementRef<HTMLElement>, tabIndex: string, _changeDetector: ChangeDetectorRef);
constructor(_element: ElementRef<HTMLElement>, 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;
Expand All @@ -136,7 +135,7 @@ export declare class MatSelectionList extends _MatSelectionListMixinBase impleme
static ngAcceptInputType_disabled: BooleanInput;
static ngAcceptInputType_multiple: BooleanInput;
static ɵcmp: i0.ɵɵComponentDefWithMeta<MatSelectionList, "mat-selection-list", ["matSelectionList"], { "disableRipple": "disableRipple"; "tabIndex": "tabIndex"; "color": "color"; "compareWith": "compareWith"; "disabled": "disabled"; "multiple": "multiple"; }, { "selectionChange": "selectionChange"; }, ["options"], ["*"]>;
static ɵfac: i0.ɵɵFactoryDef<MatSelectionList, [null, { attribute: "tabindex"; }, null]>;
static ɵfac: i0.ɵɵFactoryDef<MatSelectionList, [null, { attribute: "tabindex"; }, null, null]>;
}

export declare class MatSelectionListChange {
Expand Down

0 comments on commit 22eca83

Please sign in to comment.