Skip to content

Comments

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

Merged
cee-chen merged 3 commits intoelastic:mainfrom
cee-chen:eui-scroll-lock
Mar 20, 2023
Merged

[EUI] Add scrollLock workaround CSS to Kibana's body#153227
cee-chen merged 3 commits intoelastic:mainfrom
cee-chen:eui-scroll-lock

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 PR 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

💚 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

`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 added 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 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR 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