Skip to content

Commit

Permalink
fix(cdk/tree): fix level for nested tree nodes (#20226)
Browse files Browse the repository at this point in the history
* fix(cdk/tree): fix level for flat and nested tree nodes

* add test for material/tree

* cache parent level instead of traversing DOM on ever level get

* logging

* set aria-level in ngOnInit, clean up

* public api

* change host binding to @HostBinding

* use host:{} for mat tree and hostbinding for cdk tree

* fix tslint comments, fix class inheritance for mat tree

* clarify adding class directly to elementRef instead of on host property

* minor fixes

* fix isNodeElement method name/logic, guarantee boolean value

* clean up duplicate @HostBinding and host:{}

* replace isDevMode()

* super life cycle hooks

* public api update

* comments explaining super life hook calls, add aria-expanded with setAttribute

* set aria-expaded in ngDoCheck

* remove duplicate input from MatTreeNode, add missing inputs:[]  for classes inheriting CdkTreeNode

* add super.ngDoCheck()

* change isExpanded setter to private method

* remove setting aria-expanded in constructor
  • Loading branch information
annieyw committed Sep 9, 2020
1 parent ea628c9 commit f951bf4
Show file tree
Hide file tree
Showing 8 changed files with 268 additions and 48 deletions.
27 changes: 21 additions & 6 deletions src/cdk/tree/nested-node.ts
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
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
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
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

0 comments on commit f951bf4

Please sign in to comment.