fix(material/tree): Apply aria-level to all nodes

[martiansnoop]

Previously, only leaf nodes had aria-level applied.

This is an incremental change since this is an unfamiliar codebase for
me. The main benefit it will have on its own is that it will allow
anyone doing custom dom manipulation to know what level the node is on.
Otherwise by itself there is no change in how NVDA reads nodes with
children. (It currently reads them as literally "grouping"; no
information about the contents is provided).

This change will be necessary for a later change I'm planning, wherein
the role of parent nodes will be changed from "group" to "treeitem", in
accordance with how roles are applied in WAI-ARIA reference examples
such as
https://www.w3.org/TR/wai-aria-practices/examples/treeview/treeview-1/treeview-1b.html

[martiansnoop]

I've tested this manually via the demos on http://localhost:4200/tree, but I'm having trouble finding the unit tests to update.

[crisbeto]

You can find the tests here and here. You can run them with yarn bazel test //src/material/tree:unit_tests_chromium-local and yarn bazel test //src/cdk/tree:unit_tests_chromium-local.

[martiansnoop]

Thank you! 🤦‍♂️ I was looking for files ending in _test.ts and my eyes went right past the .spec.ts files.

I located the related tests, but it appears like all of the nodes were already leaf nodes (only leaf nodes have role="treeitem"), so the current set of test changes did pass before the changes to mat-tree/cdk-tree were applied.

I haven't looked into modifying the dataset to have a deeper tree yet (and I'm not sure that kind of change belongs in this commit) -- thoughts? [EDIT: it looks like adding items to the tree is really easy, so I think I'll just do that in this PR.]

EDIT: It appears that, as demonstrated by the CdkTree tests, that client code has control over what the level of the nodes are. Since the specific value is not relevant to this change, I just changed the assertions to match the level specified by the data object, but that concerns me slightly in that the next change I have queued up involves making the leveling start at 0 instead of 1.

[jelbourn]

Definitely an oversight that there aren't any existing aria-level tests. The best thing would be to add a new test (with either a new or existing test component) that asserts the right aria-level behavior.

[martiansnoop]

Thanks! Currently I still need to clean up an unrelated change and also figure out why the mat-tree version of the test is failing, so not quite ready for review.

[martiansnoop]

I think I may have found a bug in the flat tree tests:

1. It seems like the code in the expectFlatTreeToMatch function was not asserting that all of the expected values were present in the tree, just that all the dom nodes matched an expected value. I added that assertion and a few of the existing tests fail for me: 312471c
2. When items are added to the data source (like here), they do not seem to be reflected in the DOM yet
3. Tests where a child node was added (example) were passing erroneously because of 1.

I need some help figuring out what to do to get the nodes into the DOM (would have guessed detectChanges would have been doing that). Once that gets figured out, I'll fix the tests that were broken by adding the tree length assertion and put that into a separate CL (unless the owners prefer it in this one).

[jelbourn]

Hey, I somehow missed the notification on this PR; sorry for the super delayed response.

I took a look your test issue. You're correctly adding a new child node, but its parent is marked as collapsed, so the new node isn't rendering. You can fix it by adding component.treeControl.expandAll(); before the detectChanges.

This definitely wasn't obvious- it took me like 10 minutes to realize what was happening.

[martiansnoop]

Aha, thank you! I think everything passes now.

[jelbourn]

LGTM

[martiansnoop]

Do I need to take any actions to merge this?

[jelbourn]

I bumped the priority on this in our merge queue- it looks like it got overlooked due to being older

[jelbourn]

From a quick glance it seems like the bindings should just all be level + 1 (with corresponding test updates), since the spec uses 1-based indexing for some reason.

[andrewseguin]

@martiansnoop We can try to get this in - did you get a chance to see the message about level + 1 from @jelbourn?

[jelbourn]

@annieyw added a commit to this PR to update the level to start at one. LGTM for new changes.

[mmalerba]

LGTM

[angular-automatic-lock-bot]

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.}