From bca7c3e5f7ab9323012abab699a45421a918ea9b Mon Sep 17 00:00:00 2001 From: Cassandra Choi Date: Mon, 18 Dec 2023 19:14:31 +0000 Subject: [PATCH 1/6] fix(cdk/tree): fix errors from testing --- src/cdk/tree/tree.ts | 16 +++++++++------- src/material/tree/testing/node-harness.ts | 5 +++++ 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/cdk/tree/tree.ts b/src/cdk/tree/tree.ts index 4631a4a16d2b..3afa02834626 100644 --- a/src/cdk/tree/tree.ts +++ b/src/cdk/tree/tree.ts @@ -449,7 +449,9 @@ export class CdkTree } private _initializeDataDiffer() { - this._dataDiffer = this._differs.find([]).create(this.trackBy); + // Provide a default trackBy based on `_getExpansionKey` if one isn't provided. + const trackBy = this.trackBy ?? ((_index: number, item: T) => this._getExpansionKey(item)); + this._dataDiffer = this._differs.find([]).create(trackBy); } private _checkTreeControlUsage() { @@ -484,11 +486,8 @@ export class CdkTree parentData?: T, ) { const changes = dataDiffer.diff(data); - if (!changes) { - return; - } - changes.forEachOperation( + changes?.forEachOperation( ( item: IterableChangeRecord, adjustedPreviousIndex: number | null, @@ -511,6 +510,9 @@ export class CdkTree }, ); + // Some tree consumers expect change detection to propagate to nodes + // even when the array itself hasn't changed; we explicitly detect changes + // anyways in order for nodes to update their data. this._changeDetectorRef.detectChanges(); } @@ -682,12 +684,12 @@ export class CdkTree /** Level accessor, used for compatibility between the old Tree and new Tree */ _getLevelAccessor() { - return this.treeControl?.getLevel ?? this.levelAccessor; + return this.treeControl?.getLevel?.bind(this.treeControl) ?? this.levelAccessor; } /** Children accessor, used for compatibility between the old Tree and new Tree */ _getChildrenAccessor() { - return this.treeControl?.getChildren ?? this.childrenAccessor; + return this.treeControl?.getChildren?.bind(this.treeControl) ?? this.childrenAccessor; } /** diff --git a/src/material/tree/testing/node-harness.ts b/src/material/tree/testing/node-harness.ts index b97fcf94906b..4a771f47e649 100644 --- a/src/material/tree/testing/node-harness.ts +++ b/src/material/tree/testing/node-harness.ts @@ -35,6 +35,11 @@ export class MatTreeNodeHarness extends ContentContainerComponentHarness return coerceBooleanProperty(await (await this.host()).getAttribute('aria-expanded')); } + /** Whether the tree node is expandable. */ + async isExpandable(): Promise { + return (await (await this.host()).getAttribute('aria-expanded')) !== null; + } + /** Whether the tree node is disabled. */ async isDisabled(): Promise { return coerceBooleanProperty(await (await this.host()).getProperty('aria-disabled')); From 70f976281ad6c2265c4caa4de55f90d3c1412150 Mon Sep 17 00:00:00 2001 From: Cassandra Choi Date: Mon, 18 Dec 2023 20:32:20 +0000 Subject: [PATCH 2/6] fix(cdk/tree): tests --- .../a11y/key-manager/tree-key-manager.spec.ts | 14 ++++++++-- src/cdk/a11y/key-manager/tree-key-manager.ts | 6 +++- src/cdk/tree/tree.ts | 28 ++++++++++++++++--- src/material/tree/tree.spec.ts | 3 +- 4 files changed, 43 insertions(+), 8 deletions(-) diff --git a/src/cdk/a11y/key-manager/tree-key-manager.spec.ts b/src/cdk/a11y/key-manager/tree-key-manager.spec.ts index 338aead9c0e4..77a89d78e139 100644 --- a/src/cdk/a11y/key-manager/tree-key-manager.spec.ts +++ b/src/cdk/a11y/key-manager/tree-key-manager.spec.ts @@ -182,8 +182,18 @@ describe('TreeKeyManager', () => { itemList.notifyOnChanges(); }); - it('initializes with the first item activated', () => { - expect(keyManager.getActiveItemIndex()).withContext('active item index').toBe(0); + it('does not initialize with the first item activated', () => { + expect(keyManager.getActiveItemIndex()).withContext('active item index').toBe(-1); + }); + + it('if an item is subsequently enabled, activates it', () => { + itemList.reset([ + new itemParam.constructor('Bilbo', true), + new itemParam.constructor('Frodo', false), + new itemParam.constructor('Pippin', true), + ]); + itemList.notifyOnChanges(); + expect(keyManager.getActiveItemIndex()).withContext('active item index').toBe(1); }); }); diff --git a/src/cdk/a11y/key-manager/tree-key-manager.ts b/src/cdk/a11y/key-manager/tree-key-manager.ts index c3d1fd683e0f..81323e40ca35 100644 --- a/src/cdk/a11y/key-manager/tree-key-manager.ts +++ b/src/cdk/a11y/key-manager/tree-key-manager.ts @@ -73,7 +73,7 @@ export class TreeKeyManager implements TreeKeyMana return; } - let focusIndex = 0; + let focusIndex = -1; for (let i = 0; i < this._items.length; i++) { if (!this._skipPredicateFn(this._items[i]) && !this._isItemDisabled(this._items[i])) { focusIndex = i; @@ -81,6 +81,10 @@ export class TreeKeyManager implements TreeKeyMana } } + if (focusIndex === -1) { + return; + } + this.focusItem(focusIndex); this._hasInitialFocused = true; } diff --git a/src/cdk/tree/tree.ts b/src/cdk/tree/tree.ts index 3afa02834626..b3d692ebec83 100644 --- a/src/cdk/tree/tree.ts +++ b/src/cdk/tree/tree.ts @@ -23,6 +23,7 @@ import { import { AfterContentChecked, AfterContentInit, + AfterViewInit, ChangeDetectionStrategy, ChangeDetectorRef, Component, @@ -111,7 +112,13 @@ type RenderingData = imports: [CdkTreeNodeOutlet], }) export class CdkTree - implements AfterContentChecked, AfterContentInit, CollectionViewer, OnDestroy, OnInit + implements + AfterContentChecked, + AfterContentInit, + AfterViewInit, + CollectionViewer, + OnDestroy, + OnInit { /** Subject that emits when the component has been destroyed. */ private readonly _onDestroy = new Subject(); @@ -248,6 +255,7 @@ export class CdkTree /** The key manager for this tree. Handles focus and activation based on user keyboard input. */ _keyManager: TreeKeyManagerStrategy>; + private _viewInit = false; constructor( private _differs: IterableDiffers, @@ -288,6 +296,10 @@ export class CdkTree this._initializeDataDiffer(); } + ngAfterViewInit() { + this._viewInit = true; + } + private _updateDefaultNodeDefinition() { const defaultNodeDefs = this._nodeDefs.filter(def => !def.when); if (defaultNodeDefs.length > 1 && (typeof ngDevMode === 'undefined' || ngDevMode)) { @@ -487,6 +499,17 @@ export class CdkTree ) { const changes = dataDiffer.diff(data); + // Some tree consumers expect change detection to propagate to nodes + // even when the array itself hasn't changed; we explicitly detect changes + // anyways in order for nodes to update their data. + // + // However, if change detection is called while the component's view is + // still initing, then the order of child views initing will be incorrect; + // to prevent this, we only exit early if the view hasn't initialized yet. + if (!changes && !this._viewInit) { + return; + } + changes?.forEachOperation( ( item: IterableChangeRecord, @@ -510,9 +533,6 @@ export class CdkTree }, ); - // Some tree consumers expect change detection to propagate to nodes - // even when the array itself hasn't changed; we explicitly detect changes - // anyways in order for nodes to update their data. this._changeDetectorRef.detectChanges(); } diff --git a/src/material/tree/tree.spec.ts b/src/material/tree/tree.spec.ts index 46a72fca1f35..0790cb29c3b9 100644 --- a/src/material/tree/tree.spec.ts +++ b/src/material/tree/tree.spec.ts @@ -604,11 +604,12 @@ describe('MatTree', () => { }); it('ignores clicks on disabled items', () => { - underlyingDataSource.data[0].isDisabled = true; + underlyingDataSource.data[1].isDisabled = true; fixture.detectChanges(); // attempt to click on the first child nodes[1].click(); + fixture.detectChanges(); expect(nodes.map(x => x.getAttribute('tabindex')).join(', ')).toEqual( '0, -1, -1, -1, -1, -1', From 7dea4fb54fc3c9445594ffdd6163898066f0da69 Mon Sep 17 00:00:00 2001 From: Cassandra Choi Date: Mon, 18 Dec 2023 20:46:24 +0000 Subject: [PATCH 3/6] fix(cdk/tree): update api docs --- tools/public_api_guard/cdk/tree.md | 5 ++++- tools/public_api_guard/material/tree-testing.md | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/tools/public_api_guard/cdk/tree.md b/tools/public_api_guard/cdk/tree.md index ed2db380b279..cd8811a3a738 100644 --- a/tools/public_api_guard/cdk/tree.md +++ b/tools/public_api_guard/cdk/tree.md @@ -6,6 +6,7 @@ import { AfterContentChecked } from '@angular/core'; import { AfterContentInit } from '@angular/core'; +import { AfterViewInit } from '@angular/core'; import { BehaviorSubject } from 'rxjs'; import { ChangeDetectorRef } from '@angular/core'; import { CollectionViewer } from '@angular/cdk/collections'; @@ -76,7 +77,7 @@ export class CdkNestedTreeNode extends CdkTreeNode implements Af } // @public -export class CdkTree implements AfterContentChecked, AfterContentInit, CollectionViewer, OnDestroy, OnInit { +export class CdkTree implements AfterContentChecked, AfterContentInit, AfterViewInit, CollectionViewer, OnDestroy, OnInit { constructor(_differs: IterableDiffers, _changeDetectorRef: ChangeDetectorRef, _dir: Directionality); childrenAccessor?: (dataNode: T) => T[] | Observable; collapse(dataNode: T): void; @@ -106,6 +107,8 @@ export class CdkTree implements AfterContentChecked, AfterContentInit, // (undocumented) ngAfterContentInit(): void; // (undocumented) + ngAfterViewInit(): void; + // (undocumented) ngOnDestroy(): void; // (undocumented) ngOnInit(): void; diff --git a/tools/public_api_guard/material/tree-testing.md b/tools/public_api_guard/material/tree-testing.md index 84223f738ab0..86a25118e453 100644 --- a/tools/public_api_guard/material/tree-testing.md +++ b/tools/public_api_guard/material/tree-testing.md @@ -27,6 +27,7 @@ export class MatTreeNodeHarness extends ContentContainerComponentHarness getText(): Promise; static hostSelector: string; isDisabled(): Promise; + isExpandable(): Promise; isExpanded(): Promise; toggle(): Promise; // (undocumented) From f160dc18411e110c1b45fdb4ba93d9940724bf2c Mon Sep 17 00:00:00 2001 From: Cassandra Choi Date: Tue, 19 Dec 2023 20:03:57 +0000 Subject: [PATCH 4/6] fix(cdk/a11y): allows disabled items to receive initial focus --- src/cdk/a11y/key-manager/tree-key-manager.spec.ts | 14 ++------------ src/cdk/a11y/key-manager/tree-key-manager.ts | 6 +----- 2 files changed, 3 insertions(+), 17 deletions(-) diff --git a/src/cdk/a11y/key-manager/tree-key-manager.spec.ts b/src/cdk/a11y/key-manager/tree-key-manager.spec.ts index 77a89d78e139..338aead9c0e4 100644 --- a/src/cdk/a11y/key-manager/tree-key-manager.spec.ts +++ b/src/cdk/a11y/key-manager/tree-key-manager.spec.ts @@ -182,18 +182,8 @@ describe('TreeKeyManager', () => { itemList.notifyOnChanges(); }); - it('does not initialize with the first item activated', () => { - expect(keyManager.getActiveItemIndex()).withContext('active item index').toBe(-1); - }); - - it('if an item is subsequently enabled, activates it', () => { - itemList.reset([ - new itemParam.constructor('Bilbo', true), - new itemParam.constructor('Frodo', false), - new itemParam.constructor('Pippin', true), - ]); - itemList.notifyOnChanges(); - expect(keyManager.getActiveItemIndex()).withContext('active item index').toBe(1); + it('initializes with the first item activated', () => { + expect(keyManager.getActiveItemIndex()).withContext('active item index').toBe(0); }); }); diff --git a/src/cdk/a11y/key-manager/tree-key-manager.ts b/src/cdk/a11y/key-manager/tree-key-manager.ts index 81323e40ca35..c3d1fd683e0f 100644 --- a/src/cdk/a11y/key-manager/tree-key-manager.ts +++ b/src/cdk/a11y/key-manager/tree-key-manager.ts @@ -73,7 +73,7 @@ export class TreeKeyManager implements TreeKeyMana return; } - let focusIndex = -1; + let focusIndex = 0; for (let i = 0; i < this._items.length; i++) { if (!this._skipPredicateFn(this._items[i]) && !this._isItemDisabled(this._items[i])) { focusIndex = i; @@ -81,10 +81,6 @@ export class TreeKeyManager implements TreeKeyMana } } - if (focusIndex === -1) { - return; - } - this.focusItem(focusIndex); this._hasInitialFocused = true; } From 9a633b7cb603e02f0c61b27e8913189050a2bd66 Mon Sep 17 00:00:00 2001 From: Cassandra Choi Date: Fri, 2 Feb 2024 20:10:02 +0000 Subject: [PATCH 5/6] fix(cdk/tree): don't focus on click, corrected updating aria-sets --- src/cdk/tree/tree-with-tree-control.spec.ts | 1 + src/cdk/tree/tree.spec.ts | 1 + src/cdk/tree/tree.ts | 32 +++++++++++++------ .../tree/tree-using-tree-control.spec.ts | 1 + src/material/tree/tree.spec.ts | 1 + 5 files changed, 27 insertions(+), 9 deletions(-) diff --git a/src/cdk/tree/tree-with-tree-control.spec.ts b/src/cdk/tree/tree-with-tree-control.spec.ts index bb2a8495799a..41ab5d3aed09 100644 --- a/src/cdk/tree/tree-with-tree-control.spec.ts +++ b/src/cdk/tree/tree-with-tree-control.spec.ts @@ -1174,6 +1174,7 @@ describe('CdkTree', () => { it('maintains tabindex when component is blurred', () => { // activate the second child by clicking on it nodes[1].click(); + nodes[1].focus(); fixture.detectChanges(); expect(document.activeElement).toBe(nodes[1]); diff --git a/src/cdk/tree/tree.spec.ts b/src/cdk/tree/tree.spec.ts index 3e8e33d81daa..fac87f95adc4 100644 --- a/src/cdk/tree/tree.spec.ts +++ b/src/cdk/tree/tree.spec.ts @@ -1216,6 +1216,7 @@ describe('CdkTree', () => { it('maintains tabindex when component is blurred', () => { // activate the second child by clicking on it nodes[1].click(); + nodes[1].focus(); fixture.detectChanges(); expect(document.activeElement).toBe(nodes[1]); diff --git a/src/cdk/tree/tree.ts b/src/cdk/tree/tree.ts index b3d692ebec83..f90cee83207f 100644 --- a/src/cdk/tree/tree.ts +++ b/src/cdk/tree/tree.ts @@ -288,7 +288,9 @@ export class CdkTree this._dataSubscription = null; } - this._keyManager.destroy(); + // In certain tests, the tree might be destroyed before this is initialized + // in `ngAfterContentInit`. + this._keyManager?.destroy(); } ngOnInit() { @@ -520,12 +522,6 @@ export class CdkTree this.insertNode(data[currentIndex!], currentIndex!, viewContainer, parentData); } else if (currentIndex == null) { viewContainer.remove(adjustedPreviousIndex!); - const set = this._getAriaSet(item.item); - const key = this._getExpansionKey(item.item); - set.splice( - set.findIndex(groupItem => this._getExpansionKey(groupItem) === key), - 1, - ); } else { const view = viewContainer.get(adjustedPreviousIndex!); viewContainer.move(view!, currentIndex); @@ -1116,7 +1112,7 @@ export class CdkTree '[attr.aria-setsize]': '_getSetSize()', '[tabindex]': '_tabindex', 'role': 'treeitem', - '(click)': '_focusItem()', + '(click)': '_setActiveItem()', '(focus)': '_focusItem()', }, standalone: true, @@ -1194,6 +1190,13 @@ export class CdkTreeNode implements OnDestroy, OnInit, TreeKeyManagerI readonly _dataChanges = new Subject(); private _inputIsExpandable: boolean = false; + /** + * Flag used to determine whether or not we should be focusing the actual element based on + * some user interaction (click or focus). On click, we don't forcibly focus the element + * since the click could trigger some other component that wants to grab its own focus + * (e.g. menu, dialog). + */ + private _shouldFocus = true; private _parentNodeAriaLevel: number; /** The tree node's data. */ @@ -1295,7 +1298,9 @@ export class CdkTreeNode implements OnDestroy, OnInit, TreeKeyManagerI /** Focuses this data node. Implemented for TreeKeyManagerItem. */ focus(): void { this._tabindex = 0; - this._elementRef.nativeElement.focus(); + if (this._shouldFocus) { + this._elementRef.nativeElement.focus(); + } this._changeDetectorRef.markForCheck(); } @@ -1336,6 +1341,15 @@ export class CdkTreeNode implements OnDestroy, OnInit, TreeKeyManagerI this._tree._keyManager.focusItem(this); } + _setActiveItem() { + if (this.isDisabled) { + return; + } + this._shouldFocus = false; + this._tree._keyManager.focusItem(this); + this._shouldFocus = true; + } + _emitExpansionState(expanded: boolean) { this.expandedChange.emit(expanded); } diff --git a/src/material/tree/tree-using-tree-control.spec.ts b/src/material/tree/tree-using-tree-control.spec.ts index 909ddbf62c4a..5636c15c8761 100644 --- a/src/material/tree/tree-using-tree-control.spec.ts +++ b/src/material/tree/tree-using-tree-control.spec.ts @@ -602,6 +602,7 @@ describe('MatTree', () => { it('maintains tabindex when component is blurred', () => { // activate the second child by clicking on it nodes[1].click(); + nodes[1].focus(); fixture.detectChanges(); expect(document.activeElement).toBe(nodes[1]); diff --git a/src/material/tree/tree.spec.ts b/src/material/tree/tree.spec.ts index 0790cb29c3b9..17cb229927a9 100644 --- a/src/material/tree/tree.spec.ts +++ b/src/material/tree/tree.spec.ts @@ -587,6 +587,7 @@ describe('MatTree', () => { it('maintains tabindex when component is blurred', () => { // activate the second child by clicking on it nodes[1].click(); + nodes[1].focus(); fixture.detectChanges(); expect(nodes.map(x => x.getAttribute('tabindex')).join(', ')).toEqual( From df8b4d5e9978147bb56cac687d7a99c525531aed Mon Sep 17 00:00:00 2001 From: Cassandra Choi Date: Mon, 12 Feb 2024 22:02:40 +0000 Subject: [PATCH 6/6] fix(cdk/tree): update api goldens --- tools/public_api_guard/cdk/tree.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/public_api_guard/cdk/tree.md b/tools/public_api_guard/cdk/tree.md index cd8811a3a738..d21f0db7bb53 100644 --- a/tools/public_api_guard/cdk/tree.md +++ b/tools/public_api_guard/cdk/tree.md @@ -197,6 +197,8 @@ export class CdkTreeNode implements OnDestroy, OnInit, TreeKeyManagerI get role(): 'treeitem' | 'group'; set role(_role: 'treeitem' | 'group'); // (undocumented) + _setActiveItem(): void; + // (undocumented) protected _tabindex: number | null; // (undocumented) protected _tree: CdkTree;