Skip to content

Commit

Permalink
fix(material/list): Selection List element should not be focus… (#18445)
Browse files Browse the repository at this point in the history 
* fix(material/list): Selection list element should not take focus. Focus should move to previously active option.

* Nit changes to a few comments and accept API goldens.
  • Loading branch information
@zelliott @mmalerba
zelliott authored and mmalerba committed Feb 19, 2020
1 parent c358975 commit b61582c
Show file tree
Hide file tree
Showing 3 changed files with 117 additions and 64 deletions.
111 changes: 54 additions & 57 deletions src/material/list/selection-list.spec.ts
View file
@@ -1,4 +1,4 @@
import {DOWN_ARROW, SPACE, ENTER, UP_ARROW, HOME, END, A, D} from '@angular/cdk/keycodes';
import {DOWN_ARROW, SPACE, ENTER, UP_ARROW, HOME, END, A, D, TAB} from '@angular/cdk/keycodes';
import {
createKeyboardEvent,
dispatchFakeEvent,
Expand Down Expand Up @@ -281,6 +281,49 @@ describe('MatSelectionList without forms', () => {
expect(selectionModel.selected.length).toBe(0);
});

it('should focus the first option when the list takes focus for the first time', () => {
spyOn(listOptions[0].componentInstance, 'focus').and.callThrough();

const manager = selectionList.componentInstance._keyManager;
expect(manager.activeItemIndex).toBe(-1);

dispatchFakeEvent(selectionList.nativeElement, 'focus');
fixture.detectChanges();

expect(manager.activeItemIndex).toBe(0);
expect(listOptions[0].componentInstance.focus).toHaveBeenCalled();
});

it('should focus the previously focused option when the list takes focus a second time', () => {
spyOn(listOptions[1].componentInstance, 'focus').and.callThrough();

const manager = selectionList.componentInstance._keyManager;
expect(manager.activeItemIndex).toBe(-1);

// Focus and blur the option to move the active item index. This option is now the previously
// focused option.
listOptions[1].componentInstance._handleFocus();
listOptions[1].componentInstance._handleBlur();

dispatchFakeEvent(selectionList.nativeElement, 'focus');
fixture.detectChanges();

expect(manager.activeItemIndex).toBe(1);
expect(listOptions[1].componentInstance.focus).toHaveBeenCalled();
});

it('should allow focus to escape when tabbing away', fakeAsync(() => {
selectionList.componentInstance._keyManager.onKeydown(createKeyboardEvent('keydown', TAB));

expect(selectionList.componentInstance._tabIndex)
.toBe(-1, 'Expected tabIndex to be set to -1 temporarily.');

tick();

expect(selectionList.componentInstance._tabIndex)
.toBe(0, 'Expected tabIndex to be reset back to 0');
}));

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

Expand Down Expand Up @@ -680,49 +723,6 @@ describe('MatSelectionList without forms', () => {
});
});

describe('with tabindex', () => {

beforeEach(async(() => {
TestBed.configureTestingModule({
imports: [MatListModule],
declarations: [
SelectionListWithTabindexAttr,
SelectionListWithTabindexBinding,
]
});

TestBed.compileComponents();
}));

it('should properly handle native tabindex attribute', () => {
const fixture = TestBed.createComponent(SelectionListWithTabindexAttr);
const selectionList = fixture.debugElement.query(By.directive(MatSelectionList))!;

expect(selectionList.componentInstance.tabIndex)
.toBe(5, 'Expected the selection-list tabindex to be set to the attribute value.');
});

it('should support changing the tabIndex through binding', () => {
const fixture = TestBed.createComponent(SelectionListWithTabindexBinding);
const selectionList = fixture.debugElement.query(By.directive(MatSelectionList))!;

expect(selectionList.componentInstance.tabIndex)
.toBe(0, 'Expected the tabIndex to be set to "0" by default.');

fixture.componentInstance.tabIndex = 3;
fixture.detectChanges();

expect(selectionList.componentInstance.tabIndex)
.toBe(3, 'Expected the tabIndex to updated through binding.');

fixture.componentInstance.disabled = true;
fixture.detectChanges();

expect(selectionList.componentInstance.tabIndex)
.toBe(3, 'Expected the tabIndex to be still set to "3".');
});
});

describe('with option disabled', () => {
let fixture: ComponentFixture<SelectionListWithDisabledOption>;
let listOptionEl: HTMLElement;
Expand Down Expand Up @@ -1058,6 +1058,16 @@ describe('MatSelectionList with forms', () => {
expect(listOptions.map(option => option.selected)).toEqual([true, true, true, false, false]);
}));

it('should only be in the tab order if it has options', () => {
expect(selectionListDebug.componentInstance.options.length > 0).toBe(true);
expect(selectionListDebug.nativeElement.tabIndex).toBe(0);

fixture.componentInstance.options = [];
fixture.detectChanges();

expect(selectionListDebug.nativeElement.tabIndex).toBe(-1);
});

});

describe('and formControl', () => {
Expand Down Expand Up @@ -1352,19 +1362,6 @@ class SelectionListWithSelectedOptionAndValue {
class SelectionListWithOnlyOneOption {
}

@Component({
template: `<mat-selection-list tabindex="5"></mat-selection-list>`
})
class SelectionListWithTabindexAttr {}

@Component({
template: `<mat-selection-list [tabIndex]="tabIndex" [disabled]="disabled"></mat-selection-list>`
})
class SelectionListWithTabindexBinding {
tabIndex: number;
disabled: boolean;
}

@Component({
template: `
<mat-selection-list [(ngModel)]="selectedOptions" (ngModelChange)="modelChangeSpy()">
Expand Down
66 changes: 60 additions & 6 deletions src/material/list/selection-list.ts
View file
Expand Up @@ -51,7 +51,7 @@ import {
ThemePalette,
} from '@angular/material/core';
import {Subject} from 'rxjs';
import {takeUntil} from 'rxjs/operators';
import {startWith, takeUntil} from 'rxjs/operators';

import {MatListAvatarCssMatStyler, MatListIconCssMatStyler} from './list';

Expand Down Expand Up @@ -97,7 +97,6 @@ export class MatSelectionListChange {
'(focus)': '_handleFocus()',
'(blur)': '_handleBlur()',
'(click)': '_handleClick()',
'tabindex': '-1',
'[class.mat-list-item-disabled]': 'disabled',
'[class.mat-list-item-with-avatar]': '_avatar || _icon',
// Manually set the "primary" or "warn" class if the color has been explicitly
Expand All @@ -110,6 +109,7 @@ export class MatSelectionListChange {
'[class.mat-warn]': 'color === "warn"',
'[attr.aria-selected]': 'selected',
'[attr.aria-disabled]': 'disabled',
'[attr.tabindex]': '-1',
},
templateUrl: 'list-option.html',
encapsulation: ViewEncapsulation.None,
Expand Down Expand Up @@ -320,12 +320,13 @@ export class MatListOption extends _MatListOptionMixinBase implements AfterConte
inputs: ['disableRipple'],
host: {
'role': 'listbox',
'[tabIndex]': 'tabIndex',
'class': 'mat-selection-list mat-list-base',
'(focus)': '_onFocus()',
'(blur)': '_onTouched()',
'(keydown)': '_keydown($event)',
'aria-multiselectable': 'true',
'[attr.aria-disabled]': 'disabled.toString()',
'[attr.tabindex]': '_tabIndex',
},
template: '<ng-content></ng-content>',
styleUrls: ['list.css'],
Expand All @@ -346,7 +347,10 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements CanD
@Output() readonly selectionChange: EventEmitter<MatSelectionListChange> =
new EventEmitter<MatSelectionListChange>();

/** Tabindex of the selection list. */
/**
* Tabindex of the selection list.
* @breaking-change 11.0.0 Remove `tabIndex` input.
*/
@Input() tabIndex: number = 0;

/** Theme color of the selection list. This sets the checkbox color for all list options. */
Expand Down Expand Up @@ -376,6 +380,9 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements CanD
/** The currently selected options. */
selectedOptions: SelectionModel<MatListOption> = new SelectionModel<MatListOption>(true);

/** The tabindex of the selection list. */
_tabIndex = -1;

/** View to model callback that should be called whenever the selected options change. */
private _onChange: (value: any) => void = (_: any) => {};

Expand All @@ -391,9 +398,11 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements CanD
/** Whether the list has been destroyed. */
private _isDestroyed: boolean;

constructor(private _element: ElementRef<HTMLElement>, @Attribute('tabindex') tabIndex: string) {
constructor(private _element: ElementRef<HTMLElement>,
// @breaking-change 11.0.0 Remove `tabIndex` parameter.
@Attribute('tabindex') tabIndex: string,
private _changeDetector: ChangeDetectorRef) {
super();
this.tabIndex = parseInt(tabIndex) || 0;
}

ngAfterContentInit(): void {
Expand All @@ -409,6 +418,16 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements CanD
this._setOptionsFromValues(this._value);
}

// If the user attempts to tab out of the selection list, allow focus to escape.
this._keyManager.tabOut.pipe(takeUntil(this._destroyed)).subscribe(() => {
this._allowFocusEscape();
});

// When the number of options change, update the tabindex of the selection list.
this.options.changes.pipe(startWith(null), takeUntil(this._destroyed)).subscribe(() => {
this._updateTabIndex();
});

// Sync external changes to the model back to the options.
this.selectedOptions.changed.pipe(takeUntil(this._destroyed)).subscribe(event => {
if (event.added) {
Expand Down Expand Up @@ -536,6 +555,22 @@ 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 Expand Up @@ -640,6 +675,25 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements CanD
}
}

/**
* Removes the tabindex from the selection list and resets it back afterwards, allowing the user
* to tab out of it. This prevents the list from capturing focus and redirecting it back within
* the list, creating a focus trap if it user tries to tab away.
*/
private _allowFocusEscape() {
this._tabIndex = -1;

setTimeout(() => {
this._tabIndex = 0;
this._changeDetector.markForCheck();
});
}

/** Updates the tabindex based upon if the selection list is empty. */
private _updateTabIndex(): void {
this._tabIndex = (this.options.length === 0) ? -1 : 0;
}

static ngAcceptInputType_disabled: BooleanInput;
static ngAcceptInputType_disableRipple: BooleanInput;
}
4 changes: 3 additions & 1 deletion tools/public_api_guard/material/list.d.ts
View file
Expand Up @@ -94,6 +94,7 @@ export declare class MatNavList extends _MatListMixinBase implements CanDisableR
export declare class MatSelectionList extends _MatSelectionListMixinBase implements CanDisableRipple, AfterContentInit, ControlValueAccessor, OnDestroy, OnChanges {
_keyManager: FocusKeyManager<MatListOption>;
_onTouched: () => void;
_tabIndex: number;
_value: string[] | null;
color: ThemePalette;
compareWith: (o1: any, o2: any) => boolean;
Expand All @@ -103,9 +104,10 @@ export declare class MatSelectionList extends _MatSelectionListMixinBase impleme
selectedOptions: SelectionModel<MatListOption>;
readonly selectionChange: EventEmitter<MatSelectionListChange>;
tabIndex: number;
constructor(_element: ElementRef<HTMLElement>, tabIndex: string);
constructor(_element: ElementRef<HTMLElement>, tabIndex: string, _changeDetector: ChangeDetectorRef);
_emitChangeEvent(option: MatListOption): void;
_keydown(event: KeyboardEvent): void;
_onFocus(): void;
_removeOptionFromList(option: MatListOption): MatListOption | null;
_reportValueChange(): void;
_setFocusedOption(option: MatListOption): void;
Expand Down

0 comments on commit b61582c

Please sign in to comment.