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

[cssom-1][css-color-4] Move serialization of <color> and <alpha-value> #9798

Closed
cdoublev opened this issue Jan 15, 2024 · 13 comments
Closed
Labels

Comments

@cdoublev
Copy link
Collaborator

According to #7870 (comment), how to serialize <color> should be removed from CSSOM and how to serialize <alpha-value> should be moved to CSS Color 4.

If I am not mistaken, the algorithm for <alpha-value> is required when stored internally as an 8-bit integer, which seems to be the case for most browsers (if not all), and helps explaining why 0.499 may be serialized to 0.498, for example.

@svgeesus
Copy link
Contributor

Yes, I see that there is still an inlline issue wondering what to do about case folding while #7870 (comment) (and subsequent discussion) says that ASCII-lowercase folding is done, as does https://drafts.csswg.org/css-color-4/#serializing-color-values which was then adopted by CSSWG resolution. So that inline issue should be deleted.

@svgeesus
Copy link
Contributor

I agree that the algorithm for serializing internal 8-bit alpha values needs to be moved over to CSS Color 4, which currently does not cover that case.

@svgeesus
Copy link
Contributor

@emilio what is your preference, should I:

  • update css-color-4 then hand the editing baton to you for cssom-1
  • send a PR for cssom-1 once css-color-4 is updated
  • make the edits in both specs?

@svgeesus
Copy link
Contributor

svgeesus commented Jan 30, 2024

@emilio CSS Color 4 has now been updated to add the 8-bit unsigned alpha serialization logic from cssom-1 in a28b013

I also added a couple of worked examples.

Would you like a PR on cssom-1 to remove it from there, or should I just do it?

@svgeesus
Copy link
Contributor

@cdoublev

If I am not mistaken, the algorithm for <alpha-value> is required when stored internally as an 8-bit integer, which seems to be the case for most browsers (if not all), and helps explaining why 0.499 may be serialized to 0.498, for example.

0.499 × 255 = 127.245 so if rounded and stored as uint8_t it will be 127
Math.round(50 * 2.55) = 127
So with trailing zeroes omitted it should be serialized as "0.5", no?

@emilio
Copy link
Collaborator

emilio commented Jan 30, 2024

Would you like a PR on cssom-1 to remove it from there, or should I just do it?

Either way works for me.

@emilio
Copy link
Collaborator

emilio commented Jan 30, 2024

(Thanks for taking care of this, I had missed the previous ping)

@cdoublev
Copy link
Collaborator Author

So with trailing zeroes omitted it should be serialized as "0.5", no?

50 * 2.55 = 127.5 but it is 127.499999... because of floating point precision. Rounded up, 127.5 is 128, which is not equal to 127, so the next branch of the algorithm applies.

To be honest, I cannot say I understand all the intricacies of this algorithm and the relation with unsigned 8-bit representation, but 0.498 is the result in Chrome and Firefox.

@svgeesus
Copy link
Contributor

svgeesus commented Feb 9, 2024

@emilio @tabatkins do you know why there is a cssom-specific "rounding up" specified, rather than "round to +infinity" like everywhere else in CSS? Which would give 127 in the example @cdoublev mentioned?

Also do we know the history of this 8bit alpha algorithm, is it baked into Web compat at this point?

@tabatkins
Copy link
Member

It was added by @zcorpan ten years ago: 1796eb4

I presume we didn't have an obvious precedent at the time so they just wrote something that seemed reasonable.

@zcorpan
Copy link
Member

zcorpan commented Feb 14, 2024

Yeah, or I was trying to match implemented behavior in some browser.

@svgeesus
Copy link
Contributor

To be honest, I cannot say I understand all the intricacies of this algorithm and the relation with unsigned 8-bit representation, but 0.498 is the result in Chrome and Firefox.

I came across this in Chrome:

    // Legacy color channels get serialized with integers in the range [0,255].
    // Channels that have a value of exactly 0.5 can get incorrectly rounded
    // down to 127 when being converted to an integer. Add a small epsilon to
    // avoid this. See crbug.com/1425856.
    std::tie(r, g, b) =
        gfx::SRGBToSRGBLegacy(r + kEpsilon, g + kEpsilon, b + kEpsilon);

@cdoublev
Copy link
Collaborator Author

This is about color channels, not the alpha channel, whose related code appears a few lines below, which is matching the specified algorithm, and which has been introduced the first time in this commit.

While digging, I stumbled upon this commit message:

Previously, rgb() and rgba() parsers were compressing all four color channels and alpha to 8-bit integers. This is no longer necessary, as blink::Color stores the parameters as floats.

As we move towards implementing relative color, the more unified the color parsing is, the easier the eventual implementation will be.

After some discussion for Interop 2023, it was decided that non-finite color parameters should round-trip and be resolved sanely: http://github.com/w3c/csswg-drafts/issues/8629

This new requirement obviously puts us well out of 8-bit integer territory.

Unfortunately, there are clearly still some code paths that are independently compressing alpha to be an 8-bit integer and unless we explicitly do this in the parser, many tests fail.

I still do not understand the rounding with 3 decimal places, but maybe things could be simplified now.

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

No branches or pull requests

5 participants