[TreeView] Change focus management to aria-activedescendant #21695

joshwooding · 2020-07-06T19:08:16Z

I have followed (at least) the PR section of the contributing guide.

Docs: https://deploy-preview-21695--material-ui.netlify.app/components/tree-view/

Closes #20204
Closes #20097

Allows us to have more flexibility in the future and simplifies TreeItem. Since I was changing a lot of the tests due to the focus change I refactored them all to use screen and removed all uses of container.

Todo:

Stop allowing custom ids on TreeItems

packages/material-ui-lab/src/TreeView/descendants.js

mui-pr-bot · 2020-07-06T19:31:12Z

@material-ui/lab: parsed: -0.24% 😍, gzip: -0.11% 😍

Details of bundle changes

Generated by 🚫 dangerJS against 2845299

packages/material-ui-lab/src/TreeView/TreeView.test.js

eps1lon · 2020-07-06T20:00:47Z

packages/material-ui-lab/src/TreeView/descendants.js

+    }
+    // Is before
+    // eslint-disable-next-line no-bitwise
+    if (array[middle].element.compareDocumentPosition(element) & Node.DOCUMENT_POSITION_PRECEDING) {


This exists 😮 Is it an issue if elements are portaled?

Hmm, It shouldn't be but I should add a test :P

…irectly

test/utils/initMatchers.js

oliviertassinari · 2020-07-07T20:20:08Z

packages/material-ui-lab/src/TreeView/TreeView.test.js


-      expect(getByRole('tree')).to.have.attribute('aria-multiselectable', 'true');
+      expect(screen.getByRole('tree')).to.have.attribute('aria-multiselectable', 'true');


Maybe we should have left this change for another pull request, it makes reviewing the change more challenging.

Actually, did we settle on this approach? I have seen it mentioned in https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#not-using-screen. cc @mui-org/core-team.

I've removed it from the PR to see if it makes it easier to review.

eps1lon

Stop allowing custom ids on TreeItems

Is this for this PR or another one?

joshwooding · 2020-07-08T21:42:26Z

Stop allowing custom ids on TreeItems

Is this for this PR or another one?

Probably for this one. It's quite badly worded - active descendant requires an id on the TreeItem but currently, users can override this breaking functionality. We need to add a warning or to change nodeId -> id. What are your thoughts about the latter? @eps1lon @oliviertassinari

eps1lon · 2020-07-08T21:48:28Z

I'd prefer to not overload id because that is associated with a DOM attribute on a global scope. It sounds like you're proposing to reduce API surface (no more custom IDs) which I quite like. This component is in the lab for a reason.

stale

eps1lon

I thought we were getting rid of nodeId. Not being able to set id on the underlying DOM node for role="treeitem" could be quite problematic if people need to reference it in another IDREF attribute (e.g. aria-labelledby) though I'm not aware of concrete use cases. It's just the first component where people are not able to query it by ID.

Can we put the id in the nodeMap and read it from there instead?

joshwooding · 2020-07-09T22:21:04Z

packages/material-ui-lab/src/TreeView/TreeView.js

      setFocusedNodeId(id);
+
+      if (onNodeFocus) {
+        onNodeFocus(event, id);


I'm not a big fan of passing an event here. Sometimes the target will be the tree e.g. on keyboard events. I don't really think there is an alternative apart from creating some kind of pseudo-event where we only populate target.

@eps1lon @oliviertassinari

This is fine in my book as long as it's typed as a generic synthetic event i.e. React.SyntheticEvent. We already have this in place in a couple of other places. It's similar to how React itself doesn't make any guarantee about event.nativeEvent. #21552 has more context on this issue.

fixed.

packages/material-ui-lab/src/TreeItem/TreeItem.js

joshwooding added 2 commits July 6, 2020 20:03

Use aria-activedescendant instead of element focus

9a4d80b

Memoize descendant

7fbd1ba

joshwooding added bug 🐛 Something doesn't work new feature New feature or request component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! labels Jul 6, 2020

joshwooding mentioned this pull request Jul 6, 2020

[TreeView] Change focus management to aria-descendant #21592

Closed

1 task

docs

9ac4ff8

joshwooding commented Jul 6, 2020

View reviewed changes

packages/material-ui-lab/src/TreeView/descendants.js Show resolved Hide resolved

Remove conditional chaining

4a50f8a

joshwooding force-pushed the tree-aria-descendant branch from 2863f50 to 4a50f8a Compare July 6, 2020 19:46

eps1lon reviewed Jul 6, 2020

View reviewed changes

packages/material-ui-lab/src/TreeView/TreeView.test.js Outdated Show resolved Hide resolved

eps1lon reviewed Jul 6, 2020

View reviewed changes

joshwooding added 2 commits July 7, 2020 00:09

Remove random comment

219d8c1

use toHaveVirtualFocus instead of matching on aria-activedescendant d…

8ac0e42

…irectly

oliviertassinari approved these changes Jul 7, 2020

View reviewed changes

joshwooding added 3 commits July 7, 2020 22:35

toHaveVirtualFocus -> toBeActiveDescendant

fe656a9

Missed one

6df378c

revert migration to screen?

4018be7

joshwooding force-pushed the tree-aria-descendant branch from ec0dd83 to 4018be7 Compare July 7, 2020 21:36

eps1lon previously requested changes Jul 8, 2020

View reviewed changes

Allow programmatic focus and warn on id and onFocus prop existence

888e453

joshwooding requested a review from eps1lon July 9, 2020 01:26

joshwooding added 4 commits July 9, 2020 02:34

toBeActiveDescendant -> toHaveVirtualFocus

8a0a094

Add comment

228588e

Add another comment

12823db

Add portal test

48cf9e4

eps1lon mentioned this pull request Jul 9, 2020

[TreeView] Test current behavior of active item removal #21720

Merged

eps1lon previously requested changes Jul 9, 2020

View reviewed changes

Allow custom ids

4e15ce9

joshwooding commented Jul 9, 2020

View reviewed changes

joshwooding requested a review from eps1lon July 9, 2020 23:15

eps1lon reviewed Jul 10, 2020

View reviewed changes

packages/material-ui-lab/src/TreeItem/TreeItem.js Outdated Show resolved Hide resolved

joshwooding mentioned this pull request Jul 12, 2020

[material-ui][mui-system] Completely remove IE 11 support in v6 #14420

Closed

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 20, 2020

eps1lon added 2 commits July 22, 2020 10:47

Merge branch 'next' into pr/joshwooding/21695

84c8e36

prevent boolean cast in if

2845299

eps1lon removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 22, 2020

eps1lon merged commit 0a1551e into mui:next Jul 22, 2020

joshwooding deleted the tree-aria-descendant branch July 23, 2020 22:25

oliviertassinari mentioned this pull request Jan 18, 2021

[TreeView] Label text is not selectable mui/mui-x#9958

Closed

oliviertassinari mentioned this pull request Aug 7, 2023

[TreeView] component is not accessible in Android Talkback mui/mui-x#12275

Open

2 tasks

oliviertassinari mentioned this pull request Feb 11, 2022

[TreeView] Jumpy focus when content higher than the browser content's height mui/mui-x#10234

Closed

oliviertassinari mentioned this pull request Feb 29, 2024

[TreeView] Set focus on the focused Tree Item instead of the Tree View mui/mui-x#12226

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TreeView] Change focus management to aria-activedescendant #21695

[TreeView] Change focus management to aria-activedescendant #21695

joshwooding commented Jul 6, 2020 •

edited

Loading

mui-pr-bot commented Jul 6, 2020 •

edited

Loading

eps1lon Jul 6, 2020

joshwooding Jul 6, 2020

oliviertassinari Jul 7, 2020

oliviertassinari Jul 7, 2020

joshwooding Jul 7, 2020

eps1lon left a comment

joshwooding commented Jul 8, 2020

eps1lon commented Jul 8, 2020

eps1lon left a comment •

edited

Loading

joshwooding Jul 9, 2020 •

edited

Loading

eps1lon Jul 10, 2020


		expect(getByRole('tree')).to.have.attribute('aria-multiselectable', 'true');
		expect(screen.getByRole('tree')).to.have.attribute('aria-multiselectable', 'true');

[TreeView] Change focus management to aria-activedescendant #21695

[TreeView] Change focus management to aria-activedescendant #21695

Conversation

joshwooding commented Jul 6, 2020 • edited Loading

mui-pr-bot commented Jul 6, 2020 • edited Loading

eps1lon Jul 6, 2020

Choose a reason for hiding this comment

joshwooding Jul 6, 2020

Choose a reason for hiding this comment

oliviertassinari Jul 7, 2020

Choose a reason for hiding this comment

oliviertassinari Jul 7, 2020

Choose a reason for hiding this comment

joshwooding Jul 7, 2020

Choose a reason for hiding this comment

eps1lon left a comment

Choose a reason for hiding this comment

joshwooding commented Jul 8, 2020

eps1lon commented Jul 8, 2020

eps1lon left a comment • edited Loading

Choose a reason for hiding this comment

joshwooding Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

eps1lon Jul 10, 2020

Choose a reason for hiding this comment

joshwooding commented Jul 6, 2020 •

edited

Loading

mui-pr-bot commented Jul 6, 2020 •

edited

Loading

eps1lon left a comment •

edited

Loading

joshwooding Jul 9, 2020 •

edited

Loading