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

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

joshwooding
Copy link
Member

@joshwooding joshwooding commented Jun 27, 2020 •
edited
Loading

Opened for visibility. Built on: #21574

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.

@joshwooding joshwooding added the component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! label Jun 27, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Jun 27, 2020 •
edited
Loading

@material-ui/lab: parsed: -0.90% 😍, gzip: -0.84% 😍

Details of bundle changes

Generated by 🚫 dangerJS against 70e3084

@github-actions github-actions bot mentioned this pull request Jun 27, 2020
Closed
@joshwooding joshwooding added bug 🐛 Something doesn't work new feature New feature or request labels Jun 28, 2020
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 2, 2020
@joshwooding joshwooding force-pushed the tree-focus-changes branch 2 times, most recently from 9556ff7 to 1ebc907 Compare July 3, 2020 00:13
@joshwooding
Copy link
Member Author

Current thoughts using the old descendants allows us to separate the component API and the component that renders to the DOM. This is nice for lots of reasons but probably needs more thought but it makes it easier to support virtualisation and drag and drop. I've tested it using the react@next test mode and it passed so hopefully won't be a problem.

cc @eps1lon @oliviertassinari

joshwooding
joshwooding commented Jul 3, 2020
View reviewed changes
packages/material-ui-lab/src/TreeView/TreeView.d.ts
* @param {object} event The event source of the callback
* @param {string} value of the focused node.
*/
onNodeFocus?: (event: React.ChangeEvent<{}>, nodeId: string) => void;
Copy link
Member Author

Choose a reason for hiding this comment

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

Since you can no longer use onFocus to track focus to nodes thought I should add this.

joshwooding
joshwooding commented Jul 3, 2020
View reviewed changes
packages/material-ui-lab/src/TreeView/TreeView.js
(id) => {
const nodes = getNodesToRemove(id);
Copy link
Member Author

Choose a reason for hiding this comment

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

So much cleaner. Each node only cares about itself now and we get rid of a lot of looping :)

joshwooding
joshwooding commented Jul 3, 2020
View reviewed changes
packages/material-ui-lab/src/TreeView/TreeView.js
};

return (
<TreeViewContext.Provider
value={{
icons: { defaultCollapseIcon, defaultExpandIcon, defaultParentIcon, defaultEndIcon },
focus,
focusFirstNode,
Copy link
Member Author

Choose a reason for hiding this comment

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

A lot less stuff being passed through context is great for readability and potentially performance :)

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jul 3, 2020
@joshwooding joshwooding force-pushed the tree-focus-changes branch 2 times, most recently from 440ab04 to 70e3084 Compare July 3, 2020 18:01
@joshwooding
Copy link
Member Author

@material-ui/lab: parsed: -0.90% 😍, gzip: -0.84% 😍

Details of bundle changes

Generated by 🚫 dangerJS against 440ab04

Nice to see the size drop after the other PR was merged :)

@joshwooding joshwooding changed the title [WIP - TreeView] Change focus management to aria-descendant [TreeView] Change focus management to aria-descendant Jul 3, 2020
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jul 3, 2020
@joshwooding joshwooding force-pushed the tree-focus-changes branch from 70e3084 to e76c197 Compare July 6, 2020 17:57
@joshwooding
Copy link
Member Author

Closed in favour of #21695

Will investigate change descendants in another PR.

@joshwooding joshwooding closed this Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug 🐛 Something doesn't work component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! new feature New feature or request PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[TreeView] Cannot be controlled soley by keyboard after tabbable item removal [TreeView] Selection focus
2 participants
@joshwooding @mui-pr-bot