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(components): support automatic focus sentinel #5260

Merged
merged 8 commits into from
Feb 14, 2020

Conversation

asudoh
Copy link
Contributor

@asudoh asudoh commented Feb 4, 2020

This change eliminates the need for application to put focus sentinel by having <Modal>, <ComposedModal> and <FloatingMenu> automatically put the focus sentinels.

This change also add support for reverse-focus-wrap feature to <Modal> and <ComposedModal>, without needing using 3rd-party focus-trap-react library. This helps applications hitting adverse side-effects that focus-trap-react library causes (e.g. #3021, #3665 and #4600).

Fixes #3817.
Fixes #4036.
Fixes #4600.

Changelog

New

  • Automatic way to put focus sentinel for <Modal>, <ComposedModal> and <FloatingMenu>.
  • Reverse-focus-wrap feature to <Modal> and <ComposedModal> that works without needing using 3rd-party focus-trap-react library.

Testing / Reviewing

Testing should make sure <Modal>, <ComposedModal>, <OverflowMenu> and <Tooltip> are not broken.

This change eliminates the need for application to put focus sentinel
by having `<Modal>`, `<ComposedModal>` and `<FloatingMenu>`
automatically put the focus sentinels.

This change also add support for reverse-focus-wrap feature to
`<Modal>` and `<ComposedModal>`, without needing using 3rd-party
`focus-trap-react` library. This helps applications hitting adverse
side-effects that `focus-trap-react` library causes (e.g. carbon-design-system#3021, carbon-design-system#3665
and carbon-design-system#4600).

Fixes carbon-design-system#3817.
Fixes carbon-design-system#4036.
Fixes carbon-design-system#4600.
@netlify
Copy link

netlify bot commented Feb 4, 2020

Deploy preview for carbon-elements ready!

Built with commit 3ccfd96

https://deploy-preview-5260--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Feb 4, 2020

Deploy preview for carbon-components-react ready!

Built with commit 3ccfd96

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

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

looks good to me, floating menus function as expected and reverse focus wrap is also functional. seems like the ideal use case for Node.compareDocumentPosition()

@joshblack
Copy link
Contributor

cc @dakahn @snidersd if you all have a chance to confirm!

@dakahn dakahn self-requested a review February 11, 2020 16:08
*/
focusTrap: PropTypes.bool,
focusTrap: deprecate(
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're deprecating this prop, are we not going to give the user the ability to choose whether or not they want the modal focus-wrapped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for asking @abbeyhrt - Focus-wrapping behavior runs regardless of focusTrap property, even before it was introduced or after it was introduced. Some time ago the codebase introduced a third-party focus-trap-react library for better focus-wrapping behavior, but it introduced lots of side effects as the PR description of this PR links to. focusTrap property was introduced to disable such third-party library to avoid the side effects. This PR removes the property because the third-party is no longer needed for focus-wrapping.

Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

Can we get a UX example stories for the components in question? Maybe a button that when clicked throws the modals so we can easily test focus moving to and from and being trapped etc?

This would also be helpful moving forward protecting against regressions with changes like this

@asudoh
Copy link
Contributor Author

asudoh commented Feb 11, 2020

@dakahn The features the fix is for (focus wrapping for modal and making focus sequence as though floating menu body is placed next to the trigger button) can be tested at https://react.carbondesignsystem.com/?path=/story/modal--default and https://react.carbondesignsystem.com/?path=/story/overflowmenu--basic. For the former, you can keep hitting tab key or shift-tab key and see if the focus is within modal. For the latter, you can first focus on the trigger button, hit enter, and hit tab and see if the focus goes back to the trigger button.

For both, in the failed case, the focus goes to "Show info" button.

Co-Authored-By: Abbey Hart <abbeyhrt@gmail.com>
Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

Testing on Firefox latest on Windows 10 at https://react.carbondesignsystem.com/iframe.html?id=modal--default

For Modal and ComposedModal Initial focus is placed on close button then focus is moved to any subsequent interactable elements and then focus is lost before returning to the close button.

@asudoh
Copy link
Contributor Author

asudoh commented Feb 11, 2020

For Modal and ComposedModal Initial focus is placed on close button then focus is moved to any subsequent interactable elements and then focus is lost before returning to the close button.

It's the expected behavior (irrelevant to this PR). The initial focus can be changed either by the following:

  • Add data-modal-primary-focus attribute to the element that should get initial focus
  • Change selectorPrimaryFocus prop

@dakahn
Copy link
Contributor

dakahn commented Feb 11, 2020

@asudoh No, that's not what I'm talking about. I'm referring to focus being lost after the last interactable element in the modal before returning to the close button.

@dakahn
Copy link
Contributor

dakahn commented Feb 11, 2020

To clarify what I was seeing it seems that focus trap is off by default (I hadn't clicked the knob 🦆) -- wouldn't that be counter to the expected behavior? If this is what we've been doing then it could make sense to keep it that way, but I don't recall choosing to make our modal fail WCAG by default.

hasScrollingContent: false,
};

button = React.createRef();
outerModal = React.createRef();
innerModal = React.createRef();
startSentinel = React.createRef();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a clearer plain-language way we can name this as opposed to "sentinel". It's difficult jargon to even Google a simple definition of. I assume you're meaning like a sentinel node, but there's probably a friendlier way to describe that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@dakahn dakahn Feb 12, 2020

Choose a reason for hiding this comment

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

I understand the concept of sentinel elements in a focus-trap, but my question was if we could name these in plainer and simpler to understand language? firstElement and lastElement spring to mind.

If not that's cool too, but maybe a quick comment explaining the concept of sentinels to somebody who -- like myself -- has implemented wrapping focus but was totally unfamiliar with this term.

in a focus trap we have "sentinel" elements that designate the first and last elements in the tab order and signify when we should wrap

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "start trap" and "end trap"?

@asudoh
Copy link
Contributor Author

asudoh commented Feb 11, 2020

then focus is moved to any subsequent interactable elements and then focus is lost before returning to the close button.

@dakahn My apologies for misunderstanding your statement on your finding. Given you don't seem to have tested my change and testing https://react.carbondesignsystem.com/iframe.html?id=modal--default instead, I think it explains why you saw the problem.

Didn't have access to my Windows environment today, but with Firefox 73 on my Mac, the focus-wrap behavior seems to work as expected with this PR's code (I don't observe "then focus is lost before returning to the close button"). e.g. at http://localhost:9000/iframe.html?id=composedmodal--example-usage-with-trigger-button, hitting tab key when the Save button has focus moves the focus to the close button, and hitting shift-tab key when the close button has focus moves the focus to the Save button:

focus-wrap

Same (correct) behavior is observed at http://localhost:9000/iframe.html?id=modal--default.

To clarify it seems that focus trap is off by default -- wouldn't that be counter to the expected behavior?

The change makes the focus-wrap behavior always, by making focus-wrap behavior work without third-party focus-trap-react library. To do that, introduced some new code (e.g. wrapFocus.js) for focus-wrap behavior.

@dakahn
Copy link
Contributor

dakahn commented Feb 12, 2020

The focus wrap behavior is off in the story by default for modal and composed modal. That's what I'm seeing at the testing link, which is where the confusion came from. My question was is this intentional?

@asudoh
Copy link
Contributor Author

asudoh commented Feb 13, 2020

I forgot removing focusTrap property from the story (my apologies), fixed. This PR is for making focus-wrapping behavior work regardless of that property.

Also renamed startSentinelNode to startTrapNode as suggested.

@aledavila
Copy link
Contributor

Question since this PR is about focus. Have you tested these changes on iOS. The focus trap still doesn't seem to work when using voice over in safari.

@emyarod
Copy link
Member

emyarod commented Feb 13, 2020

I don't have an iOS device to test for this PR but the iOS issue is tracked separately over in #2739 and doesn't seem to be referenced in this PR. not sure if the fix for that can be rolled into the current PR

@asudoh
Copy link
Contributor Author

asudoh commented Feb 13, 2020

@emyarod is right - This is solely about keyboard focus, not about screen reader cursor (focus).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants