Skip to content

fix(material/tree): Do not add aria-expanded to leaf nodes #18032

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

fix(material/tree): Do not add aria-expanded to leaf nodes #18032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
32808a6
fix(material/tree): Apply aria-level to all nodes
martiansnoop Nov 26, 2019
11ae3ce
Illustrate bug in tree test
martiansnoop Dec 5, 2019
b58d9bb
fix(material/tree): Apply aria-level to all nodes
martiansnoop Nov 26, 2019
de2a626
fix(material/tree): Don't add aria-expanded to leaf nodes
martiansnoop Nov 27, 2019
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
80 changes: 76 additions & 4 deletions src/cdk/tree/tree.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,43 @@ describe('CdkTree', () => {
});

describe('flat tree', () => {
describe('should be accessible', () => {
let fixture: ComponentFixture<SimpleCdkTreeApp>;
let component: SimpleCdkTreeApp;

beforeEach(() => {
configureCdkTreeTestingModule([SimpleCdkTreeAppWithChildren]);
fixture = TestBed.createComponent(SimpleCdkTreeAppWithChildren);

fixture.detectChanges();

component = fixture.componentInstance;
dataSource = component.dataSource as FakeDataSource;
tree = component.tree;
treeElement = fixture.nativeElement.querySelector('cdk-tree');
});

it('with aria-expanded attribute on parent nodes only', () => {
const ariaExpandedVals = getNodes(treeElement)
.map(node => node.getAttribute('aria-expanded'));
expect(ariaExpandedVals).toEqual(['false', null, null, null]);
});

// Failing because CdkTreeNode.data is not set on the parent, so the
// role-setting logic does not run.
xit('updates aria-expanded when children are added', () => {
const data = dataSource.data;
dataSource.addChild(data[3], true);
fixture.detectChanges();

const ariaExpandedVals = getNodes(treeElement)
.map(node => node.getAttribute('aria-expanded'));
expect(ariaExpandedVals).toEqual(['false', null, null, 'false', null]);
});
});

describe('should initialize', () => {

let fixture: ComponentFixture<SimpleCdkTreeApp>;
let component: SimpleCdkTreeApp;

Expand Down Expand Up @@ -103,9 +139,18 @@ describe('CdkTree', () => {
it('with the right accessibility roles', () => {
expect(treeElement.getAttribute('role')).toBe('tree');

expect(getNodes(treeElement).every(node => {
return node.getAttribute('role') === 'treeitem';
})).toBe(true);
expect(getNodes(treeElement).every(node => {
return node.getAttribute('role') === 'treeitem';
})).toBe(true);
});

it('with the right aria-levels', () => {
// add a child to the first node
let data = dataSource.data;
const child = 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', () => {
Expand Down Expand Up @@ -1015,11 +1060,15 @@ class FakeDataSource extends DataSource<TestData> {
get data() { return this._dataChange.getValue(); }
set data(data: TestData[]) { this._dataChange.next(data); }

constructor(public treeControl: TreeControl<TestData>) {
constructor(public treeControl: TreeControl<TestData>, addChildren = false) {
super();
for (let i = 0; i < 3; i++) {
this.addData();
}
if (addChildren) {
const parent = this.data[0];
this.addChild(parent, true);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this addChildren flag as a way to optionally add children to the data source before the tree is rendered, because aria attribute changes were not reflected in the DOM otherwise. Ideally I would remove this. See other comment -- I need to find a way to make the parent node update its data when a child is added to it. I think this might just be a test change, but I could use another pair of eyes on this.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is probably the same issue as I commented on in #17818

}

connect(collectionViewer: CollectionViewer): Observable<TestData[]> {
Expand Down Expand Up @@ -1173,6 +1222,29 @@ class SimpleCdkTreeApp {
@ViewChildren(CdkTreeNodePadding) paddingNodes: QueryList<CdkTreeNodePadding<TestData>>;
}

@Component({
template: `
<cdk-tree [dataSource]="dataSource" [treeControl]="treeControl">
<cdk-tree-node *cdkTreeNodeDef="let node" class="customNodeClass"
cdkTreeNodePadding [cdkTreeNodePaddingIndent]="indent"
cdkTreeNodeToggle>
{{node.pizzaTopping}} - {{node.pizzaCheese}} + {{node.pizzaBase}}
</cdk-tree-node>
</cdk-tree>
`
})
class SimpleCdkTreeAppWithChildren {
getLevel = (node: TestData) => node.level;
isExpandable = (node: TestData) => node.children.length > 0;

treeControl: TreeControl<TestData> = new FlatTreeControl(this.getLevel, this.isExpandable);
dataSource: FakeDataSource | null = new FakeDataSource(this.treeControl, /*addChildren*/ true);
indent: number | string = 28;

@ViewChild(CdkTree) tree: CdkTree<TestData>;
@ViewChildren(CdkTreeNodePadding) paddingNodes: QueryList<CdkTreeNodePadding<TestData>>;
}

@Component({
template: `
<cdk-tree [dataSource]="dataSource" [treeControl]="treeControl">
Expand Down
8 changes: 6 additions & 2 deletions src/cdk/tree/tree.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,8 +290,8 @@ export class CdkTree<T> implements AfterContentChecked, CollectionViewer, OnDest
selector: 'cdk-tree-node',
exportAs: 'cdkTreeNode',
host: {
'[attr.aria-expanded]': 'isExpanded',
'[attr.aria-level]': 'role === "treeitem" ? level : null',
'[attr.aria-expanded]': 'expandable ? isExpanded : null',
'[attr.aria-level]': 'level',
'[attr.role]': 'role',
'class': 'cdk-tree-node',
},
Expand Down Expand Up @@ -334,6 +334,8 @@ export class CdkTreeNode<T> implements FocusableOption, OnDestroy {
*/
@Input() role: 'treeitem' | 'group' = 'treeitem';

expandable = false;
Copy link
Member

Choose a reason for hiding this comment

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

Can you prefix this member with an underscore to signal that it's not part of the public API?


constructor(protected _elementRef: ElementRef<HTMLElement>,
protected _tree: CdkTree<T>) {
CdkTreeNode.mostRecentTreeNode = this as CdkTreeNode<T>;
Expand All @@ -358,6 +360,7 @@ export class CdkTreeNode<T> implements FocusableOption, OnDestroy {

protected _setRoleFromData(): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I added the isExpandable check to this function because it was an expedient way to avoid duplicating the logic. Do you recommend a rename to setRoleAndExpandedFromData, or another solution? Same goes for setRoleFromChildren.

  2. This function is only called when the data setter in this class is invoked. In the CDK tree tests, this function is NOT invoked when you add a new node to the tree, so aria-expanded will not be updated. In the demo page (yarn dev-app) it does appear to be being called, so I think it might just be a bug in the tests. But I'm not sure, and could use some additional advice.

Copy link
Member

Choose a reason for hiding this comment

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

For 1, I would rename this method to _updateExpandedState since, IIRC, you mentioned that the role should always be treeitem anyway, right?

For 2, I think it's the same issue as #17818

if (this._tree.treeControl.isExpandable) {
this.expandable = this._tree.treeControl.isExpandable(this._data);
this.role = this._tree.treeControl.isExpandable(this._data) ? 'group' : 'treeitem';
} else {
if (!this._tree.treeControl.getChildren) {
Expand All @@ -374,6 +377,7 @@ export class CdkTreeNode<T> implements FocusableOption, OnDestroy {
}

protected _setRoleFromChildren(children: T[]) {
this.expandable = children && children.length > 0;
this.role = children && children.length ? 'group' : 'treeitem';
}
}
6 changes: 3 additions & 3 deletions src/material/tree/node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ const _MatTreeNodeMixinBase: HasTabIndexCtor & CanDisableCtor & typeof CdkTreeNo
exportAs: 'matTreeNode',
inputs: ['disabled', 'tabIndex'],
host: {
'[attr.aria-expanded]': 'isExpanded',
'[attr.aria-level]': 'role === "treeitem" ? level : null',
'[attr.aria-expanded]': 'expandable ? isExpanded : null',
'[attr.aria-level]': 'level',
'[attr.role]': 'role',
'class': 'mat-tree-node'
},
Expand Down Expand Up @@ -86,7 +86,7 @@ export class MatTreeNodeDef<T> extends CdkTreeNodeDef<T> {
selector: 'mat-nested-tree-node',
exportAs: 'matNestedTreeNode',
host: {
'[attr.aria-expanded]': 'isExpanded',
'[attr.aria-expanded]': 'expandable ? isExpanded : null',
'[attr.role]': 'role',
'class': 'mat-nested-tree-node',
},
Expand Down
57 changes: 51 additions & 6 deletions src/material/tree/tree.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/
import {FlatTreeControl, NestedTreeControl, TreeControl} from '@angular/cdk/tree';
import {Component, ViewChild, Type} from '@angular/core';
import {Component, ViewChild, Type, Inject, InjectionToken} from '@angular/core';
import {ComponentFixture, TestBed} from '@angular/core/testing';
import {BehaviorSubject, Observable} from 'rxjs';
import {
Expand All @@ -18,21 +18,48 @@ import {
} from './index';


const INIT_WITH_CHILDREN = new InjectionToken<boolean>('initWithChildren');
describe('MatTree', () => {
/** Represents an indent for expectNestedTreeToMatch */
const _ = {};

let treeElement: HTMLElement;
let underlyingDataSource: FakeDataSource;

function configureMatTreeTestingModule(declarations: Type<any>[]) {
function configureMatTreeTestingModule(declarations: Type<any>[], initWithChildren = false) {
TestBed.configureTestingModule({
imports: [MatTreeModule],
declarations: declarations,
providers: [
{provide: INIT_WITH_CHILDREN, useValue: initWithChildren}
]
}).compileComponents();
}

describe('flat tree', () => {
describe('should be accessible', () => {
let fixture: ComponentFixture<SimpleMatTreeApp>;
let component: SimpleMatTreeApp;
beforeEach(() => {
configureMatTreeTestingModule([SimpleMatTreeApp], /*initWithChildren*/ true);
fixture = TestBed.createComponent(SimpleMatTreeApp);

component = fixture.componentInstance;
underlyingDataSource = component.underlyingDataSource;
treeElement = fixture.nativeElement.querySelector('mat-tree');

fixture.detectChanges();
});

it('with aria-expanded attribute on parent nodes only', () => {
const ariaExpandedVals = getNodes(treeElement)
.map(node => node.getAttribute('aria-expanded'));
// TODO the fourth node is not reflected in the DOM, but its parent has
// aria-expanded on it. Figure out why the node is not in the DOM.
expect(ariaExpandedVals).toEqual(['false', null, null]);
});
});

describe('should initialize', () => {
let fixture: ComponentFixture<SimpleMatTreeApp>;
let component: SimpleMatTreeApp;
Expand Down Expand Up @@ -61,9 +88,20 @@ describe('MatTree', () => {

getNodes(treeElement).forEach(node => {
expect(node.getAttribute('role')).toBe('treeitem');
expect(node.getAttribute('aria-level')).toBe('0');
});
});

it('with the right aria-levels', () => {
// add a child to the first node
let data = underlyingDataSource.data;
underlyingDataSource.addChild(data[2]);
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);

Expand Down Expand Up @@ -485,10 +523,14 @@ class FakeDataSource {

disconnect() {}

constructor() {
constructor(addChildren = false) {
for (let i = 0; i < 3; i++) {
this.addData();
}
if (addChildren) {
const parent = this.data[0];
this.addChild(parent);
}
}

addChild(parent: TestData, isSpecial: boolean = false) {
Expand Down Expand Up @@ -557,7 +599,10 @@ function expectFlatTreeToMatch(treeElement: Element, expectedPaddingIndent: numb
}
}

getNodes(treeElement).forEach((node, index) => {
const nodes = getNodes(treeElement);
expect(nodes.length).toBe(expectedTree.length);

nodes.forEach((node, index) => {
const expected = expectedTree ?
expectedTree[index] :
null;
Expand Down Expand Up @@ -642,11 +687,11 @@ class SimpleMatTreeApp {

dataSource = new MatTreeFlatDataSource(this.treeControl, this.treeFlattener);

underlyingDataSource = new FakeDataSource();
underlyingDataSource = new FakeDataSource(this.initWithChildren);

@ViewChild(MatTree) tree: MatTree<TestData>;

constructor() {
constructor(@Inject(INIT_WITH_CHILDREN) private readonly initWithChildren: boolean) {
this.underlyingDataSource.connect().subscribe(data => {
this.dataSource.data = data;
});
Expand Down