Skip to content

refactor(material/tree): fork tests for using and not using tree control #27925

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

Merged
merged 2 commits into from
Oct 11, 2023
Merged

refactor(material/tree): fork tests for using and not using tree control #27925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4c2069a
refactor(material/tree): fork tests for using and not using tree control
zarend Oct 9, 2023
01dbecd
fix(material/tree): backwards compatible isExpandable
zarend Oct 10, 2023
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: 18 additions & 9 deletions src/cdk/tree/tree-with-tree-control.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {

import {CollectionViewer, DataSource} from '@angular/cdk/collections';
import {Directionality, Direction} from '@angular/cdk/bidi';
import {createKeyboardEvent} from '@angular/cdk/testing/testbed/fake-events';
import {combineLatest, BehaviorSubject, Observable} from 'rxjs';
import {map} from 'rxjs/operators';

Expand All @@ -27,6 +28,7 @@ import {FlatTreeControl} from './control/flat-tree-control';
import {NestedTreeControl} from './control/nested-tree-control';
import {CdkTreeModule, CdkTreeNodePadding} from './index';
import {CdkTree, CdkTreeNode} from './tree';
import {LEFT_ARROW, RIGHT_ARROW} from '../keycodes';

describe('CdkTree', () => {
/** Represents an indent for expectNestedTreeToMatch */
Expand Down Expand Up @@ -758,7 +760,7 @@ describe('CdkTree', () => {
it('with the right aria-expanded attrs', () => {
expect(getNodeAttributes(getNodes(treeElement), 'aria-expanded'))
.withContext('aria-expanded attributes')
.toEqual([null, null, null]);
.toEqual(['false', 'false', 'false']);

component.toggleRecursively = false;
let data = dataSource.data;
Expand All @@ -773,10 +775,10 @@ describe('CdkTree', () => {
// in DOM unless the parent node is expanded.
expect(getNodeAttributes(getNodes(treeElement), 'aria-expanded'))
.withContext('aria-expanded attributes')
.toEqual([null, 'true', 'false', null]);
.toEqual(['false', 'true', 'false', 'false']);
});

it('should expand/collapse the node multiple times', () => {
it('should expand/collapse the node multiple times using keyboard', () => {
component.toggleRecursively = false;
let data = dataSource.data;
const child = dataSource.addChild(data[1], false);
Expand All @@ -793,7 +795,10 @@ describe('CdkTree', () => {

fixture.detectChanges();

(getNodes(treeElement)[1] as HTMLElement).click();
let node = getNodes(treeElement)[1] as HTMLElement;

node.focus();
node.dispatchEvent(createKeyboardEvent('keydown', RIGHT_ARROW));
fixture.detectChanges();

expect(component.treeControl.expansionModel.selected.length)
Expand All @@ -807,7 +812,9 @@ describe('CdkTree', () => {
[`topping_3 - cheese_3 + base_3`],
);

(getNodes(treeElement)[1] as HTMLElement).click();
node = getNodes(treeElement)[1] as HTMLElement;
node.focus();
node.dispatchEvent(createKeyboardEvent('keydown', LEFT_ARROW));
fixture.detectChanges();

expectNestedTreeToMatch(
Expand All @@ -820,7 +827,9 @@ describe('CdkTree', () => {
.withContext(`Expect node collapsed`)
.toBe(0);

(getNodes(treeElement)[1] as HTMLElement).click();
node = getNodes(treeElement)[1] as HTMLElement;
node.focus();
node.dispatchEvent(createKeyboardEvent('keydown', RIGHT_ARROW));
fixture.detectChanges();

expect(component.treeControl.expansionModel.selected.length)
Expand Down Expand Up @@ -1585,9 +1594,9 @@ class NestedCdkTreeAppWithToggle {

getChildren = (node: TestData) => node.observableChildren;

treeControl: TreeControl<TestData> = new NestedTreeControl(this.getChildren, {
isExpandable: node => node.children.length > 0,
});
isExpandable?: (node: TestData) => boolean;

treeControl: TreeControl<TestData> = new NestedTreeControl(this.getChildren);
dataSource: FakeDataSource | null = new FakeDataSource(this.treeControl);

@ViewChild(CdkTree) tree: CdkTree<TestData>;
Expand Down
20 changes: 12 additions & 8 deletions src/cdk/tree/tree.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1175,8 +1175,14 @@ export class CdkTreeNode<T, K = T> implements OnDestroy, OnInit, TreeKeyManagerI

/** Determines if the tree node is expandable. */
_isExpandable(): boolean {
if (typeof this._tree.treeControl?.isExpandable === 'function') {
return this._tree.treeControl.isExpandable(this._data);
if (this._tree.treeControl) {
if (typeof this._tree.treeControl?.isExpandable === 'function') {
return this._tree.treeControl.isExpandable(this._data);
}

// For compatibility with trees created using TreeControl before we added
// CdkTreeNode#isExpandable.
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's not clear to me if backwards compatibility is desired in this case given that the original behaviour is incorrect from an a11y standpoint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Angular components is required to follow semantic versioning. When releasing this in a minor version, we're required not to break apps that were made before before introducing CdkTreeNode#isExpandable.

I know that the a11y is incorrect, but this is not causing an a11y regression. It means that consumers will get the correct behavior when moving off the the deprecated Tree Control. I know this isn't particularly satisfying, but we have to follow Angular's semantic versioning policy.

If it were for a major version, then we would have a bit more flexibility, but doing this in a minor will allow us to release this in a timely manor. There are a lot of a11y improvements in this PR, and I think it would be good to do what we can feasibly land.

I'm definitely open to suggestions if there is another way we can do this that is backwards compatible for Trees made before cdk-tree-revamp.

Copy link
Collaborator

@BobobUnicorn BobobUnicorn Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We should document this then, I can make some edits to the readmes to that effect. (i.e. upgrade to use this API to get a11y improvements)

}
return this._inputIsExpandable;
}
Expand Down Expand Up @@ -1260,18 +1266,16 @@ export class CdkTreeNode<T, K = T> implements OnDestroy, OnInit, TreeKeyManagerI

/** Collapses this data node. Implemented for TreeKeyManagerItem. */
collapse(): void {
if (!this._isExpandable()) {
return;
if (this.isExpandable) {
this._tree.collapse(this._data);
}
this._tree.collapse(this._data);
}

/** Expands this data node. Implemented for TreeKeyManagerItem. */
expand(): void {
if (!this._isExpandable()) {
return;
if (this.isExpandable) {
this._tree.expand(this._data);
}
this._tree.expand(this._data);
}

_setTabFocusable() {
Expand Down
3 changes: 1 addition & 2 deletions src/components-examples/material/tree/tree-nested-overview/tree-nested-overview-example.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@
<!-- This is the tree node template for expandable nodes -->
<mat-nested-tree-node
*matTreeNodeDef="let node; when: hasChild"
matTreeNodeToggle
isExpandable>
matTreeNodeToggle>
<div class="mat-tree-node">
<button mat-icon-button matTreeNodeToggle
[attr.aria-label]="'Toggle ' + node.name">
Expand Down
Loading