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

EuiFocusTrap; dependency changes #1550

Merged
merged 15 commits into from
Feb 20, 2019
Merged

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Feb 11, 2019

Summary

Enables focus trapping in mixed content-in-portal flows.

  • Move from focus-trap-react to react-focus-lock for focus trapping needs.
    • Our current focus trapping solution does not account for or support content projected via React portals (e.g., EuiPopover) in component subtrees. That is, focus state and tab order are lost when users encounter content in React portals during normal keyboard interaction.
    • react-focus-lock solves the above problem by default.
  • Adds a new component: EuiFocusTrap
    • Set reasonable defaults for passthrough props (it wraps react-focus-lock).
    • Composes EuiOutsideClickDetector and a portal-specific React event handler to determine if clicks/keypresses happen outside the component subtree.
    • Clearer initalFocus API for giving certain elements focus on intialization

Resolves #1542, resolves #1495, resolves #1051
Gets at #995

Note: EUI's package react-datepicker still uses focus-trap-react, but maintains its own dependency.

Checklist

  • This was checked in mobile
  • This was checked in IE11

- [ ] This was checked in dark mode

  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios

- [ ] This required updates to Framer X components

@chandlerprall
Copy link
Contributor

Confirmed locally that this PR addresses the issues mentioned in the description.

src-docs/src/views/focus_trap/focus_trap_example.js Outdated Show resolved Hide resolved
src-docs/src/views/focus_trap/focus_trap_example.js Outdated Show resolved Hide resolved
src/components/focus_trap/focus_trap.js Show resolved Hide resolved
src/components/focus_trap/focus_trap.test.js Show resolved Hide resolved
src/components/focus_trap/focus_trap.js Outdated Show resolved Hide resolved
src/components/focus_trap/focus_trap.js Outdated Show resolved Hide resolved
src/components/focus_trap/focus_trap.js Outdated Show resolved Hide resolved
src/components/focus_trap/focus_trap.js Outdated Show resolved Hide resolved
src/components/focus_trap/focus_trap.js Outdated Show resolved Hide resolved
src/components/focus_trap/focus_trap.js Outdated Show resolved Hide resolved
src/components/focus_trap/focus_trap.js Outdated Show resolved Hide resolved
src/components/focus_trap/focus_trap.js Outdated Show resolved Hide resolved
src/components/focus_trap/focus_trap.test.js Outdated Show resolved Hide resolved
src/components/focus_trap/focus_trap.test.js Show resolved Hide resolved
src/components/focus_trap/focus_trap.test.js Show resolved Hide resolved
src/components/focus_trap/focus_trap.test.js Show resolved Hide resolved
src/components/focus_trap/focus_trap.test.js Outdated Show resolved Hide resolved
@thompsongl thompsongl requested review from cchaos and snide February 19, 2019 21:11
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM, pulled locally and verified examples in docs are working as intended (and fixes the targeted bugs)

@thompsongl
Copy link
Contributor Author

@cchaos and @snide I'd appreciate a second look and test run if you have some time. @chandlerprall put in a thorough technical review already.

@snide
Copy link
Contributor

snide commented Feb 19, 2019

I'll give it a pass today for functionality. I'll yarn link kibana over, @bevacqua might want to do the same with cloud.

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Tested for functionality only, not code.

Ran with this for about 30 minutes testing in components we use this in in Kibana, and then against the issues mentioned for closure.

The only weirdness I noticed was that Flyouts with the ownFocus prop didn't shift their focus over in some cases. The only reproducable one I could get was the Discover app's "open" functionality. The flyout opens, but the focus stays on the button itself, rather than the item. This doesn't happen with the previous solution. Overall a pretty minor one, and the EUI docs seem to work correctly so I don't know what's up, but I did flip between the two with yarn link and could replicate. All other usages seemed to work as expected and fixed the issues mentioned.

@thompsongl
Copy link
Contributor Author

I'm going to take another look at flyouts. I suspect there's something extra going on with that Discover implementation, but I want to see if there's any more documentation that could be helpful.

@thompsongl
Copy link
Contributor Author

@snide So what's happening with that Discover > Open > Flyout is the focus trap is getting immediately disabled. The outsideClickDetector is catching the Open button click event and returning focus. Best guess right now is that it has to do with the way Angular is propagating that event, although it's been a long time (luckily; thankfully) since I've really triaged Angular 1.x code.

Copy link
Contributor

@bevacqua bevacqua left a comment

Choose a reason for hiding this comment

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

LGTM. We could really use this in Cloud

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

We're gonna leave the Kibana / angular issue for the angular side. We'll create a follow up ticket on that side of the fence. Ultimately, the focus issue with flyouts is more minor than the larger bugs this fixes around portals.

@thompsongl thompsongl merged commit 647b786 into elastic:master Feb 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants