mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.12k stars 1.28k forks source link

[fields] Arrow editing is allowed on a disabled field with `enableAccessibleFieldDOMStructure` property #13387

Closed CodeforEvolution closed 2 months ago

CodeforEvolution commented 4 months ago

Steps to reproduce

Link to live example: https://codesandbox.io/p/devbox/vs6gf3?migrateFrom=jtvcqg

Steps:

  1. Click on MM, DD, or YYYY
  2. Press on the up or down arrow keys

Current behavior

Despite the DatePicker being disabled, with the enableAccessibleFieldDOMStructure property, the DatePicker will still react to the arrow keys, and even generate onChange events! This behavior is not observed when the enableAccessibleFieldDOMStructure property is not specified.

Expected behavior

The DatePicker should not react to the arrow keys and especially should not generate onChange events in the disabled state, whether the enableAccessibleFieldDOMStructure property is specified or not.

Context

I am using DatePicker in the context of Storybook for automated testing and could benefit from the enableAccessibleFieldDOMStructure property which adds easy to query aria-roles and properties.

Your environment

npx @mui/envinfo ``` System: OS: Windows 10 10.0.19045 Binaries: Node: 20.14.0 - C:\Program Files\nodejs\node.EXE npm: 10.7.0 - C:\Program Files\nodejs\npm.CMD pnpm: 9.1.2 - ~\AppData\Local\pnpm\pnpm.EXE Browsers: Edge: Chromium (125.0.2535.67) Firefox: Gecko (126.0.1) npmPackages: @emotion/react: 11.11.1 @emotion/styled: ^11.11.0 => 11.11.0 @mui/base: 5.0.0-beta.40 @mui/core-downloads-tracker: 5.15.19 @mui/icons-material: ^5.15.19 => 5.15.19 @mui/material: ^5.15.19 => 5.15.19 @mui/private-theming: 5.15.14 @mui/styled-engine: 5.15.14 @mui/system: 5.15.15 @mui/types: 7.2.14 @mui/utils: 5.15.14 @mui/x-data-grid: ^7.6.1 => 7.6.1 @mui/x-date-pickers: ^7.6.1 => 7.6.1 @mui/x-tree-view: ^7.6.1 => 7.6.1 @types/react: ^18.2.21 => 18.2.42 react: ^18.2.0 => 18.2.0 react-dom: ^18.2.0 => 18.2.0 typescript: ^5.2.2 => 5.2.2 ```

This was tested in both Microsoft Edge and Mozilla Firefox.

Search keywords: enableAccessibleFieldDOMStructure, DatePicker, DesktopDatePicker, disabled

michelengelen commented 4 months ago

Hey @CodeforEvolution could you please make the sandbox public, so we can have a look at the issue? Thanks!

CodeforEvolution commented 4 months ago

My apologies, I just updated the sandbox to be public.

LukasTy commented 4 months ago

@CodeforEvolution Thank you for reporting this problem! 🙏 The issue is clear and needs to be addressed. 👍

Just clarifying that it's both arrow editing as well as Del, PageUp, PageDown, Home, and End keys.

flaviendelangle commented 4 months ago

This should fix it:

--- a/packages/x-date-pickers/src/internals/hooks/useField/useField.ts
+++ b/packages/x-date-pickers/src/internals/hooks/useField/useField.ts
@@ -188,7 +188,7 @@ export const useField = <
       case ['ArrowUp', 'ArrowDown', 'Home', 'End', 'PageUp', 'PageDown'].includes(event.key): {
         event.preventDefault();

-        if (readOnly || activeSectionIndex == null) {
+        if (disabled || readOnly || activeSectionIndex == null) {
           break;
         }

But maybe we want to block all keyboard interactions when disabled

--- a/packages/x-date-pickers/src/internals/hooks/useField/useField.ts
+++ b/packages/x-date-pickers/src/internals/hooks/useField/useField.ts
@@ -119,6 +119,10 @@ export const useField = <
   const handleContainerKeyDown = useEventCallback((event: React.KeyboardEvent<HTMLSpanElement>) => {
     onKeyDown?.(event);

+    if (disabled) {
+      return;
+    }
+
     // eslint-disable-next-line default-case
     switch (true) {
LukasTy commented 4 months ago

Thanks for the diff @flaviendelangle. 🙏 I've added a good first issue label, because of that. 👌 It would be great to add some testing in this regard. 🤞

flaviendelangle commented 4 months ago

Yeah, we clearly lack tests if this was not spotted during the creation of the new DOM structure :grimacing:

@LukasTy do you see a reason not to add the early return?

LukasTy commented 4 months ago

do you see a reason not to add the early return?

I don't see a clear reason not to do it. 🤔

github-actions[bot] commented 2 months ago

:warning: This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue. Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

@CodeforEvolution: How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.