Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cdk/tree): react properly to expansion changes #29751

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/cdk/tree/tree-using-legacy-key-manager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ describe('CdkTree when provided LegacyTreeKeyManager', () => {

fixture = TestBed.createComponent(SimpleCdkTreeApp);
fixture.detectChanges();
fixture.detectChanges();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These consecutive detectChanges calls seem a bit suspicious. Do we know why they're necessary?

});

describe('with default node options', () => {
Expand Down
5 changes: 3 additions & 2 deletions src/cdk/tree/tree-with-tree-control.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ describe('CdkTree with TreeControl', () => {
configureCdkTreeTestingModule([SimpleCdkTreeApp]);
fixture = TestBed.createComponent(SimpleCdkTreeApp);

fixture.detectChanges();
fixture.detectChanges();

component = fixture.componentInstance;
Expand Down Expand Up @@ -217,8 +218,6 @@ describe('CdkTree with TreeControl', () => {

it('should be able to set zero as the indent level', () => {
component.paddingNodes.forEach(node => (node.level = 0));
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();

const data = dataSource.data;

Expand Down Expand Up @@ -532,7 +531,9 @@ describe('CdkTree with TreeControl', () => {

// Add new data
component.dataSource.data = copiedData;
fixture.detectChanges();
component.dataSource.addData();
fixture.detectChanges();
}

it('should add/remove/move nodes with reference-based trackBy', () => {
Expand Down
49 changes: 48 additions & 1 deletion src/cdk/tree/tree.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ describe('CdkTree', () => {
configureCdkTreeTestingModule([SimpleCdkTreeApp]);
fixture = TestBed.createComponent(SimpleCdkTreeApp);

fixture.detectChanges();
fixture.detectChanges();

component = fixture.componentInstance;
Expand Down Expand Up @@ -223,7 +224,6 @@ describe('CdkTree', () => {

it('should be able to set zero as the indent level', () => {
component.paddingNodes.forEach(node => (node.level = 0));
fixture.detectChanges();

const data = dataSource.data;

Expand Down Expand Up @@ -561,7 +561,9 @@ describe('CdkTree', () => {

// Add new data
component.dataSource.data = copiedData;
fixture.detectChanges();
component.dataSource.addData();
fixture.detectChanges();
}

function mutateProperties() {
Expand Down Expand Up @@ -612,6 +614,7 @@ describe('CdkTree', () => {
component.dataSource.data = component.dataSource.data.map(
item => new TestData(item.pizzaTopping, item.pizzaCheese, item.pizzaBase),
);
fixture.detectChanges();

// Expect first two to be the same since they were swapped but indicies are consistent.
// The third element was removed and caught by the tree so it was removed before another
Expand Down Expand Up @@ -1459,6 +1462,17 @@ describe('CdkTree', () => {
.withContext(`expect an expanded node`)
.toBe(1);
});

it('statically renders nested children', () => {
configureCdkTreeTestingModule([NestedChildrenExpansionTest]);
const fixture = TestBed.createComponent(NestedChildrenExpansionTest);
fixture.detectChanges();

const component = fixture.componentInstance;
expect(getExpandedNodes(component.allNodes, component.tree).length)
.withContext(`expect all expanded nodes`)
.toBe(3);
});
});

export class TestData {
Expand Down Expand Up @@ -2139,3 +2153,36 @@ class IsExpandableOrderingTest {
this.dataSource = data;
}
}

@Component({
template: `
<cdk-tree [dataSource]="dataSource" [childrenAccessor]="getChildren">
<cdk-tree-node
*cdkTreeNodeDef="let node"
[isExpandable]="true"
[isExpanded]="true">
{{node.name}}
</cdk-tree-node>
</cdk-tree>
`,
})
class NestedChildrenExpansionTest {
getChildren = (node: MinimalTestData) => node.children;

@ViewChild(CdkTree) tree: CdkTree<MinimalTestData>;

dataSource: MinimalTestData[];

allNodes: MinimalTestData[];

constructor() {
const nestedChildren = [new MinimalTestData('subchild')];
const children = [new MinimalTestData('child')];
children[0].children = nestedChildren;
const data = [new MinimalTestData('parent')];
data[0].children = children;

this.dataSource = data;
this.allNodes = [...data, ...children, ...nestedChildren];
}
}
120 changes: 69 additions & 51 deletions src/cdk/tree/tree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ import {
Output,
QueryList,
TrackByFunction,
signal,
effect,
ViewChild,
ViewContainerRef,
ViewEncapsulation,
Expand Down Expand Up @@ -223,6 +225,10 @@ export class CdkTree<T, K = T>
})
_nodeDefs: QueryList<CdkTreeNodeDef<T>>;

// signals?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover?

private readonly _data = signal<readonly T[]>([]);
private readonly _selection = signal<readonly K[]>([]);

// TODO(tinayuangao): Setup a listener for scrolling, emit the calculated view to viewChange.
// Remove the MAX_VALUE in viewChange
/**
Expand All @@ -245,9 +251,7 @@ export class CdkTree<T, K = T>
private _flattenedNodes: BehaviorSubject<readonly T[]> = new BehaviorSubject<readonly T[]>([]);

/** The automatically determined node type for the tree. */
private _nodeType: BehaviorSubject<'flat' | 'nested' | null> = new BehaviorSubject<
'flat' | 'nested' | null
>(null);
private _nodeType = signal<'flat' | 'nested' | null>(null);

/** The mapping between data and the node that is rendered. */
private _nodes: BehaviorSubject<Map<K, CdkTreeNode<T, K>>> = new BehaviorSubject(
Expand All @@ -268,7 +272,23 @@ export class CdkTree<T, K = T>
private _viewInit = false;

constructor(...args: unknown[]);
constructor() {}
constructor() {
effect(
onCleanup => {
const data = this._data();
const nodeType = this._nodeType();
const expandedKeys = this._selection();

const sub = this._getRenderData(data, nodeType, expandedKeys).subscribe(renderData => {
this._renderDataChanges(renderData);
});
onCleanup(() => {
sub.unsubscribe();
});
},
{allowSignalWrites: true},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use computed instead of writing inside of an effect?

);
}

ngAfterContentInit() {
this._initializeKeyManager();
Expand Down Expand Up @@ -303,6 +323,7 @@ export class CdkTree<T, K = T>
ngOnInit() {
this._checkTreeControlUsage();
this._initializeDataDiffer();
this._subscribeToExpansionChanges();
}

ngAfterViewInit() {
Expand All @@ -324,8 +345,8 @@ export class CdkTree<T, K = T>
* to determine what data transformations are required.
*/
_setNodeTypeIfUnset(nodeType: 'flat' | 'nested') {
if (this._nodeType.value === null) {
this._nodeType.next(nodeType);
if (this._nodeType() === null) {
this._nodeType.set(nodeType);
}
}

Expand Down Expand Up @@ -355,6 +376,15 @@ export class CdkTree<T, K = T>
}
}

private _subscribeToExpansionChanges() {
const model = this._getExpansionModel();
this._selection.set([...model.selected]);
model.changed.pipe(takeUntil(this._onDestroy)).subscribe(changes => {
this._emitExpansionChanges(changes);
this._selection.set([...model.selected]);
});
}

_getExpansionModel() {
if (!this.treeControl) {
this._expansionModel ??= new SelectionModel<K>(true);
Expand Down Expand Up @@ -386,39 +416,25 @@ export class CdkTree<T, K = T>
return;
}

this._dataSubscription = this._getRenderData(dataStream)
.pipe(takeUntil(this._onDestroy))
.subscribe(renderingData => {
this._renderDataChanges(renderingData);
});
this._dataSubscription = dataStream.pipe(takeUntil(this._onDestroy)).subscribe(data => {
this._data.set(data);
});
}

/** Given an Observable containing a stream of the raw data, returns an Observable containing the RenderingData */
private _getRenderData(dataStream: Observable<readonly T[]>): Observable<RenderingData<T>> {
const expansionModel = this._getExpansionModel();
return combineLatest([
dataStream,
this._nodeType,
// We don't use the expansion data directly, however we add it here to essentially
// trigger data rendering when expansion changes occur.
expansionModel.changed.pipe(
startWith(null),
tap(expansionChanges => {
this._emitExpansionChanges(expansionChanges);
}),
),
]).pipe(
switchMap(([data, nodeType]) => {
if (nodeType === null) {
return observableOf({renderNodes: data, flattenedNodes: null, nodeType} as const);
}
/** Given the raw data, returns an Observable containing the RenderingData */
private _getRenderData(
data: readonly T[],
nodeType: 'flat' | 'nested' | null,
selection: readonly K[],
): Observable<RenderingData<T>> {
if (nodeType === null) {
return observableOf({renderNodes: data, flattenedNodes: null, nodeType} as const);
}

// If we're here, then we know what our node type is, and therefore can
// perform our usual rendering pipeline, which necessitates converting the data
return this._computeRenderingData(data, nodeType).pipe(
map(convertedData => ({...convertedData, nodeType}) as const),
);
}),
// If we're here, then we know what our node type is, and therefore can
// perform our usual rendering pipeline, which necessitates converting the data
return this._computeRenderingData(data, nodeType, selection).pipe(
map(convertedData => ({...convertedData, nodeType}) as const),
);
}

Expand All @@ -433,6 +449,8 @@ export class CdkTree<T, K = T>
this._updateCachedData(data.flattenedNodes);
this.renderNodeChanges(data.renderNodes);
this._updateKeyManagerItems(data.flattenedNodes);
// Explicitly detect the initial set of changes to this component subtree
this._changeDetectorRef.detectChanges();
Copy link
Member

@crisbeto crisbeto Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a heads-up that the detectChanges call was causing a "Maximum call stack exceeded" in #29733 which I've fixed with #29754. I think that this specific call is fine since it's outside of renderNodeChanges which was the recursive one.

}

private _emitExpansionChanges(expansionChanges: SelectionChange<K> | null) {
Expand Down Expand Up @@ -989,7 +1007,11 @@ export class CdkTree<T, K = T>
* This will still traverse all nested children in order to build up our internal data
* models, but will not include them in the returned array.
*/
private _flattenNestedNodesWithExpansion(nodes: readonly T[], level = 0): Observable<T[]> {
private _flattenNestedNodesWithExpansion(
nodes: readonly T[],
selection: readonly K[],
level = 0,
): Observable<T[]> {
const childrenAccessor = this._getChildrenAccessor();
// If we're using a level accessor, we don't need to flatten anything.
if (!childrenAccessor) {
Expand Down Expand Up @@ -1021,8 +1043,8 @@ export class CdkTree<T, K = T>
if (!childNodes) {
return observableOf([]);
}
return this._flattenNestedNodesWithExpansion(childNodes, level + 1).pipe(
map(nestedNodes => (this.isExpanded(node) ? nestedNodes : [])),
return this._flattenNestedNodesWithExpansion(childNodes, selection, level + 1).pipe(
map(nestedNodes => (selection.includes(parentKey) ? nestedNodes : [])),
);
}),
),
Expand All @@ -1043,6 +1065,7 @@ export class CdkTree<T, K = T>
private _computeRenderingData(
nodes: readonly T[],
nodeType: 'flat' | 'nested',
selection: readonly K[],
): Observable<{
renderNodes: readonly T[];
flattenedNodes: readonly T[];
Expand All @@ -1054,7 +1077,7 @@ export class CdkTree<T, K = T>
if (this.childrenAccessor && nodeType === 'flat') {
// This flattens children into a single array.
this._ariaSets.set(null, [...nodes]);
return this._flattenNestedNodesWithExpansion(nodes).pipe(
return this._flattenNestedNodesWithExpansion(nodes, selection).pipe(
map(flattenedNodes => ({
renderNodes: flattenedNodes,
flattenedNodes,
Expand Down Expand Up @@ -1087,7 +1110,7 @@ export class CdkTree<T, K = T>
// For nested nodes, we still need to perform the node flattening in order
// to maintain our caches for various tree operations.
this._ariaSets.set(null, [...nodes]);
return this._flattenNestedNodesWithExpansion(nodes).pipe(
return this._flattenNestedNodesWithExpansion(nodes, selection).pipe(
map(flattenedNodes => ({
renderNodes: nodes,
flattenedNodes,
Expand Down Expand Up @@ -1141,7 +1164,7 @@ export class CdkTree<T, K = T>
'[attr.aria-level]': 'level + 1',
'[attr.aria-posinset]': '_getPositionInSet()',
'[attr.aria-setsize]': '_getSetSize()',
'[tabindex]': '_tabindex',
'[tabindex]': '_tabindex()',
'role': 'treeitem',
'(click)': '_setActiveItem()',
'(focus)': '_focusItem()',
Expand All @@ -1151,7 +1174,7 @@ export class CdkTree<T, K = T>
export class CdkTreeNode<T, K = T> implements OnDestroy, OnInit, TreeKeyManagerItem {
protected _elementRef = inject<ElementRef<HTMLElement>>(ElementRef);
protected _tree = inject<CdkTree<T, K>>(CdkTree);
protected _tabindex: number | null = -1;
protected readonly _tabindex = signal<number | null>(-1);

/**
* The role of the tree node.
Expand Down Expand Up @@ -1380,19 +1403,15 @@ export class CdkTreeNode<T, K = T> implements OnDestroy, OnInit, TreeKeyManagerI

/** Focuses this data node. Implemented for TreeKeyManagerItem. */
focus(): void {
this._tabindex = 0;
this._tabindex.set(0);
if (this._shouldFocus) {
this._elementRef.nativeElement.focus();
}

this._changeDetectorRef.markForCheck();
}

/** Defocus this data node. */
unfocus(): void {
this._tabindex = -1;

this._changeDetectorRef.markForCheck();
this._tabindex.set(-1);
}

/** Emits an activation event. Implemented for TreeKeyManagerItem. */
Expand All @@ -1419,8 +1438,7 @@ export class CdkTreeNode<T, K = T> implements OnDestroy, OnInit, TreeKeyManagerI

/** Makes the node focusable. Implemented for TreeKeyManagerItem. */
makeFocusable(): void {
this._tabindex = 0;
this._changeDetectorRef.markForCheck();
this._tabindex.set(0);
}

_focusItem() {
Expand Down
1 change: 1 addition & 0 deletions src/material/tree/tree-using-legacy-key-manager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ describe('MatTree when provided LegacyTreeKeyManager', () => {

fixture = TestBed.createComponent(SimpleMatTreeApp);
fixture.detectChanges();
fixture.detectChanges();
});

describe('when nodes have default options', () => {
Expand Down
1 change: 1 addition & 0 deletions src/material/tree/tree.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ describe('MatTree', () => {
// add a child to the first node
const data = underlyingDataSource.data;
underlyingDataSource.addChild(data[2]);
fixture.detectChanges();
component.tree.expandAll();
fixture.detectChanges();

Expand Down
Loading
Loading