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

Chromium workaround for CSS mask-image bug #7855

Merged

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Jun 25, 2024

Summary

This bug was originally addressed in #6328 / #6343, but it turns out that it's affecting more components than just EuiModal (at least EuiFlyout as well - see elastic/kibana#180828). The browser-level bug in question is being tracked in https://issues.chromium.org/issues/40778541.

The amazing @kqualters-elastic helped us look into the issue and came up with an excellent workaround. Chromium appears to be not correctly applying a stacking context to mask-images with linear gradients and certain other children/properties. Using transform: translateZ(0) forces that stacking context and fixes the bug.

NOTE: will-change: transform and position: relative also works around the bug, but I deemed them to potentially have outsized side effects compared to translateZ.

Broken Fixed

QA

  • Open Chrome or Edge and go to https://eui.elastic.co/pr_7855/#/layout/modal (bug does not affect Firefox or Safari)
  • Click the Show Modal button
  • Confirm the modal body content does not have a red overlay
  • Open your browser devtools and inspect .euiModalBody__overflow
  • Hide/uncheck the transform: translateZ(0) CSS and confirm that it does have a red overlay without the workaround CSS

General checklist

  • Revert REVERT ME commit
  • Browser QA
    • Checked in both light and dark modes
    • Checked in Chrome, Safari, Edge, and Firefox
      - [ ] Checked in mobile
      - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA - N/A, CSS only
  • Code quality checklist - N/A, CSS only
  • Release checklist
    • A changelog entry exists and is marked appropriately.
      - [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist - N/A

@cee-chen cee-chen changed the title Chromium workaround for mask-image CSS bug Chromium workaround for CSS mask-image bug Jun 25, 2024
@cee-chen cee-chen marked this pull request as ready for review June 25, 2024 23:44
@cee-chen cee-chen requested a review from a team as a code owner June 25, 2024 23:44
@kqualters-elastic
Copy link
Contributor

might want to do the same on these lines in a scss mixin https://github.com/elastic/eui/blob/main/packages/eui/src/global_styling/mixins/_shadow.scss#L97-#L99

Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

🚢 🐈‍⬛ Looks good to me! Thanks for digging into this. I can confirm it's fixed in Chrome and Edge 👍

- eventually going to be deprecated, but we can cross that bridge when we get to it
@cee-chen
Copy link
Contributor Author

@kqualters-elastic Fantastic suggestion! I'll add that here as well, but as a heads up, we're looking to deprecate Sass mixins in the future as there isn't an easy way to keep them dynamic to theme changes/customizations compared to CSS-in-JS.

@cee-chen cee-chen enabled auto-merge (squash) June 26, 2024 15:42
@cee-chen cee-chen self-assigned this Jun 26, 2024
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @cee-chen

@cee-chen cee-chen merged commit b1370af into elastic:main Jun 26, 2024
5 checks passed
@cee-chen cee-chen deleted the overflow-shadow-mask-chromium-workaround branch June 26, 2024 16:28
@asharma-5
Copy link

Hi Team,
we are also facing issue while expanding document from discover tab.
Will this fix work for below issue as well.
image

@cee-chen
Copy link
Contributor Author

@asharma-5 It will, yes!

@asharma-5
Copy link

@cee-chen Thanks for the response.
Getting this same issue in older releases (8.7.0, 8.8.2) too, are we going to backport to older releases?

@cee-chen
Copy link
Contributor Author

@asharma-5 We can if requested, but we typically only do so if there's a specific Kibana team or customer requesting the fix. Please let us know if you would like to make that request!

@asharma-5
Copy link

asharma-5 commented Jul 1, 2024

@cee-chen can you please provide the version of Kibana/elasticsearch in which this fix is released.

tkajtoch added a commit to elastic/kibana that referenced this pull request Jul 13, 2024
`v95.2.0`⏩`v95.3.0`

_[Questions? Please see our Kibana upgrade
FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_

---

## [`v95.3.0`](https://github.com/elastic/eui/releases/v95.3.0)

- Updated `EuiThemeProvider`s to allow modifying/setting custom
`breakpoint`s in nested usage (as opposed to only at the top
`EuiProvider` level) ([#7862](elastic/eui#7862))

**Bug fixes**

- Fixed a Chrome/Edge CSS `mask-image` bug that was affecting scroll
overflow shadow utilties
([#7855](elastic/eui#7855))

**CSS-in-JS conversions**

- Converted `EuiColorPicker` to Emotion; Removed `$euiColorPickerWidth`
([#7845](elastic/eui#7845))
- Converted `EuiColorPickerSwatch` to Emotion
([#7853](elastic/eui#7853))
- Converted `EuiColorPalettePicker` and `EuiColorPaletteDisplay` to
Emotion ([#7854](elastic/eui#7854))
  - Removed `$euiColorPaletteDisplaySizes`
  - Removed `@mixin euiColorPaletteInnerBorder`
- Removed `$euiColorPickerValueRange0`, `$euiColorPickerValueRange1`,
`$euiColorPickerSaturationRange0`, `$euiColorPickerSaturationRange1`,
and `$euiColorPickerIndicatorSize`
([#7859](elastic/eui#7859))

**Accessibility**

- Updated the `aria-label` attribute for the `EuiFilePicker` remove file
button ([#7860](elastic/eui#7860))

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@cee-chen
Copy link
Contributor Author

@asharma-5 Sorry for the delay in responding, I was out on PTO. The fix will be in Kibana v8.15.0+.

nickpeihl added a commit to elastic/kibana that referenced this pull request Jul 24, 2024
## Summary

Fixes positioning of dragged links in the Links panel editor.

The issue was caused by the `transform: translateZ(0)` workaround in EUI
to fix a [mask image bug](elastic/eui#7855) in
Chromium. We fix this by overriding the `transform` for in the
`FlyoutBody` of the Links panel editor.

Before:


https://github.com/user-attachments/assets/8ad10732-dfaa-4464-845b-0a9c4fc6b173

After:



https://github.com/user-attachments/assets/e6f0bffe-7eb0-4590-affc-a89bc86b973d
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 24, 2024
## Summary

Fixes positioning of dragged links in the Links panel editor.

The issue was caused by the `transform: translateZ(0)` workaround in EUI
to fix a [mask image bug](elastic/eui#7855) in
Chromium. We fix this by overriding the `transform` for in the
`FlyoutBody` of the Links panel editor.

Before:

https://github.com/user-attachments/assets/8ad10732-dfaa-4464-845b-0a9c4fc6b173

After:

https://github.com/user-attachments/assets/e6f0bffe-7eb0-4590-affc-a89bc86b973d
(cherry picked from commit e566abf)
kibanamachine added a commit to elastic/kibana that referenced this pull request Jul 24, 2024
…) (#189129)

# Backport

This will backport the following commits from `main` to `8.15`:
- [[Links] Fix positioning of dragged link in links editor
(#189122)](#189122)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Nick
Peihl","email":"nick.peihl@elastic.co"},"sourceCommit":{"committedDate":"2024-07-24T19:49:43Z","message":"[Links]
Fix positioning of dragged link in links editor (#189122)\n\n##
Summary\r\n\r\nFixes positioning of dragged links in the Links panel
editor.\r\n\r\nThe issue was caused by the `transform: translateZ(0)`
workaround in EUI\r\nto fix a [mask image
bug](elastic/eui#7855) in\r\nChromium. We fix
this by overriding the `transform` for in the\r\n`FlyoutBody` of the
Links panel
editor.\r\n\r\nBefore:\r\n\r\n\r\nhttps://github.com/user-attachments/assets/8ad10732-dfaa-4464-845b-0a9c4fc6b173\r\n\r\nAfter:\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/e6f0bffe-7eb0-4590-affc-a89bc86b973d","sha":"e566abf1cb6317c381431c4902abd5554fea6217","branchLabelMapping":{"^v8.16.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:Presentation","v8.15.0","v8.16.0"],"title":"[Links]
Fix positioning of dragged link in links
editor","number":189122,"url":"https://github.com/elastic/kibana/pull/189122","mergeCommit":{"message":"[Links]
Fix positioning of dragged link in links editor (#189122)\n\n##
Summary\r\n\r\nFixes positioning of dragged links in the Links panel
editor.\r\n\r\nThe issue was caused by the `transform: translateZ(0)`
workaround in EUI\r\nto fix a [mask image
bug](elastic/eui#7855) in\r\nChromium. We fix
this by overriding the `transform` for in the\r\n`FlyoutBody` of the
Links panel
editor.\r\n\r\nBefore:\r\n\r\n\r\nhttps://github.com/user-attachments/assets/8ad10732-dfaa-4464-845b-0a9c4fc6b173\r\n\r\nAfter:\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/e6f0bffe-7eb0-4590-affc-a89bc86b973d","sha":"e566abf1cb6317c381431c4902abd5554fea6217"}},"sourceBranch":"main","suggestedTargetBranches":["8.15"],"targetPullRequestStates":[{"branch":"8.15","label":"v8.15.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/189122","number":189122,"mergeCommit":{"message":"[Links]
Fix positioning of dragged link in links editor (#189122)\n\n##
Summary\r\n\r\nFixes positioning of dragged links in the Links panel
editor.\r\n\r\nThe issue was caused by the `transform: translateZ(0)`
workaround in EUI\r\nto fix a [mask image
bug](elastic/eui#7855) in\r\nChromium. We fix
this by overriding the `transform` for in the\r\n`FlyoutBody` of the
Links panel
editor.\r\n\r\nBefore:\r\n\r\n\r\nhttps://github.com/user-attachments/assets/8ad10732-dfaa-4464-845b-0a9c4fc6b173\r\n\r\nAfter:\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/e6f0bffe-7eb0-4590-affc-a89bc86b973d","sha":"e566abf1cb6317c381431c4902abd5554fea6217"}}]}]
BACKPORT-->

Co-authored-by: Nick Peihl <nick.peihl@elastic.co>
@asharma-5
Copy link

asharma-5 commented Jul 26, 2024

@cee-chen Thanks for the response, when 8.15.0 will be available ?

@cee-chen
Copy link
Contributor Author

That I'm not sure about, sorry, I don't closely track Elastic's release schedule. It should be sometime soon however.

@asharma-5
Copy link

@cee-chen Can you please check internally and provide an tentative date ?

@cee-chen
Copy link
Contributor Author

cee-chen commented Aug 6, 2024

I am not going to make that information publicly available before Elastic's official announcement, sorry!

@asharma-5
Copy link

asharma-5 commented Sep 10, 2024

@cee-chen i have upgraded to 8.15.0 but still seeing the same issue, Is the fix included in 8.15.0 ??

image

elasticsearch@elasticsearch-0:/opt/elasticsearch/bin$ ./elasticsearch --version
Version: 8.15.0, Build: tar/1a77947f34deddb41af25e6f0ddb8e830159c179/2024-08-05T10:05:34.233336849Z, JVM: 22.0.1

./kibana --version
{"log.level":"info","@timestamp":"2024-09-10T10:41:03.389Z","log.logger":"elastic-apm-node","ecs.version":"8.10.0","agentVersion":"4.7.0","env":{"pid":109,"proctitle":"./../node/glibc-217/bin/node","os":"linux 5.4.0-100-generic","arch":"x64","host":"kibana-0","timezone":"UTC+00","runtime":"Node.js v20.15.1"},"config":{"active":{"source":"start","value":true},"breakdownMetrics":{"source":"start","value":false},"captureBody":{"source":"start","value":"off","commonName":"capture_body"},"captureHeaders":{"source":"start","value":false},"centralConfig":{"source":"start","value":false},"contextPropagationOnly":{"source":"start","value":true},"environment":{"source":"start","value":"production"},"globalLabels":{"source":"start","value":[["git_rev","8aa0b59da12c996e3048d8875446667ee6e15c7f"]],"sourceValue":{"git_rev":"8aa0b59da12c996e3048d8875446667ee6e15c7f"}},"logLevel":{"source":"default","value":"info","commonName":"log_level"},"metricsInterval":{"source":"start","value":120,"sourceValue":"120s"},"serverUrl":{"source":"start","value":"https://kibana-cloud-apm.apm.us-east-1.aws.found.io/","commonName":"server_url"},"transactionSampleRate":{"source":"start","value":0.1,"commonName":"transaction_sample_rate"},"captureSpanStackTraces":{"source":"start","sourceValue":false},"secretToken":{"source":"start","value":"[REDACTED]","commonName":"secret_token"},"serviceName":{"source":"start","value":"kibana","commonName":"service_name"},"serviceVersion":{"source":"start","value":"8.15.0","commonName":"service_version"}},"activationMethod":"require","message":"Elastic APM Node.js Agent v4.7.0"}
8.15.0

can you please help
cc @tkajtoch

@cee-chen
Copy link
Contributor Author

cee-chen commented Sep 12, 2024

Ah, it looks like Tomasz missed the feature freeze cutoff for 8.15 in elastic/kibana#187342 by a few days, and unfortunately the backport:skip label was added to the PR which prevented it from being automatically backported to the 8.15 branch. I'll manually kick off a backport here in a bit, and the fix will make it into Kibana v8.15.2.

Thank you for your patience on this @asharma-5. As a heads up, this bug only affects Chrome, so temporarily switching to the Firefox or Safari browser will resolve the issue.

cee-chen pushed a commit to cee-chen/kibana that referenced this pull request Sep 12, 2024
`v95.2.0`⏩`v95.3.0`

_[Questions? Please see our Kibana upgrade
FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_

---

- Updated `EuiThemeProvider`s to allow modifying/setting custom
`breakpoint`s in nested usage (as opposed to only at the top
`EuiProvider` level) ([elastic#7862](elastic/eui#7862))

**Bug fixes**

- Fixed a Chrome/Edge CSS `mask-image` bug that was affecting scroll
overflow shadow utilties
([elastic#7855](elastic/eui#7855))

**CSS-in-JS conversions**

- Converted `EuiColorPicker` to Emotion; Removed `$euiColorPickerWidth`
([elastic#7845](elastic/eui#7845))
- Converted `EuiColorPickerSwatch` to Emotion
([elastic#7853](elastic/eui#7853))
- Converted `EuiColorPalettePicker` and `EuiColorPaletteDisplay` to
Emotion ([elastic#7854](elastic/eui#7854))
  - Removed `$euiColorPaletteDisplaySizes`
  - Removed `@mixin euiColorPaletteInnerBorder`
- Removed `$euiColorPickerValueRange0`, `$euiColorPickerValueRange1`,
`$euiColorPickerSaturationRange0`, `$euiColorPickerSaturationRange1`,
and `$euiColorPickerIndicatorSize`
([elastic#7859](elastic/eui#7859))

**Accessibility**

- Updated the `aria-label` attribute for the `EuiFilePicker` remove file
button ([elastic#7860](elastic/eui#7860))

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
cee-chen added a commit to elastic/kibana that referenced this pull request Sep 12, 2024
## Backport

This is a manual backport of EUI v95.3.0 (#187342) which was intended to
target 8.15 but was missed due to the `backport:skip` label.

The release contains a requested visual bugfix for Chromium users:
elastic/eui#7855 (comment)

Co-authored-by: Tomasz Kajtoch <tomasz.kajtoch@elastic.co>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@asharma-5
Copy link

@cee-chen Thanks for the response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants