Skip to content
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

fix(cdk/tree): assorted bug fixes #28305

Merged
merged 6 commits into from May 15, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 12 additions & 2 deletions src/cdk/a11y/key-manager/tree-key-manager.spec.ts
Expand Up @@ -182,8 +182,18 @@ describe('TreeKeyManager', () => {
itemList.notifyOnChanges();
});

it('initializes with the first item activated', () => {
expect(keyManager.getActiveItemIndex()).withContext('active item index').toBe(0);
it('does not initialize with the first item activated', () => {
expect(keyManager.getActiveItemIndex()).withContext('active item index').toBe(-1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, I thought we do want disabled options to be keyboard focusable? That's to align with WAI ARIA recommendation.

https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface/#focusabilityofdisabledcontrols

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm... true. do we still want to ignore clicks as well? or rather, should clicking a disabled item still focus it

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface/#focusabilityofdisabledcontrols recommends that disabled treeitems be focusable, but I don't think it says anything about keyboard vs mouse focus. I'll see if I can look more to see if there's a recommendation for mouse focus.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think ultimately we can try it as-is (we ignore a click if it's disabled), and see if anyone reports a bug?

});

it('if an item is subsequently enabled, activates it', () => {
itemList.reset([
new itemParam.constructor('Bilbo', true),
new itemParam.constructor('Frodo', false),
new itemParam.constructor('Pippin', true),
]);
itemList.notifyOnChanges();
expect(keyManager.getActiveItemIndex()).withContext('active item index').toBe(1);
});
});

Expand Down
6 changes: 5 additions & 1 deletion src/cdk/a11y/key-manager/tree-key-manager.ts
Expand Up @@ -73,14 +73,18 @@ export class TreeKeyManager<T extends TreeKeyManagerItem> implements TreeKeyMana
return;
}

let focusIndex = 0;
let focusIndex = -1;
for (let i = 0; i < this._items.length; i++) {
if (!this._skipPredicateFn(this._items[i]) && !this._isItemDisabled(this._items[i])) {
focusIndex = i;
break;
}
}

if (focusIndex === -1) {
return;
}

this.focusItem(focusIndex);
this._hasInitialFocused = true;
}
Expand Down
34 changes: 28 additions & 6 deletions src/cdk/tree/tree.ts
Expand Up @@ -23,6 +23,7 @@ import {
import {
AfterContentChecked,
AfterContentInit,
AfterViewInit,
ChangeDetectionStrategy,
ChangeDetectorRef,
Component,
Expand Down Expand Up @@ -111,7 +112,13 @@ type RenderingData<T> =
imports: [CdkTreeNodeOutlet],
})
export class CdkTree<T, K = T>
implements AfterContentChecked, AfterContentInit, CollectionViewer, OnDestroy, OnInit
implements
AfterContentChecked,
AfterContentInit,
AfterViewInit,
CollectionViewer,
OnDestroy,
OnInit
{
/** Subject that emits when the component has been destroyed. */
private readonly _onDestroy = new Subject<void>();
Expand Down Expand Up @@ -248,6 +255,7 @@ export class CdkTree<T, K = T>

/** The key manager for this tree. Handles focus and activation based on user keyboard input. */
_keyManager: TreeKeyManagerStrategy<CdkTreeNode<T, K>>;
private _viewInit = false;

constructor(
private _differs: IterableDiffers,
Expand Down Expand Up @@ -288,6 +296,10 @@ export class CdkTree<T, K = T>
this._initializeDataDiffer();
}

ngAfterViewInit() {
this._viewInit = true;
}

private _updateDefaultNodeDefinition() {
const defaultNodeDefs = this._nodeDefs.filter(def => !def.when);
if (defaultNodeDefs.length > 1 && (typeof ngDevMode === 'undefined' || ngDevMode)) {
Expand Down Expand Up @@ -449,7 +461,9 @@ export class CdkTree<T, K = T>
}

private _initializeDataDiffer() {
this._dataDiffer = this._differs.find([]).create(this.trackBy);
// Provide a default trackBy based on `_getExpansionKey` if one isn't provided.
const trackBy = this.trackBy ?? ((_index: number, item: T) => this._getExpansionKey(item));
this._dataDiffer = this._differs.find([]).create(trackBy);
}

private _checkTreeControlUsage() {
Expand Down Expand Up @@ -484,11 +498,19 @@ export class CdkTree<T, K = T>
parentData?: T,
) {
const changes = dataDiffer.diff(data);
if (!changes) {

// Some tree consumers expect change detection to propagate to nodes
// even when the array itself hasn't changed; we explicitly detect changes
// anyways in order for nodes to update their data.
//
// However, if change detection is called while the component's view is
// still initing, then the order of child views initing will be incorrect;
// to prevent this, we only exit early if the view hasn't initialized yet.
if (!changes && !this._viewInit) {
return;
}

changes.forEachOperation(
changes?.forEachOperation(
(
item: IterableChangeRecord<T>,
adjustedPreviousIndex: number | null,
Expand Down Expand Up @@ -682,12 +704,12 @@ export class CdkTree<T, K = T>

/** Level accessor, used for compatibility between the old Tree and new Tree */
_getLevelAccessor() {
return this.treeControl?.getLevel ?? this.levelAccessor;
return this.treeControl?.getLevel?.bind(this.treeControl) ?? this.levelAccessor;
}

/** Children accessor, used for compatibility between the old Tree and new Tree */
_getChildrenAccessor() {
return this.treeControl?.getChildren ?? this.childrenAccessor;
return this.treeControl?.getChildren?.bind(this.treeControl) ?? this.childrenAccessor;
}

/**
Expand Down
5 changes: 5 additions & 0 deletions src/material/tree/testing/node-harness.ts
Expand Up @@ -35,6 +35,11 @@ export class MatTreeNodeHarness extends ContentContainerComponentHarness<string>
return coerceBooleanProperty(await (await this.host()).getAttribute('aria-expanded'));
}

/** Whether the tree node is expandable. */
async isExpandable(): Promise<boolean> {
return (await (await this.host()).getAttribute('aria-expanded')) !== null;
}

/** Whether the tree node is disabled. */
async isDisabled(): Promise<boolean> {
return coerceBooleanProperty(await (await this.host()).getProperty('aria-disabled'));
Expand Down
3 changes: 2 additions & 1 deletion src/material/tree/tree.spec.ts
Expand Up @@ -604,11 +604,12 @@ describe('MatTree', () => {
});

it('ignores clicks on disabled items', () => {
underlyingDataSource.data[0].isDisabled = true;
underlyingDataSource.data[1].isDisabled = true;
fixture.detectChanges();

// attempt to click on the first child
nodes[1].click();
fixture.detectChanges();

expect(nodes.map(x => x.getAttribute('tabindex')).join(', ')).toEqual(
'0, -1, -1, -1, -1, -1',
Expand Down
5 changes: 4 additions & 1 deletion tools/public_api_guard/cdk/tree.md
Expand Up @@ -6,6 +6,7 @@

import { AfterContentChecked } from '@angular/core';
import { AfterContentInit } from '@angular/core';
import { AfterViewInit } from '@angular/core';
import { BehaviorSubject } from 'rxjs';
import { ChangeDetectorRef } from '@angular/core';
import { CollectionViewer } from '@angular/cdk/collections';
Expand Down Expand Up @@ -76,7 +77,7 @@ export class CdkNestedTreeNode<T, K = T> extends CdkTreeNode<T, K> implements Af
}

// @public
export class CdkTree<T, K = T> implements AfterContentChecked, AfterContentInit, CollectionViewer, OnDestroy, OnInit {
export class CdkTree<T, K = T> implements AfterContentChecked, AfterContentInit, AfterViewInit, CollectionViewer, OnDestroy, OnInit {
constructor(_differs: IterableDiffers, _changeDetectorRef: ChangeDetectorRef, _dir: Directionality);
childrenAccessor?: (dataNode: T) => T[] | Observable<T[]>;
collapse(dataNode: T): void;
Expand Down Expand Up @@ -106,6 +107,8 @@ export class CdkTree<T, K = T> implements AfterContentChecked, AfterContentInit,
// (undocumented)
ngAfterContentInit(): void;
// (undocumented)
ngAfterViewInit(): void;
// (undocumented)
ngOnDestroy(): void;
// (undocumented)
ngOnInit(): void;
Expand Down
1 change: 1 addition & 0 deletions tools/public_api_guard/material/tree-testing.md
Expand Up @@ -27,6 +27,7 @@ export class MatTreeNodeHarness extends ContentContainerComponentHarness<string>
getText(): Promise<string>;
static hostSelector: string;
isDisabled(): Promise<boolean>;
isExpandable(): Promise<boolean>;
isExpanded(): Promise<boolean>;
toggle(): Promise<void>;
// (undocumented)
Expand Down