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

Add implementation warning for browsers #145

Merged
merged 12 commits into from
Dec 19, 2023
Merged

Conversation

jamesnw
Copy link
Contributor

@jamesnw jamesnw commented Nov 28, 2023

Description

Fixes #95.

I ended up with the text "Browser rendering of Oklab for colors outside of the display gamut is incorrect according to the CSS specification."

The link is to Miriam's CSS WG issue.

Steps to test/reproduce

Change between formats, and see that the warning shows for all unbounded formats.

Copy link

netlify bot commented Nov 28, 2023

Deploy Preview for oddcontrast ready!

Name Link
🔨 Latest commit 794feaf
🔍 Latest deploy log https://app.netlify.com/sites/oddcontrast/deploys/6579e19f1ff78d0008f5a011
😎 Deploy Preview https://deploy-preview-145--oddcontrast.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@svgeesus
Copy link

I ended up with the text "Browser rendering of Oklab for colors outside of the display gamut is incorrect according to the CSS specification."

It isn't specific to Oklab (and Oklch) though, is it? You would get the same with CIE Lab and LCH, or color(rec2020 r g b) or indeed `color(srgb r g b) with any component values that are negative.

"Browser rendering for colors outside of the display gamut is incorrect according to the CSS specification."

seems more accurate. I'm concerned that people internalize "Oklab has problems" instead of "browsers unwisely prioritized speed over color consistency, so users have to do extra work".

@jamesnw
Copy link
Contributor Author

jamesnw commented Nov 29, 2023

It isn't specific to Oklab (and Oklch)

@svgeesus Good point, and my description glossed over that the message updates to match whatever space is selected. But your point still stands that it isn't something about oklab itself.

Currently it is only shown for unbounded spaces, but you bring up a fair point about the same issue for values that are out of bounds in srgb, for example.

However, our tool doesn't easily allow the creation of out of bounds values in sRGB, but in the unbounded spaces, it is quite easy to make a color that isn't displayed properly.

My proposal-

  • Remove the color space from the message
  • Continue to only show this banner for unbounded spaces
  • In the coming story where we provide more context on this (and other caveats, like alpha), we make this point more clear.

src/lib/stores.ts Show resolved Hide resolved
src/lib/components/colors/index.svelte Outdated Show resolved Hide resolved
Copy link
Member

@mirisuzanne mirisuzanne left a comment

Choose a reason for hiding this comment

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

Not clear to me if this is waiting for style cleanup? We should either fix the error layout issues here, or make sure that's captured in another issue.

It seems like we show the implementation warning any time you're in a (ok)lab/lch format. Maybe we can only show the warning when you're outside the srgb gamut? (I'm not sure if there's a way for us to query the actual display gamut?)

@jamesnw
Copy link
Contributor Author

jamesnw commented Dec 5, 2023

In grooming today, we decided to remove the banner from the top, and instead add more context to the known color issues panel.

@jamesnw jamesnw changed the base branch from main to color-issues December 13, 2023 16:54
@jamesnw
Copy link
Contributor Author

jamesnw commented Dec 13, 2023

This is now moved to the Known Issues section. The known issue in #146 speaks to browser implementation with clipping. This known issue talks about how the new features now allow creation of out of gamut colors, common ways to make those colors, and why to avoid it.

@jamesnw jamesnw mentioned this pull request Dec 13, 2023
Copy link
Contributor

@dvdherron dvdherron left a comment

Choose a reason for hiding this comment

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

Left some minor copy suggestions but having the warning moved to this section looks good to me. I should be able to pick back on styles before this week ends.

src/lib/components/ratio/ColorIssues.svelte Outdated Show resolved Hide resolved
src/lib/components/ratio/ColorIssues.svelte Outdated Show resolved Hide resolved
src/lib/components/ratio/ColorIssues.svelte Outdated Show resolved Hide resolved
jamesnw and others added 3 commits December 13, 2023 15:44
* color-issues:
  sveltekit does not support vite v5 yet
  pin back stylelint for now
  Automated dependency upgrades
  Automated dependency upgrades
  Bump @adobe/css-tools from 4.3.1 to 4.3.2
src/lib/components/ratio/ColorIssues.svelte Outdated Show resolved Hide resolved
src/lib/components/ratio/ColorIssues.svelte Outdated Show resolved Hide resolved
* color-issues:
  adjust grid spacing
  strong to bold
  alpha order
  adjust grid for color issues
  relocate some styles to lists.scss
  make link styles consistent
  put description text in paragraph
  start cleaning up color issues component styles
@stacyk stacyk removed their assignment Dec 19, 2023
@jgerigmeyer jgerigmeyer merged commit 8833240 into color-issues Dec 19, 2023
3 checks passed
@jgerigmeyer jgerigmeyer deleted the space-warning branch December 19, 2023 19:35
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.

Warn about browser bugs in (ok)lch rendering
6 participants