fix(cdk/tree): fix level for nested tree nodes #20226

annieyw · 2020-08-06T22:05:48Z

set aria-level for nested nodes
fix level for nested nodes
- previously level was always 0 for nested nodes

src/cdk/tree/nested-node.ts

src/cdk/tree/tree.ts

src/material/tree/node.ts

src/material/tree/tree.ts

src/cdk/tree/tree.ts

src/cdk/tree/nested-node.ts

src/cdk/tree/tree.spec.ts

src/cdk/tree/tree.ts

jelbourn · 2020-08-10T19:45:27Z

src/cdk/tree/tree.ts

@@ -128,7 +128,10 @@ export class CdkTree<T> implements AfterContentChecked, CollectionViewer, OnDest
    new BehaviorSubject<{start: number, end: number}>({start: 0, end: Number.MAX_VALUE});

  constructor(private _differs: IterableDiffers,
-              private _changeDetectorRef: ChangeDetectorRef) {}
+              private _changeDetectorRef: ChangeDetectorRef,
+              private _elementRef: ElementRef<HTMLElement>) {


Changing the constructor signature for CdkTree seems like it would be a breaking change since we do expect people will extend this class. Is there a way we can do this without changing the signature?

The typical way we handle this is to make the new parameter optional and add a TODO

// @breaking-change 11.0.0 make this required private _elementRef?: ElementRef<HTMLElement>

and then of course we need to handle the fact that the argument might be undefined. In this case I'm not sure how we could handle it being undefined though, since we've switched to applying the class this way...

maybe instead we could do individual host bindings?

@HostBinding('class.cdk-tree') _cdkTreeClass = true;

Probably, I can't think of another way we could do it

Solution:
Add the class on the host property for CdkTree and have both cdk-tree and mat-tree classes on MatTree.

Justification:

Cannot add the class as a @HostBinding as the class is not applied yet when the children nodes try to read the class off of it.

Moving the logic setting aria-level later causes further timing issues with MatTree

Cannot directly add the class in the constructor as it is a breaking change (discussed above).

Added comment in the MatTree class for clarification

src/cdk/tree/nested-node.ts

src/cdk/tree/tree.ts

mmalerba

LGTM

src/material/tree/tree.ts

src/cdk/tree/nested-node.ts

src/cdk/tree/tree.ts

jelbourn

lgtm

src/cdk/tree/nested-node.ts

src/cdk/tree/tree.ts

src/cdk/tree/nested-node.ts

src/cdk/tree/tree.ts

src/material/tree/node.ts

…Attribute

googlebot · 2020-08-31T17:34:26Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

…asses inheriting CdkTreeNode

mmalerba

LGTM

mmalerba · 2020-08-31T19:36:01Z

Thanks for being so patient and thorough with this change. This uncovered a lot of subtle tricky bugs, but taking the time to work through them and fix them is definitely going to pay off for our users

@HostBinding

* fix(cdk/tree): fix level for flat and nested tree nodes * add test for material/tree * cache parent level instead of traversing DOM on ever level get * logging * set aria-level in ngOnInit, clean up * public api * change host binding to @HostBinding * use host:{} for mat tree and hostbinding for cdk tree * fix tslint comments, fix class inheritance for mat tree * clarify adding class directly to elementRef instead of on host property * minor fixes * fix isNodeElement method name/logic, guarantee boolean value * clean up duplicate @HostBinding and host:{} * replace isDevMode() * super life cycle hooks * public api update * comments explaining super life hook calls, add aria-expanded with setAttribute * set aria-expaded in ngDoCheck * remove duplicate input from MatTreeNode, add missing inputs:[] for classes inheriting CdkTreeNode * add super.ngDoCheck() * change isExpanded setter to private method * remove setting aria-expanded in constructor (cherry picked from commit f951bf4)

angular-automatic-lock-bot · 2020-10-10T00:11:17Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 6, 2020

annieyw requested review from jelbourn and mmalerba August 6, 2020 22:49

annieyw marked this pull request as ready for review August 7, 2020 20:24

annieyw requested a review from andrewseguin as a code owner August 7, 2020 20:24

mmalerba changed the title ~~fix(cdk/tree): fix level nested tree nodes~~ fix(cdk/tree): fix level for nested tree nodes Aug 7, 2020

mmalerba reviewed Aug 7, 2020

View reviewed changes

src/cdk/tree/tree.ts Outdated Show resolved Hide resolved

src/cdk/tree/tree.ts Outdated Show resolved Hide resolved

mmalerba reviewed Aug 7, 2020

View reviewed changes

src/cdk/tree/nested-node.ts Outdated Show resolved Hide resolved

annieyw force-pushed the dom-parent branch 2 times, most recently from a5f8a37 to ffff172 Compare August 7, 2020 23:22

crisbeto mentioned this pull request Aug 9, 2020

Does mat-tree support MatNestedTreeNode.level property? #17945

Closed

annieyw requested a review from mmalerba August 10, 2020 17:29

annieyw force-pushed the dom-parent branch from d3ed3f0 to f5f489b Compare August 10, 2020 19:36

jelbourn reviewed Aug 10, 2020

View reviewed changes

mmalerba reviewed Aug 10, 2020

View reviewed changes

src/cdk/tree/nested-node.ts Outdated Show resolved Hide resolved

src/cdk/tree/tree.ts Outdated Show resolved Hide resolved

src/cdk/tree/tree.ts Outdated Show resolved Hide resolved

src/cdk/tree/tree.ts Outdated Show resolved Hide resolved

mmalerba added G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround labels Aug 10, 2020

annieyw requested review from mmalerba and jelbourn August 10, 2020 22:08

mmalerba approved these changes Aug 10, 2020

View reviewed changes

src/material/tree/tree.ts Outdated Show resolved Hide resolved

crisbeto reviewed Aug 11, 2020

View reviewed changes

src/cdk/tree/nested-node.ts Outdated Show resolved Hide resolved

src/cdk/tree/tree.ts Outdated Show resolved Hide resolved

src/cdk/tree/tree.ts Outdated Show resolved Hide resolved

src/cdk/tree/tree.ts Outdated Show resolved Hide resolved

mmalerba added the target: patch This PR is targeted for the next patch release label Aug 11, 2020

annieyw force-pushed the dom-parent branch from 980388d to 25d1f00 Compare August 11, 2020 21:29

jelbourn reviewed Aug 12, 2020

View reviewed changes

src/cdk/tree/tree.ts Outdated Show resolved Hide resolved

jelbourn approved these changes Aug 12, 2020

View reviewed changes

annieyw added the action: merge The PR is ready for merge by the caretaker label Aug 13, 2020

annieyw mentioned this pull request Aug 18, 2020

feat(material/tree): add test harness for MatTree #20323

Merged

annieyw force-pushed the dom-parent branch from 738ec92 to f6f8b10 Compare August 21, 2020 22:23

mmalerba reviewed Aug 27, 2020

View reviewed changes

annieyw added 9 commits August 31, 2020 10:33

clarify adding class directly to elementRef instead of on host property

b14d21c

minor fixes

309a18a

fix isNodeElement method name/logic, guarantee boolean value

a0d39dd

clean up duplicate @HostBinding and host:{}

b682ea7

replace isDevMode()

e1b0704

super life cycle hooks

15187c3

public api update

0251094

comments explaining super life hook calls, add aria-expanded with set…

efbb7b7

…Attribute

set aria-expaded in ngDoCheck

0fe2e7d

annieyw force-pushed the dom-parent branch from 44659c5 to 0fe2e7d Compare August 31, 2020 17:34

annieyw removed request for a team, devversion and andrewseguin August 31, 2020 17:34

googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Aug 31, 2020

remove duplicate input from MatTreeNode, add missing inputs:[] for cl…

8e70d18

…asses inheriting CdkTreeNode

annieyw force-pushed the dom-parent branch from 500820d to 8e70d18 Compare August 31, 2020 18:22

add super.ngDoCheck()

b14dec0

mmalerba approved these changes Aug 31, 2020

View reviewed changes

change isExpanded setter to private method

0806b92

annieyw force-pushed the dom-parent branch from 8f4fe41 to 0806b92 Compare August 31, 2020 19:30

mmalerba added the action: merge The PR is ready for merge by the caretaker label Aug 31, 2020

remove setting aria-expanded in constructor

3181e15

andrewseguin merged commit f951bf4 into angular:master Sep 9, 2020

annieyw deleted the dom-parent branch September 9, 2020 17:11

angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 10, 2020

fix(cdk/tree): fix level for nested tree nodes #20226

fix(cdk/tree): fix level for nested tree nodes #20226

Uh oh!

Conversation

annieyw commented Aug 6, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jelbourn Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

mmalerba Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

mmalerba Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

jelbourn Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

annieyw Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mmalerba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

googlebot commented Aug 31, 2020

Uh oh!

mmalerba left a comment

Choose a reason for hiding this comment

Uh oh!

mmalerba commented Aug 31, 2020

Uh oh!

angular-automatic-lock-bot bot commented Oct 10, 2020

Uh oh!

Uh oh!