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

LibWeb/CSS: Interpret NaN as 0 when resolving alpha #2724

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

LucasChollet
Copy link
Contributor

@LucasChollet LucasChollet commented Dec 3, 2024

Fixes the crash in css/css-color/parsing/color-valid-hwb.html.

The crash was probably introduced in 248e4bb, as it was the first
commit to VERIFY that the value given to Color::with_opacity were in
the correct range. As the values in color-valid-hwb.html were resolved
as NaN, the check never passed.


And that I'm not sure this is the correct place to do this "clamping". Indeed, no clamping is done for other channels, and it seems that the spec only requires it for rgb-based color space (here and that spec issue seems to be relevant).

EDIT from the next day: After reading the spec a bit more:

  1. Clamping + NaN handling is required for rgb and alpha values. So I've also updated the CSSRGB class to handle it.
  2. This is indeed an open question for components of the color function (other than alpha). So let's not touch it for now.

Fixes the crash in css/css-color/parsing/color-valid-hwb.html.

The crash was probably introduced in 248e4bb, as it was the first
commit to VERIFY that the value given to `Color::with_opacity` were in
the correct range. As the values in color-valid-hwb.html were resolved
as NaN, the check never passed.
@LucasChollet LucasChollet marked this pull request as ready for review December 4, 2024 13:45
@AtkinsSJ AtkinsSJ merged commit 6804ce3 into LadybirdBrowser:master Dec 4, 2024
6 checks passed
@LucasChollet LucasChollet deleted the no_more_nan branch December 4, 2024 16:43
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.

2 participants