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(FloatingMenu): conditionally set focus in floating menu on menu open #5489

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Feb 28, 2020

Closes #1264
Closes #5365
Closes #5488
related #4414

depends on #5456

This PR adds keyboard navigation features to our floating menus within the confines of the current component architecture.

Changelog

New

Changed

  • consolidate shared keyboard navigation logic for overflow menu and interactive tooltip in floating menu code
  • deprecate primaryFocus in OverflowMenuItem (which followed vanilla library pattern) in favor of CCR backwards compatible selectorPrimaryFocus

Testing / Reviewing

Ensure floating menu contents are keyboard navigable. Ensure keyboard navigation flow in floating menu components is as expected.

@emyarod emyarod requested a review from asudoh February 28, 2020 19:05
@emyarod emyarod requested a review from a team as a code owner February 28, 2020 19:05
@ghost ghost requested a review from abbeyhrt February 28, 2020 19:06
@netlify
Copy link

netlify bot commented Feb 28, 2020

Deploy preview for carbon-elements ready!

Built with commit ef1d9dc

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

@netlify
Copy link

netlify bot commented Feb 28, 2020

Deploy preview for carbon-components-react ready!

Built with commit 0ec84f6

https://deploy-preview-5489--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Feb 28, 2020

Deploy preview for carbon-components-react ready!

Built with commit ef1d9dc

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

@joshblack
Copy link
Contributor

Would it be helpful if we made a dedicated non-modal dialog component to help out with the tooltip use-cases? Would hate to mix up the menu and dialog use-cases between tooltip and overflow menu, similarly it seems like they have enough behavioral differences that we might want to specialize.

Happy to chat about this in our Slack channel too if you want to talk approaches 👍

Separately, in terms of API design how do you feel about Reach's initialFocusRef convention over on their dialog? https://reacttraining.com/reach-ui/dialog#dialogoverlay-initialfocusref

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 - Good fix for what this PR is addressing (focus management). Thanks @emyarod!

@emyarod
Copy link
Member Author

emyarod commented Feb 28, 2020

converting the interactive tooltip to a modeless dialog may be interesting, but this PR is focused on keyboard navigation and focus management based on the current behavior of the components inheriting from floating menu. Unless there is a missing requirement (let me know if there is), I see rewriting interactive tooltip to no longer extend floating menu as outside the scope of this PR and the referenced issues

WRT the API, the selectorPrimaryFocus prop is borrowed from our current Modal/ModalWrapper/ComposedModal implementations. I think renaming this convention can be something to consider for v11

@joshblack
Copy link
Contributor

@emyarod if I understand correctly, the menu pattern for an overflow menu and the non-modal dialog pattern for our tooltip with interactive content would be distinct behaviors, right? I believe we can't close the issues that are linked for these kinds of tooltips unless we implement that behavior, but let me know if I'm misunderstanding what the reqs are 👍

@emyarod
Copy link
Member Author

emyarod commented Mar 2, 2020

@joshblack from what I can tell based on the descriptions in the referenced issues this PR would fulfill the remaining missing requirements. is there something missing that you're referencing?

@joshblack
Copy link
Contributor

@emyarod I believe if we're saying that we're closing #5488 then we would need the dialog to be rendered in the content flow instead of in a portal otherwise tab order is broken, correct?

Separately I think we should all chat (either here or on Slack) to better understand the tooltip requirements because if it is a dialog then, in theory, it should have a focus trap, right? Not sure how being a non-modal dialog change this behavior though.

@emyarod
Copy link
Member Author

emyarod commented Mar 2, 2020

@joshblack this PR should handle all of those requirements except for the one about where the tooltip would be rendered. Is the render location an implied requirement (not seeing it in the issue but I assume that's part of the definition of "dialog" then)?

but if the reason for that is just for tab order, I believe the changes from #5260 should address that already. It looks like it's still functioning correctly in the uncontrolled tooltip example but broken for the default tooltip so that's just a regression in the current PR that I can address

@joshblack
Copy link
Contributor

@emyarod I would recommend starting a conversation in Slack to go over requirements, I don't think it's clear cut what we need to do for this special kind of tooltip with interactive content and might help to get us all on the same page before moving forward 👍

@emyarod emyarod force-pushed the 5365-interactive-tooltip-keyboard-navigation branch from a582b46 to 89ada54 Compare March 5, 2020 16:10
This was referenced Mar 20, 2020
@emyarod emyarod mentioned this pull request May 21, 2020
67 tasks
@emyarod emyarod force-pushed the 5365-interactive-tooltip-keyboard-navigation branch 2 times, most recently from 2d9b487 to 0786687 Compare May 29, 2020 08:58
@emyarod emyarod requested a review from a team as a code owner May 29, 2020 09:03
@emyarod emyarod requested review from dakahn and joshblack and removed request for abbeyhrt May 29, 2020 14:03
@emyarod emyarod force-pushed the 5365-interactive-tooltip-keyboard-navigation branch from df4727b to ef1d9dc Compare June 1, 2020 16:49
@emyarod emyarod merged commit 3e1a8f0 into carbon-design-system:master Jun 1, 2020
@emyarod emyarod deleted the 5365-interactive-tooltip-keyboard-navigation branch June 1, 2020 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants