Skip to content

fix(cdk/tree): fix level for nested tree nodes #20226

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

Merged
merged 22 commits into from
Sep 9, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
8de8e18
fix(cdk/tree): fix level for flat and nested tree nodes
annieyw Aug 6, 2020
9948dbe
add test for material/tree
annieyw Aug 6, 2020
674ea88
cache parent level instead of traversing DOM on ever level get
annieyw Aug 7, 2020
27ecfdf
logging
annieyw Aug 7, 2020
098d008
set aria-level in ngOnInit, clean up
annieyw Aug 7, 2020
ef61e2a
public api
annieyw Aug 7, 2020
23ac01e
change host binding to @HostBinding
annieyw Aug 7, 2020
ff74132
use host:{} for mat tree and hostbinding for cdk tree
annieyw Aug 10, 2020
ee99948
fix tslint comments, fix class inheritance for mat tree
annieyw Aug 10, 2020
b14d21c
clarify adding class directly to elementRef instead of on host property
annieyw Aug 10, 2020
309a18a
minor fixes
annieyw Aug 11, 2020
a0d39dd
fix isNodeElement method name/logic, guarantee boolean value
annieyw Aug 12, 2020
b682ea7
clean up duplicate @HostBinding and host:{}
annieyw Aug 21, 2020
e1b0704
replace isDevMode()
annieyw Aug 21, 2020
15187c3
super life cycle hooks
annieyw Aug 26, 2020
0251094
public api update
annieyw Aug 26, 2020
efbb7b7
comments explaining super life hook calls, add aria-expanded with set…
annieyw Aug 27, 2020
0fe2e7d
set aria-expaded in ngDoCheck
annieyw Aug 31, 2020
8e70d18
remove duplicate input from MatTreeNode, add missing inputs:[] for c…
annieyw Aug 31, 2020
b14dec0
add super.ngDoCheck()
annieyw Aug 31, 2020
0806b92
change isExpanded setter to private method
annieyw Aug 31, 2020
3181e15
remove setting aria-expanded in constructor
annieyw Sep 2, 2020
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
27 changes: 21 additions & 6 deletions src/cdk/tree/nested-node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@ import {
AfterContentInit,
ContentChildren,
Directive,
DoCheck,
ElementRef,
IterableDiffer,
IterableDiffers,
OnDestroy,
OnInit,
QueryList,
} from '@angular/core';
import {isObservable} from 'rxjs';
Expand All @@ -31,17 +33,15 @@ import {getTreeControlFunctionsMissingError} from './tree-errors';
@Directive({
selector: 'cdk-nested-tree-node',
exportAs: 'cdkNestedTreeNode',
host: {
'[attr.aria-expanded]': 'isExpanded',
'[attr.role]': 'role',
'class': 'cdk-tree-node cdk-nested-tree-node',
},
inputs: ['role', 'disabled', 'tabIndex'],
providers: [
{provide: CdkTreeNode, useExisting: CdkNestedTreeNode},
{provide: CDK_TREE_NODE_OUTLET_NODE, useExisting: CdkNestedTreeNode}
]
})
export class CdkNestedTreeNode<T> extends CdkTreeNode<T> implements AfterContentInit, OnDestroy {
export class CdkNestedTreeNode<T> extends CdkTreeNode<T> implements AfterContentInit, DoCheck,
OnDestroy,
OnInit {
/** Differ used to find the changes in the data provided by the data source. */
private _dataDiffer: IterableDiffer<T>;

Expand All @@ -60,6 +60,11 @@ export class CdkNestedTreeNode<T> extends CdkTreeNode<T> implements AfterContent
protected _tree: CdkTree<T>,
protected _differs: IterableDiffers) {
super(_elementRef, _tree);
// The classes are directly added here instead of in the host property because classes on
// the host property are not inherited with View Engine. It is not set as a @HostBinding because
// it is not set by the time it's children nodes try to read the class from it.
// TODO: move to host after View Engine deprecation
this._elementRef.nativeElement.classList.add('cdk-nested-tree-node');
}

ngAfterContentInit() {
Expand All @@ -78,6 +83,16 @@ export class CdkNestedTreeNode<T> extends CdkTreeNode<T> implements AfterContent
.subscribe(() => this.updateChildrenNodes());
}

// This is a workaround for https://github.com/angular/angular/issues/23091
// In aot mode, the lifecycle hooks from parent class are not called.
ngOnInit() {
super.ngOnInit();
}

ngDoCheck() {
super.ngDoCheck();
}

ngOnDestroy() {
this._clear();
super.ngOnDestroy();
Expand Down
50 changes: 50 additions & 0 deletions src/cdk/tree/tree.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,23 @@ describe('CdkTree', () => {
expect(ariaLevels).toEqual(['2', '3', '2', '2']);
});

it('with the right aria-expanded attrs', () => {
// add a child to the first node
let data = dataSource.data;
dataSource.addChild(data[2]);
fixture.detectChanges();
expect(getNodes(treeElement).every(node => {
return node.getAttribute('aria-expanded') === 'false';
})).toBe(true);

component.treeControl.expandAll();
fixture.detectChanges();

expect(getNodes(treeElement).every(node => {
return node.getAttribute('aria-expanded') === 'true';
})).toBe(true);
});

it('with the right data', () => {
expect(dataSource.data.length).toBe(3);

Expand Down Expand Up @@ -595,6 +612,21 @@ describe('CdkTree', () => {
[_, _, `topping_6 - cheese_6 + base_6`],
[`topping_3 - cheese_3 + base_3`]);
});

it('with correct aria-level on nodes', () => {
expect(getNodes(treeElement).every(node => {
return node.getAttribute('aria-level') === '1';
})).toBe(true);

let data = dataSource.data;
const child = dataSource.addChild(data[1], false);
dataSource.addChild(child, false);
fixture.detectChanges();

const nodes = getNodes(treeElement);
const levels = nodes.map(n => n.getAttribute('aria-level'));
expect(levels).toEqual(['1', '1', '2', '3', '1']);
});
});

describe('with static children', () => {
Expand Down Expand Up @@ -676,6 +708,24 @@ describe('CdkTree', () => {
treeElement = fixture.nativeElement.querySelector('cdk-tree');
});

it('with the right aria-expanded attrs', () => {
expect(getNodes(treeElement).every(node => {
return node.getAttribute('aria-expanded') === 'false';
})).toBe(true);

component.toggleRecursively = false;
let data = dataSource.data;
const child = dataSource.addChild(data[1], false);
dataSource.addChild(child, false);
fixture.detectChanges();

(getNodes(treeElement)[1] as HTMLElement).click();
fixture.detectChanges();

const ariaExpanded = getNodes(treeElement).map(n => n.getAttribute('aria-expanded'));
expect(ariaExpanded).toEqual(['false', 'true', 'false', 'false']);
});

it('should expand/collapse the node multiple times', () => {
component.toggleRecursively = false;
let data = dataSource.data;
Expand Down
89 changes: 75 additions & 14 deletions src/cdk/tree/tree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
Component,
ContentChildren,
Directive,
DoCheck,
ElementRef,
Input,
IterableChangeRecord,
Expand Down Expand Up @@ -46,6 +47,7 @@ import {
getTreeMultipleDefaultNodeDefsError,
getTreeNoValidDataSourceError
} from './tree-errors';
import {coerceNumberProperty} from '@angular/cdk/coercion';

/**
* CDK tree component that connects with a data source to retrieve data of type `T` and renders
Expand Down Expand Up @@ -300,14 +302,21 @@ export class CdkTree<T> implements AfterContentChecked, CollectionViewer, OnDest
@Directive({
selector: 'cdk-tree-node',
exportAs: 'cdkTreeNode',
host: {
'[attr.aria-expanded]': 'isExpanded',
'[attr.aria-level]': 'level + 1',
'[attr.role]': 'role',
'class': 'cdk-tree-node',
},
})
export class CdkTreeNode<T> implements FocusableOption, OnDestroy {
export class CdkTreeNode<T> implements DoCheck, FocusableOption, OnDestroy, OnInit {
/**
* The role of the tree node.
* @deprecated The correct role is 'treeitem', 'group' should not be used. This input will be
* removed in a future version.
* @breaking-change 12.0.0 Remove this input
*/
@Input() get role(): 'treeitem'|'group' { return 'treeitem'; }

set role(_role: 'treeitem'|'group') {
// TODO: move to host after View Engine deprecation
this._elementRef.nativeElement.setAttribute('role', _role);
}

/**
* The most recently created `CdkTreeNode`. We save it in static variable so we can retrieve it
* in `CdkTree` and set the data to it.
Expand All @@ -320,6 +329,8 @@ export class CdkTreeNode<T> implements FocusableOption, OnDestroy {
/** Emits when the node's data has changed. */
_dataChanges = new Subject<void>();

private _parentNodeAriaLevel: number;

/** The tree node's data. */
get data(): T { return this._data; }
set data(value: T) {
Expand All @@ -335,19 +346,45 @@ export class CdkTreeNode<T> implements FocusableOption, OnDestroy {
return this._tree.treeControl.isExpanded(this._data);
}

get level(): number {
return this._tree.treeControl.getLevel ? this._tree.treeControl.getLevel(this._data) : 0;
private _setExpanded(_expanded: boolean) {
this._isAriaExpanded = _expanded;
this._elementRef.nativeElement.setAttribute('aria-expanded', `${_expanded}`);
}

/**
* The role of the node should always be 'treeitem'.
*/
// TODO: mark as deprecated
@Input() role: 'treeitem' | 'group' = 'treeitem';
protected _isAriaExpanded: boolean;

get level(): number {
// If the treeControl has a getLevel method, use it to get the level. Otherwise read the
// aria-level off the parent node and use it as the level for this node (note aria-level is
// 1-indexed, while this property is 0-indexed, so we don't need to increment).
return this._tree.treeControl.getLevel ?
this._tree.treeControl.getLevel(this._data) : this._parentNodeAriaLevel;
}

constructor(protected _elementRef: ElementRef<HTMLElement>,
protected _tree: CdkTree<T>) {
CdkTreeNode.mostRecentTreeNode = this as CdkTreeNode<T>;
// The classes are directly added here instead of in the host property because classes on
// the host property are not inherited with View Engine. It is not set as a @HostBinding because
// it is not set by the time it's children nodes try to read the class from it.
// TODO: move to host after View Engine deprecation
this._elementRef.nativeElement.classList.add('cdk-tree-node');
this.role = 'treeitem';
}

ngOnInit(): void {
this._parentNodeAriaLevel = getParentNodeAriaLevel(this._elementRef.nativeElement);
this._elementRef.nativeElement.setAttribute('aria-level', `${this.level + 1}`);
}

ngDoCheck() {
// aria-expanded is be set here because the expanded state is stored in the tree control and
// the node isn't aware when the state is changed.
// It is not set using a @HostBinding because they sometimes get lost with Mixin based classes.
// TODO: move to host after View Engine deprecation
if (this.isExpanded != this._isAriaExpanded) {
this._setExpanded(this.isExpanded);
}
}

ngOnDestroy() {
Expand Down Expand Up @@ -376,3 +413,27 @@ export class CdkTreeNode<T> implements FocusableOption, OnDestroy {
this.role = 'treeitem';
}
}

function getParentNodeAriaLevel(nodeElement: HTMLElement): number {
let parent = nodeElement.parentElement;
while (parent && !isNodeElement(parent)) {
parent = parent.parentElement;
}
if (!parent) {
if (typeof ngDevMode === 'undefined' || ngDevMode) {
throw Error('Incorrect tree structure containing detached node.');
} else {
return -1;
}
} else if (parent.classList.contains('cdk-nested-tree-node')) {
return coerceNumberProperty(parent.getAttribute('aria-level')!);
} else {
// The ancestor element is the cdk-tree itself
return 0;
}
}

function isNodeElement(element: HTMLElement) {
const classList = element.classList;
return !!(classList?.contains('cdk-nested-tree-node') || classList?.contains('cdk-tree'));
}
57 changes: 40 additions & 17 deletions src/material/tree/node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,11 @@ import {
AfterContentInit,
Attribute,
Directive,
DoCheck,
ElementRef,
Input,
IterableDiffers,
OnDestroy,
OnDestroy, OnInit,
} from '@angular/core';
import {
CanDisable,
Expand All @@ -41,25 +42,38 @@ const _MatTreeNodeMixinBase: HasTabIndexCtor & CanDisableCtor & typeof CdkTreeNo
@Directive({
selector: 'mat-tree-node',
exportAs: 'matTreeNode',
inputs: ['disabled', 'tabIndex'],
host: {
'[attr.aria-expanded]': 'isExpanded',
'[attr.aria-level]': 'level + 1',
'[attr.role]': 'role',
'class': 'mat-tree-node'
},
inputs: ['role', 'disabled', 'tabIndex'],
providers: [{provide: CdkTreeNode, useExisting: MatTreeNode}]
})
export class MatTreeNode<T> extends _MatTreeNodeMixinBase<T>
implements CanDisable, HasTabIndex {
@Input() role: 'treeitem' | 'group' = 'treeitem';
implements CanDisable, DoCheck, HasTabIndex, OnInit, OnDestroy {


constructor(protected _elementRef: ElementRef<HTMLElement>,
protected _tree: CdkTree<T>,
@Attribute('tabindex') tabIndex: string) {
super(_elementRef, _tree);

this.tabIndex = Number(tabIndex) || 0;
// The classes are directly added here instead of in the host property because classes on
// the host property are not inherited with View Engine. It is not set as a @HostBinding because
// it is not set by the time it's children nodes try to read the class from it.
// TODO: move to host after View Engine deprecation
this._elementRef.nativeElement.classList.add('mat-tree-node');
}

// This is a workaround for https://github.com/angular/angular/issues/23091
// In aot mode, the lifecycle hooks from parent class are not called.
ngOnInit() {
super.ngOnInit();
}

ngDoCheck() {
super.ngDoCheck();
}

ngOnDestroy() {
super.ngOnDestroy();
}

static ngAcceptInputType_disabled: BooleanInput;
Expand All @@ -86,19 +100,15 @@ export class MatTreeNodeDef<T> extends CdkTreeNodeDef<T> {
@Directive({
selector: 'mat-nested-tree-node',
exportAs: 'matNestedTreeNode',
host: {
'[attr.aria-expanded]': 'isExpanded',
'[attr.role]': 'role',
'class': 'mat-nested-tree-node',
},
inputs: ['role', 'disabled', 'tabIndex'],
providers: [
{provide: CdkNestedTreeNode, useExisting: MatNestedTreeNode},
{provide: CdkTreeNode, useExisting: MatNestedTreeNode},
{provide: CDK_TREE_NODE_OUTLET_NODE, useExisting: MatNestedTreeNode}
]
})
export class MatNestedTreeNode<T> extends CdkNestedTreeNode<T> implements AfterContentInit,
OnDestroy {
export class MatNestedTreeNode<T> extends CdkNestedTreeNode<T> implements AfterContentInit, DoCheck,
OnDestroy, OnInit {
@Input('matNestedTreeNode') node: T;

/** Whether the node is disabled. */
Expand All @@ -122,11 +132,24 @@ export class MatNestedTreeNode<T> extends CdkNestedTreeNode<T> implements AfterC
@Attribute('tabindex') tabIndex: string) {
super(_elementRef, _tree, _differs);
this.tabIndex = Number(tabIndex) || 0;
// The classes are directly added here instead of in the host property because classes on
// the host property are not inherited with View Engine. It is not set as a @HostBinding because
// it is not set by the time it's children nodes try to read the class from it.
// TODO: move to host after View Engine deprecation
this._elementRef.nativeElement.classList.add('mat-nested-tree-node');
}

// This is a workaround for https://github.com/angular/angular/issues/23091
// In aot mode, the lifecycle hooks from parent class are not called.
// TODO(tinayuangao): Remove when the angular issue #23091 is fixed
ngOnInit() {
super.ngOnInit();
}

ngDoCheck() {
super.ngDoCheck();
}

ngAfterContentInit() {
super.ngAfterContentInit();
}
Expand Down
Loading