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

feat(Tooltip): allow focusTrap prop passthrough #6722

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Aug 25, 2020

Closes #6719

Related #6458

This PR allows users to set the focusTrap prop on Tooltikps to passthrough to the underlying FloatingMenu. A check for the focusTrap prop is also added to the FloatingMenu to prevent moving focus away from tooltip trigger elements

Changelog

New

  • focusTrap prop on Tooltip to control the underlying FloatingMenu

Changed

  • check focusTrap prop before calling focus method in FloatingMenu

Testing / Reviewing

Confirm that tooltips do not steal focus away from focusable trigger elements when focusTrap is false

@emyarod emyarod requested a review from a team as a code owner August 25, 2020 17:02
@emyarod emyarod force-pushed the 6719-uncontrolled-tooltip-focus-behavior branch from 3796d1f to 71b0ac5 Compare August 25, 2020 17:03
@netlify
Copy link

netlify bot commented Aug 25, 2020

Deploy preview for carbon-elements ready!

Built with commit 910423a

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

@netlify
Copy link

netlify bot commented Aug 25, 2020

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 910423a

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

@joshblack joshblack mentioned this pull request Aug 27, 2020
38 tasks
@emyarod emyarod requested a review from a team August 31, 2020 14:40
@ghost ghost requested review from joshblack and removed request for a team August 31, 2020 14:44
@joshblack
Copy link
Contributor

Would the intended behavior here be that if the dialog doesn't trap focus (nonmodal dialog) that it should stay in tab order? Similarly, if we are using role="dialog" should we be shifting the focus still to the first focusable element?

Side-note, would this be a good candidate for revisiting with the nested roles? (tooltip -> dialog)

@emyarod
Copy link
Member Author

emyarod commented Aug 31, 2020

yes it should stay in tab order when the focus trap behavior is disabled. we can remove the role attribute when the focus trap is disabled if that's what you mean. and yes this should probably be a use case to consider in those discussions (I'm out of the loop on what explorations have been done)

@joshblack
Copy link
Contributor

I'm out of the loop on what explorations have been done

Honestly there hasn't been much outside of conversations on PRs like this one haha

yes it should stay in tab order when the focus trap behavior is disabled

Makes sense, was trying to reproduce in the storybook but was having trouble tabbing into the content:

demo

we can remove the role attribute when the focus trap is disabled if that's what you mean

I believe if we have something with role="dialog" we need to shift focus into the dialog, whether or not it traps focus in the dialog.

@emyarod
Copy link
Member Author

emyarod commented Sep 1, 2020

I'm not sure about how your example looks but there is an uncontrolled tooltip example in the storybook. This prop was only meant to be disabled in the uncontrolled component scenario, since by default we are trapping focus right? To avoid that confusion I guess we can remove the prop from the default story maybe. As for the role attribute, what would be the verdict on that? it sounds like it should be removed when focus trap is disabled

@joshblack
Copy link
Contributor

@emyarod just was noting that for this story (which I think is the uncontrolled one) that when focusTrap is false in knobs you can't tab into the content of the tooltip. I think whether or not we trap focus that we should keep the contents of the popup in tab order/reachable by keyboard.

@emyarod
Copy link
Member Author

emyarod commented Sep 21, 2020

right that's what I was referring to with my earlier comment

This prop was only meant to be disabled in the uncontrolled component scenario, since by default we are trapping focus right? To avoid that confusion I guess we can remove the prop from the default story

this prop is only relevant in the controlled example so should I just remove it entirely from the uncontrolled example?

@joshblack
Copy link
Contributor

Got it, sounds good! Sorry about the confusion 😄

@tw15egan
Copy link
Collaborator

@emyarod I was unsure if you wanted to add anything else to this PR. Looks good on my end, merge when ready 👍

@emyarod emyarod force-pushed the 6719-uncontrolled-tooltip-focus-behavior branch from 85fc253 to 9a937ec Compare September 23, 2020 21:53
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 focus grab is breaking previous usages of Tooltip
3 participants