Bug 220940 – Support percentages when parsing color(srgb ...) and color(display-p3 ...) per-spec

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sam Weinig]

Per-spec, we should support percentages when parsing color(srgb ...) and color(display-p3 ...) - https://www.w3.org/TR/css-color-4/#predefined.

[Sam Weinig]

Created attachment 418325 [details]
Patch

[Sam Weinig]

Created attachment 418326 [details]
Patch

[Darin Adler]

Comment on attachment 418326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418326&action=review

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:886
>          if (consumeNumberRaw(args, value))

Seems like it’s time to change consumeNumberRaw to return Optional<double>. I’m tempted to put that patch together.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:906
> +    double colorChannels[3] = { 0, 0, 0 };

IN a short function like this that is all about color, why not just name this "channels"?

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:911
> +        colorChannels[0] = static_cast<float>(*lightness);

Don’t think we need a cast here.

> LayoutTests/TestExpectations:4590
> +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-003.html [ Failure ] # Invalid test, no colorspace specified
> +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-004.html [ Failure ] # Invalid test, no colorspace specified
> +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-007.html [ Failure ] # Requires a98-rgb support
> +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-008.html [ Failure ] # Requires a98-rgb support
> +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-009.html [ Failure ] # Requires prophoto-rgb support
> +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-010.html [ Failure ] # Requires prophoto-rgb support
> +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-011.html [ Failure ] # Requires rec2020 support
> +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-012.html [ Failure ] # Requires rec2020 support
> +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-014.html [ Failure ] # Requires fallback (at parse time) support
> +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-015.html [ Failure ] # Requires fallback (at parse time) support (unclear if this makes sense)
> +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-016.html [ Failure ] # Requires xyz support
> +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-017.html [ Failure ] # Requires xyz support

Love these comments!

[Sam Weinig]

(In reply to Darin Adler from comment #3)
> Comment on attachment 418326 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418326&action=review
> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:886
> >          if (consumeNumberRaw(args, value))
> 
> Seems like it’s time to change consumeNumberRaw to return Optional<double>.
> I’m tempted to put that patch together.

If you don't I will!

> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:906
> > +    double colorChannels[3] = { 0, 0, 0 };
> 
> IN a short function like this that is all about color, why not just name
> this "channels"?

Yeah, that's better.

> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:911
> > +        colorChannels[0] = static_cast<float>(*lightness);
> 
> Don’t think we need a cast here.

Cool.

> 
> > LayoutTests/TestExpectations:4590
> > +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-003.html [ Failure ] # Invalid test, no colorspace specified
> > +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-004.html [ Failure ] # Invalid test, no colorspace specified
> > +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-007.html [ Failure ] # Requires a98-rgb support
> > +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-008.html [ Failure ] # Requires a98-rgb support
> > +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-009.html [ Failure ] # Requires prophoto-rgb support
> > +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-010.html [ Failure ] # Requires prophoto-rgb support
> > +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-011.html [ Failure ] # Requires rec2020 support
> > +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-012.html [ Failure ] # Requires rec2020 support
> > +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-014.html [ Failure ] # Requires fallback (at parse time) support
> > +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-015.html [ Failure ] # Requires fallback (at parse time) support (unclear if this makes sense)
> > +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-016.html [ Failure ] # Requires xyz support
> > +webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-017.html [ Failure ] # Requires xyz support
> 
> Love these comments!

Yeah, meant to land these the first time.

[Sam Weinig]

Created attachment 418335 [details]
Patch

[EWS]

Committed r271866: <https://trac.webkit.org/changeset/271866>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418335 [details].

[Radar WebKit Bug Importer]

<rdar://problem/73591361>