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.

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

Jump to bottom

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

Merged
merged 6 commits into from May 15, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bca7c3e
fix(cdk/tree): fix errors from testing
BobobUnicorn Dec 18, 2023
70f9762
fix(cdk/tree): tests
BobobUnicorn Dec 18, 2023
7dea4fb
fix(cdk/tree): update api docs
BobobUnicorn Dec 18, 2023
f160dc1
fix(cdk/a11y): allows disabled items to receive initial focus
BobobUnicorn Dec 19, 2023
9a633b7
fix(cdk/tree): don't focus on click, corrected updating aria-sets
BobobUnicorn Feb 2, 2024
df8b4d5
fix(cdk/tree): update api goldens
BobobUnicorn Feb 12, 2024
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
1 change: 1 addition & 0 deletions src/cdk/tree/tree-with-tree-control.spec.ts
View file
Open in desktop
Expand Up @@ -1174,6 +1174,7 @@ describe('CdkTree', () => {
it('maintains tabindex when component is blurred', () => {
// activate the second child by clicking on it
nodes[1].click();
nodes[1].focus();
fixture.detectChanges();

expect(document.activeElement).toBe(nodes[1]);
Expand Down
1 change: 1 addition & 0 deletions src/cdk/tree/tree.spec.ts
View file
Open in desktop
Expand Up @@ -1216,6 +1216,7 @@ describe('CdkTree', () => {
it('maintains tabindex when component is blurred', () => {
// activate the second child by clicking on it
nodes[1].click();
nodes[1].focus();
fixture.detectChanges();

expect(document.activeElement).toBe(nodes[1]);
Expand Down
66 changes: 51 additions & 15 deletions src/cdk/tree/tree.ts
View file
Open in desktop
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 @@ -280,14 +288,20 @@ export class CdkTree<T, K = T>
this._dataSubscription = null;
}

this._keyManager.destroy();
// In certain tests, the tree might be destroyed before this is initialized
// in `ngAfterContentInit`.
this._keyManager?.destroy();
}

ngOnInit() {
this._checkTreeControlUsage();
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 +463,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 +500,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 All @@ -498,12 +522,6 @@ export class CdkTree<T, K = T>
this.insertNode(data[currentIndex!], currentIndex!, viewContainer, parentData);
} else if (currentIndex == null) {
viewContainer.remove(adjustedPreviousIndex!);
const set = this._getAriaSet(item.item);
const key = this._getExpansionKey(item.item);
set.splice(
set.findIndex(groupItem => this._getExpansionKey(groupItem) === key),
1,
);
} else {
const view = viewContainer.get(adjustedPreviousIndex!);
viewContainer.move(view!, currentIndex);
Expand Down Expand Up @@ -682,12 +700,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 Expand Up @@ -1094,7 +1112,7 @@ export class CdkTree<T, K = T>
'[attr.aria-setsize]': '_getSetSize()',
'[tabindex]': '_tabindex',
'role': 'treeitem',
'(click)': '_focusItem()',
'(click)': '_setActiveItem()',
'(focus)': '_focusItem()',
},
standalone: true,
Expand Down Expand Up @@ -1172,6 +1190,13 @@ export class CdkTreeNode<T, K = T> implements OnDestroy, OnInit, TreeKeyManagerI
readonly _dataChanges = new Subject<void>();

private _inputIsExpandable: boolean = false;
/**
* Flag used to determine whether or not we should be focusing the actual element based on
* some user interaction (click or focus). On click, we don't forcibly focus the element
* since the click could trigger some other component that wants to grab its own focus
* (e.g. menu, dialog).
*/
private _shouldFocus = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I understand why we wouldn't want to focus on click. Native elements usual focus themselves when clicked.

Do we have an example of other component that grabs its own 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.

native elements have focus by virtue of the browser automatically leaving focus on the last interacted element; they do not force focus to be on them. the problem here is essentially:

<mat-tree-node
  [matMenuTriggerFor]="menu">
</mat-tree-node>

in this scenario, without this change:

  1. mat-menu tries to focus the first menu item when it opens
  2. the tree then steals focus back to the tree item

similar behaviour occurs with dialogs where we take focus back from the dialog.

with this change, if there is no additional behaviour where focus is taken from the tree item, the browser will still leave the tree item as the currently active element since it's marked as focusable via tabindex="0"

private _parentNodeAriaLevel: number;

/** The tree node's data. */
Expand Down Expand Up @@ -1273,7 +1298,9 @@ export class CdkTreeNode<T, K = T> implements OnDestroy, OnInit, TreeKeyManagerI
/** Focuses this data node. Implemented for TreeKeyManagerItem. */
focus(): void {
this._tabindex = 0;
this._elementRef.nativeElement.focus();
if (this._shouldFocus) {
this._elementRef.nativeElement.focus();
}

this._changeDetectorRef.markForCheck();
}
Expand Down Expand Up @@ -1314,6 +1341,15 @@ export class CdkTreeNode<T, K = T> implements OnDestroy, OnInit, TreeKeyManagerI
this._tree._keyManager.focusItem(this);
}

_setActiveItem() {
if (this.isDisabled) {
return;
}
this._shouldFocus = false;
this._tree._keyManager.focusItem(this);
this._shouldFocus = true;
}

_emitExpansionState(expanded: boolean) {
this.expandedChange.emit(expanded);
}
Expand Down
5 changes: 5 additions & 0 deletions src/material/tree/testing/node-harness.ts
View file
Open in desktop
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
1 change: 1 addition & 0 deletions src/material/tree/tree-using-tree-control.spec.ts
View file
Open in desktop
Expand Up @@ -602,6 +602,7 @@ describe('MatTree', () => {
it('maintains tabindex when component is blurred', () => {
// activate the second child by clicking on it
nodes[1].click();
nodes[1].focus();
fixture.detectChanges();

expect(document.activeElement).toBe(nodes[1]);
Expand Down
4 changes: 3 additions & 1 deletion src/material/tree/tree.spec.ts
View file
Open in desktop
Expand Up @@ -587,6 +587,7 @@ describe('MatTree', () => {
it('maintains tabindex when component is blurred', () => {
// activate the second child by clicking on it
nodes[1].click();
nodes[1].focus();
fixture.detectChanges();

expect(nodes.map(x => x.getAttribute('tabindex')).join(', ')).toEqual(
Expand All @@ -604,11 +605,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
7 changes: 6 additions & 1 deletion tools/public_api_guard/cdk/tree.md
View file
Open in desktop
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 Expand Up @@ -194,6 +197,8 @@ export class CdkTreeNode<T, K = T> implements OnDestroy, OnInit, TreeKeyManagerI
get role(): 'treeitem' | 'group';
set role(_role: 'treeitem' | 'group');
// (undocumented)
_setActiveItem(): void;
// (undocumented)
protected _tabindex: number | null;
// (undocumented)
protected _tree: CdkTree<T, K>;
Expand Down
1 change: 1 addition & 0 deletions tools/public_api_guard/material/tree-testing.md
View file
Open in desktop
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