[TreeView] Set focus on the focused Tree Item instead of the Tree View #12226

flaviendelangle · 2024-02-27T13:20:36Z

See mui/material-ui#21695 as prior work on this topic.

Preview: https://deploy-preview-12226--material-ui-x.netlify.app/x/react-tree-view/#simple-tree-view

I did not explore if totally removing the focus from the state of the Tree View was doable, because the timing is tight before stable and this would only be an internal change 👍

Changelog

Breaking changes

The focus is now applied to the Tree Item root element instead of the Tree View root element.

This change will allow new features that require the focus to be on the Tree Item,
like the drag and drop reordering of items.
It also solves several issues with focus management,
like the inability to scroll to the focused item when a lot of items are rendered.

This will mostly impact how you write tests to interact with the Tree View:

For example, if you were writing a test with react-testing-library, here is what the changes could look like:

 it('test example on first item', () => {
-  const { getByRole } = render(
+  const { getAllByRole } = render(
     <SimpleTreeView>
       <TreeItem nodeId="one" />
       <TreeItem nodeId="two" />
    </SimpleTreeView>
   );

-  const tree = getByRole('tree');
+  const firstTreeItem = getAllByRole('treeitem')[0];
   act(() => {
-    tree.focus();
+    firstTreeItem.focus();
   });
-  fireEvent.keyDown(tree, { key: 'ArrowDown' });
+  fireEvent.keyDown(firstTreeItem, { key: 'ArrowDown' });
 })

mui-bot · 2024-02-27T13:27:52Z

Deploy preview: https://deploy-preview-12226--material-ui-x.netlify.app/

Updated pages:

docs/data/migration/migration-tree-view-v6/migration-tree-view-v6.md

Generated by 🚫 dangerJS against 4a31e2d

flaviendelangle · 2024-02-27T13:23:37Z

packages/x-tree-view/src/internals/utils/utils.ts

@@ -0,0 +1,14 @@
+// https://www.abeautifulsite.net/posts/finding-the-active-element-in-a-shadow-root/
+export const getActiveElement = (root: Document | ShadowRoot = document): Element | null => {


Copied from the pickers and the grid

flaviendelangle · 2024-02-27T13:24:10Z

packages/x-tree-view/src/internals/plugins/useTreeViewSelection/useTreeViewSelection.types.ts

@@ -7,8 +7,8 @@ export interface UseTreeViewSelectionInstance {
  isNodeSelected: (nodeId: string) => boolean;
  selectNode: (event: React.SyntheticEvent, nodeId: string, multiple?: boolean) => void;
  selectRange: (event: React.SyntheticEvent, nodes: TreeViewItemRange, stacked?: boolean) => void;
-  rangeSelectToFirst: (event: React.KeyboardEvent<HTMLUListElement>, nodeId: string) => void;
-  rangeSelectToLast: (event: React.KeyboardEvent<HTMLUListElement>, nodeId: string) => void;
+  rangeSelectToFirst: (event: React.KeyboardEvent, nodeId: string) => void;


Was always fired by the Tree View component
But it was not needed an UL specifically so I reduced the type precision

flaviendelangle · 2024-02-27T13:25:04Z

packages/x-tree-view/src/internals/plugins/useTreeViewNodes/useTreeViewNodes.ts

-      .filter((node) => node.parentId === nodeId)
-      .sort((a, b) => a.index - b.index)
-      .map((child) => child.id),
+  const getChildrenIds = React.useCallback(


useEventCallback is not compatible with methods fired in the render because they are always outdated (since the update is made in a useLayoutEffect.

This method is now needed to compute the tabbable node.

flaviendelangle · 2024-02-27T13:25:32Z

...ee-view/src/internals/plugins/useTreeViewKeyboardNavigation/useTreeViewKeyboardNavigation.ts

-      // If the tree is empty there will be no focused node
-      if (event.altKey || event.currentTarget !== event.target || state.focusedNodeId == null) {
-        return;
+    const ctrlPressed = event.ctrlKey || event.metaKey;


All the changes below just replace state.focusedNodeId with nodeId

flaviendelangle · 2024-02-27T13:26:39Z

packages/x-tree-view/src/internals/plugins/useTreeViewFocus/useTreeViewFocus.ts

+    return node && (node.parentId == null || instance.isNodeExpanded(node.parentId));
+  };
+
+  let tabbableNodeId: string | null | undefined;


Do you think this is worth memoizing?

flaviendelangle · 2024-02-27T13:28:47Z

packages/x-tree-view/src/TreeItem/TreeItem.test.tsx

-        expect(getByTestId('one')).toHaveVirtualFocus();
-      });
-
-      it('should focus the selected node if a node is selected before the tree receives focus', () => {


Replaced by should set tabIndex={0} on the selected item (multi select) and should set tabIndex={0} on the selected item

flaviendelangle · 2024-02-27T14:08:10Z

packages/x-tree-view/src/TreeItem/TreeItem.test.tsx

-        expect(getByTestId('parent')).toHaveVirtualFocus();
-      });
-
-      it('should focus on tree with scroll prevented', () => {


Don't make sense anymore

flaviendelangle · 2024-02-27T14:08:49Z

packages/x-tree-view/src/TreeItem/TreeItem.test.tsx

-          expect(getByTestId('two')).toHaveVirtualFocus();
-        });
-
-        it('should be focused on tree focus', () => {


Replaced by should not prevent programmatic focus

github-actions · 2024-02-28T12:29:23Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

flaviendelangle · 2024-02-28T13:44:08Z

packages/x-tree-view/src/internals/plugins/useTreeViewFocus/useTreeViewFocus.types.ts

 }
 export interface UseTreeViewFocusPublicAPI {
-  focusNode: (event: React.SyntheticEvent, nodeId: string | null) => void;


@noraleonte with the introduction of focusDefaultNode, do we need to support null here?

flaviendelangle · 2024-02-28T13:45:00Z

packages/x-tree-view/src/internals/plugins/useTreeViewFocus/useTreeViewFocus.types.ts

+  isTreeViewFocused: () => boolean;
+  canNodeBeTabbed: (nodeId: string) => boolean;
+  focusNode: (event: React.SyntheticEvent, nodeId: string) => void;
+  focusDefaultNode: (event: React.SyntheticEvent | null) => void;


I'm adding null on the event so that when you remove the focused node, it can switch to another node (was already the case before) AND call onNodeFocus for this new node (was not the case before).

github-actions · 2024-03-06T09:55:04Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2024-03-11T08:13:02Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2024-03-12T13:08:40Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

LukasTy

Great work, looks and works nicely! 👏

docs/data/migration/migration-tree-view-v6/migration-tree-view-v6.md

LukasTy · 2024-03-14T14:15:01Z

packages/x-tree-view/src/SimpleTreeView/SimpleTreeView.test.tsx

      </SimpleTreeView>,
    );
    act(() => {
-      getByRole('tree').focus();
+      getByTestId('one').focus();


Nitpick: It seems possible to avoid the usage of testId by keeping the getByRole.
We could add the explicit "name" field if we so desire extra stability.

Nitpick2: Is there a specific reason why we are using the methods de-structured from the render function instead of the root import of screen? 🤔

Relates a lot to #12428

Nitpick: It seems possible to avoid the usage of testId by keeping the getByRole.
We could add the explicit "name" field if we so desire extra stability.

It's mostly for consistency across the tests, the vast majority uses testId.
I think we can align on a single approach in #12428 and use it everywhere while migrating all the tests to describeTreeView.

Nitpick2: Is there a specific reason why we are using the methods de-structured from the render function instead of the root import of screen?

screen is the advised way by the created or react-testing-library from what I remember, so no specific reason.
This topic will go away with #12428 if we create the right abstractions (which I think we should for readability).

It's mostly for consistency across the tests, the vast majority uses testId.

getByRole has ~2x more usages than getByTestId (and almost all of them are on TreeView tests). 🤔
IMHO, it would be best to use getByTestId when there is no other way to select elements. 🙈

🤔 not sure I have the same results 😆

(and all getByRole are for tree AFAIK)

Oh, I was referring to the global usage in the mui-x codebase, sorry for the confusion. 🙈

packages/x-tree-view/src/SimpleTreeView/SimpleTreeView.test.tsx

LukasTy · 2024-03-14T14:21:03Z

packages/x-tree-view/src/SimpleTreeView/SimpleTreeView.test.tsx

      </SimpleTreeView>,
    );
    act(() => {
-      getByRole('tree').focus();
+      getByTestId('one').focus();


The same question/nitpick regarding the usage of getByRole applies here and in the whole test file. 🤔

packages/x-tree-view/src/SimpleTreeView/SimpleTreeView.test.tsx

LukasTy · 2024-03-14T14:30:45Z

packages/x-tree-view/src/TreeItem/TreeItem.test.tsx

      </SimpleTreeView>,
    );

    act(() => {
-      getByRole('tree').focus();
+      getByTestId('one').focus();


A friendly suggestion just like in the other test file to prefer using screen, getByRole and saving the result of getByRole in a variable to reuse instead of calling it multiple times.

packages/x-tree-view/src/TreeItem/TreeItem.test.tsx

packages/x-tree-view/src/internals/plugins/useTreeViewFocus/useTreeViewFocus.ts

...ee-view/src/internals/plugins/useTreeViewKeyboardNavigation/useTreeViewKeyboardNavigation.ts

packages/x-tree-view/src/internals/plugins/useTreeViewFocus/useTreeViewFocus.ts

github-actions · 2024-03-14T17:31:27Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

noraleonte

Looks and works great! Nice improvement! 🎉

flaviendelangle changed the title ~~[TreeView] Set focus on the focused TreeItem instead of the Tree View~~ [TreeView] Set focus on the focused Tree Item instead of the Tree View Feb 27, 2024

flaviendelangle self-assigned this Feb 27, 2024

flaviendelangle added breaking change component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! labels Feb 27, 2024

flaviendelangle force-pushed the focus-item branch 3 times, most recently from daac4c2 to 302ee63 Compare February 27, 2024 13:23

flaviendelangle force-pushed the focus-item branch 4 times, most recently from f0eae01 to 40f6c9f Compare February 27, 2024 14:19

flaviendelangle commented Feb 27, 2024

View reviewed changes

flaviendelangle mentioned this pull request Feb 27, 2024

[TreeView] Add public API and expose focus method #12143

Merged

1 task

[TreeView] Set focus on the focused TreeItem instead of the Tree View

2558231

flaviendelangle force-pushed the focus-item branch from 40f6c9f to 2558231 Compare February 27, 2024 14:30

flaviendelangle added 2 commits February 28, 2024 10:08

Merge branch 'next' into focus-item

1714d8f

Add BC

6a0f860

flaviendelangle requested review from LukasTy and noraleonte February 28, 2024 09:19

flaviendelangle added 2 commits February 28, 2024 11:07

Merge branch 'next' into focus-item

f1e6613

Merge branch 'next' into focus-item

968f9ce

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

Merge

f288225

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 28, 2024

flaviendelangle added 2 commits February 28, 2024 14:41

Fix

3060ce3

Fix

c4b4aa2

flaviendelangle commented Feb 28, 2024

View reviewed changes

Fix

1763bf6

Doc review: Olivier

b38f3ec

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

Merge

2c3a705

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 6, 2024

flaviendelangle mentioned this pull request Mar 7, 2024

[TreeView] Introduce a new TreeItem2 component and a new useTreeItem2 hook #11721

Merged

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

Merge

30d502e

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 11, 2024

flaviendelangle added 2 commits March 11, 2024 09:49

Fix

5f649c3

Add new focus on useTreeItem2

3544711

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

Merge

35f1725

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 13, 2024

Fix

9583704

LukasTy approved these changes Mar 14, 2024

View reviewed changes

Review: Lukas

bf4c891

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

Merge

19447ec

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 18, 2024

Fix

4a31e2d

noraleonte approved these changes Mar 18, 2024

View reviewed changes

LukasTy mentioned this pull request Mar 19, 2024

[TreeView] Investigate if aria-activedescendant attribute is necessary #12494

Closed

flaviendelangle merged commit eeb9603 into mui:next Mar 19, 2024
17 checks passed

flaviendelangle deleted the focus-item branch March 19, 2024 12:49

This was referenced May 16, 2024

[tree view] Cleanup onKeyDown handler #11481

Merged

[TreeView] onKey handling should be configurable #9962

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TreeView] Set focus on the focused Tree Item instead of the Tree View #12226

[TreeView] Set focus on the focused Tree Item instead of the Tree View #12226

flaviendelangle commented Feb 27, 2024 •

edited

mui-bot commented Feb 27, 2024 •

edited

flaviendelangle Feb 27, 2024

flaviendelangle Feb 27, 2024

flaviendelangle Feb 27, 2024

flaviendelangle Feb 27, 2024

flaviendelangle Feb 27, 2024

flaviendelangle Feb 27, 2024

flaviendelangle Feb 27, 2024

flaviendelangle Feb 27, 2024

github-actions bot commented Feb 28, 2024

flaviendelangle Feb 28, 2024

flaviendelangle Feb 28, 2024

github-actions bot commented Mar 6, 2024

github-actions bot commented Mar 11, 2024

github-actions bot commented Mar 12, 2024

LukasTy left a comment

LukasTy Mar 14, 2024

flaviendelangle Mar 14, 2024

LukasTy Mar 15, 2024

flaviendelangle Mar 15, 2024

flaviendelangle Mar 15, 2024

LukasTy Mar 15, 2024

LukasTy Mar 14, 2024

LukasTy Mar 14, 2024

github-actions bot commented Mar 14, 2024

noraleonte left a comment

		@@ -0,0 +1,14 @@
		// https://www.abeautifulsite.net/posts/finding-the-active-element-in-a-shadow-root/
		export const getActiveElement = (root: Document \| ShadowRoot = document): Element \| null => {

[TreeView] Set focus on the focused Tree Item instead of the Tree View #12226

[TreeView] Set focus on the focused Tree Item instead of the Tree View #12226

Conversation

flaviendelangle commented Feb 27, 2024 • edited

Changelog

Breaking changes

mui-bot commented Feb 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 6, 2024

github-actions bot commented Mar 11, 2024

github-actions bot commented Mar 12, 2024

LukasTy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 14, 2024

noraleonte left a comment

Choose a reason for hiding this comment

flaviendelangle commented Feb 27, 2024 •

edited

mui-bot commented Feb 27, 2024 •

edited