fix(material/tree): Do not add aria-expanded to leaf nodes #18032
Conversation
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
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
googlebot
added
the
cla: yes
| @@ -358,6 +360,7 @@ export class CdkTreeNode<T> implements FocusableOption, OnDestroy { | |||
|
|
|||
| protected _setRoleFromData(): void { | |||
There was a problem hiding this comment.
-
I added the isExpandable check to this function because it was an expedient way to avoid duplicating the logic. Do you recommend a rename to
setRoleAndExpandedFromData, or another solution? Same goes for setRoleFromChildren. -
This function is only called when the
datasetter in this class is invoked. In the CDK tree tests, this function is NOT invoked when you add a new node to the tree, so aria-expanded will not be updated. In the demo page (yarn dev-app) it does appear to be being called, so I think it might just be a bug in the tests. But I'm not sure, and could use some additional advice.
There was a problem hiding this comment.
For 1, I would rename this method to _updateExpandedState since, IIRC, you mentioned that the role should always be treeitem anyway, right?
For 2, I think it's the same issue as #17818
| if (addChildren) { | ||
| const parent = this.data[0]; | ||
| this.addChild(parent, true); | ||
| } |
There was a problem hiding this comment.
I added this addChildren flag as a way to optionally add children to the data source before the tree is rendered, because aria attribute changes were not reflected in the DOM otherwise. Ideally I would remove this. See other comment -- I need to find a way to make the parent node update its data when a child is added to it. I think this might just be a test change, but I could use another pair of eyes on this.
There was a problem hiding this comment.
I think this is probably the same issue as I commented on in #17818
| if (addChildren) { | ||
| const parent = this.data[0]; | ||
| this.addChild(parent, true); | ||
| } |
There was a problem hiding this comment.
I think this is probably the same issue as I commented on in #17818
| @@ -334,6 +334,8 @@ export class CdkTreeNode<T> implements FocusableOption, OnDestroy { | |||
| */ | |||
| @Input() role: 'treeitem' | 'group' = 'treeitem'; | |||
|
|
|||
| expandable = false; | |||
There was a problem hiding this comment.
Can you prefix this member with an underscore to signal that it's not part of the public API?
| @@ -358,6 +360,7 @@ export class CdkTreeNode<T> implements FocusableOption, OnDestroy { | |||
|
|
|||
| protected _setRoleFromData(): void { | |||
There was a problem hiding this comment.
For 1, I would rename this method to _updateExpandedState since, IIRC, you mentioned that the role should always be treeitem anyway, right?
For 2, I think it's the same issue as #17818
andrewseguin
removed
the
cla: yes
|
Closing due to inactivity after review comments were left |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This is a work-in-progress. It is also dependent on #17818, so that's why there are unrelated aria-level changes.
When I test a mat-tree with a screen reader, for leaf nodes it will read out that the node is collapsed, which does not make sense. The previous code was just checking if the node was expanded; the solution is to first check if the node is expandable, and if not, set the value to null so the attribute is not added to the element.
I have a few questions that I ran into that I will leave in comments closer to the code in question.