Skip to content

Commit

Permalink
fix(cdk/tree): assorted bug fixes (#28305)
Browse files Browse the repository at this point in the history
* fix(cdk/tree): fix errors from testing

* fix(cdk/tree): tests

* fix(cdk/tree): update api docs

* fix(cdk/a11y): allows disabled items to receive initial focus

* fix(cdk/tree): don't focus on click, corrected updating aria-sets

* fix(cdk/tree): update api goldens
  • Loading branch information
BobobUnicorn committed May 15, 2024
1 parent ee50445 commit 38cfe94
Show file tree
Hide file tree
Showing 8 changed files with 69 additions and 17 deletions.
1 change: 1 addition & 0 deletions src/cdk/tree/tree-with-tree-control.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Expand Down
1 change: 1 addition & 0 deletions src/cdk/tree/tree.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Expand Down
66 changes: 51 additions & 15 deletions src/cdk/tree/tree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
import {
AfterContentChecked,
AfterContentInit,
AfterViewInit,
ChangeDetectionStrategy,
ChangeDetectorRef,
Component,
Expand Down Expand Up @@ -111,7 +112,13 @@ type RenderingData<T> =
imports: [CdkTreeNodeOutlet],
})
export class CdkTree<T, K = T>
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<void>();
Expand Down Expand Up @@ -248,6 +255,7 @@ export class CdkTree<T, K = T>

/** The key manager for this tree. Handles focus and activation based on user keyboard input. */
_keyManager: TreeKeyManagerStrategy<CdkTreeNode<T, K>>;
private _viewInit = false;

constructor(
private _differs: IterableDiffers,
Expand Down Expand Up @@ -280,14 +288,20 @@ export class CdkTree<T, K = T>
this._dataSubscription = null;
}

this._keyManager.destroy();
// In certain tests, the tree might be destroyed before this is initialized
// in `ngAfterContentInit`.
this._keyManager?.destroy();
}

ngOnInit() {
this._checkTreeControlUsage();
this._initializeDataDiffer();
}

ngAfterViewInit() {
this._viewInit = true;
}

private _updateDefaultNodeDefinition() {
const defaultNodeDefs = this._nodeDefs.filter(def => !def.when);
if (defaultNodeDefs.length > 1 && (typeof ngDevMode === 'undefined' || ngDevMode)) {
Expand Down Expand Up @@ -449,7 +463,9 @@ export class CdkTree<T, K = T>
}

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() {
Expand Down Expand Up @@ -484,11 +500,19 @@ export class CdkTree<T, K = T>
parentData?: T,
) {
const changes = dataDiffer.diff(data);
if (!changes) {

// 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(
changes?.forEachOperation(
(
item: IterableChangeRecord<T>,
adjustedPreviousIndex: number | null,
Expand All @@ -498,12 +522,6 @@ export class CdkTree<T, K = T>
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);
Expand Down Expand Up @@ -682,12 +700,12 @@ export class CdkTree<T, K = T>

/** 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;
}

/**
Expand Down Expand Up @@ -1094,7 +1112,7 @@ export class CdkTree<T, K = T>
'[attr.aria-setsize]': '_getSetSize()',
'[tabindex]': '_tabindex',
'role': 'treeitem',
'(click)': '_focusItem()',
'(click)': '_setActiveItem()',
'(focus)': '_focusItem()',
},
standalone: true,
Expand Down Expand Up @@ -1172,6 +1190,13 @@ export class CdkTreeNode<T, K = T> implements OnDestroy, OnInit, TreeKeyManagerI
readonly _dataChanges = new Subject<void>();

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. */
Expand Down Expand Up @@ -1273,7 +1298,9 @@ export class CdkTreeNode<T, K = T> 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();
}
Expand Down Expand Up @@ -1314,6 +1341,15 @@ export class CdkTreeNode<T, K = T> 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);
}
Expand Down
5 changes: 5 additions & 0 deletions src/material/tree/testing/node-harness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ export class MatTreeNodeHarness extends ContentContainerComponentHarness<string>
return coerceBooleanProperty(await (await this.host()).getAttribute('aria-expanded'));
}

/** Whether the tree node is expandable. */
async isExpandable(): Promise<boolean> {
return (await (await this.host()).getAttribute('aria-expanded')) !== null;
}

/** Whether the tree node is disabled. */
async isDisabled(): Promise<boolean> {
return coerceBooleanProperty(await (await this.host()).getProperty('aria-disabled'));
Expand Down
1 change: 1 addition & 0 deletions src/material/tree/tree-using-tree-control.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Expand Down
4 changes: 3 additions & 1 deletion src/material/tree/tree.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -604,11 +605,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',
Expand Down
7 changes: 6 additions & 1 deletion tools/public_api_guard/cdk/tree.md
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -76,7 +77,7 @@ export class CdkNestedTreeNode<T, K = T> extends CdkTreeNode<T, K> implements Af
}

// @public
export class CdkTree<T, K = T> implements AfterContentChecked, AfterContentInit, CollectionViewer, OnDestroy, OnInit {
export class CdkTree<T, K = T> implements AfterContentChecked, AfterContentInit, AfterViewInit, CollectionViewer, OnDestroy, OnInit {
constructor(_differs: IterableDiffers, _changeDetectorRef: ChangeDetectorRef, _dir: Directionality);
childrenAccessor?: (dataNode: T) => T[] | Observable<T[]>;
collapse(dataNode: T): void;
Expand Down Expand Up @@ -106,6 +107,8 @@ export class CdkTree<T, K = T> implements AfterContentChecked, AfterContentInit,
// (undocumented)
ngAfterContentInit(): void;
// (undocumented)
ngAfterViewInit(): void;
// (undocumented)
ngOnDestroy(): void;
// (undocumented)
ngOnInit(): void;
Expand Down Expand Up @@ -194,6 +197,8 @@ export class CdkTreeNode<T, K = T> 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<T, K>;
Expand Down
1 change: 1 addition & 0 deletions tools/public_api_guard/material/tree-testing.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export class MatTreeNodeHarness extends ContentContainerComponentHarness<string>
getText(): Promise<string>;
static hostSelector: string;
isDisabled(): Promise<boolean>;
isExpandable(): Promise<boolean>;
isExpanded(): Promise<boolean>;
toggle(): Promise<void>;
// (undocumented)
Expand Down

0 comments on commit 38cfe94

Please sign in to comment.