Skip to content

Commit 0f04f11

Browse files
committed
fix(material-experimental/mdc-list): error with latest MDC list canary version
MDC updated the focus strategy for lists with multi-selection enabled. This breaks our tests as we relied on the old strategy where the previously focused item always becomes the focus target. This is no longer the case as MDC now intends to focus the first selected item of a list instead (except for non-selection lists which are interactive; here the previously focused item is still used). We update the tests and update the selection list so that pre-selected options are recognized correctly so that MDC's new focus strategy works as expected. Before this change we didn't sync the pre-selected options with the MDC list foundation.
1 parent 20d3469 commit 0f04f11

File tree

5 files changed

+698
-656
lines changed

5 files changed

+698
-656
lines changed

package.json

Lines changed: 48 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@
6262
"@types/googlemaps": "^3.43.1",
6363
"@types/youtube": "^0.0.42",
6464
"core-js-bundle": "^3.8.2",
65-
"material-components-web": "12.0.0-canary.be999eb08.0",
65+
"material-components-web": "12.0.0-canary.03f525f9f.0",
6666
"rxjs": "^6.5.3",
6767
"rxjs-tslint-rules": "^4.33.1",
6868
"systemjs": "0.19.43",
@@ -91,53 +91,53 @@
9191
"@bazel/terser": "3.3.0",
9292
"@bazel/typescript": "3.3.0",
9393
"@firebase/app-types": "^0.6.1",
94-
"@material/animation": "12.0.0-canary.be999eb08.0",
95-
"@material/auto-init": "12.0.0-canary.be999eb08.0",
96-
"@material/banner": "12.0.0-canary.be999eb08.0",
97-
"@material/base": "12.0.0-canary.be999eb08.0",
98-
"@material/button": "12.0.0-canary.be999eb08.0",
99-
"@material/card": "12.0.0-canary.be999eb08.0",
100-
"@material/checkbox": "12.0.0-canary.be999eb08.0",
101-
"@material/chips": "12.0.0-canary.be999eb08.0",
102-
"@material/circular-progress": "12.0.0-canary.be999eb08.0",
103-
"@material/data-table": "12.0.0-canary.be999eb08.0",
104-
"@material/density": "12.0.0-canary.be999eb08.0",
105-
"@material/dialog": "12.0.0-canary.be999eb08.0",
106-
"@material/dom": "12.0.0-canary.be999eb08.0",
107-
"@material/drawer": "12.0.0-canary.be999eb08.0",
108-
"@material/elevation": "12.0.0-canary.be999eb08.0",
109-
"@material/fab": "12.0.0-canary.be999eb08.0",
110-
"@material/feature-targeting": "12.0.0-canary.be999eb08.0",
111-
"@material/floating-label": "12.0.0-canary.be999eb08.0",
112-
"@material/form-field": "12.0.0-canary.be999eb08.0",
113-
"@material/icon-button": "12.0.0-canary.be999eb08.0",
114-
"@material/image-list": "12.0.0-canary.be999eb08.0",
115-
"@material/layout-grid": "12.0.0-canary.be999eb08.0",
116-
"@material/line-ripple": "12.0.0-canary.be999eb08.0",
117-
"@material/linear-progress": "12.0.0-canary.be999eb08.0",
118-
"@material/list": "12.0.0-canary.be999eb08.0",
119-
"@material/menu": "12.0.0-canary.be999eb08.0",
120-
"@material/menu-surface": "12.0.0-canary.be999eb08.0",
121-
"@material/notched-outline": "12.0.0-canary.be999eb08.0",
122-
"@material/radio": "12.0.0-canary.be999eb08.0",
123-
"@material/ripple": "12.0.0-canary.be999eb08.0",
124-
"@material/rtl": "12.0.0-canary.be999eb08.0",
125-
"@material/segmented-button": "12.0.0-canary.be999eb08.0",
126-
"@material/select": "12.0.0-canary.be999eb08.0",
127-
"@material/shape": "12.0.0-canary.be999eb08.0",
128-
"@material/slider": "12.0.0-canary.be999eb08.0",
129-
"@material/snackbar": "12.0.0-canary.be999eb08.0",
130-
"@material/switch": "12.0.0-canary.be999eb08.0",
131-
"@material/tab": "12.0.0-canary.be999eb08.0",
132-
"@material/tab-bar": "12.0.0-canary.be999eb08.0",
133-
"@material/tab-indicator": "12.0.0-canary.be999eb08.0",
134-
"@material/tab-scroller": "12.0.0-canary.be999eb08.0",
135-
"@material/textfield": "12.0.0-canary.be999eb08.0",
136-
"@material/theme": "12.0.0-canary.be999eb08.0",
137-
"@material/tooltip": "12.0.0-canary.be999eb08.0",
138-
"@material/top-app-bar": "12.0.0-canary.be999eb08.0",
139-
"@material/touch-target": "12.0.0-canary.be999eb08.0",
140-
"@material/typography": "12.0.0-canary.be999eb08.0",
94+
"@material/animation": "12.0.0-canary.03f525f9f.0",
95+
"@material/auto-init": "12.0.0-canary.03f525f9f.0",
96+
"@material/banner": "12.0.0-canary.03f525f9f.0",
97+
"@material/base": "12.0.0-canary.03f525f9f.0",
98+
"@material/button": "12.0.0-canary.03f525f9f.0",
99+
"@material/card": "12.0.0-canary.03f525f9f.0",
100+
"@material/checkbox": "12.0.0-canary.03f525f9f.0",
101+
"@material/chips": "12.0.0-canary.03f525f9f.0",
102+
"@material/circular-progress": "12.0.0-canary.03f525f9f.0",
103+
"@material/data-table": "12.0.0-canary.03f525f9f.0",
104+
"@material/density": "12.0.0-canary.03f525f9f.0",
105+
"@material/dialog": "12.0.0-canary.03f525f9f.0",
106+
"@material/dom": "12.0.0-canary.03f525f9f.0",
107+
"@material/drawer": "12.0.0-canary.03f525f9f.0",
108+
"@material/elevation": "12.0.0-canary.03f525f9f.0",
109+
"@material/fab": "12.0.0-canary.03f525f9f.0",
110+
"@material/feature-targeting": "12.0.0-canary.03f525f9f.0",
111+
"@material/floating-label": "12.0.0-canary.03f525f9f.0",
112+
"@material/form-field": "12.0.0-canary.03f525f9f.0",
113+
"@material/icon-button": "12.0.0-canary.03f525f9f.0",
114+
"@material/image-list": "12.0.0-canary.03f525f9f.0",
115+
"@material/layout-grid": "12.0.0-canary.03f525f9f.0",
116+
"@material/line-ripple": "12.0.0-canary.03f525f9f.0",
117+
"@material/linear-progress": "12.0.0-canary.03f525f9f.0",
118+
"@material/list": "12.0.0-canary.03f525f9f.0",
119+
"@material/menu": "12.0.0-canary.03f525f9f.0",
120+
"@material/menu-surface": "12.0.0-canary.03f525f9f.0",
121+
"@material/notched-outline": "12.0.0-canary.03f525f9f.0",
122+
"@material/radio": "12.0.0-canary.03f525f9f.0",
123+
"@material/ripple": "12.0.0-canary.03f525f9f.0",
124+
"@material/rtl": "12.0.0-canary.03f525f9f.0",
125+
"@material/segmented-button": "12.0.0-canary.03f525f9f.0",
126+
"@material/select": "12.0.0-canary.03f525f9f.0",
127+
"@material/shape": "12.0.0-canary.03f525f9f.0",
128+
"@material/slider": "12.0.0-canary.03f525f9f.0",
129+
"@material/snackbar": "12.0.0-canary.03f525f9f.0",
130+
"@material/switch": "12.0.0-canary.03f525f9f.0",
131+
"@material/tab": "12.0.0-canary.03f525f9f.0",
132+
"@material/tab-bar": "12.0.0-canary.03f525f9f.0",
133+
"@material/tab-indicator": "12.0.0-canary.03f525f9f.0",
134+
"@material/tab-scroller": "12.0.0-canary.03f525f9f.0",
135+
"@material/textfield": "12.0.0-canary.03f525f9f.0",
136+
"@material/theme": "12.0.0-canary.03f525f9f.0",
137+
"@material/tooltip": "12.0.0-canary.03f525f9f.0",
138+
"@material/top-app-bar": "12.0.0-canary.03f525f9f.0",
139+
"@material/touch-target": "12.0.0-canary.03f525f9f.0",
140+
"@material/typography": "12.0.0-canary.03f525f9f.0",
141141
"@octokit/rest": "18.3.5",
142142
"@schematics/angular": "^12.0.0-rc.0",
143143
"@types/autoprefixer": "^9.7.2",

scripts/check-mdc-tests-config.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,10 @@ export const config = {
8282
'should work in a step'
8383
],
8484
'mdc-list': [
85+
// MDC does focus previously focused options, but rather always selects the first selected
86+
// option. We have different test in the MDC-based list that captures this behavior.
87+
'should focus the previously focused option when the list takes focus a second time',
88+
8589
// TODO: these tests need to be double-checked for missing functionality.
8690
'should not apply any additional class to a list without lines',
8791
'should not add the mat-list-single-selected-option class (in multiple mode)',

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

Lines changed: 36 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -289,17 +289,24 @@ describe('MDC-based MatSelectionList without forms', () => {
289289
expect(listOptions.slice(1).every(o => o.nativeElement.tabIndex === -1)).toBe(true);
290290
});
291291

292-
it('should focus the previously focused option when the list takes focus a second time',
293-
fakeAsync(() => {
294-
expect(listOptions[0].nativeElement.tabIndex).toBe(0);
295-
expect(listOptions[1].nativeElement.tabIndex).toBe(-1);
292+
it('should focus the first selected option when list receives focus', fakeAsync(() => {
293+
dispatchMouseEvent(listOptions[2].nativeElement, 'click');
294+
fixture.detectChanges();
295+
296+
expect(listOptions.map(o => o.nativeElement.tabIndex)).toEqual([-1, -1, 0, -1, -1]);
297+
298+
dispatchMouseEvent(listOptions[1].nativeElement, 'click');
299+
fixture.detectChanges();
296300

297-
dispatchFakeEvent(listOptions[1].nativeElement, 'focusin', true);
298-
dispatchFakeEvent(listOptions[1].nativeElement, 'focusout', true);
299-
tick(1);
301+
expect(listOptions.map(o => o.nativeElement.tabIndex)).toEqual([-1, 0, -1, -1, -1]);
300302

301-
expect(listOptions[0].nativeElement.tabIndex).toBe(-1);
302-
expect(listOptions[1].nativeElement.tabIndex).toBe(0);
303+
// De-select both options to ensure that the first item in the list-item
304+
// becomes the designated option for focus.
305+
dispatchMouseEvent(listOptions[1].nativeElement, 'click');
306+
dispatchMouseEvent(listOptions[2].nativeElement, 'click');
307+
fixture.detectChanges();
308+
309+
expect(listOptions.map(o => o.nativeElement.tabIndex)).toEqual([0, -1, -1, -1, -1]);
303310
}));
304311

305312
it('should focus previous item when press UP ARROW', () => {
@@ -567,7 +574,7 @@ describe('MDC-based MatSelectionList without forms', () => {
567574

568575
describe('with list option selected', () => {
569576
let fixture: ComponentFixture<SelectionListWithSelectedOption>;
570-
let listItemEl: DebugElement;
577+
let listOptionElements: DebugElement[];
571578
let selectionList: DebugElement;
572579

573580
beforeEach(waitForAsync(() => {
@@ -581,16 +588,28 @@ describe('MDC-based MatSelectionList without forms', () => {
581588

582589
beforeEach(waitForAsync(() => {
583590
fixture = TestBed.createComponent(SelectionListWithSelectedOption);
584-
listItemEl = fixture.debugElement.query(By.directive(MatListOption))!;
591+
listOptionElements = fixture.debugElement.queryAll(By.directive(MatListOption))!;
585592
selectionList = fixture.debugElement.query(By.directive(MatSelectionList))!;
586593
fixture.detectChanges();
587594
}));
588595

589596
it('should set its initial selected state in the selectedOptions', () => {
590-
let optionEl = listItemEl.injector.get<MatListOption>(MatListOption);
597+
let options = listOptionElements.map(optionEl =>
598+
optionEl.injector.get<MatListOption>(MatListOption));
591599
let selectedOptions = selectionList.componentInstance.selectedOptions;
592-
expect(selectedOptions.isSelected(optionEl)).toBeTruthy();
600+
expect(selectedOptions.isSelected(options[0])).toBeFalse();
601+
expect(selectedOptions.isSelected(options[1])).toBeTrue();
602+
expect(selectedOptions.isSelected(options[2])).toBeTrue();
603+
expect(selectedOptions.isSelected(options[3])).toBeFalse();
593604
});
605+
606+
it('should focus the first selected option on first focus if an item is pre-selected',
607+
fakeAsync(() => {
608+
// MDC manages the focus through setting a `tabindex` on the designated list item. We
609+
// assert that the proper tabindex is set on the pre-selected option at index 1, and
610+
// ensure that other options are not reachable through tab.
611+
expect(listOptionElements.map(el => el.nativeElement.tabIndex)).toEqual([-1, 0, -1, -1]);
612+
}));
594613
});
595614

596615
describe('with option disabled', () => {
@@ -1414,7 +1433,10 @@ class SelectionListWithDisabledOption {
14141433

14151434
@Component({template: `
14161435
<mat-selection-list>
1417-
<mat-list-option [selected]="true">Item</mat-list-option>
1436+
<mat-list-option>Not selected - Item #1</mat-list-option>
1437+
<mat-list-option [selected]="true">Pre-selected - Item #2</mat-list-option>
1438+
<mat-list-option [selected]="true">Pre-selected - Item #3</mat-list-option>
1439+
<mat-list-option>Not selected - Item #4</mat-list-option>
14181440
</mat-selection-list>`})
14191441
class SelectionListWithSelectedOption {
14201442
}

src/material-experimental/mdc-list/selection-list.ts

Lines changed: 38 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ import {
2828
} from '@angular/core';
2929
import {ControlValueAccessor, NG_VALUE_ACCESSOR} from '@angular/forms';
3030
import {ThemePalette} from '@angular/material-experimental/mdc-core';
31-
import {MDCListAdapter} from '@material/list';
31+
import {MDCListAdapter, numbers as mdcListNumbers} from '@material/list';
3232
import {Subject} from 'rxjs';
3333
import {takeUntil} from 'rxjs/operators';
3434
import {getInteractiveListAdapter, MatInteractiveListBase} from './interactive-list-base';
@@ -141,34 +141,26 @@ export class MatSelectionList extends MatInteractiveListBase<MatListOption>
141141
// binding can no longer be changed.
142142
this._initialized = true;
143143

144+
// Start monitoring the selected options so that the list foundation can be
145+
// updated accordingly.
146+
this._watchForSelectionChange();
147+
144148
// Update the options if a control value has been set initially.
145149
if (this._value) {
146150
this._setOptionsFromValues(this._value);
147151
}
148152

149-
// Sync external changes to the model back to the options.
150-
this.selectedOptions.changed.pipe(takeUntil(this._destroyed)).subscribe(event => {
151-
if (event.added) {
152-
for (let item of event.added) {
153-
item.selected = true;
154-
}
155-
}
156-
157-
if (event.removed) {
158-
for (let item of event.removed) {
159-
item.selected = false;
160-
}
161-
}
153+
// Initialize the list foundation, including the initial `layout()` invocation.
154+
super.ngAfterViewInit();
162155

163-
// Sync the newly selected options with the foundation. Also reset tabindex for all
164-
// items if the list is currently not focused. We do this so that always the first
165-
// selected list item is focused when users tab into the selection list.
156+
// List options can be pre-selected using the `selected` input. We need to sync the selected
157+
// options after view initialization with the foundation so that focus can be managed
158+
// accordingly. Note that this needs to happen after the initial `layout()` call because the
159+
// list wouldn't know about multi-selection and throw.
160+
if (this._items.length !== 0) {
166161
this._syncSelectedOptionsWithFoundation();
167162
this._resetTabindexForItemsIfBlurred();
168-
});
169-
170-
// Complete the list foundation initialization.
171-
super.ngAfterViewInit();
163+
}
172164
}
173165

174166
ngOnChanges(changes: SimpleChanges) {
@@ -262,13 +254,37 @@ export class MatSelectionList extends MatInteractiveListBase<MatListOption>
262254
}
263255
}
264256

257+
private _watchForSelectionChange() {
258+
// Sync external changes to the model back to the options.
259+
this.selectedOptions.changed.pipe(takeUntil(this._destroyed)).subscribe(event => {
260+
if (event.added) {
261+
for (let item of event.added) {
262+
item.selected = true;
263+
}
264+
}
265+
266+
if (event.removed) {
267+
for (let item of event.removed) {
268+
item.selected = false;
269+
}
270+
}
271+
272+
// Sync the newly selected options with the foundation. Also reset tabindex for all
273+
// items if the list is currently not focused. We do this so that always the first
274+
// selected list item is focused when users tab into the selection list.
275+
this._syncSelectedOptionsWithFoundation();
276+
this._resetTabindexForItemsIfBlurred();
277+
});
278+
}
279+
265280
private _syncSelectedOptionsWithFoundation() {
266281
if (this._multiple) {
267282
this._foundation.setSelectedIndex(this.selectedOptions.selected
268283
.map(o => this._itemsArr.indexOf(o)));
269284
} else {
270285
const selected = this.selectedOptions.selected[0];
271-
const index = selected === undefined ? -1 : this._itemsArr.indexOf(selected);
286+
const index = selected === undefined ?
287+
mdcListNumbers.UNSET_INDEX : this._itemsArr.indexOf(selected);
272288
this._foundation.setSelectedIndex(index);
273289
}
274290
}

0 commit comments

Comments
 (0)