fix(material/list): Selection List element should not be focus… (#18445)

* 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.

Author: zelliott

src/material/list/selection-list.spec.ts

@@ -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,
@@ -291,6 +291,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;
 
@@ -690,49 +733,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;
@@ -1142,6 +1142,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', () => {
@@ -1438,19 +1448,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()">

src/material/list/selection-list.ts

@@ -53,7 +53,7 @@ import {
 } from '@angular/material/core';
 
 import {Subject} from 'rxjs';
-import {takeUntil} from 'rxjs/operators';
+import {startWith, takeUntil} from 'rxjs/operators';
 
 import {MatListAvatarCssMatStyler, MatListIconCssMatStyler} from './list';
 
@@ -99,7 +99,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
@@ -113,6 +112,7 @@ export class MatSelectionListChange {
     '[class.mat-list-single-selected-option]': 'selected && !selectionList.multiple',
     '[attr.aria-selected]': 'selected',
     '[attr.aria-disabled]': 'disabled',
+    '[attr.tabindex]': '-1',
   },
   templateUrl: 'list-option.html',
   encapsulation: ViewEncapsulation.None,
@@ -323,12 +323,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)',
     '[attr.aria-multiselectable]': 'multiple',
     '[attr.aria-disabled]': 'disabled.toString()',
+    '[attr.tabindex]': '_tabIndex',
   },
   template: '<ng-content></ng-content>',
   styleUrls: ['list.css'],
@@ -351,7 +352,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. */
@@ -398,6 +402,9 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements CanD
   /** The currently selected options. */
   selectedOptions = new SelectionModel<MatListOption>(this._multiple);
 
+  /** 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) => {};
 
@@ -413,9 +420,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 {
@@ -433,6 +442,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) {
@@ -560,6 +579,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;
@@ -664,6 +699,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;
   static ngAcceptInputType_multiple: BooleanInput;

tools/public_api_guard/material/list.d.ts

@@ -99,6 +99,7 @@ export declare class MatNavList extends _MatListMixinBase implements CanDisable,
 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;
@@ -110,9 +111,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;