Bug 180528 – Update HSL/HSLA parsing to match CSS Color 4

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

[Chris Nardi]

In CSS Color 4, the HSL and HSLA functions were synonymized, the hue was allowed to be an angle value, the alpha value was allowed to be a percent, and a new syntax was described with whitespace separating the parameters and a backslash separating the optional alpha value. Safari has not implemented any of these changes. This should be tested in the future by https://wpt.fyi/css/css-color (see https://github.com/w3c/web-platform-tests/pull/8598). The spec is at https://drafts.csswg.org/css-color-4/#the-hsl-notation.

[Radar WebKit Bug Importer]

<rdar://problem/35926675>

[Chris Nardi]

Created attachment 329855 [details]
Patch

[Chris Nardi]

Created attachment 329856 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 329855 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

It needs some, obviously. Look in LayoutTests/fast/css/.... for examples.

> Source/WebCore/platform/graphics/Color.cpp:108
> +        hueVal += 6.0;
> +    else if (hueVal >= 6.0)
> +        hueVal -= 6.0;
> +    if (hueVal < 1.0)
> +        return temp1 + (temp2 - temp1) * hueVal;
> +    if (hueVal < 3.0)

So is this changing how we compute hue? That's a little surprising.

> Tools/Scripts/webkitpy/common/checkout/scm/detection.py:6
> +# Copyright (c) 2009, 2010, 2011 Google Inc. All rights reserved.
> +# Copyright (c) 2009 Apple Inc. All rights reserved.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions are
> +# met:

I guess you nuked the line endings on this file.

[Chris Nardi]

Yeah, I messed up with that. I think the new patch should make more sense -- doing all of this from Windows is hard.

As to an overview of this change: this is basically porting my Chromium change at https://chromium-review.googlesource.com/c/chromium/src/+/812125 over to WebKit. I changed the implementation of HSL to RGB because the pseudocode changed in the spec. Functionally, this should fix two bugs as the comparisons were missing equality tests. Overall, it should just be easier to read this implementation as it should be nearly identical with the pseudocode in the spec.

[Chris Nardi]

Additionally, this is tested in WPT, and I notice that none of those tests have been imported by WebKit, so I'm not sure what the best course would be there for testing.

[Chris Nardi]

Created attachment 329857 [details]
Patch

[Simon Fraser (smfr)]

(In reply to Chris Nardi from comment #6)
> Additionally, this is tested in WPT, and I notice that none of those tests
> have been imported by WebKit, so I'm not sure what the best course would be
> there for testing.

If there are behavior changes detected by WPT which webkit has imported, then we'll see that in the EWS output, and your patch should contain some rebaselining of results. If not, then it suggests that the code is not tested and you should add new tests (WPT or otherwise).

[Chris Nardi]

Created attachment 329860 [details]
Patch

[Chris Nardi]

Created attachment 329861 [details]
Patch

[EWS Watchlist]

Comment on attachment 329861 [details]
Patch

Attachment 329861 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5763541

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 329863 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 329861 [details]
Patch

Attachment 329861 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5763566

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 329864 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 329861 [details]
Patch

Attachment 329861 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5763649

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 329869 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Chris Nardi]

Created attachment 329872 [details]
Patch

[Chris Nardi]

Created attachment 329873 [details]
Patch

[EWS Watchlist]

Attachment 329873 [details] did not pass style-queue:


ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/TestExpectations:1029:  Path does not exist.  [test/expectations] [5]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/TestExpectations:1030:  Path does not exist.  [test/expectations] [5]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/TestExpectations:1031:  Path does not exist.  [test/expectations] [5]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:1765:  Path does not exist.  [test/expectations] [5]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:1766:  Path does not exist.  [test/expectations] [5]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:2913:  Path does not exist.  [test/expectations] [5]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:2914:  Path does not exist.  [test/expectations] [5]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:3375:  Path does not exist.  [test/expectations] [5]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:3376:  Path does not exist.  [test/expectations] [5]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:3377:  Path does not exist.  [test/expectations] [5]
Total errors found: 10 in 261 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 329873 [details]
Patch

Attachment 329873 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5764960

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 329879 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 329873 [details]
Patch

Attachment 329873 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5764888

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 329881 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Chris Nardi]

Created attachment 329882 [details]
Patch

[Chris Nardi]

Created attachment 329890 [details]
Patch

[EWS Watchlist]

Attachment 329890 [details] did not pass style-queue:


ERROR: LayoutTests/TestExpectations:1070:  expecting "[", "#", or end of line instead of "ImageOnlyFailure"  [test/expectations] [5]
ERROR: LayoutTests/TestExpectations:1070:  Path does not exist.  [test/expectations] [5]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/TestExpectations:1070:  expecting "[", "#", or end of line instead of "ImageOnlyFailure"  [test/expectations] [5]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/TestExpectations:1070:  Path does not exist.  [test/expectations] [5]
Total errors found: 4 in 265 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Nardi]

Created attachment 329895 [details]
Patch

[Chris Nardi]

Created attachment 329897 [details]
Patch

[Chris Nardi]

It looks like this change should be ready for review. Only wincairo didn't pass, but I suspect the cause is something similar to https://bugs.webkit.org/show_bug.cgi?id=179682#c72.

[Chris Nardi]

Created attachment 331000 [details]
Patch

[Chris Nardi]

Since this is adding new syntax to the HSL/HSLA functions, do I need to take any additional steps with this patch? I notice that https://webkit.org/status/ shows supported for Color 4.

[Dean Jackson]

Comment on attachment 331000 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        This change imports web-platform-tests/css/css-color to test the new behavior.

It also updates the syntax.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:534
> -    colorArray[0] = (((hslValue->intValue() % 360) + 360) % 360) / 360.0;
> +    colorArray[0] = fmod(fmod(angleValue, 360.0) + 360.0, 360.0) / 60.0;

Please add a comment explaining why this is / 60 now. Or, keep it as [0,1] and adjust in calcHue.

(I don't really understand why the specification decided [0,6) was a "simplification")

[Chris Nardi]

(In reply to Dean Jackson from comment #32)
> Comment on attachment 331000 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331000&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        This change imports web-platform-tests/css/css-color to test the new behavior.
> 
> It also updates the syntax.
> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:534
> > -    colorArray[0] = (((hslValue->intValue() % 360) + 360) % 360) / 360.0;
> > +    colorArray[0] = fmod(fmod(angleValue, 360.0) + 360.0, 360.0) / 60.0;
> 
> Please add a comment explaining why this is / 60 now. Or, keep it as [0,1]
> and adjust in calcHue.
> 
> (I don't really understand why the specification decided [0,6) was a
> "simplification")

I added a comment above calcHue(), but I can add this inline in CSSPropertyParserHelpers.cpp too.

[Chris Nardi]

Created attachment 331679 [details]
Patch

[Chris Nardi]

Created attachment 332286 [details]
Rebase after r227595

[EWS Watchlist]

Comment on attachment 332286 [details]
Rebase after r227595

Attachment 332286 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6211307

New failing tests:
http/tests/preload/onerror_event.html
http/tests/misc/bubble-drag-events.html

[EWS Watchlist]

Created attachment 332301 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Chris Nardi]

Dean: any thoughts on the latest version of this patch?

[Chris Nardi]

Created attachment 338448 [details]
Rebase

[Simon Fraser (smfr)]

Comment on attachment 338448 [details]
Rebase

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

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:536
> +    RefPtr<CSSPrimitiveValue> hslValue = consumeAngle(args, cssParserMode, UnitlessQuirk::Forbid);

auto

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:537
> +    double angleValue;

Bonus points if the name indicates if it's degrees or radians.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:546
> +    // The hue needs to be in the range [0.0, 6.0)

This needs a comment to say where the 6 comes from. I assume this feeds into the calcHue logic. Should we convert to the 6-based values here, or keep it as an angle at this point?

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:570
> +            RefPtr<CSSPrimitiveValue> alphaPercent = consumePercent(args, ValueRangeAll);

auto

[Chris Nardi]

Created attachment 338462 [details]
Address comments by Simon

[Chris Nardi]

(In reply to Simon Fraser (smfr) from comment #40)
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:546
> > +    // The hue needs to be in the range [0.0, 6.0)
> 
> This needs a comment to say where the 6 comes from. I assume this feeds into
> the calcHue logic. Should we convert to the 6-based values here, or keep it
> as an angle at this point?

I added "for calcHue()" to hopefully clarify; the other values are already converted to their [0.0, 1.0) values right below so I think it makes sense to convert at this point.

For reference, changing it to 6-based instead of 1-based is because the spec's pseudocode changed in Color 4 (https://drafts.csswg.org/css-color/#hsl-to-rgb), and matching our implementation to the pseudocode should help spot any bugs/make updates easier.

[EWS Watchlist]

Comment on attachment 338462 [details]
Address comments by Simon

Rejecting attachment 338462 [details] from commit-queue.

[e-mail] does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 338462 [details]
Address comments by Simon

Clearing flags on attachment: 338462

Committed r230861: <https://trac.webkit.org/changeset/230861>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This patch broke round-tripping HSL values between color.getHSL() and makeRGBAFromHSLA(). 

makeRGBAFromHSLA () says:
// Hue is in the range of 0 to 6.0, the remainder are in the range 0 to 1.0

and getHSL() wants 0-1 hue.

[Simon Fraser (smfr)]

I filed bug 186642.