From c80f400dac7cdfd82b06c372f46055557d227b5f Mon Sep 17 00:00:00 2001 From: Zach Arend Date: Thu, 10 Aug 2023 20:29:07 +0000 Subject: [PATCH] fix(cdk/tree): CdkTreeNodeToggle focuses node when toggling it Fix focus behavior of CdkTreeNodeToggle. When toggling the expanded or collapsed state of a node, focus that node. Fix issue where end user cannot tab into tree component when collaping the parent of the active node. Before this commit is applied, there is a bug where end user cannot tab into the tree. Reproduction steps 1. Active a tree node 2. (focus state renders) 3. Using mouse, collapse parent of node from step 1. 4. (tree node collapses) 5. Press Tab, then shift + tab 6. (item before tree is focused. Can tab into the tree) With this commit applied, above issue is no longer expected to reproduce. This commit message is only for reviewers of this PR and can be deleted when landing this change in main. --- src/cdk/a11y/key-manager/tree-key-manager.ts | 22 ++++++------ src/cdk/tree/toggle.ts | 8 ++++- src/cdk/tree/tree-redesign.spec.ts | 37 ++++++++++++++++++-- tools/public_api_guard/cdk/a11y.md | 4 +++ 4 files changed, 56 insertions(+), 15 deletions(-) diff --git a/src/cdk/a11y/key-manager/tree-key-manager.ts b/src/cdk/a11y/key-manager/tree-key-manager.ts index f5103d05b42b..d14cb7f9087b 100644 --- a/src/cdk/a11y/key-manager/tree-key-manager.ts +++ b/src/cdk/a11y/key-manager/tree-key-manager.ts @@ -263,7 +263,7 @@ export class TreeKeyManager { * @param treeItem The item that was clicked by the user. */ onClick(treeItem: T) { - this._setActiveItem(treeItem); + this.setActiveItem(treeItem); } /** Index of the currently active item. */ @@ -284,9 +284,9 @@ export class TreeKeyManager { this._focusFirstItem(); } - private _setActiveItem(index: number): void; - private _setActiveItem(item: T): void; - private _setActiveItem(itemOrIndex: number | T) { + setActiveItem(index: number): void; + setActiveItem(item: T): void; + setActiveItem(itemOrIndex: number | T) { let index = typeof itemOrIndex === 'number' ? itemOrIndex @@ -358,7 +358,7 @@ export class TreeKeyManager { !this._skipPredicateFn(item) && item.getLabel?.().toLocaleUpperCase().trim().indexOf(inputString) === 0 ) { - this._setActiveItem(index); + this.setActiveItem(index); break; } } @@ -370,19 +370,19 @@ export class TreeKeyManager { //// Navigational methods private _focusFirstItem() { - this._setActiveItem(this._findNextAvailableItemIndex(-1)); + this.setActiveItem(this._findNextAvailableItemIndex(-1)); } private _focusLastItem() { - this._setActiveItem(this._findPreviousAvailableItemIndex(this._items.length)); + this.setActiveItem(this._findPreviousAvailableItemIndex(this._items.length)); } private _focusPreviousItem() { - this._setActiveItem(this._findPreviousAvailableItemIndex(this._activeItemIndex)); + this.setActiveItem(this._findPreviousAvailableItemIndex(this._activeItemIndex)); } private _focusNextItem() { - this._setActiveItem(this._findNextAvailableItemIndex(this._activeItemIndex)); + this.setActiveItem(this._findNextAvailableItemIndex(this._activeItemIndex)); } private _findNextAvailableItemIndex(startingIndex: number) { @@ -418,7 +418,7 @@ export class TreeKeyManager { if (!parent || this._skipPredicateFn(parent as T)) { return; } - this._setActiveItem(parent as T); + this.setActiveItem(parent as T); } } @@ -440,7 +440,7 @@ export class TreeKeyManager { if (!firstChild) { return; } - this._setActiveItem(firstChild as T); + this.setActiveItem(firstChild as T); }); } } diff --git a/src/cdk/tree/toggle.ts b/src/cdk/tree/toggle.ts index 3a4466f776ca..e438784f98c4 100644 --- a/src/cdk/tree/toggle.ts +++ b/src/cdk/tree/toggle.ts @@ -7,7 +7,7 @@ */ import {BooleanInput, coerceBooleanProperty} from '@angular/cdk/coercion'; -import {Directive, Input} from '@angular/core'; +import {ChangeDetectorRef, Directive, Input, inject} from '@angular/core'; import {CdkTree, CdkTreeNode} from './tree'; @@ -34,11 +34,17 @@ export class CdkTreeNodeToggle { constructor(protected _tree: CdkTree, protected _treeNode: CdkTreeNode) {} + // Toggle the expanded or collapsed state of this node. + + // Focus this node with expanding or collapsing it. This ensures that the active node will always + // be visible when expanding and collapsing. _toggle(event: Event): void { this.recursive ? this._tree.toggleDescendants(this._treeNode.data) : this._tree.toggle(this._treeNode.data); + this._tree._keyManager.setActiveItem(this._treeNode); + event.stopPropagation(); } } diff --git a/src/cdk/tree/tree-redesign.spec.ts b/src/cdk/tree/tree-redesign.spec.ts index 53f96977fc6b..32caf84d329c 100644 --- a/src/cdk/tree/tree-redesign.spec.ts +++ b/src/cdk/tree/tree-redesign.spec.ts @@ -270,6 +270,26 @@ describe('CdkTree redesign', () => { .toBe(0); }); + it('should focus a node when collapsing it', () => { + dataSource.clear(); + const parent = dataSource.addData(); + const child = dataSource.addChild(parent); + component.tree.expandAll(); + fixture.detectChanges(); + + // focus the child node + getNodes(treeElement)[1].click(); + fixture.detectChanges(); + + // collapse the parent node + getNodes(treeElement)[0].click(); + fixture.detectChanges(); + + expect(getNodes(treeElement).map(x => x.getAttribute('tabindex'))) + .withContext(`Expecting parent node to be focused since it was collapsed.`) + .toEqual(['0', '-1']); + }); + it('should expand/collapse the node recursively', () => { expect(dataSource.data.length).toBe(3); @@ -1310,15 +1330,21 @@ class FakeDataSource extends DataSource { return child; } - addData(level: number = 1) { + addData(level: number = 1): TestData { const nextIndex = ++this.dataIndex; let copiedData = this.data.slice(); - copiedData.push( - new TestData(`topping_${nextIndex}`, `cheese_${nextIndex}`, `base_${nextIndex}`, level), + const newData = new TestData( + `topping_${nextIndex}`, + `cheese_${nextIndex}`, + `base_${nextIndex}`, + level, ); + copiedData.push(newData); this.data = copiedData; + + return newData; } getRecursiveData(nodes: TestData[] = this._dataChange.getValue()): TestData[] { @@ -1326,6 +1352,11 @@ class FakeDataSource extends DataSource { ...new Set(nodes.flatMap(parent => [parent, ...this.getRecursiveData(parent.children)])), ]; } + + clear() { + this.data = []; + this.dataIndex = 0; + } } function getNodes(treeElement: Element): HTMLElement[] { diff --git a/tools/public_api_guard/cdk/a11y.md b/tools/public_api_guard/cdk/a11y.md index 4e8ab944446e..a28e5a29efbd 100644 --- a/tools/public_api_guard/cdk/a11y.md +++ b/tools/public_api_guard/cdk/a11y.md @@ -433,6 +433,10 @@ export class TreeKeyManager { onClick(treeItem: T): void; onInitialFocus(): void; onKeydown(event: KeyboardEvent): void; + // (undocumented) + setActiveItem(index: number): void; + // (undocumented) + setActiveItem(item: T): void; readonly tabOut: Subject; }