[EuiFocusTrap] Use `react-focus-on` #3631

thompsongl · 2020-06-17T21:44:04Z

Summary

Replace react-focus-lock with react-focus-on, an updated and preferred-by-the-shared-author take on the original.
The main improvement EUI will see is enhanced screen reader and keyboard navigation behavior for portal- and popover-based components and contexts.
For the most part, this is a 1:1 replacement, with a couple adjustments to maintain EUI's API.

Additional features include:

noIsolation: for opting into heavy-handed pointer-events control (non-standard use cases)
scrollLock: for opting into body scrolling prevention
onOutsideClick: callback that gives EUI additional options for event handling

See #502 for the initial idea and discussion.

Lots of manual testing appreciated

Draft checklist

Resolve snapshot discrepancies with disabled state
Discern and document appropriate use of noIsolation
Consider onClickOutside instead of EuiOutsideClickDetector

Checklist

~~- [ ] Check against all themes for compatibility in both light and dark modes~~
- [ ] Checked in mobile

Checked in IE11 and Firefox
Props have proper autodocs
Added documentation

~~- [ ] Checked Code Sandbox works for the any docs examples~~ Excluded

Added or updated jest tests
Checked for breaking changes and labeled appropriately
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

kibanamachine · 2020-06-17T22:05:48Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3631/

myasonik · 2020-06-18T19:03:26Z

This looks great! It's already doing what it's supposed to!

(Sorry about the janky photos, I can never get it to take a screen shot...)

With the old system, here's what a screen reader could still get to:

With the new system, screen readers are correctly constrained to just the modal:

`noIsolation` prop

Any time we're using a focus lock, I think we want noIsolation off (said another way, aria-hidden should always be set on the rest of the DOM whenever we're using a focus lock). This is the default and I wouldn't even expose an an API to change it...

Swapping true and false doesn't seem to do anything though so I investigated further and I think there's a bug in the plugin which prevents this from ever being really togglable (which, I again, I think is fine). But it seems like he's passing this as a 3rd prop into a function which doesn't do anything of value with the 3rd prop.

Nice to have...

It'd be nice if all of our portals were guarded only on the one side and allowed tabbing into the browser chrome. There's an example in react-focus-lock and the right API seems to be exposed in react-focus-on.

thompsongl · 2020-06-18T19:44:43Z

noIsolation [...] Swapping true and false doesn't seem to do anything

The lack of documentation is a bummer, but through looking at the code I've seen that noIsolation is for excluding outside components from getting pointer-events: none !important. So the SR benefits of aria-hidden are not lost, which is good.

For the most part, leaving noIsolation to the default is fine. EuiCollapsibleNav is the one component (so far) that is negatively affected. Its combo with EuiOverlayMask and EuiOutsideClickDetector would require pointer events on outside elements. We appear to be in-bounds in this case ("shadowbox" caveat) by explicitly handling outside clicks and setting noIsolation=true:

[noIsolation=false] - disables outer event capturing. Event capturing is React friendly and unlikely be a problem. But if you are using shadowbox of some sort - you dont need it.
Usage

thompsongl · 2020-06-18T21:41:48Z

@cchaos EuiCollapsibleNav has a unique approach to trapping and releasing focus, and I want to make sure I understand the intended interaction before making any changes.

What I see:

When the nav is open and undocked, clicking the overlay mask will both disable the focus trap and close the menu.
When the nav is open and undocked, clicking the header bar will temporarily remove focus, but tabbing back into the nav traps focus again. The nav does not close.

The main question is: is it intentional that clicking the header bar does not close the nav?

cchaos · 2020-06-23T13:40:11Z

Yes, at least for now until we hear back from users, the intention is to keep the nav open when they're interacting with the header.

kibanamachine · 2020-07-06T19:47:31Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3631/

kibanamachine · 2020-07-06T22:26:56Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3631/

thompsongl · 2020-07-06T23:08:14Z

jenkins test this

"PUPPETEER_SKIP_CHROMIUM_DOWNLOAD"

thompsongl · 2020-07-06T23:14:21Z

It'd be nice if all of our portals were guarded only on the one side and allowed tabbing into the browser chrome.

This likely won't make this PR. We have no guarantee that a given portal is the last thing in the DOM, so there would be some case-by-case basis decisions to make.

kibanamachine · 2020-07-06T23:32:43Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3631/

kibanamachine · 2020-07-07T16:20:42Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3631/

myasonik · 2020-07-07T20:39:40Z

In an ideal world, flyouts are just modals with a different design.

Meaning:
- ownFocus is always on
- they always handle their own scrolling and scrolling with the body is disabled

But, flyouts default to ownFocus=false and is then user customizable.

So, at this point, I think it makes sense for ownFocus and the scroll stuff to work in tandem. If ownFocus=true we disable scroll on the body and make sure modals (and flyouts and super long popovers with ownFocus) can scroll inside themselves. If ownFocus=false we probably just leave the body scroll alone.

thompsongl · 2020-07-07T23:43:06Z

Confirmed that disabling scroll locking is easy (scrollLock=false), but noticed other places where scroll prevention could be discussed:

EuiPopover w/ ownFocus=true
EuiDataGrid cells (similar to above)

chandlerprall · 2020-07-08T15:51:55Z

Thoughts only

As EuiFocusTrap is an existing component, I have a reservation about making scroll locking and to a lesser extent isolation being on by default. I confirmed with a long focus-trapping form that tabbing between elements always scrolls between them, but it isn't possible to navigate to them otherwise. We don't know and can't test everywhere these trappings are being used in Kibana, let alone other projects.

For components we manage like flyout & modal, I think there's a strong argument for enabling those values. Otherwise, I worry a bit about push back and complaints - "everyone was happy with this until EUI changed" - even if, ultimately, this is the right direction.

thompsongl · 2020-07-08T16:16:14Z

Good points, @chandlerprall

As a utility component that does not prescribe DOM context, perhaps we should default to scrollLock=false and noIsolation=true for EuiFocusTrap. For EuiModal, EuiFlyout, EuiCollapsibleNav where usage guidelines exist, we apply scroll and pointer-event prevention as needed.

chandlerprall · 2020-07-08T17:18:38Z

Thought some more on the defaults, and I think my primary resistance to isolation & scroll lock is they seem unexpected to the user outside of modal/flyout experiences. If we could add a visual (and likely? screen reader) indication about the active focus lock then I'd be much more okay with that experience, e.g. something like an input element's focus ring. This doesn't necessarily need to be built into EUI's component, instead it could be a provided design pattern. Either way I'd like to get @cchaos's input on this as a whole, especially how these changes may interact with supported edge cases in EUI's component interactions.

I would not have a problem merging this & releasing it after Kibana's FF to help give a full minor for finding consequences.

thompsongl · 2020-07-08T17:34:51Z

Either way I'd like to get @cchaos's input on this as a whole [...] I would not have a problem merging this & releasing it after Kibana's FF to help give a full minor for finding consequences.

Definitely. There is no need to rush getting this in.

We can come back to this next week and perhaps have a more synchronous conversation now that the scope of implications is more know.

kibanamachine · 2020-07-14T19:52:31Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3631/

kibanamachine · 2020-07-14T20:04:08Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3631/

thompsongl · 2020-07-14T22:24:06Z

Updated per sync discussion earlier:

scrollLock is false by default, maintaining scroll behavior as it is currently in EUI
noIsolation is true by default, maintaining point-event behavior as it is currently in EUI

We can merge as-is and will get all the aria-hidden benefits without introducing breaking changes.
I'd suggest we do this and follow up with modal- and flyout-specific PR(s) that layer on additional opinions and potential breaking changes, including reversing ownFocus defaults.

chandlerprall

Changes LGTM, tested in PR's docs and didn't find any issues

cchaos

Flyouts, modals and popovers all seem to be behaving as they did. 👍

kibanamachine · 2020-07-15T23:49:15Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3631/

* Break up release.js (elastic#3723) * Switch release.js to use arguments instead of env vars * Switch MFA code back to env var so it doesn't leak in CI logs * Update job definition to use --type arg * Support breaking up release steps with args * Break release up to fetch time-sensitive MFA token right before publish * Strip whitespace from each step Co-authored-by: Chandler Prall <chandler.prall@gmail.com> Co-authored-by: Chandler Prall <chandler.prall@gmail.com> * Allow prop-setting on the FlexItems within DescribedFormGroup (elastic#3717) * Allow prop-setting on the FlexItems within DescribedFormGroup * Add changelog entry * Honor classes on fieldFlexItem * Update src/components/form/described_form_group/described_form_group.test.tsx Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Update snap name Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Re-focus EuiSuperSelect input after making a value change (elastic#3734) * Re-focus EuiSuperSelect input after making a value change * changelog * [EuiStat] Allow customizing the render of the title and description HTML tags (elastic#3693) * Add titleElement and descriptionElement to EuiStat * Updated snapshots * Updated changelog * Fixed issue with screenreader * Updated snapshots * Use screen reader only if title and description is a string * updated snapshots * Merge remote-tracking branch 'upstream/master' into fix/stat * Fixed typo in changelod * removed titleChildren * titlechildren as variable * Update CHANGELOG.md Remove an extra line left over from a merge resolution Co-authored-by: Chandler Prall <chandler.prall@gmail.com> * [EuiTable] Expand item action name to allow a function (elastic#3739) * allow for item -> ReactNode in name * docs * CL * Add ssh keys so new tags can be pushed to Github from Jenkins (elastic#3740) * Add ssh keys so new tags can be pushed to Github * Need a vault token before we can pull secrets * update i18ntokens * 27.1.0 * Updated documentation. * Chore/decouple button content (elastic#3730) * [New] EuiButtonContent For rendering the contents of buttons, icon (loading spinner) and text wrapper for children * Use EuiButtonContent in EuiButtonEmpty * Fixing classNames and disabled states * Create EuiButtonDisplay for internal usage * Fix snaps * ts gymnastics * Added tests for EuiButtonContent * More optimization - Extend EuiButtonContentProps - Content styles are in button_content.scss * Restricted list of `element`s * [Docs] Adding more acccessibility-focused notes and examples (elastic#3714) * making more a11y callouts in docs * Apply suggestions from code review Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com> * prettier update Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com> * [EuiFocusTrap] Use `react-focus-on` (elastic#3631) * WIP: react-focus-on * WIP: fix GuideFullScreen * noIsolation; onClickOutside * euiflyout snapshot updates * euiflyout snapshot updates * euimodal snapshot updates * euidatagrid snapshot updates * euicolorpalettepicker snapshot updates * euisuperselect snapshot updates * euicollapsible snapshot updates * euifocustrap snapshot updates * allow for array snapshots with takeMountedSnapshot * docs improvements * default to noIsolation=true and scrollLock=false * CL * Fixed bug in EuiComboBox always showing a scrollbar (elastic#3744) * Fixed EuiComboBox always showing a scrollbar * Adding the right PR number to CL * Added useEuiI18n hook (elastic#3749) * Added useEuiI18n hook * Updated docs with useEuiI18n hook, added snippets * Add support to fetch-1i8n-strings or useEuiI18n to match EuiI18n extraction * Fix up return types for useEuiI18n * Updated custom eslint i18n rule/package to lint useEuiI18n usages * changelog * Remove something I was testing with and lost where I had placeed it. * [EuiSuperDatePicker] Bypass formatting `null` dates (elastic#3750) * prevent formatting on null value * remove unnecessary cast * account for keyboard nav with null selection * CL * Updated EuiComboBox to allow the options list to open for single selection custom options (elastic#3706) * Fixing includes to return true when object exists in array * changelog * Allowing list to open for single selection custom options * Updated changelog * PR review * Improving example * Improving example * Addind isClearable * Improving examples * Improving explanation text * Adding note * Addressing PR issues * Update src-docs/src/views/combo_box/combo_box_example.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Update src-docs/src/views/combo_box/combo_box_example.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Snippet Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Add analyze event glyph to EuiIcon (elastic#3729) * adding analyze event security icon * updating analyze_event icon * updating again * Update CHANGELOG.md * Update consuming.md (elastic#3769) * Path has changed and the wiki has not been updated. * Fix zIndex for two popup ups (elastic#3768) Clicking both buttons on https://eui.elastic.co/#/tabular-content/data-grid-styling-and-toolbar demo brings up partially hidden popups because their z-index is too low. Increasing by one seems to do the trick. * [Playground] EuiBadge, EuiNotificationBadge, EuiBetaBadge (elastic#3722) * created playground for badges * removed commented code * used validator for iconType and colour * updated variable name * removed colour validation * removed unnecessary imports * removed default values, fullscreen mode * suggesstions * removed placeholder, added required, some badge props set to string * used actual value of child in text field * added sanitize function * fixed unique-key warning * added validation * updated to identify the change whenthe state changes * suggestions * added onCLick function snippet * removed error popup by react-view * removed lint err * removed commented code Co-authored-by: Josh Mock <joshua.mock@elastic.co> Co-authored-by: Chandler Prall <chandler.prall@gmail.com> Co-authored-by: Scott Gould <sbg@elastic.co> Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> Co-authored-by: Ashik Meerankutty <ashik9591@gmail.com> Co-authored-by: Greg Thompson <thompson.glowe@gmail.com> Co-authored-by: Michail Yasonik <michail.yasonik@elastic.co> Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com> Co-authored-by: Elizabet Oliveira <elizabet.oliveira@gmail.com> Co-authored-by: Marra Sherrier <45169477+marrasherrier@users.noreply.github.com> Co-authored-by: Alberto Andújar <josealbertoandujar@gmail.com> Co-authored-by: Yuri Astrakhan <yuriastrakhan@gmail.com>

* WIP: react-focus-on * WIP: fix GuideFullScreen * noIsolation; onClickOutside * euiflyout snapshot updates * euiflyout snapshot updates * euimodal snapshot updates * euidatagrid snapshot updates * euicolorpalettepicker snapshot updates * euisuperselect snapshot updates * euicollapsible snapshot updates * euifocustrap snapshot updates * allow for array snapshots with takeMountedSnapshot * docs improvements * default to noIsolation=true and scrollLock=false * CL

This reverts commit cdfa280.

stil · 2024-09-20T23:11:38Z

react-focus-lock should be probably removed from resolutions of package.json in @elastic/eui-monorepo

https://github.com/elastic/eui/blob/main/package.json#L31

thompsongl added 2 commits June 17, 2020 15:34

WIP: react-focus-on

361934d

WIP: fix GuideFullScreen

319ac9c

thompsongl added the accessibility label Jun 17, 2020

Merge branch 'master' into focus-on

e2e1154

thompsongl added 2 commits July 6, 2020 12:08

Merge branch 'master' into focus-on

53ff16f

noIsolation; onClickOutside

72af3a4

thompsongl added 9 commits July 6, 2020 13:43

euiflyout snapshot updates

d6f538b

euiflyout snapshot updates

ea48454

euimodal snapshot updates

d6e02ea

euidatagrid snapshot updates

3aef4fd

euicolorpalettepicker snapshot updates

22b84a3

euisuperselect snapshot updates

c55dcea

euicollapsible snapshot updates

b20c79d

euifocustrap snapshot updates

c750d0e

allow for array snapshots with takeMountedSnapshot

98a2fad

docs improvements

29fbba9

thompsongl requested review from chandlerprall and myasonik July 7, 2020 16:38

thompsongl marked this pull request as ready for review July 7, 2020 16:38

Merge branch 'master' into focus-on

85989e3

thompsongl added 2 commits July 14, 2020 12:29

Merge branch 'master' into focus-on

dcc7db3

default to noIsolation=true and scrollLock=false

26bc04c

Merge branch 'master' into focus-on

cfca928

thompsongl requested a review from cchaos July 14, 2020 22:25

chandlerprall approved these changes Jul 15, 2020

View reviewed changes

cchaos approved these changes Jul 15, 2020

View reviewed changes

CL

6b83b20

thompsongl merged commit a7d7348 into elastic:master Jul 16, 2020

anishagg17 added a commit to anishagg17/eui that referenced this pull request Jul 20, 2020

Revert "[EuiFocusTrap] Use react-focus-on (elastic#3631)"

cc8585a

This reverts commit cdfa280.

anishagg17 added a commit to anishagg17/eui that referenced this pull request Jul 20, 2020

Revert "[EuiFocusTrap] Use react-focus-on (elastic#3631)"

139094d

This reverts commit cdfa280.

thompsongl mentioned this pull request Jul 23, 2020

Upgrade EUI to v27.2.0 elastic/kibana#72993

Closed

thompsongl mentioned this pull request Jul 31, 2020

Upgrade EUI to v27.4.0 elastic/kibana#74004

Merged

Kerry350 mentioned this pull request Aug 5, 2020

[Logs UI] [Metrics UI] Keyboard control audit elastic/kibana#74108

Closed

This was referenced Aug 11, 2020

[EuiModal] Accessibilty and experience updates #3889

Closed

Add test-env mock for EuiFocusTrap #3930

Merged

thompsongl mentioned this pull request Aug 31, 2021

[EuiSelect in EuiPopover] Focus trap requires two clicks when focus is on select #3172

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiFocusTrap] Use `react-focus-on` #3631

[EuiFocusTrap] Use `react-focus-on` #3631

thompsongl commented Jun 17, 2020 •

edited

Loading

kibanamachine commented Jun 17, 2020

myasonik commented Jun 18, 2020

thompsongl commented Jun 18, 2020

thompsongl commented Jun 18, 2020

cchaos commented Jun 23, 2020

kibanamachine commented Jul 6, 2020

kibanamachine commented Jul 6, 2020

thompsongl commented Jul 6, 2020

thompsongl commented Jul 6, 2020

kibanamachine commented Jul 6, 2020

kibanamachine commented Jul 7, 2020

myasonik commented Jul 7, 2020

thompsongl commented Jul 7, 2020

chandlerprall commented Jul 8, 2020

thompsongl commented Jul 8, 2020

chandlerprall commented Jul 8, 2020 •

edited

Loading

thompsongl commented Jul 8, 2020

kibanamachine commented Jul 14, 2020

kibanamachine commented Jul 14, 2020

thompsongl commented Jul 14, 2020

chandlerprall left a comment •

edited

Loading

cchaos left a comment

kibanamachine commented Jul 15, 2020

stil commented Sep 20, 2024

[EuiFocusTrap] Use react-focus-on #3631

[EuiFocusTrap] Use react-focus-on #3631

Conversation

thompsongl commented Jun 17, 2020 • edited Loading

Summary

Draft checklist

Checklist

kibanamachine commented Jun 17, 2020

myasonik commented Jun 18, 2020

noIsolation prop

Nice to have...

thompsongl commented Jun 18, 2020

thompsongl commented Jun 18, 2020

cchaos commented Jun 23, 2020

kibanamachine commented Jul 6, 2020

kibanamachine commented Jul 6, 2020

thompsongl commented Jul 6, 2020

thompsongl commented Jul 6, 2020

kibanamachine commented Jul 6, 2020

kibanamachine commented Jul 7, 2020

myasonik commented Jul 7, 2020

thompsongl commented Jul 7, 2020

chandlerprall commented Jul 8, 2020

thompsongl commented Jul 8, 2020

chandlerprall commented Jul 8, 2020 • edited Loading

thompsongl commented Jul 8, 2020

kibanamachine commented Jul 14, 2020

kibanamachine commented Jul 14, 2020

thompsongl commented Jul 14, 2020

chandlerprall left a comment • edited Loading

Choose a reason for hiding this comment

cchaos left a comment

Choose a reason for hiding this comment

kibanamachine commented Jul 15, 2020

stil commented Sep 20, 2024

[EuiFocusTrap] Use `react-focus-on` #3631

[EuiFocusTrap] Use `react-focus-on` #3631

thompsongl commented Jun 17, 2020 •

edited

Loading

`noIsolation` prop

chandlerprall commented Jul 8, 2020 •

edited

Loading

chandlerprall left a comment •

edited

Loading