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

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

Merged
merged 4 commits into from
Jul 18, 2023

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Jul 17, 2023

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

cee-chen added 3 commits July 17, 2023 12:16
- 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
@cee-chen cee-chen requested a review from Heenawter July 17, 2023 19:18
@cee-chen
Copy link
Contributor Author

cee-chen commented Jul 17, 2023

@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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor Author

jenkins test this

@cee-chen cee-chen marked this pull request as ready for review July 17, 2023 22:40
@cee-chen cee-chen requested a review from 1Copenut July 17, 2023 22:40
@cee-chen cee-chen changed the title [EuiPopover] Allow more control via focusTrapProps; deprecate onTrapDeactivation in favor of focusTrapProps.onDeactivation [EuiPopover][EuiInputPopover] Allow more control via focusTrapProps; deprecate onTrapDeactivation in favor of focusTrapProps.onDeactivation Jul 17, 2023
@cee-chen 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
@cee-chen
Copy link
Contributor Author

@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
Copy link

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

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

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

@cee-chen cee-chen merged commit 2228bef into elastic:main Jul 18, 2023
@cee-chen cee-chen deleted the popover/focus-trap-props branch July 18, 2023 15:57
Ikuni17 pushed a commit to elastic/kibana that referenced this pull request Jul 27, 2023
## 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>
ThomThomson pushed a commit to ThomThomson/kibana that referenced this pull request Aug 1, 2023
## 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants