From e084538e28341560695ce738585a37d7b4eb28ca Mon Sep 17 00:00:00 2001 From: Escher Wenberg Date: Tue, 26 Nov 2019 12:40:48 -0800 Subject: [PATCH 1/6] fix(material/tree): Apply aria-level to all nodes Previously, only leaf nodes had aria-level applied. This is an incremental change since this is an unfamiliar codebase for me. The main benefit it will have on its own is that it will allow anyone doing custom dom manipulation to know what level the node is on. Otherwise by itself there is no change in how NVDA reads nodes with children. (It currently reads them as literally "grouping"; no information about the contents is provided). This change will be necessary for a later change I'm planning, wherein the role of parent nodes will be changed from "group" to "treeitem", in accordance with how roles are applied in WAI-ARIA reference examples such as https://www.w3.org/TR/wai-aria-practices/examples/treeview/treeview-1/treeview-1b.html --- src/cdk/tree/tree.spec.ts | 9 +++++++++ src/cdk/tree/tree.ts | 2 +- src/material/tree/node.ts | 2 +- src/material/tree/tree.spec.ts | 11 +++++++++++ 4 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/cdk/tree/tree.spec.ts b/src/cdk/tree/tree.spec.ts index cff2df597175..fa5b134fd9e6 100644 --- a/src/cdk/tree/tree.spec.ts +++ b/src/cdk/tree/tree.spec.ts @@ -120,6 +120,15 @@ describe('CdkTree', () => { })).toBe(true); }); + it('with the right aria-levels', () => { + // add a child to the first node + let data = dataSource.data; + dataSource.addChild(data[0], true); + + const ariaLevels = getNodes(treeElement).map(n => n.getAttribute('aria-level')); + expect(ariaLevels).toEqual(['1', '2', '1', '1']); + }); + it('with the right data', () => { expect(dataSource.data.length).toBe(3); diff --git a/src/cdk/tree/tree.ts b/src/cdk/tree/tree.ts index 585e3734cf5e..f327347d85b4 100644 --- a/src/cdk/tree/tree.ts +++ b/src/cdk/tree/tree.ts @@ -299,7 +299,7 @@ export class CdkTree implements AfterContentChecked, CollectionViewer, OnDest exportAs: 'cdkTreeNode', host: { '[attr.aria-expanded]': 'isExpanded', - '[attr.aria-level]': 'role === "treeitem" ? level : null', + '[attr.aria-level]': 'level', '[attr.role]': 'role', 'class': 'cdk-tree-node', }, diff --git a/src/material/tree/node.ts b/src/material/tree/node.ts index 06e689dff7d7..c9a1c4a6117b 100644 --- a/src/material/tree/node.ts +++ b/src/material/tree/node.ts @@ -44,7 +44,7 @@ const _MatTreeNodeMixinBase: HasTabIndexCtor & CanDisableCtor & typeof CdkTreeNo inputs: ['disabled', 'tabIndex'], host: { '[attr.aria-expanded]': 'isExpanded', - '[attr.aria-level]': 'role === "treeitem" ? level : null', + '[attr.aria-level]': 'level', '[attr.role]': 'role', 'class': 'mat-tree-node' }, diff --git a/src/material/tree/tree.spec.ts b/src/material/tree/tree.spec.ts index c9b3ad2d9e36..d70e88fcc9e9 100644 --- a/src/material/tree/tree.spec.ts +++ b/src/material/tree/tree.spec.ts @@ -64,6 +64,17 @@ describe('MatTree', () => { }); }); + it('with the right aria-level attrs', () => { + // add a child to the first node + let data = underlyingDataSource.data; + underlyingDataSource.addChild(data[2]); + component.treeControl.expandAll(); + fixture.detectChanges(); + + const ariaLevels = getNodes(treeElement).map(n => n.getAttribute('aria-level')); + expect(ariaLevels).toEqual(['0', '0', '0', '1']); + }); + it('with the right data', () => { expect(underlyingDataSource.data.length).toBe(3); From 034c2cbe086c4f44b940a4697253d2af82a97151 Mon Sep 17 00:00:00 2001 From: Annie Wang Date: Tue, 21 Jul 2020 12:59:18 -0700 Subject: [PATCH 2/6] change aria-level binding to one-based --- src/cdk/tree/tree.spec.ts | 2 +- src/cdk/tree/tree.ts | 2 +- src/material/tree/node.ts | 2 +- src/material/tree/tree.spec.ts | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cdk/tree/tree.spec.ts b/src/cdk/tree/tree.spec.ts index fa5b134fd9e6..784e64775394 100644 --- a/src/cdk/tree/tree.spec.ts +++ b/src/cdk/tree/tree.spec.ts @@ -126,7 +126,7 @@ describe('CdkTree', () => { dataSource.addChild(data[0], true); const ariaLevels = getNodes(treeElement).map(n => n.getAttribute('aria-level')); - expect(ariaLevels).toEqual(['1', '2', '1', '1']); + expect(ariaLevels).toEqual(['2', '3', '2', '2']); }); it('with the right data', () => { diff --git a/src/cdk/tree/tree.ts b/src/cdk/tree/tree.ts index f327347d85b4..4091af8c5bab 100644 --- a/src/cdk/tree/tree.ts +++ b/src/cdk/tree/tree.ts @@ -299,7 +299,7 @@ export class CdkTree implements AfterContentChecked, CollectionViewer, OnDest exportAs: 'cdkTreeNode', host: { '[attr.aria-expanded]': 'isExpanded', - '[attr.aria-level]': 'level', + '[attr.aria-level]': 'level + 1', '[attr.role]': 'role', 'class': 'cdk-tree-node', }, diff --git a/src/material/tree/node.ts b/src/material/tree/node.ts index c9a1c4a6117b..c7fd9b128477 100644 --- a/src/material/tree/node.ts +++ b/src/material/tree/node.ts @@ -44,7 +44,7 @@ const _MatTreeNodeMixinBase: HasTabIndexCtor & CanDisableCtor & typeof CdkTreeNo inputs: ['disabled', 'tabIndex'], host: { '[attr.aria-expanded]': 'isExpanded', - '[attr.aria-level]': 'level', + '[attr.aria-level]': 'level + 1', '[attr.role]': 'role', 'class': 'mat-tree-node' }, diff --git a/src/material/tree/tree.spec.ts b/src/material/tree/tree.spec.ts index d70e88fcc9e9..9934b345e8d9 100644 --- a/src/material/tree/tree.spec.ts +++ b/src/material/tree/tree.spec.ts @@ -72,7 +72,7 @@ describe('MatTree', () => { fixture.detectChanges(); const ariaLevels = getNodes(treeElement).map(n => n.getAttribute('aria-level')); - expect(ariaLevels).toEqual(['0', '0', '0', '1']); + expect(ariaLevels).toEqual(['1', '1', '1', '2']); }); it('with the right data', () => { From de439a51f953294ceb447d979f4468e77da70349 Mon Sep 17 00:00:00 2001 From: Annie Wang Date: Mon, 27 Jul 2020 16:44:33 -0700 Subject: [PATCH 3/6] change role to treeitem --- src/material/tree/node.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/material/tree/node.ts b/src/material/tree/node.ts index c7fd9b128477..045c1751d177 100644 --- a/src/material/tree/node.ts +++ b/src/material/tree/node.ts @@ -45,7 +45,7 @@ const _MatTreeNodeMixinBase: HasTabIndexCtor & CanDisableCtor & typeof CdkTreeNo host: { '[attr.aria-expanded]': 'isExpanded', '[attr.aria-level]': 'level + 1', - '[attr.role]': 'role', + '[attr.role]': `'treeitem'`, 'class': 'mat-tree-node' }, providers: [{provide: CdkTreeNode, useExisting: MatTreeNode}] @@ -88,7 +88,7 @@ export class MatTreeNodeDef extends CdkTreeNodeDef { exportAs: 'matNestedTreeNode', host: { '[attr.aria-expanded]': 'isExpanded', - '[attr.role]': 'role', + '[attr.role]': `'treeitem'`, 'class': 'mat-nested-tree-node', }, providers: [ From ab3476d37d97eff0955e443ace04fde3dfd77732 Mon Sep 17 00:00:00 2001 From: Annie Wang Date: Thu, 30 Jul 2020 14:45:09 -0700 Subject: [PATCH 4/6] always set role to treeitem --- src/cdk/tree/tree.ts | 11 +++++------ src/material/tree/node.ts | 4 ++-- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/cdk/tree/tree.ts b/src/cdk/tree/tree.ts index 4091af8c5bab..1d831b156a79 100644 --- a/src/cdk/tree/tree.ts +++ b/src/cdk/tree/tree.ts @@ -337,8 +337,7 @@ export class CdkTreeNode implements FocusableOption, OnDestroy { } /** - * The role of the node should be 'group' if it's an internal node, - * and 'treeitem' if it's a leaf node. + * The role of the node should always be 'treeitem'. */ @Input() role: 'treeitem' | 'group' = 'treeitem'; @@ -363,10 +362,10 @@ export class CdkTreeNode implements FocusableOption, OnDestroy { focus(): void { this._elementRef.nativeElement.focus(); } - + protected _setRoleFromData(): void { if (this._tree.treeControl.isExpandable) { - this.role = this._tree.treeControl.isExpandable(this._data) ? 'group' : 'treeitem'; + this.role = this._tree.treeControl.isExpandable(this._data) ? 'treeitem' : 'treeitem'; } else { if (!this._tree.treeControl.getChildren) { throw getTreeControlFunctionsMissingError(); @@ -376,12 +375,12 @@ export class CdkTreeNode implements FocusableOption, OnDestroy { this._setRoleFromChildren(childrenNodes as T[]); } else if (isObservable(childrenNodes)) { childrenNodes.pipe(takeUntil(this._destroyed)) - .subscribe(children => this._setRoleFromChildren(children)); + .subscribe(children => this._setRoleFromChildren(children)); } } } protected _setRoleFromChildren(children: T[]) { - this.role = children && children.length ? 'group' : 'treeitem'; + this.role = 'treeitem'; } } diff --git a/src/material/tree/node.ts b/src/material/tree/node.ts index 045c1751d177..c7fd9b128477 100644 --- a/src/material/tree/node.ts +++ b/src/material/tree/node.ts @@ -45,7 +45,7 @@ const _MatTreeNodeMixinBase: HasTabIndexCtor & CanDisableCtor & typeof CdkTreeNo host: { '[attr.aria-expanded]': 'isExpanded', '[attr.aria-level]': 'level + 1', - '[attr.role]': `'treeitem'`, + '[attr.role]': 'role', 'class': 'mat-tree-node' }, providers: [{provide: CdkTreeNode, useExisting: MatTreeNode}] @@ -88,7 +88,7 @@ export class MatTreeNodeDef extends CdkTreeNodeDef { exportAs: 'matNestedTreeNode', host: { '[attr.aria-expanded]': 'isExpanded', - '[attr.role]': `'treeitem'`, + '[attr.role]': 'role', 'class': 'mat-nested-tree-node', }, providers: [ From 639dbd69a851aeecb9c2457efc77a8cd6717b745 Mon Sep 17 00:00:00 2001 From: Annie Wang Date: Fri, 31 Jul 2020 11:41:34 -0700 Subject: [PATCH 5/6] simplify logic for setting role --- src/cdk/tree/tree.ts | 18 ++---------------- tools/public_api_guard/cdk/tree.d.ts | 1 - 2 files changed, 2 insertions(+), 17 deletions(-) diff --git a/src/cdk/tree/tree.ts b/src/cdk/tree/tree.ts index 1d831b156a79..a080ba9a9bf3 100644 --- a/src/cdk/tree/tree.ts +++ b/src/cdk/tree/tree.ts @@ -364,23 +364,9 @@ export class CdkTreeNode implements FocusableOption, OnDestroy { } protected _setRoleFromData(): void { - if (this._tree.treeControl.isExpandable) { - this.role = this._tree.treeControl.isExpandable(this._data) ? 'treeitem' : 'treeitem'; - } else { - if (!this._tree.treeControl.getChildren) { - throw getTreeControlFunctionsMissingError(); - } - const childrenNodes = this._tree.treeControl.getChildren(this._data); - if (Array.isArray(childrenNodes)) { - this._setRoleFromChildren(childrenNodes as T[]); - } else if (isObservable(childrenNodes)) { - childrenNodes.pipe(takeUntil(this._destroyed)) - .subscribe(children => this._setRoleFromChildren(children)); - } + if (!this._tree.treeControl.isExpandable && !this._tree.treeControl.getChildren) { + throw getTreeControlFunctionsMissingError(); } - } - - protected _setRoleFromChildren(children: T[]) { this.role = 'treeitem'; } } diff --git a/tools/public_api_guard/cdk/tree.d.ts b/tools/public_api_guard/cdk/tree.d.ts index 108112677ac3..6c2cfc6082d9 100644 --- a/tools/public_api_guard/cdk/tree.d.ts +++ b/tools/public_api_guard/cdk/tree.d.ts @@ -74,7 +74,6 @@ export declare class CdkTreeNode implements FocusableOption, OnDestroy { get level(): number; role: 'treeitem' | 'group'; constructor(_elementRef: ElementRef, _tree: CdkTree); - protected _setRoleFromChildren(children: T[]): void; protected _setRoleFromData(): void; focus(): void; ngOnDestroy(): void; From 17c07bc294c2dd0644df742c08ac7c925caf2e19 Mon Sep 17 00:00:00 2001 From: Annie Wang Date: Fri, 31 Jul 2020 12:36:53 -0700 Subject: [PATCH 6/6] add follow up TODO to makr role as deprecated --- src/cdk/tree/tree.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/cdk/tree/tree.ts b/src/cdk/tree/tree.ts index a080ba9a9bf3..9f6778c1c103 100644 --- a/src/cdk/tree/tree.ts +++ b/src/cdk/tree/tree.ts @@ -22,18 +22,18 @@ import { OnDestroy, OnInit, QueryList, + TrackByFunction, ViewChild, ViewContainerRef, - ViewEncapsulation, - TrackByFunction + ViewEncapsulation } from '@angular/core'; import { BehaviorSubject, + isObservable, Observable, of as observableOf, Subject, Subscription, - isObservable, } from 'rxjs'; import {takeUntil} from 'rxjs/operators'; import {TreeControl} from './control/tree-control'; @@ -339,6 +339,7 @@ export class CdkTreeNode implements FocusableOption, OnDestroy { /** * The role of the node should always be 'treeitem'. */ + // TODO: mark as deprecated @Input() role: 'treeitem' | 'group' = 'treeitem'; constructor(protected _elementRef: ElementRef, @@ -362,7 +363,8 @@ export class CdkTreeNode implements FocusableOption, OnDestroy { focus(): void { this._elementRef.nativeElement.focus(); } - + + // TODO: role should eventually just be set in the component host protected _setRoleFromData(): void { if (!this._tree.treeControl.isExpandable && !this._tree.treeControl.getChildren) { throw getTreeControlFunctionsMissingError();