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

Don't use color-mix(…) on currentColor #17247

Merged
merged 2 commits into from
Mar 17, 2025
Merged

Conversation

philipp-spiess
Copy link
Member

@philipp-spiess philipp-spiess commented Mar 17, 2025

Closes #17194

This PR works around a crash when rendering opacity on currentColor (as used by the placeholder styles in preflight) on Safari 16.4 and Safari 16.5. Unfortunately it seems that the color-mix(…) function is not compatible with currentColor for these versions of Safari. We tried a few different ways to work around this without success:

The work-around of using the oklab(…) function does seem to work for ::placeholder styles in preflight though according to our testing so this PR applies this change to preflight.

Test plan

Screenshot 2025-03-17 at 11 32 47

@philipp-spiess philipp-spiess requested a review from a team as a code owner March 17, 2025 10:33
Copy link
Member

@RobinMalfait RobinMalfait left a comment

Choose a reason for hiding this comment

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

Do we also want to apply this for other usages of currentColor?

E.g.: bg-current/20 would still use color-mix(…) I think

We might be able to replace our withAlpha helper internally to use the same trick here if it's used with currentColor

@philipp-spiess
Copy link
Member Author

@RobinMalfait Yeah I'm not sure. I guess it's better than hard crashing but the fix does not appear to work as expected in those cases (see PR description). It also won't work with CSS or theme variables set to currentColor, only when the literal value is used. That said, prob should still do it because the alternative is literally broken CSS :/ WDYT?

@philipp-spiess
Copy link
Member Author

Updated the PR

@RobinMalfait
Copy link
Member

Good morning! Oops completely read past that part and just looked at the code 🙈.

That's really strange that it doesn't work in other spots. Really annoying that it also still crashes when using a @supports query. So if I understand correctly:

  1. color-mix(…) with currentColor -> crash
  2. oklab(…) workaround with currentColor in ::placeholder -> works
  3. oklab(…) workaround with currentColor in anywhere else -> no crash, but doesn't work visually either

If that's true, then I think we should update it to oklab to get rid of the crash. Just need to verify that it works in other browsers as expected because if bg-current/20 stopped working in other browsers that would be really sad.

My initial testing says that that still works: https://play.tailwindcss.com/adWyxqL0Ns?file=css 🤞

@philipp-spiess
Copy link
Member Author

@RobinMalfait Yep, if you take a look at my test plan, I tested on Chrome/Safari 16.4/Safari 18.3/Firefox and also found it working everywhere (even on latest Safari) as expected. Just old Safari does not apply the opacity which IMO is, like you said, better than a hard-crash.

Copy link
Member

@RobinMalfait RobinMalfait left a comment

Choose a reason for hiding this comment

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

Once CI passes this is good 👍

@philipp-spiess philipp-spiess merged commit d6d913e into main Mar 17, 2025
5 checks passed
@philipp-spiess philipp-spiess deleted the fix/safari-16-crash branch March 17, 2025 12:23
philipp-spiess added a commit that referenced this pull request Mar 20, 2025
philipp-spiess added a commit that referenced this pull request Mar 20, 2025
philipp-spiess added a commit that referenced this pull request Mar 20, 2025
philipp-spiess added a commit that referenced this pull request Mar 20, 2025
… around Preflight crash

This reverts commit d6d913e.
philipp-spiess added a commit that referenced this pull request Mar 20, 2025
… around Preflight crash

This reverts commit d6d913e.
philipp-spiess added a commit that referenced this pull request Mar 20, 2025
… around Preflight crash (#17306)

Closes #17194.

This reverts commit d6d913e.

The initial fix does breaks older versions of Chrome (where text won't
render with a color for the placeholder at all anymore) and the usage of
the _relative colors_ features also means it'll require a much newer
version of Safari/Firefox/Chrome to work correctly. The implementation
was also wrong as it always set alpha to the specific percent instead of
applying it additively (note that this can be fixed with `calc(alpha *
opacity)` though).

Instead we decided to fix this by adding a `@supports` query to
Preflight that only targets browsers that aren't affected by the crash.
We currently use the following workaround:

```css
/*
  Set the default placeholder color to a semi-transparent version of the current text color in browsers that do not
  crash when using `color-mix(…)` with `currentColor`. (#17194)
*/

@supports (not (-webkit-appearance: -apple-pay-button)) /* Not Safari */ or
  (contain-intrinsic-size: 1px) /* Safari 17+ */ {
  ::placeholder {
    color: color-mix(in oklab, currentColor 50%, transparent);
  }
}
```

## Test plan

When testing the `color-mix(currentColor)` vs `oklab(from currentColor
…)` we created the following support matrix. I'm extending it with _our
fix_ which is the fix ended up using:

| Browser | Version | `color-mix(… currentColor …)` | `oklab(from
currentColor …)` | `@supports { color-mix(…) }` |
| ------- | ------- | ----------------------------- |
---------------------------- | ------- |
| Chrome | 111 | ❌ | ❌ | ❌ |
| Chrome | 116 | ✅ | ❌ | ✅ |
| Chrome | 131+ | ✅ | ✅ | ✅ |
| Safari | 16.4 | 💥 | ❌ | ❌ |
| Safari | 16.6+ | ✅ | ❌ | ❌ |
| Safari | 18+ | ✅ | ✅ | ✅ |
| Firefox | 128 | ✅ | ❌ | ✅ |
| Firefox | 133 | ✅ | ✅ | ✅ |

Note that on Safari 16, this change makes it so that the browser does
not crash yet it still won't work either. That's because now the browser
will fall back to the default placeholder color instead. We used the
following play to test the fix: https://play.tailwindcss.com/RF1RYbZLKY
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.

Safari 16.4 crash on <input placeholder> or text-current + opacity
2 participants