[EuiPopover] Removed `false` as an option for `initialFocus`, removed unnecessary focus logic #6044

cee-chen · 2022-07-11T23:41:14Z

Summary

I think what's happening is actually due to react-focus-on's autoFocus behavior. If I set EuiFocusTrap's autoFocus={false} when initialFocus={false} - focus is removed completely from the document. This is actually technically 'correct' behavior if a focus trap is enabled on the popover - because the toggling button is outside the popover, focus actually shouldn't have remained on it.

I think the best way forward frankly (after #5784) is to remove initialFocus={false} as an option at all. It no longer really makes sense after its first implementation in #4768, since false/focusing the panel is basically the default at this point.

⚠️ The main fix is 4c9bac5, but I actually realized while testing more that there's some underlying / overlapping focus behavior going on.

I initially thought the initialFocus overrides were the issue (84fe068), but after some more testing I quickly realized the auto focus behavior is coming from EuiFocusTrap/react-focus-on/react-focus-lock itself.

After even more thinking, I realized there's a lot of overlapping logic between EuiPopover's updateFocus logic and the focus logic that comes OOTB with FocusTrap/focus-lock ... so I experimented with removing updateFocus completely (74e38dc) and bam - everything still works as-is 🙈

We should definitely test thoroughly considering EuiPopover's ubiquitousness. I'll make a QA list below:

QA

Checklist

Checked in Chrome, Safari, Edge, and Firefox
Added or updated jest and cypress 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

~~- [ ] Checked in both light and dark modes~~
- [ ] Checked in mobile
- [ ] Props have proper autodocs and playground toggles
- [ ] Added documentation
~~- [ ] Checked Code Sandbox works for any docs examples~~
~~- [ ] Updated the Figma library counterpart~~

- since the panel popover is now always used as a default, false feels unnecessary - initialFocus should only be passed if something other than the popover parent should be focused

+ do not override to the panel if it's already set + update spec tests to check for `data-autofocus` to confirm that react-focus-on is being passed the correct `initialFocus` as expected

AFAICT, this is already being handled automatically by `EuiFocusTrap`, and works exactly as before / in Cypress, so I don't see the need for the extra focus logic

- now that EuiPopoverFocus no longer has an `updateFocus` method, it turns out the focus fighting is coming from the focus trap

kibanamachine · 2022-07-12T00:16:11Z

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

thompsongl · 2022-07-12T21:08:24Z

After even more thinking, I realized there's a lot of overlapping logic between EuiPopover's updateFocus logic and the focus logic that comes OOTB with FocusTrap/focus-lock ... so I experimented with removing updateFocus completely (74e38dc) and bam - everything still works as-is 🙈

Dang that would be cool if we could outsource the focus logic. This is definitely a case of unintentional overlap. The EuiPopover logic was added when it used a different focus trap library and react-focus-on/lock has gotten even better in the last couple years.

Going to do some test on the various components tomorrow!

thompsongl · 2022-07-13T21:20:51Z

Still doing manual testing but commenting to say that all instances of initialFocus=false in Kibana appear to just avoid focusing the first focusable element, so no changes will be necessary.

thompsongl

Super happy about this 🙌

Ran through a bunch of components manually and found no focus regressions. Even the ownFocus=false scenario behaves as it did, which I had initial concerns about (I thought perhaps updateFocus still added focus, but it did not).

I wouldn't mind having one other person take a look, though, as this change has far-reaching implications. Also perhaps considering whether to not have this go into Kibana before 8.4 FF. cc// @chandlerprall

cee-chen · 2022-07-14T15:47:23Z

Agreed on getting a second pair of eyes/QA, but I'd argue this PR has the same implications that #5784 does (less so because it doesn't actually affect functionality), which is already in Kibana 8.4. That being said I wouldn't rush to get it in before FF though, if it doesn't make it, it doesn't make it, not a big deal

thompsongl · 2022-07-14T15:59:51Z

I'd argue this PR has the same implications that #5784 does (less so because it doesn't actually affect functionality), which is already in Kibana 8.4

Yeah agreed. The only difference is Kibana teams have had 2 months to catch any bugs from #5784, whereas if this merged those teams would only have a couple days to catch regressions. More timing than severity was my point there.

cee-chen · 2022-07-14T16:15:15Z

Gotcha, makes sense! PRs like this definitely make me wish we had the ability to run Kibana CI against EUI dev branches, their FTR tests are super robust and would def catch regressions. Maybe we can bribe the inestimable @kertal sometime? 😇

chandlerprall · 2022-07-14T16:28:53Z

Very excited at the prospect of deleting this code! I'll take a pass, but may not be until next week. Having only skimmed the changes, I don't think there's a need to avoid 8.4.

kertal · 2022-07-19T12:21:38Z

Gotcha, makes sense! PRs like this definitely make me wish we had the ability to run Kibana CI against EUI dev branches, their FTR tests are super robust and would def catch regressions. Maybe we can bribe the inestimable @kertal sometime? 😇

Sounds like a very useful idea ... so the idea is planted 🪴 ... which is a start

cee-chen · 2022-07-20T16:07:13Z

@chandlerprall Quick ping on QAing/approving this PR!

chandlerprall

Played with a number of popovers across the examples, making changes to / adding some initialFocus definitions and everything continues working as expected 🎉

cee-chen · 2022-07-20T17:24:12Z

Thanks Chandler!! Merging now. Just to confirm, it sounds like this PR won't be released in time for Kibana 8.4, which I'm personally totally fine with - let me know if anyone feels strongly otherwise!

* Upgrade to v62.0.3 * Update EUI i18n tokens * Update html string snapshots - Emotion CSS hash changed * [EuiIcon] Update instances of `keyboardShortcut` icons to `keyboard` * [EuiErrorBoundary] Update snapshots from Emotion conversion * [EuiImage] Update snapshots, tests, and CSS to account for Emotion conversion * [EuiImage][RTL] Fix test failures caused by EuiImage changes * [EuiCommentList] Deprecate EuiCommentProps.type * [EuiCommentList] Rename `timelineIcon` prop to `timelineAvatar` - see elastic/eui#6071 * [EuiCommentList] Fix selectors deprecated by Emotion conversion * [EuiPopover][EuiCommentEvent][Enzyme] Fix mounted test failures caused by Emotion conversions - Mounting displays the Emotion wrapper with the data-test-subj on them - we need to specify the output div renders in order for text assertions to be correct * [EuiPopover] Deprecate `initialFocus={false}` as an option see elastic/eui#6044 * [EuiPopover] Rename `display=inlineBlock` to `inline-block` - see elastic/eui#5977 * [EuiPopover] Update snapshots from Emotion conversion * [EuiPopover] Replace deprecated `.euiPopover__panel-isOpen` class with new `[data-popover-open]` attribute * [EuiPopover][RTL] Fix test failures caused by not waiting for EuiPopover animation/transition * Skip failing a11y tests - test w/ similar error already skipped in another test above - requires closing the popover for next test to pass - not sure why delete action is no longer available * Fix failing Security Cypress tests * Attempt to squash flaky FTR tests around Add Filter popover Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Jonathan Budzenski <[email protected]>

cee-chen added 5 commits July 11, 2022 15:34

Remove initialFocus={false} as an option

4c9bac5

- since the panel popover is now always used as a default, false feels unnecessary - initialFocus should only be passed if something other than the popover parent should be focused

Clarify initialFocus override behavior

84fe068

+ do not override to the panel if it's already set + update spec tests to check for `data-autofocus` to confirm that react-focus-on is being passed the correct `initialFocus` as expected

Remove unnecessary updateFocus() logic

74e38dc

AFAICT, this is already being handled automatically by `EuiFocusTrap`, and works exactly as before / in Cypress, so I don't see the need for the extra focus logic

[Misc perf] Return early if no initialFocus is defined

e35bc7d

[Misc] Update EuiContextMenuPanel references to EuiPopover focus

6f5a49a

- now that EuiPopoverFocus no longer has an `updateFocus` method, it turns out the focus fighting is coming from the focus trap

cee-chen requested a review from thompsongl July 11, 2022 23:41

Changelog

04ad0f1

cee-chen added the breaking change label Jul 11, 2022

cee-chen marked this pull request as ready for review July 12, 2022 00:05

thompsongl approved these changes Jul 14, 2022

View reviewed changes

cee-chen mentioned this pull request Jul 18, 2022

[EuiPortal] convert to a function component, fix ssr bug #6055

Merged

6 tasks

chandlerprall approved these changes Jul 20, 2022

View reviewed changes

cee-chen merged commit 7fb31f4 into elastic:main Jul 20, 2022

cee-chen deleted the popover/initialFocus branch July 20, 2022 17:24

cee-chen mentioned this pull request Jul 27, 2022

Upgrade EUI to v62.0.3 elastic/kibana#136865

Closed

cee-chen mentioned this pull request Aug 9, 2022

Upgrade EUI to v62.0.3 elastic/kibana#138351

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiPopover] Removed `false` as an option for `initialFocus`, removed unnecessary focus logic #6044

[EuiPopover] Removed `false` as an option for `initialFocus`, removed unnecessary focus logic #6044

cee-chen commented Jul 11, 2022 •

edited

Loading

kibanamachine commented Jul 12, 2022

thompsongl commented Jul 12, 2022

thompsongl commented Jul 13, 2022

thompsongl left a comment •

edited

Loading

cee-chen commented Jul 14, 2022

thompsongl commented Jul 14, 2022 •

edited

Loading

cee-chen commented Jul 14, 2022

chandlerprall commented Jul 14, 2022

kertal commented Jul 19, 2022

cee-chen commented Jul 20, 2022

chandlerprall left a comment

cee-chen commented Jul 20, 2022 •

edited

Loading

[EuiPopover] Removed false as an option for initialFocus, removed unnecessary focus logic #6044

[EuiPopover] Removed false as an option for initialFocus, removed unnecessary focus logic #6044

Conversation

cee-chen commented Jul 11, 2022 • edited Loading

Summary

QA

Checklist

kibanamachine commented Jul 12, 2022

thompsongl commented Jul 12, 2022

thompsongl commented Jul 13, 2022

thompsongl left a comment • edited Loading

Choose a reason for hiding this comment

cee-chen commented Jul 14, 2022

thompsongl commented Jul 14, 2022 • edited Loading

cee-chen commented Jul 14, 2022

chandlerprall commented Jul 14, 2022

kertal commented Jul 19, 2022

cee-chen commented Jul 20, 2022

chandlerprall left a comment

Choose a reason for hiding this comment

cee-chen commented Jul 20, 2022 • edited Loading

[EuiPopover] Removed `false` as an option for `initialFocus`, removed unnecessary focus logic #6044

[EuiPopover] Removed `false` as an option for `initialFocus`, removed unnecessary focus logic #6044

cee-chen commented Jul 11, 2022 •

edited

Loading

thompsongl left a comment •

edited

Loading

thompsongl commented Jul 14, 2022 •

edited

Loading

cee-chen commented Jul 20, 2022 •

edited

Loading