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

fix(Tooltip): prevent focus hijacking by other tooltips with focus trap #8713

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented May 18, 2021

Closes #8653
related #7260
related #7319

#7260 reported focus hijacking when clicking from tooltip to tooltip and was resolved in #7319. However, this only addressed the use case where the tooltips contained no focusable children. Focus would be restored to a tooltip's trigger element on loss of focus and multiple tooltips with focus traps would conflict with each other, leading to situations where multiple tooltips could be open at once. The order of the focus events was different in Firefox, but this conflict of focus events occurred in all browsers. This PR updates tooltip focus logic so that pages with multiple tooltips (particularly ones with focusable children/focus traps) will not hijack focus from the already focused elements

Changelog

New

Changed

  • tooltip focus logic on tooltip close

Testing / Reviewing

Verify that focus is applied to the expected elements when navigating pages with Carbon tooltips using the mouse or keyboard in all browsers, regardless of whether or not the tooltips contain focusable children

@netlify
Copy link

netlify bot commented May 18, 2021

Deploy preview for carbon-elements ready!

Built with commit 7c140b7

https://deploy-preview-8713--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented May 18, 2021

Deploy preview for carbon-components-react ready!

Built with commit 7c140b7

https://deploy-preview-8713--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented May 18, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: c199ae6

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/60b5233e6643880008984fed

😎 Browse the preview: https://deploy-preview-8713--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented May 18, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: c199ae6

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/60b5233ef682250008979680

😎 Browse the preview: https://deploy-preview-8713--carbon-components-react.netlify.app/iframe

only occurs if focus is not applied to another element after blur
@emyarod emyarod force-pushed the 8653-tooltip-with-focusable-children-hijacking-focus branch from 7c140b7 to 1644a75 Compare May 18, 2021 19:18
@jnm2377
Copy link
Contributor

jnm2377 commented May 18, 2021

It's working fine with mouse, but ran into an issue on keyboard, that if I tab into a non interactive tooltip then tab somewhere else, it stays open, even if I open another tooltip. With the interactive tooltip, I couldn't tab out of it bc it had the focus trap, so I had to press esc, which then closed both of the tooltips.

Screen.Recording.2021-05-18.at.2.34.55.PM.mov

@emyarod
Copy link
Member Author

emyarod commented May 18, 2021

this is the existing behavior and I believe also the expected behavior. these tooltips are more like toggletips in that they require explicit dismissal (as they are referred to as "interactive tooltips"), so the tooltip with no focusable children needs an explicit Esc keypress or clicking on a non-child element to dismiss it (as opposed to the definition and icon tooltips which will dismiss on blur)

@jnm2377
Copy link
Contributor

jnm2377 commented May 18, 2021

I'm not sure I would call that the expected behavior. I think if it doesn't have interactive children, it behaves more like an icon tooltip or definition tooltip, whose expected behavior is to be shown on hover and focus. I would expect the non interactive one to close when tabbing to another element since it no longer has focus.

@jnm2377
Copy link
Contributor

jnm2377 commented May 18, 2021

I think if it's expected to work more like a toggle tooltip, then we need to add some sort of logic so that if the focus is on the "toggle" button, and the tooltip is shown, then you can't tab out of it until you press esc to close the tooltip, or if you can tab out of it, then the tooltip should close automatically.

@jnm2377
Copy link
Contributor

jnm2377 commented May 18, 2021

cc: @dakahn any thoughts? (see comments above)

@emyarod
Copy link
Member Author

emyarod commented May 18, 2021

I was referring to the behavior for the interactive tooltip above ("They persist [sic] until intentionally dismissed by clicking outside of the tooltip") which is meant to be different from the icon tooltip (remember the interactive tooltip's trigger doesn't require an icon)

If the expected behavior should be revised in that regard, I think it can be revisited by the design team. but in any case, given that it is not a regression and is not the topic of the referenced issues, I believe its scope lies outside of the current PR

@emyarod
Copy link
Member Author

emyarod commented May 18, 2021

regarding the point about keeping focus on the trigger button if the tooltip does not contain a focusable element, I believe that is an a11y violation and would also prevent selection of text within the tooltip (e.g. #4268 #4232)

@jnm2377
Copy link
Contributor

jnm2377 commented May 18, 2021

I was referring to the behavior for the interactive tooltip above ("They persist [sic] until intentionally dismissed by clicking outside of the tooltip") which is meant to be different from the icon tooltip (remember the interactive tooltip's trigger doesn't require an icon)

If the expected behavior should be revised in that regard, I think it can be revisited by the design team. but in any case, given that it is not a regression and is not the topic of the referenced issues, I believe its scope lies outside of the current PR

Right. But if you're navigating via keyboard, you can't click outside of the tooltip. Moving focus is the equivalent of clicking outside of the tooltip for keyboard users, so shouldn't the non interactive one close? I thought the scope of this PR was to not have tooltips remain open if they lose focus (whether its via mouse or keyboard).

@jnm2377
Copy link
Contributor

jnm2377 commented May 18, 2021

regarding the point about keeping focus on the trigger button if the tooltip does not contain a focusable element, I believe that is an a11y violation and would also prevent selection of text within the tooltip (e.g. #4268 #4232)

I'm not sure what you mean. I wasn't suggesting that.

@emyarod
Copy link
Member Author

emyarod commented May 18, 2021

this PR is resolving focus hijacking described in the referenced issues but it seems that you are describing keyboard navigation behavior which doesn't appear to be a regression WRT this PR or the referenced issues. on that topic, the dismissal action for keyboard users would be with Esc

@emyarod
Copy link
Member Author

emyarod commented May 25, 2021

bump @jnm2377 when you have some time!

@kodiakhq kodiakhq bot merged commit 3b4b69a into carbon-design-system:main May 31, 2021
@emyarod emyarod mentioned this pull request Jun 1, 2021
87 tasks
@emyarod emyarod deleted the 8653-tooltip-with-focusable-children-hijacking-focus branch June 1, 2021 14:47
@jnm2377 jnm2377 mentioned this pull request Jun 9, 2021
22 tasks
emyarod added a commit to emyarod/carbon that referenced this pull request Jun 10, 2021
…ap (carbon-design-system#8713)

* fix(Tooltip): prevent focus hijacking with focusable children

* fix(Tooltip): restore focus to trigger element

only occurs if focus is not applied to another element after blur

* docs(Tooltip): add temporary test story

Co-authored-by: TJ Egan <[email protected]>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tooltip has closing issue on Firefox
4 participants