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

EuiFormRow with nested EuiPopovers using ownFocus #2110

Merged
merged 6 commits into from
Jul 15, 2019

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Jul 11, 2019

Summary

Lots of manual testing welcome

Discussed primarily in #eui Slack, the issue as raised by @legrego shows serious performance problems resulting from DOM focus updates in EuiPopover.

Causes

  1. React portal component subtrees vs. real DOM position: we rely on .contains() to prevent unnecessary focus updates, which simply doesn't cut it when portals get involved.
  2. EuiPopover is likely too eager to manually change focus during its componentDidUpdate method.

Solution

No. 1 is a real thing we (and every other app in the React ecosystem) need to solve. The best answer is likely to let React fix it with the proposed FocusManager API.

We have full control over No. 2, so that's where to start.

Events where updating focus should happen

Event Update focus? Why
open We've change active elements and users have entered an altered flow/tab order
close EuiFocusTrap handles this case and returns focus to the last active element
scroll If no click event happens, focus has not changed. If a click is involved, EuiFocusTrap has already (correctly) removed focus from the popover
reposition Same as 'scroll'
blur Same as 'scroll'
prop updates initialFocus and ownFocus are the only props relevant to focus. Neither should alter an already open popover. They will update on subsequent open calls.

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

@thompsongl thompsongl marked this pull request as ready for review July 11, 2019 20:49
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; tested locally, not seeing performance issues from the focus fight

@thompsongl thompsongl merged commit 36251ab into elastic:master Jul 15, 2019
@legrego
Copy link
Member

legrego commented Jul 16, 2019

Thanks so much for the fast turnaround on this! 🎉

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

Successfully merging this pull request may close these issues.

3 participants