[EuiPopover][EuiInputPopover] Allow more control via `focusTrapProps` by cee-chen · Pull Request #6955 · elastic/eui

cee-chen · 2023-07-17T19:16:57Z

Summary

@Heenawter is working on a custom EuiSelectable component that has a filter dropdown/popover selection menu in between the search box and the listbox. This filter popover has special custom focus behavior that returns focus to the searchbox after filter select, and not to the toggling popover button.

The solution their usage needs is something like this:

<EuiPopover
  closePopover={}
  focusTrapProps={{
    returnFocus: false,
    onEscapeKey: // custom callback closing the popover and focusing the searchbox
    onDeactivation: // custom callback focusing the searchbox
  }}
/>

Unfortunately EuiPopover's default focus return behavior is getting in the way of their custom focus behavior. We should loosen the spread behavior of focusTrapProps to allow consumers to control focus UX as needed.

Breaking change

This PR also incidentally removes a prop (onTrapDeactivation) that is completely unused in EUI and Kibana (as well as untested and undocumented in EUI) that is duplicative of focusTrapProps behavior. I've opinionatedly left in other semi-duplicative focus trap props such as initialFocus and ownFocus because those have actual documentation in EUI as well as usages in Kibana.

QA

Mostly regression testing to ensure past behavior works as before; @Heenawter has QA'd that new behavior works in her app.

Confirm EuiInputPopover continues to focus trap & dismiss its focus trap the same as on production
Confirm the basic EuiPopover examples continue to focus trap & dismiss its focus trap as on prod
Confirm the focus examples on EuiPopover's docs pages work the same as on prod

General checklist

Checked for accessibility including keyboard-only and screenreader modes
Props have proper autodocs (using @default if default values are missing) and playground toggles
- [ ] Added documentation - No onTrapDeactivation docs, and IMO the concept of focusTrapProps is self-evident and already documented in its props
- [ ] Added or updated jest and cypress tests - also skipped because no meaningful logic other than basic spread behavior was changed, which should also be self-evident
Checked for breaking changes and labeled appropriately
A changelog entry exists and is marked appropriately

~~- [ ] Checked in both light and dark modes~~
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
~~- [ ] Checked Code Sandbox works for any docs examples~~
~~- [ ] Updated the Figma library counterpart~~

- the number of consumers with edge cases / enough knowledge to start overriding props is going to be very small, and they should be allowed to do so if needed

…vation`

cee-chen · 2023-07-17T19:19:14Z

@Heenawter Do you mind testing this branch either in Kibana a la carte or by using our local Testing in Kibana steps to ensure this change does actually fix your issue? 🙏

kibanamachine · 2023-07-17T19:42:44Z

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

- this is more of a bugfix IMO since this should have been working before

Heenawter · 2023-07-17T22:34:48Z

Tested this in a local Kibana instance and it indeed makes the fix discussed in #6627 (comment) possible without the need for any delay/timeout - thanks so much for working on this 🎉

cee-chen · 2023-07-17T22:39:38Z

jenkins test this

cee-chen · 2023-07-17T22:44:15Z

@1Copenut Do you mind giving this PR a quick review and smoke test? (see regression QA steps in PR description)

Should hopefully not take too long - hoping to get this in for tomorrow's release, although please feel free to shout if you'd rather hold or if you think it's at all risky.

kibanamachine · 2023-07-17T23:01:56Z

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

1Copenut

👍 LGTM! I tested the manual QA processes in Chrome for keyboard navigation behavior, and Safari, Firefox + VO for screen reader consistency with prod.

## Summary `eui@84.0.0` ⏩ `eui@85.0.1` ## [`85.0.1`](https://github.com/elastic/eui/tree/v85.0.1) **Bug fixes** - Fixed `EuiFilterGroup`'s responsive styles ([#6983](elastic/eui#6983)) ## [`85.0.0`](https://github.com/elastic/eui/tree/v85.0.0) - Updated `EuiThemeProvider` to set an Emotion theme context that returns the values of `useEuiTheme()` ([#6913](elastic/eui#6913)) - Added `size` prop to `EuiStepsHorizontal`, defaulting to the previous size of `m` ([#6928](elastic/eui#6928)) - Added new `s` sizing to `EuiStepsHorizontal` ([#6928](elastic/eui#6928)) - Added `at` and `key` icon glyphs. ([#6934](elastic/eui#6934)) - Added a new `cloneElementWithCss` Emotion utility ([#6939](elastic/eui#6939)) - Updated `EuiPopover` to allow consumer control of all `focusTrapProps` ([#6955](elastic/eui#6955)) **Bug fixes** - Fixed `EuiDataGrid` height calculation bug when browser zoom levels are not 100% ([#6895](elastic/eui#6895)) - Fixed `EuiTab` not correctly passing selection color state to `prepend` and `append` children ([#6938](elastic/eui#6938)) - Fixed `EuiInputPopover` to allow consumer control of its focus trap via `focusTrapProps` ([#6955](elastic/eui#6955)) **Breaking changes** - `EuiProvider` will no longer render multiple or duplicate nested instances of itself. If a nested `EuiProvider` is detected, that instance will return early without further processing, and will warn if configured to do so via `setEuiDevProviderWarning`. For nested theming, use `EuiThemeProvider` instead. ([#6949](elastic/eui#6949)) - Removed `onTrapDeactivation` prop from `EuiPopover`. Use `focusTrapProps.onDeactivation` instead ([#6955](elastic/eui#6955)) **CSS-in-JS conversions** - Converted `EuiFilterGroup` and `EuiFilterButton` to Emotion; Removed styles attached to `.euiFilterGroup__popoverPanel` ([#6957](elastic/eui#6957)) --------- Co-authored-by: Cee Chen <constance.chen@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

## Summary `eui@84.0.0` ⏩ `eui@85.0.1` ## [`85.0.1`](https://github.com/elastic/eui/tree/v85.0.1) **Bug fixes** - Fixed `EuiFilterGroup`'s responsive styles ([elastic#6983](elastic/eui#6983)) ## [`85.0.0`](https://github.com/elastic/eui/tree/v85.0.0) - Updated `EuiThemeProvider` to set an Emotion theme context that returns the values of `useEuiTheme()` ([elastic#6913](elastic/eui#6913)) - Added `size` prop to `EuiStepsHorizontal`, defaulting to the previous size of `m` ([elastic#6928](elastic/eui#6928)) - Added new `s` sizing to `EuiStepsHorizontal` ([elastic#6928](elastic/eui#6928)) - Added `at` and `key` icon glyphs. ([elastic#6934](elastic/eui#6934)) - Added a new `cloneElementWithCss` Emotion utility ([elastic#6939](elastic/eui#6939)) - Updated `EuiPopover` to allow consumer control of all `focusTrapProps` ([elastic#6955](elastic/eui#6955)) **Bug fixes** - Fixed `EuiDataGrid` height calculation bug when browser zoom levels are not 100% ([elastic#6895](elastic/eui#6895)) - Fixed `EuiTab` not correctly passing selection color state to `prepend` and `append` children ([elastic#6938](elastic/eui#6938)) - Fixed `EuiInputPopover` to allow consumer control of its focus trap via `focusTrapProps` ([elastic#6955](elastic/eui#6955)) **Breaking changes** - `EuiProvider` will no longer render multiple or duplicate nested instances of itself. If a nested `EuiProvider` is detected, that instance will return early without further processing, and will warn if configured to do so via `setEuiDevProviderWarning`. For nested theming, use `EuiThemeProvider` instead. ([elastic#6949](elastic/eui#6949)) - Removed `onTrapDeactivation` prop from `EuiPopover`. Use `focusTrapProps.onDeactivation` instead ([elastic#6955](elastic/eui#6955)) **CSS-in-JS conversions** - Converted `EuiFilterGroup` and `EuiFilterButton` to Emotion; Removed styles attached to `.euiFilterGroup__popoverPanel` ([elastic#6957](elastic/eui#6957)) --------- Co-authored-by: Cee Chen <constance.chen@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

cee-chen added 3 commits July 17, 2023 12:16

Give consumers more control over focusTrapProps

3e8782a

- the number of consumers with edge cases / enough knowledge to start overriding props is going to be very small, and they should be allowed to do so if needed

Remove onTrapDeactivation prop in favor of `focusTrapProps.onDeacti…

03f4b2f

…vation`

changelog

75090ef

cee-chen requested a review from Heenawter July 17, 2023 19:18

[EuiInputPopover] Extend same focusTrapProps flexibility

99ede2f

- this is more of a bugfix IMO since this should have been working before

Heenawter mentioned this pull request Jul 17, 2023

[Controls] [PresentationUtil] QoL improvements to control creation form elastic/kibana#162067

Merged

5 tasks

cee-chen marked this pull request as ready for review July 17, 2023 22:40

cee-chen requested a review from 1Copenut July 17, 2023 22:40

cee-chen added the breaking change PRs with breaking changes. (Don't delete - used for automation) label Jul 17, 2023

cee-chen changed the title ~~[EuiPopover][EuiInputPopover] Allow more control via focusTrapProps; deprecate onTrapDeactivation in favor of focusTrapProps.onDeactivation~~ [EuiPopover][EuiInputPopover] Allow more control via focusTrapProps Jul 17, 2023

1Copenut approved these changes Jul 18, 2023

View reviewed changes

cee-chen merged commit 2228bef into elastic:main Jul 18, 2023

cee-chen deleted the popover/focus-trap-props branch July 18, 2023 15:57

tkajtoch mentioned this pull request Jul 19, 2023

Upgrade @elastic/eui to 85.0.1 elastic/kibana#162209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiPopover][EuiInputPopover] Allow more control via `focusTrapProps`#6955

[EuiPopover][EuiInputPopover] Allow more control via `focusTrapProps`#6955
cee-chen merged 4 commits intoelastic:mainfrom
cee-chen:popover/focus-trap-props

cee-chen commented Jul 17, 2023 •

edited

Loading

Uh oh!

cee-chen commented Jul 17, 2023 •

edited

Loading

Uh oh!

kibanamachine commented Jul 17, 2023

Uh oh!

Heenawter commented Jul 17, 2023

Uh oh!

cee-chen commented Jul 17, 2023

Uh oh!

cee-chen commented Jul 17, 2023

Uh oh!

kibanamachine commented Jul 17, 2023

Uh oh!

1Copenut left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

cee-chen commented Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Breaking change

QA

General checklist

Uh oh!

cee-chen commented Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibanamachine commented Jul 17, 2023

Uh oh!

Heenawter commented Jul 17, 2023

Uh oh!

cee-chen commented Jul 17, 2023

Uh oh!

cee-chen commented Jul 17, 2023

Uh oh!

kibanamachine commented Jul 17, 2023

Uh oh!

1Copenut left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cee-chen commented Jul 17, 2023 •

edited

Loading

cee-chen commented Jul 17, 2023 •

edited

Loading