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.

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

[a11y]: Tearsheet selectorPrimaryFocus broken #5152

Closed
2 tasks done
wkeese opened this issue May 13, 2024 · 3 comments · Fixed by #5264
Closed
2 tasks done

[a11y]: Tearsheet selectorPrimaryFocus broken #5152

wkeese opened this issue May 13, 2024 · 3 comments · Fixed by #5264
Assignees
Labels
component: Tearsheet Sev 3 Visible or noticeable to users but does not impede functionality. Has a workaround. type: a11y ♿ Issues not following accessibility standards type: bug 🐛 Something isn't working

Comments

@wkeese
Copy link
Contributor

wkeese commented May 13, 2024

Package

@carbon/ibm-products

Browser

Chrome

Operating System

MacOS

Package version

v2.37.0

React version

v18

Automated testing tool and ruleset

manual tests and jest tests

Assistive technology

no

Description

selectorPrimaryFocus is broken with the latest @carbon/ibm-products. It regressed somewhere between 2.15 and 2.37. (It was working in 2.15 except for #3988.)

ComposedModal.js correctly sets focus to the element specified by selectorPrimaryFocus.

But immediately after that, TearsheetShell's handleStackChange.claimFocus() code moves focus to the first element, i.e. to the Tearsheet’s close button.

TearsheetShell.js also has some other code in useEffect() to do that.

All this code breaks the work done by ComposedModal.js.

WCAG 2.1 Violation

No response

Reproduction/example

https://codesandbox.io/p/live/7215b329-7aa2-4c32-8858-cb36287ce0ef
https://codesandbox.io/p/devbox/tearsheet-example-wrq59h

Steps to reproduce

Just click the "open tearsheet" button and watch where focus goes. It should go to the <a> but it goes to the close button instead.

The key facets of the test case are:

  1. hasCloseIcon={true} - This makes the first focusable element the close icon.
  2. selectorPrimaryFocus="a" - This tells Tearsheet that it should focus the <a> instead of the close icon.
  3. Make Tearsheet content include Focus should go <a href="#">here</a>

Code of Conduct

@wkeese wkeese added the type: a11y ♿ Issues not following accessibility standards label May 13, 2024
@matthewgallo
Copy link
Member

Hey @wkeese, thanks for the detailed issue, very helpful for us! The code sandbox included above might have permission issues, I get a 404 when trying to access it. I also am not seeing the issues with selectorPrimaryFocus. The first story for the tearsheet includes selectorPrimaryFocus and the focus moves directly to the input that was passed to that prop.

Can you double check the code sandbox permissions? Maybe that will highlight the difference in behavior, thanks!

@wkeese
Copy link
Contributor Author

wkeese commented May 17, 2024

Woops, does https://codesandbox.io/p/devbox/tearsheet-example-wrq59h work for you?

The problem with https://carbon-for-ibm-products.netlify.app/?path=/story/ibm-products-components-tearsheet--tearsheet&globals=viewport:basic is that it doesn't have a close button (in the upper right), so it's not a good test.

Also, here's the code from TearsheetShell.tsx:

    // Callback to give the tearsheet the opportunity to claim focus
    handleStackChange.claimFocus = function () {
      firstElement?.focus();
    };

It should be clear how that's focusing the first element rather than selectorPrimaryFocus (assuming they are different). So at best, you have a race condition.

@matthewgallo
Copy link
Member

Ahh yes, it works now. Right, forgot that story excludes the close icon. Thanks for the extra clarification @wkeese

@matthewgallo matthewgallo self-assigned this May 17, 2024
@matthewgallo matthewgallo added type: bug 🐛 Something isn't working Sev 3 Visible or noticeable to users but does not impede functionality. Has a workaround. and removed status: waiting for author 💬 labels May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Tearsheet Sev 3 Visible or noticeable to users but does not impede functionality. Has a workaround. type: a11y ♿ Issues not following accessibility standards type: bug 🐛 Something isn't working
Projects
Status: Done 🚀
Development

Successfully merging a pull request may close this issue.

3 participants