-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix(Tooltip): prevent focus hijacking on tooltip dismissal #7319
fix(Tooltip): prevent focus hijacking on tooltip dismissal #7319
Conversation
197d78d
to
881fed9
Compare
Deploy preview for carbon-elements ready! Built with commit 197d78d |
Deploy preview for carbon-components-react ready! Built with commit 197d78d https://deploy-preview-7319--carbon-components-react.netlify.app |
✔️ Deploy preview for carbon-elements ready! 🔨 Explore the source changes: aaab77b 🔍 Inspect the deploy logs: https://app.netlify.com/sites/carbon-elements/deploys/5fd106491b2e8e0007911dc3 😎 Browse the preview: https://deploy-preview-7319--carbon-elements.netlify.app |
Deploy preview for carbon-components-react ready! Built without sensitive environment variables with commit aaab77b https://deploy-preview-7319--carbon-components-react.netlify.app |
added a temporary Storybook test to make testing easier (can be removed before merge) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the quick story for testing, LGTM 👍 ✅
There's odd behavior when tabbing (or shift+tabbing) to a tooltip trigger: every second time the trigger gets focus, the tooltip doesn't open. This only happens with tooltips without focusable content. To see this (I saw this in Windows Chrome/Firefox and macOS Safari - probably happens everywhere):
I think it's important to understand what's going on here. :) |
e2b4478
to
2cdc12b
Compare
@carmacleod I believe the issue is resolved now |
Much better, @emyarod - thanks! Question: Do people usually put these info tooltips inside a form control's label element? Is that a thing? It kind of messes with the focus, because the label content is a click target for the form control, so clicking on either the label text or the info icon focuses the form control, but then focus immediately moves to the info "button", so there's a focus flash that looks positively weird. We really need to get together and seriously design out all of the Carbon tooltips and make sure all use cases are covered. |
yeah I think we can address that in a separate issue as it's existing behavior and not a regression caused by the current PR @carmacleod
I don't think it's a pattern that Carbon endorses but I have seen examples of it before |
2cdc12b
to
0feefa6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0feefa6
to
3cce435
Compare
Closes #7260
ref #7165
This PR prevents the tooltip from hijacking keyboard focus after dismissing the tooltip. We check whether or not the tooltip contains focusable content, and if it doesn't then we can preserve the existing tab order. otherwise if it does contain focusable content, we will place focus back on the trigger button (like we do currently)
Changelog
Changed
Removed
Testing / Reviewing
Confirm the reported focus issues are no longer present, and that there are no regressions with tooltip focus management