[tree view] Cleanup `onKeyDown` handler #11481

flaviendelangle · 2023-12-21T11:21:57Z

Closes #10577
Closes #10397
Closes #9962

Improve the overall readability of the function
Support defaultMuiPrevented
Do not stop the propagation of the event
Only prevent the default behavior when we really use the event for some custom behavior

mui-bot · 2023-12-21T11:28:55Z

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

Generated by 🚫 dangerJS against b8a3dfd

github-actions · 2024-01-02T08:25:30Z

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

LukasTy

Nice improvement. 👍
LGTM, but not approving yet before discussing the defaultMuiPrevented topic.

LukasTy · 2024-01-08T14:36:28Z

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

+          });
+          fireEvent.keyDown(getByRole('tree'), { key: 'Enter' });
+
+          expect(getByTestId('one')).to.have.attribute('aria-selected');


Nitpick: have you considered using getByRole('treeitem') instead? 🤔

I kept the DX used on the other tests, but I did not think about the alternative tbh

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

LukasTy · 2024-01-08T17:06:07Z

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

      otherHandlers.onKeyDown?.(event);

-      let flag = false;
-      const key = event.key;
+      if (event.defaultMuiPrevented) {


Do you think that this feature will be transparent enough for the users? 🤔
Have you considered checking event.defaultPrevented instead?
It seems that in cases, where this behavior is supported, we do explicitly document it. 🤔
If we go with defaultMuiPrevented, do we agree that it should be explicitly documented?

defaultMuiPrevented is a pattern that a several recent core components uses and the grid as well.
I think the reasoning behind it is to be able to distinguish browser behavior and MUI behavior. To let people say "hey I want the browser behavior, but not the MUI one".

With that being said, I can't find any doc about it except in this tech file: https://github.com/mui/material-ui/blob/master/packages/mui-base/CONTRIBUTING.md
Once the pattern is widely used in the MUI codebase, it should probably have a dedicated doc section somewhere.
And the Tree View onKeyDown event is probably common enough to justify a doc examples as well.

Maybe @michaldudak has more context here

AFAIK Material UI uses this pattern in several places, and I adopted it in Base UI as well (it was necessary to have an option to prevent just the library code from executing).
I agree about the docs lacking in this area. I'm making a note to update them.

@flaviendelangle If you think that not providing an explicit documentation section about this possibility in the current state of all products documentation is fine, I'm not blocking the PR merging, but IMHO, such a section wouldn't hurt. 😉

I was wondering if this shouldn't be addressed at the API page level.
To have the onKeyDown prop listed here with a description that sends to a MUI-wide page explaining how to use defaultMuiPrevented.

If we want to do it on the product pages, we could create a new "Interaction" pages on both SimpleTreeView and RichTreeView and add a section "Customize the keyboard behaviors".
Or we wait for the "Accessibility" page and we put it there since it will be the page that explains the keyboard behavior in depth.

What do you think?

Good points. 👍
The first suggestion sounds the best, but AFAIK, it's more or less a future solution.
I'm not against just waiting a bit until there is a common page that we can link to. 🤔

Adding this info to the Accessibility page is also an option, but I'd argue that it is not the best place for such content. 🤔
We could link to the above-mentioned page there. 💡

I'm not the biggest fan of the Interaction page suggestion if it would only include this information, but IMHO it's a better alternative than the Accessibility page. 🤔

We could re-purpose the Accessibility page to be more a Keyboard usage page (this name is terrible though).

Tbh I did not add this doc section because I did not find a good place to put it 😆

I'll merge without a doc, we can discuss later what we want to do 👍

noraleonte

Nice improvement! 🎉 The changes make sense and overall looks a lot cleaner 👍

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

LukasTy · 2024-01-09T12:36:31Z

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

      otherHandlers.onKeyDown?.(event);

-      let flag = false;
-      const key = event.key;
+      if (event.defaultMuiPrevented) {


@flaviendelangle If you think that not providing an explicit documentation section about this possibility in the current state of all products documentation is fine, I'm not blocking the PR merging, but IMHO, such a section wouldn't hurt. 😉

ilbrando · 2024-05-16T09:23:38Z

It appears to me that the prop defaultMuiPrevented is not part of the typings for onKeyDown. Am I right about this, or am I missing something? Intellisence gives me this: DOMAttributes<HTMLUListElement>.onKeyDown?: KeyboardEventHandler<HTMLUListElement> | undefined

flaviendelangle · 2024-05-16T10:03:31Z

Our key down handler is called by the item component since #12226)
I will update the typing of TreeItem2Props and TreeItemProps to respect the event handlers that support defaultMuiPrevented.
I will comment also #9962 with the updated example because the current one no longer works.

flaviendelangle · 2024-05-16T10:15:11Z

#13152

flaviendelangle self-assigned this Dec 21, 2023

flaviendelangle force-pushed the refacto-onKeyDown branch 2 times, most recently from c5fe0de to 0f66d83 Compare December 21, 2023 11:24

flaviendelangle mentioned this pull request Dec 21, 2023

Don't hijack CTRL+ENTER #11358

Closed

[tree view] Cleanup onKeyDown

2245baa

flaviendelangle force-pushed the refacto-onKeyDown branch from 0f66d83 to 2245baa Compare December 21, 2023 11:33

flaviendelangle added the component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! label Dec 21, 2023

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

Merge

c99ac07

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

flaviendelangle added 2 commits January 2, 2024 09:45

Fix

abce9b0

Merge branch 'next' into refacto-onKeyDown

07e4e5d

flaviendelangle force-pushed the refacto-onKeyDown branch from 199c2e7 to 07e4e5d Compare January 2, 2024 09:30

flaviendelangle marked this pull request as ready for review January 2, 2024 10:32

flaviendelangle requested review from LukasTy and noraleonte January 2, 2024 10:32

LukasTy reviewed Jan 8, 2024

View reviewed changes

noraleonte approved these changes Jan 9, 2024

View reviewed changes

...ee-view/src/internals/plugins/useTreeViewKeyboardNavigation/useTreeViewKeyboardNavigation.ts Show resolved Hide resolved

flaviendelangle added 2 commits January 9, 2024 13:29

Merge branch 'next' into refacto-onKeyDown

df1e230

Code review: Lukas and Nora

1293d31

flaviendelangle requested a review from LukasTy January 9, 2024 12:30

LukasTy approved these changes Jan 9, 2024

View reviewed changes

flaviendelangle added 2 commits January 10, 2024 08:43

Merge branch 'next' into refacto-onKeyDown

966df65

Next

b8a3dfd

flaviendelangle merged commit 0946a1b into mui:next Jan 10, 2024
17 checks passed

flaviendelangle deleted the refacto-onKeyDown branch January 10, 2024 08:00

flaviendelangle mentioned this pull request Jan 17, 2024

[TreeView] onKey handling should be configurable #9962

Closed

2 tasks

flaviendelangle mentioned this pull request May 16, 2024

[TreeView] Improve the typing of the cancelable events #13152

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tree view] Cleanup `onKeyDown` handler #11481

[tree view] Cleanup `onKeyDown` handler #11481

flaviendelangle commented Dec 21, 2023 •

edited

Loading

mui-bot commented Dec 21, 2023 •

edited

Loading

github-actions bot commented Jan 2, 2024

LukasTy left a comment

LukasTy Jan 8, 2024

flaviendelangle Jan 8, 2024

LukasTy Jan 8, 2024

flaviendelangle Jan 8, 2024

flaviendelangle Jan 8, 2024

michaldudak Jan 8, 2024

LukasTy Jan 9, 2024

flaviendelangle Jan 9, 2024

LukasTy Jan 9, 2024

flaviendelangle Jan 9, 2024

flaviendelangle Jan 10, 2024

noraleonte left a comment

LukasTy Jan 9, 2024

ilbrando commented May 16, 2024

flaviendelangle commented May 16, 2024

flaviendelangle commented May 16, 2024

[tree view] Cleanup onKeyDown handler #11481

[tree view] Cleanup onKeyDown handler #11481

Conversation

flaviendelangle commented Dec 21, 2023 • edited Loading

mui-bot commented Dec 21, 2023 • edited Loading

github-actions bot commented Jan 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noraleonte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilbrando commented May 16, 2024

flaviendelangle commented May 16, 2024

flaviendelangle commented May 16, 2024

[tree view] Cleanup `onKeyDown` handler #11481

[tree view] Cleanup `onKeyDown` handler #11481

flaviendelangle commented Dec 21, 2023 •

edited

Loading

mui-bot commented Dec 21, 2023 •

edited

Loading