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

[EUI] Add scrollLock workaround CSS to Kibana's body #153227

Merged
merged 3 commits into from
Mar 20, 2023

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Mar 15, 2023

Summary

The scrollLock property on EuiFocusTrap "freezes" the width of the of the body so that the removal/addition of the scrollbar when a full screen focus trap opens (e.g. a modal, flyout, or other fullscreen mode) doesn't cause the page width to jump/rerender.

It turns however, that this behavior (which sets a margin-right on the body for the width of the scrollbar) does not work as expected in Kibana due to its existing width: 100%; min-width: 100% CSS on its body. As such, we need to temporarily work around this by setting padding-right instead until theKashey/react-focus-on#65 is fixed.

d59faae can be reverted if not desired in this PR.

Checklist

@cee-chen
Copy link
Contributor Author

@Heenawter Do you want to keep d59faae in this PR, or leave it out for now and wait for elastic/eui#6645 to merge/land in Kibana?

@Heenawter
Copy link
Contributor

@cee-chen I think it's fine to keep it! And then I'll just add the scrollLock to the controls flyout as part of #153065 💃

Copy link
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

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

Oh boy, this was a fun one 🙃 Change discussed at length - LGTM 👍

@cee-chen
Copy link
Contributor Author

Sounds good, thanks! We'll handle reverting d59faae in our next EUI upgrade!

@cee-chen cee-chen marked this pull request as ready for review March 15, 2023 18:40
@cee-chen cee-chen requested review from a team as code owners March 15, 2023 18:40
@cee-chen cee-chen added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.7.0 labels Mar 15, 2023
@cee-chen
Copy link
Contributor Author

Hm, I'm not sure your draft approval counted for some mysterious GitHub reason - you may have to reapprove 😖

@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Mar 15, 2023
@cee-chen
Copy link
Contributor Author

@elasticmachine merge upstream

@elizabetdev elizabetdev self-requested a review March 20, 2023 16:22
Copy link
Contributor

@elizabetdev elizabetdev 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'm approving based on the explanation which makes sense to me.

@cee-chen
Copy link
Contributor Author

Thanks y'all!

@cee-chen cee-chen enabled auto-merge (squash) March 20, 2023 17:01
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
core 350.8KB 350.9KB +122.0B
embeddable 75.3KB 75.3KB +31.0B
total +153.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 433 436 +3

Total ESLint disabled count

id before after diff
securitySolution 513 516 +3

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@cee-chen cee-chen merged commit b665651 into elastic:main Mar 20, 2023
@cee-chen cee-chen deleted the eui-scroll-lock branch March 20, 2023 17:19
v1v added a commit to v1v/kibana that referenced this pull request Mar 20, 2023
…loy-my-kibana-oblt

* upstream/main: (727 commits)
  Upgrade caniuse-lite db (elastic#153318)
  [Security Solution] expanded flyout - right section - json tab implementation (elastic#152935)
  chore(slo): Make APM indicator's index required (elastic#153311)
  skip failing test suite (elastic#136688)
  [Security Solution] Fix security-solution storybook package codeowners (elastic#153307)
  [EUI] Add `scrollLock` workaround CSS to Kibana's `body` (elastic#153227)
  [Cloud Security] Show coming soon deployments of vulnerability management (elastic#153249)
  [Cloud Security] fixed onboarding link directs to cspm integration (elastic#153268)
  [Response Ops][Alerting] Reusable functions for FAAD resource installation (elastic#152849)
  remove geohash_grid aggregation support (elastic#152952)
  [Tech Debt] Reorder Rules page (elastic#152897)
  [Saved Object Finder] Add help text & left button (elastic#152742)
  [Transform] Replace SavedObjectsFinder component (elastic#153128)
  Make pipeline creation endpoint accept a full pipeline definition (elastic#153133)
  [Fleet] Displaying policy changes in Agent activity (elastic#153237)
  skip flaky suite (elastic#152852)
  [Security Solution][Endpoint] Add tests to cover RBAC entries in the Role Kibana Privileges flyout (elastic#153068)
  [Security Solution][Endpoint] Additional tests for Response Console History Log page (covers TestRail manual tests) (elastic#153042)
  [Monitoring] Display node roles in Nodes table (elastic#152127)
  Rename getEditAlertFlyout to getEditRuleFlyout (elastic#153243)
  ...
cee-chen added a commit that referenced this pull request May 1, 2023
## Summary

`[email protected]` ⏩ `[email protected]`

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 <[email protected]>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request May 1, 2023
## Summary

`[email protected]` ⏩ `[email protected]`

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 <[email protected]>
(cherry picked from commit 0564e54)
kibanamachine referenced this pull request 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":"[email protected]"},"sourceCommit":{"committedDate":"2023-05-01T17:05:11Z","message":"Upgrade
EUI to v77.1.2 (#156232)\n\n## Summary\r\n\r\n`[email protected]` ⏩
`[email protected]`\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
<[email protected]>","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`[email protected]` ⏩
`[email protected]`\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
<[email protected]>","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`[email protected]` ⏩
`[email protected]`\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
<[email protected]>","sha":"0564e5434e56ca24c7cbb3fbf4a58d6242714e71"}}]}]
BACKPORT-->

Co-authored-by: Cee Chen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Embedding Embedding content via iFrame release_note:skip Skip the PR/issue when compiling release notes v8.7.0 v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants