[EuiFocusTrap] Workaround for scrollLock prop causing nested portals/popovers to be unscrollable
#6744
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…bana - using a workaround recommended by `react-focus-on`'s author
- to confirm that things are working as expected and try to prevent future regressions
- Kibana ignores `margin`, so we default to `padding` instead
cee-chen
force-pushed
the
scroll-lock-fix
branch
from
fa542ce to
4efed01
Compare
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6744/ |
1Copenut
approved these changes
Apr 28, 2023
There was a problem hiding this comment.
👍 LGTM!
I tested using the following QA instructions from @cee-chen:
- Reproduce the bug locally using Kibana issue 156161
- Pull the EUI bugfix branch and build a local lib using
yarn build-pack - Run Kibana using the local EUI lib built in step 2.
- Verify the nested portal allows mouse scroll
I tested using the same Discover view as logged in the original issue. Attaching a short screencast video to affirm the QA change.
scroll-lock-nested-portal-mouseMove.mov
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6744/ |
cee-chen
added a commit
to elastic/kibana
that referenced
this pull request
May 1, 2023
## Summary `eui@77.1.1` ⏩ `eui@77.1.2` This upgrade consists of a backport release intended to fix a major bug where portals within `EuiFlyout`s and `EuiModal`s are not scrollable. fixes #156161 This release also adds functionality that resolves the need for a TODO workaround added in #153227 --- ## [`77.1.2`](https://github.com/elastic/eui/tree/v77.1.2) - Updated `EuiFocusTrap` to support the `gapMode` prop configuration (now defaults to `padding`) ([#6744](elastic/eui#6744)) **Bug fixes** - Fixed the `scrollLock` property on `EuiFocusTrap` (and other components using `EuiFocusTrap`, such as `EuiFlyout` and `EuiModal`) to no longer block scrolling on nested portalled content, such as combobox dropdowns ([#6744](elastic/eui#6744)) --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
May 1, 2023
## Summary `eui@77.1.1` ⏩ `eui@77.1.2` This upgrade consists of a backport release intended to fix a major bug where portals within `EuiFlyout`s and `EuiModal`s are not scrollable. fixes elastic#156161 This release also adds functionality that resolves the need for a TODO workaround added in elastic#153227 --- ## [`77.1.2`](https://github.com/elastic/eui/tree/v77.1.2) - Updated `EuiFocusTrap` to support the `gapMode` prop configuration (now defaults to `padding`) ([elastic#6744](elastic/eui#6744)) **Bug fixes** - Fixed the `scrollLock` property on `EuiFocusTrap` (and other components using `EuiFocusTrap`, such as `EuiFlyout` and `EuiModal`) to no longer block scrolling on nested portalled content, such as combobox dropdowns ([elastic#6744](elastic/eui#6744)) --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 0564e54)
kibanamachine
referenced
this pull request
in elastic/kibana
May 1, 2023
# Backport This will backport the following commits from `main` to `8.8`: - [Upgrade EUI to v77.1.2 (#156232)](#156232) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Cee Chen","email":"549407+cee-chen@users.noreply.github.com"},"sourceCommit":{"committedDate":"2023-05-01T17:05:11Z","message":"Upgrade EUI to v77.1.2 (#156232)\n\n## Summary\r\n\r\n`eui@77.1.1` ⏩ `eui@77.1.2`\r\n\r\nThis upgrade consists of a backport release intended to fix a major bug\r\nwhere portals within `EuiFlyout`s and `EuiModal`s are not scrollable.\r\nfixes https://github.com/elastic/kibana/issues/156161\r\n\r\nThis release also adds functionality that resolves the need for a TODO\r\nworkaround added in https://github.com/elastic/kibana/pull/153227\r\n\r\n---\r\n\r\n## [`77.1.2`](https://github.com/elastic/eui/tree/v77.1.2)\r\n\r\n- Updated `EuiFocusTrap` to support the `gapMode` prop configuration\r\n(now defaults to `padding`)\r\n([#6744](https://github.com/elastic/eui/pull/6744))\r\n\r\n**Bug fixes**\r\n\r\n- Fixed the `scrollLock` property on `EuiFocusTrap` (and other\r\ncomponents using `EuiFocusTrap`, such as `EuiFlyout` and `EuiModal`) to\r\nno longer block scrolling on nested portalled content, such as combobox\r\ndropdowns ([#6744](https://github.com/elastic/eui/pull/6744))\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"0564e5434e56ca24c7cbb3fbf4a58d6242714e71","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","EUI","v8.8.0","v8.9.0"],"number":156232,"url":"https://github.com/elastic/kibana/pull/156232","mergeCommit":{"message":"Upgrade EUI to v77.1.2 (#156232)\n\n## Summary\r\n\r\n`eui@77.1.1` ⏩ `eui@77.1.2`\r\n\r\nThis upgrade consists of a backport release intended to fix a major bug\r\nwhere portals within `EuiFlyout`s and `EuiModal`s are not scrollable.\r\nfixes https://github.com/elastic/kibana/issues/156161\r\n\r\nThis release also adds functionality that resolves the need for a TODO\r\nworkaround added in https://github.com/elastic/kibana/pull/153227\r\n\r\n---\r\n\r\n## [`77.1.2`](https://github.com/elastic/eui/tree/v77.1.2)\r\n\r\n- Updated `EuiFocusTrap` to support the `gapMode` prop configuration\r\n(now defaults to `padding`)\r\n([#6744](https://github.com/elastic/eui/pull/6744))\r\n\r\n**Bug fixes**\r\n\r\n- Fixed the `scrollLock` property on `EuiFocusTrap` (and other\r\ncomponents using `EuiFocusTrap`, such as `EuiFlyout` and `EuiModal`) to\r\nno longer block scrolling on nested portalled content, such as combobox\r\ndropdowns ([#6744](https://github.com/elastic/eui/pull/6744))\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"0564e5434e56ca24c7cbb3fbf4a58d6242714e71"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/156232","number":156232,"mergeCommit":{"message":"Upgrade EUI to v77.1.2 (#156232)\n\n## Summary\r\n\r\n`eui@77.1.1` ⏩ `eui@77.1.2`\r\n\r\nThis upgrade consists of a backport release intended to fix a major bug\r\nwhere portals within `EuiFlyout`s and `EuiModal`s are not scrollable.\r\nfixes https://github.com/elastic/kibana/issues/156161\r\n\r\nThis release also adds functionality that resolves the need for a TODO\r\nworkaround added in https://github.com/elastic/kibana/pull/153227\r\n\r\n---\r\n\r\n## [`77.1.2`](https://github.com/elastic/eui/tree/v77.1.2)\r\n\r\n- Updated `EuiFocusTrap` to support the `gapMode` prop configuration\r\n(now defaults to `padding`)\r\n([#6744](https://github.com/elastic/eui/pull/6744))\r\n\r\n**Bug fixes**\r\n\r\n- Fixed the `scrollLock` property on `EuiFocusTrap` (and other\r\ncomponents using `EuiFocusTrap`, such as `EuiFlyout` and `EuiModal`) to\r\nno longer block scrolling on nested portalled content, such as combobox\r\ndropdowns ([#6744](https://github.com/elastic/eui/pull/6744))\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"0564e5434e56ca24c7cbb3fbf4a58d6242714e71"}}]}] BACKPORT--> Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
mistic
pushed a commit
to elastic/kibana
that referenced
this pull request
May 5, 2023
## Summary `eui@77.1.1` ⏩ `eui@77.2.2` Closes elastic/eui#6724 --- ## [`77.2.2`](https://github.com/elastic/eui/tree/v77.2.2) - Updated `EuiFocusTrap` to support the `gapMode` prop configuration (now defaults to `padding`) ([#6744](elastic/eui#6744)) **Bug fixes** - Fixed the `scrollLock` property on `EuiFocusTrap` (and other components using `EuiFocusTrap`, such as `EuiFlyout` and `EuiModal`) to no longer block scrolling on nested portalled content, such as combobox dropdowns ([#6744](elastic/eui#6744)) ## [`77.2.1`](https://github.com/elastic/eui/tree/v77.2.1) **Bug fixes** - Fixed `EuiFieldNumber`'s native browser validity detection causing extra unnecessary rerenders ([#6741](elastic/eui#6741)) ## [`77.2.0`](https://github.com/elastic/eui/tree/v77.2.0) - Updated `EuiFieldNumber` to detect native browser invalid state and show an invalid icon ([#6704](elastic/eui#6704)) - Improved the input widths of `EuiRange` and `EuiDualRange` when `showInput={true}` to account for invalid icons ([#6704](elastic/eui#6704)) - Improved the `isInvalid` styling of `EuiDualRange` when `showInput="inputWithPopover"` ([#6704](elastic/eui#6704)) - Updated `EuiFormControlLayoutIcons` to render left icons in expected DOM order ([#6705](elastic/eui#6705)) - Updated `EuiDatePickerRange`'s `isInvalid` state to match other range inputs ([#6705](elastic/eui#6705)) - Updated `EuiSuperDatePicker`'s `isInvalid` state to match other range inputs ([#6705](elastic/eui#6705)) **Bug fixes** - Fixed `EuiValidatableControl` to correctly display `isInvalid` states on mount ([#6705](elastic/eui#6705)) - Fixed an issue with `EuiSearchBar` where quoted phrases were not quoted when generating an Elasticsearch query. ([#6714](elastic/eui#6714)) --------- Co-authored-by: Constance Chen <constance.chen.3@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Constance Chen <constance.chen@elastic.co>
This was referenced May 5, 2023
jbudz
pushed a commit
to elastic/kibana
that referenced
this pull request
May 15, 2023
## Summary `eui@77.2.2` ⏩ `eui@79.0.1` 🦴 The primary changes in this upgrade are around the deprecated `EuiLoadingContent` being removed in favor of `EuiSkeletonText`. - Most instances have been a [direct swap of usage](327626a), but [some replacements were a bit more opinionated](e6ceb36) as I saw them as potential to take advantage of `EuiSkeletonText`'s syntactical sugar and screen reader announcements for when state switches to loaded. --- ## [`79.0.1`](https://github.com/elastic/eui/tree/v79.0.1) **Bug fixes** - Fixed broken push `EuiFlyout` behavior ([#6764](elastic/eui#6764)) ## [`79.0.0`](https://github.com/elastic/eui/tree/v79.0.0) - Updated all `EuiSkeleton` components with new props that allow for more control over screen reader live announcements: `announceLoadingStatus`, `announceLoadedStatus`, and `ariaLiveProps` ([#6752](elastic/eui#6752)) - Improved keyboard accessibility in `EuiPageHeader` by ensuring the right side menu items come into focus from left to right. ([#6753](elastic/eui#6753)) **Breaking changes** - Removed deprecated `EuiLoadingContent`. Use the `EuiSkeleton` components instead. ([#6754](elastic/eui#6754)) ## [`78.0.0`](https://github.com/elastic/eui/tree/v78.0.0) - Improved the contrast ratio of `EuiCheckbox`, `EuiRadio`, and `EuiSwitch` in their unchecked states to meet WCAG AA guidelines. ([#6729](elastic/eui#6729)) - Added React Testing Library `*ByTestSubject` custom commands to `within()`. RTL utilities can be imported from `@elastic/eui/lib/test/rtl`. ([#6737](elastic/eui#6737)) - Updated `EuiAvatar` to support a new letter `casing` prop that allow customizing text capitalization ([#6739](elastic/eui#6739)) - Updated `EuiFocusTrap` to support the `gapMode` prop configuration (now defaults to `padding`) ([#6744](elastic/eui#6744)) **Bug fixes** - Fixed inconsistency in `EuiSearchBar`'s AND/OR semantics between DSL and query string generation ([#6717](elastic/eui#6717)) - Fixed `EuiFieldNumber`'s native browser validity detection causing extra unnecessary rerenders ([#6741](elastic/eui#6741)) - Fixed the `scrollLock` property on `EuiFocusTrap` (and other components using `EuiFocusTrap`, such as `EuiFlyout` and `EuiModal`) to no longer block scrolling on nested portalled content, such as combobox dropdowns ([#6744](elastic/eui#6744)) **Breaking changes** - `EuiAvatar`s with the default `user` type will now default to capitalizing all initials in uppercase ([#6739](elastic/eui#6739)) --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jasonrhodes
pushed a commit
to elastic/kibana
that referenced
this pull request
May 17, 2023
## Summary `eui@77.2.2` ⏩ `eui@79.0.1` 🦴 The primary changes in this upgrade are around the deprecated `EuiLoadingContent` being removed in favor of `EuiSkeletonText`. - Most instances have been a [direct swap of usage](327626a), but [some replacements were a bit more opinionated](e6ceb36) as I saw them as potential to take advantage of `EuiSkeletonText`'s syntactical sugar and screen reader announcements for when state switches to loaded. --- ## [`79.0.1`](https://github.com/elastic/eui/tree/v79.0.1) **Bug fixes** - Fixed broken push `EuiFlyout` behavior ([#6764](elastic/eui#6764)) ## [`79.0.0`](https://github.com/elastic/eui/tree/v79.0.0) - Updated all `EuiSkeleton` components with new props that allow for more control over screen reader live announcements: `announceLoadingStatus`, `announceLoadedStatus`, and `ariaLiveProps` ([#6752](elastic/eui#6752)) - Improved keyboard accessibility in `EuiPageHeader` by ensuring the right side menu items come into focus from left to right. ([#6753](elastic/eui#6753)) **Breaking changes** - Removed deprecated `EuiLoadingContent`. Use the `EuiSkeleton` components instead. ([#6754](elastic/eui#6754)) ## [`78.0.0`](https://github.com/elastic/eui/tree/v78.0.0) - Improved the contrast ratio of `EuiCheckbox`, `EuiRadio`, and `EuiSwitch` in their unchecked states to meet WCAG AA guidelines. ([#6729](elastic/eui#6729)) - Added React Testing Library `*ByTestSubject` custom commands to `within()`. RTL utilities can be imported from `@elastic/eui/lib/test/rtl`. ([#6737](elastic/eui#6737)) - Updated `EuiAvatar` to support a new letter `casing` prop that allow customizing text capitalization ([#6739](elastic/eui#6739)) - Updated `EuiFocusTrap` to support the `gapMode` prop configuration (now defaults to `padding`) ([#6744](elastic/eui#6744)) **Bug fixes** - Fixed inconsistency in `EuiSearchBar`'s AND/OR semantics between DSL and query string generation ([#6717](elastic/eui#6717)) - Fixed `EuiFieldNumber`'s native browser validity detection causing extra unnecessary rerenders ([#6741](elastic/eui#6741)) - Fixed the `scrollLock` property on `EuiFocusTrap` (and other components using `EuiFocusTrap`, such as `EuiFlyout` and `EuiModal`) to no longer block scrolling on nested portalled content, such as combobox dropdowns ([#6744](elastic/eui#6744)) **Breaking changes** - `EuiAvatar`s with the default `user` type will now default to capitalizing all initials in uppercase ([#6739](elastic/eui#6739)) --------- 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes an issue discovered by @Heenawter and reported by many other Elastic engineers - all the credit to them for narrowing down this problematic behavior. This fix/workaround will need to be backported to Kibana v8.8.
The problem
The
scrollLockfunctionality added to EuiFlyout in #6645 unfortunately appears to prevent scrolling on anything that's not the flyout itself (in Kibana specifically, but not in CodeSandbox, strangely enough). This includes popovers, dropdowns, and comboboxes within flyouts, and by extrapolation, modals.Screenshot in elastic/kibana#156161
The solution
The solution came from the author of our 3rd party focus trap dependency: theKashey/react-focus-on#49:
I've followed the exact recommendations of the library author in 4412a1f, and am super happy to report that not only is the issue fixed, we can now support the
gapModeprop & change it topaddingby default, which means we can remove the Kibana-specific padding workaround added in elastic/kibana#153227.Other notes
As is the way of things, writing the Cypress tests for this bug took more time than the actual bugfix itself 💦 Thankfully these tests should give us confidence that 1. the fix actually works, and 2. we won't regress it again in the future, if our 3rd party dependencies change.
QA
General checklist
@defaultif default values are missing)and playground togglesjest andcypress tests- [ ] Checked for breaking changes and labeled appropriately- I'm leaning against marking this as a breaking change (despite the new default topadding, where react-focus-on previously defaulted tomargin), because it's an implementation detail I doubt many devs will notice. If I'm wrong, I'll take the fall for it, but we also need to move a little faster than normal to get this fix in for v8.8.