[EuiPopover] Fix popover returning focus to correct element #4071

chandlerprall · 2020-09-23T20:12:09Z

Summary

Fixes #4003 and fixes #4107

I've removed a lot of logic from EuiPopover, as it has been duplicated by the stack of focus lock libraries used by the popovers.

This has apparently introduced visual issue (or at least exasperated an existing one) where clicking on a popover anchor button often causes that button to move a lot.

Checklist

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

Checked in Chrome, Safari, Edge, and Firefox
- [ ] Props have proper autodocs
- [ ] Added documentation
Checked Code Sandbox works for the any docs examples
- [ ] 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

chandlerprall · 2020-09-23T20:12:44Z

src-docs/src/views/popover/popover.js

-      Show popover
-    </EuiButton>
-  );
+  const [openPopover, setOpenPopover] = useState(null);


this change is temporary, for testing

Don't forget to revert this before merging. 😜

snide · 2020-09-23T21:45:32Z

Seems like a good time to just remove the animation. We've been fighting it for awhile. If we can't get around it, I think we should be ok removing that as a blocker. My guess is we'll still have an issue, now with shadows "bouncing" in the animation, so I'd still look into the root.

cchaos · 2020-09-24T15:33:15Z

Seems like a good time to just remove the animation.

I disagree that this is a good time/good reason. We rely on this animation in other buttons especially EuiButtonIcon for hover states. I don't want to block the fix for popovers while we investigate all the uses of translate hover state. We do have a specific discuss issue in #2184 for this and should probably be visited via Amsterdam.

I can look into the cause for the double-animation but it might be ok to ship with this tiny regression for now.

chandlerprall · 2020-09-24T15:45:39Z

I can look into the cause for the double-animation but it might be ok to ship with this tiny regression for now.

I'll move forward with this PR on Monday (with the importance on the focus bug, would like to include it in the next release) - if someone can take a look into the animation before then that'd be great, otherwise we can try to re-prioritize #2184

[working through the lint issues, some tests, and other issues now]

cchaos · 2020-09-24T16:58:13Z

The focus behavior (aside from the animation) on this one has gotten really funky. Just play around with the "Anchor position" example which "toggles" the popover and you'll see 3 major things.

Every third click doesn't close the popover
When in that ^ state, clicking outside no longer closes it
When clicking outside does work. It should still return focus to original trigger IF what they click on is not interactable. Otherwise the next tabbed item seems arbitrary.

Happy to zoom about or record these for clarification.

I think it's best to finalize the interaction and focus behaviors (JS) before trying to look at the anim bug because they could be related.

kibanamachine · 2020-09-24T18:24:55Z

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

chandlerprall · 2020-09-28T18:58:50Z

I am aware of and working on one last issue that this change has created.

kibanamachine · 2020-09-28T19:12:26Z

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

chandlerprall · 2020-09-29T18:34:22Z

@thompsongl @cchaos this is ready for a full review - no rush!

kibanamachine · 2020-09-29T18:53:34Z

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

cchaos · 2020-09-30T20:36:37Z

We still have a bit of a jumpy screen when closing the popover. Here's a video: https://share.getcloudapp.com/DOu9Nvq1

What's happening is I open a popover and then scroll so that it is out of view, when I then click anywhere on the page, it scrolls back up to the original popover's trigger. BUT, if I then tab, the next focusable element is after where I had clicked to dismiss the popover (basically then shooting back down the page).

So essentially if a popover is still open but not visible in the viewport, clicking anywhere will force the window to put that old trigger button back into view even though it's not the currently focused element.

chandlerprall · 2020-10-01T21:17:47Z

@cchaos pushed a fix for jumping back to the anchor when it is re-focused. The fix - which relies on passing a preventScroll configuration to .focus() - reportedly does not work in Safari. However, there is an existing bug in our popovers+FF and Safari where the anchor does not receive focus when pressing esc, which masks this discrepancy.

kibanamachine · 2020-10-01T21:36:09Z

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

thompsongl · 2020-10-05T17:21:26Z

This appears to be an improvement overall 🎉
A couple things I noticed during manual testing:

EuiFlyout, after clicking the overlay mask to close. Focus is not returned to the button.
(Not actually sure that I'd expect focus to return to the button in this case, but it's different from the current behavior.)
EuiColorPicker, navigating with keyboard. Focus does not move to the picker handle.
(Could be the timeout needs to be updated. Setting this focus is somewhat manual.)

…EuiFocusTrap

…s is false

chandlerprall · 2020-10-29T16:12:21Z

Force pushed after rebasing on master, only new commit since last comment is Address unintentional changes to flyouts & color picker 6d6e1be

EuiFlyout, after clicking the overlay mask to close. Focus is not returned to the button.
(Not actually sure that I'd expect focus to return to the button in this case, but it's different from the current behavior.)

I changed EuiFlyout's clickOutsideDisables on EuiFocusTrap to false, which corrects this. It does change the existing behaviour where opening a flyout without ownFocus would reset focus to the button when clicking out of the flyout, instead of selecting the button again when the flyout closes. This is a change in behaviour but feels much better to me.

EuiColorPicker, navigating with keyboard. Focus does not move to the picker handle.
(Could be the timeout needs to be updated. Setting this focus is somewhat manual.)

Greg contributed the fix for this, making color picker explicitly decide where focus should go when the picker pops up.

kibanamachine · 2020-10-29T16:30:56Z

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

thompsongl

Code changes look good; very happy to see focus_trap simplified 🎉
Manually tested a bunch of docs examples and didn't see any regressions aside from the noted button double-animation.

Don't forget a changelog entry

cchaos · 2020-11-04T17:30:38Z

src-docs/src/views/popover/popover.js

-      Show popover
-    </EuiButton>
-  );
+  const [openPopover, setOpenPopover] = useState(null);


Don't forget to revert this before merging. 😜

src/components/flyout/__snapshots__/flyout.test.tsx.snap

kibanamachine · 2020-11-04T17:43:39Z

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

…ting

kibanamachine · 2020-11-04T18:37:53Z

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

…4071) * Move onclickoutside / onescape handling to focus-lock lib itself * Get popover focus return functioning * Fix lint issues & snapshots, re-enable clickOutsideDisables prop for EuiFocusTrap * fix popover re-toggling when clicking its anchor * Re-enable popover outside click and escape key detection when ownFocus is false * Only call popover's closePopover callback when it is open * Don't scroll back to a popover anchor when the popover closes * Address unintentional changes to flyouts & color picker * changelog * revert changes to popover example, which had been made to augment testing * Fix flyout's snapshot tests

chandlerprall commented Sep 23, 2020

View reviewed changes

chandlerprall force-pushed the bug/4003-popover-re-focus branch from 6a44e12 to 87eb56a Compare September 24, 2020 17:59

chandlerprall marked this pull request as ready for review September 29, 2020 18:32

chandlerprall requested review from thompsongl and cchaos September 29, 2020 18:32

thompsongl mentioned this pull request Oct 5, 2020

[EuiDataGrid] Quick double clicks on the data grid header freezes the browser and results in an error #4107

Closed

chandlerprall mentioned this pull request Oct 15, 2020

[EuiDataGrid] Column header control exception #4141

Closed

chandlerprall added 8 commits October 28, 2020 15:01

Move onclickoutside / onescape handling to focus-lock lib itself

cf88a0c

Get popover focus return functioning

7c97d64

Fix lint issues & snapshots, re-enable clickOutsideDisables prop for …

a587534

…EuiFocusTrap

fix popover re-toggling when clicking its anchor

bc5b666

Re-enable popover outside click and escape key detection when ownFocu…

172af2d

…s is false

Only call popover's closePopover callback when it is open

5bc02fb

Don't scroll back to a popover anchor when the popover closes

374c41b

Address unintentional changes to flyouts & color picker

6d6e1be

chandlerprall force-pushed the bug/4003-popover-re-focus branch from 57abce2 to 6d6e1be Compare October 29, 2020 16:07

thompsongl approved these changes Nov 3, 2020

View reviewed changes

chandlerprall added 2 commits November 4, 2020 08:47

Merge branch 'master' into bug/4003-popover-re-focus

baebfb1

changelog

206c7b0

elizabetdev mentioned this pull request Nov 4, 2020

[EuiPopover] Default to ownFocus #4228

Merged

5 tasks

Merge branch 'master' into bug/4003-popover-re-focus

ae58e17

cchaos reviewed Nov 4, 2020

View reviewed changes

chandlerprall added 2 commits November 4, 2020 11:06

revert changes to popover example, which had been made to augment tes…

a68536b

…ting

Fix flyout's snapshot tests

3e99a23

cchaos approved these changes Nov 4, 2020

View reviewed changes

chandlerprall merged commit d3284d5 into elastic:master Nov 4, 2020

chandlerprall deleted the bug/4003-popover-re-focus branch November 4, 2020 19:02

thompsongl mentioned this pull request Nov 6, 2020

[EuiFlyout] Clarify clickOutsideDisables decision #4236

Merged

5 tasks

chandlerprall mentioned this pull request Nov 23, 2020

Upgrade EUI to v30.4.2 elastic/kibana#84158

Closed

chandlerprall mentioned this pull request Dec 1, 2020

Upgrade EUI to v30.5.1 elastic/kibana#84677

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiPopover] Fix popover returning focus to correct element #4071

[EuiPopover] Fix popover returning focus to correct element #4071

chandlerprall commented Sep 23, 2020 •

edited

Loading

chandlerprall Sep 23, 2020

cchaos Nov 4, 2020

snide commented Sep 23, 2020

cchaos commented Sep 24, 2020

chandlerprall commented Sep 24, 2020 •

edited

Loading

cchaos commented Sep 24, 2020 •

edited

Loading

kibanamachine commented Sep 24, 2020

chandlerprall commented Sep 28, 2020

kibanamachine commented Sep 28, 2020

chandlerprall commented Sep 29, 2020

kibanamachine commented Sep 29, 2020

cchaos commented Sep 30, 2020

chandlerprall commented Oct 1, 2020

kibanamachine commented Oct 1, 2020

thompsongl commented Oct 5, 2020

chandlerprall commented Oct 29, 2020 •

edited

Loading

kibanamachine commented Oct 29, 2020

thompsongl left a comment

cchaos Nov 4, 2020

kibanamachine commented Nov 4, 2020

kibanamachine commented Nov 4, 2020

[EuiPopover] Fix popover returning focus to correct element #4071

[EuiPopover] Fix popover returning focus to correct element #4071

Conversation

chandlerprall commented Sep 23, 2020 • edited Loading

Summary

Checklist

chandlerprall Sep 23, 2020

Choose a reason for hiding this comment

cchaos Nov 4, 2020

Choose a reason for hiding this comment

snide commented Sep 23, 2020

cchaos commented Sep 24, 2020

chandlerprall commented Sep 24, 2020 • edited Loading

cchaos commented Sep 24, 2020 • edited Loading

kibanamachine commented Sep 24, 2020

chandlerprall commented Sep 28, 2020

kibanamachine commented Sep 28, 2020

chandlerprall commented Sep 29, 2020

kibanamachine commented Sep 29, 2020

cchaos commented Sep 30, 2020

chandlerprall commented Oct 1, 2020

kibanamachine commented Oct 1, 2020

thompsongl commented Oct 5, 2020

chandlerprall commented Oct 29, 2020 • edited Loading

kibanamachine commented Oct 29, 2020

thompsongl left a comment

Choose a reason for hiding this comment

cchaos Nov 4, 2020

Choose a reason for hiding this comment

kibanamachine commented Nov 4, 2020

kibanamachine commented Nov 4, 2020

chandlerprall commented Sep 23, 2020 •

edited

Loading

chandlerprall commented Sep 24, 2020 •

edited

Loading

cchaos commented Sep 24, 2020 •

edited

Loading

chandlerprall commented Oct 29, 2020 •

edited

Loading