mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.85k stars 32.25k forks source link

[joy-ui][Accordion] Expanding breaks the AccordionDetails children tab index #40116

Closed vanessag closed 1 month ago

vanessag commented 11 months ago

Duplicates

Latest version

Steps to reproduce πŸ•Ή

Steps:

  1. Create an Accordion component with 2 inputs inside of the AccordionDetails
  2. Expand the Accordion
  3. Click the first input inside of the accordion and then press tab
  4. Observe that the focus does not go to the next input

Current behavior 😯

When I click the Accordion to expand it and then click on the first Input within my AccordionDetails then press tab it does not focus the next Input.

Expected behavior πŸ€”

I expect that when I click on the Accordion to expand it and then click on an Input that is inside of AccordionDetails that pressing tab goes to the next Input.

Context πŸ”¦

I am trying to accomplish an Accordion component that has form elements inside of it. These are advanced settings that the user can expand/collapse.

Your environment 🌎

npx @mui/envinfo ``` System: OS: macOS 14.1.1 Binaries: Node: 20.10.0 - ~/.nvm/versions/node/v20.10.0/bin/node Yarn: Not Found npm: 10.2.3 - ~/.nvm/versions/node/v20.10.0/bin/npm Browsers: Chrome: 119.0.6045.199 npmPackages: @emotion/react: ^11.11.1 => 11.11.1 @emotion/styled: ^11.11.0 => 11.11.0 @mui/base: ^5.0.0-beta.22 => 5.0.0-beta.22 @mui/core-downloads-tracker: 5.14.16 @mui/icons-material: ^5.14.16 => 5.14.16 @mui/joy: ^5.0.0-beta.13 => 5.0.0-beta.13 @mui/material: 5.14.16 @mui/private-theming: 5.14.16 @mui/styled-engine: 5.14.16 @mui/system: 5.14.16 @mui/types: 7.2.8 @mui/utils: 5.14.16 @types/react: ^18.2.34 => 18.2.34 react: ^18.2.0 => 18.2.0 react-dom: ^18.2.0 => 18.2.0 typescript: ^5.2.2 => 5.2.2 ```
ZeeshanTamboli commented 10 months ago

Why do you need inputs inside the AccordionDetails? I think I have never seen that. What is your use case? Please provide a CodeSandbox reproducing the issue.

JessiPio4cronn commented 7 months ago

Why do you need inputs inside the AccordionDetails? I think I have never seen that. What is your use case? Please provide a CodeSandbox reproducing the issue.

See e.g. this example of the Joy UI documentation.

Furthermore, this would be the recommended behaviour according to the ARIA Authoring Practices Guide.

Tab: Moves focus to the next focusable element; all focusable elements in the accordion are included in the page Tab sequence.

basti4242 commented 7 months ago

The issue seems to be, that certain elements (a, button, input, textarea, select, details) get a tabIndex of -1 when inside an AccordionDetails component. The implementation implies that this should only be the case when the accordion is closed and the regular tabIndex should be restored when the accordion is expanded again.

When I explicitly set a tabIndex of 0 for a component inside an AccordionDetails component (e.g. for a Textarea: <Textarea slotProps={{ textarea: { tabIndex: 0 } }} />, the mechanism is working. When no tabIndex is explicitly set, the tabIndex is -1 even if the accordion is expanded. I think this is a bug that should be fixed.

JessiPio4cronn commented 7 months ago

The solution of @basti4242 only seems to work, if the accordion is initially expanded (defaultExpanded is set to true). Otherwise the tabIndex and data-prev-tabindex will be set to -1 during initialisation:

https://github.com/mui/material-ui/assets/62110002/2d9aaca5-d5e1-42c7-a67d-d086104a20b7

It would be beneficial to add two more test for these cases.

  1. Test that the tab index is 0 when defaultExpanded is set to false and the accordion is expanded for the first time.
  2. Test that the tab index is 0 when the accordion is collapsed and expanded again.
jacobmoshipco commented 5 months ago

Took a look at this because I also ran into this problem.

I'm pretty sure this line needs to have the && !currentTabIndex condition removed. currentTabIndex will always be "-1" after the first render, so this will fail to remove the attribute unless it already doesn't exist.

The portion that adds the tab-index="-1" will mistakingly set data-prev-tabindex to "-1" when double-rendered, such as what happens when in react development mode. When testing this, making that previous change fixed things in production but not development.

The other issue with this partial fix is that elements with an explicit tab-index in a default expanded details will get their tab index cleared. It would probably be better to use an explicit data-prev-tabindex="unset" or something to keep track.

Shreypatel13ll commented 2 months ago

If no one is working can I take this up.

github-actions[bot] commented 1 month ago

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.

[!NOTE] We value your feedback @vanessag! How was your experience with our support team? We'd love to hear your thoughts in this brief Support Satisfaction survey. Your insights help us improve!