Bug 180528 - Update HSL/HSLA parsing to match CSS Color 4
Summary: Update HSL/HSLA parsing to match CSS Color 4
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar, WebExposed
Depends on:
Blocks:
 
Reported: 2017-12-07 07:34 PST by Chris Nardi
Modified: 2018-06-14 20:55 PDT (History)
9 users (show)

See Also:


Attachments
Patch (15.93 KB, patch)
2017-12-19 16:55 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (8.15 KB, patch)
2017-12-19 17:00 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (8.17 KB, patch)
2017-12-19 17:13 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (10.22 KB, patch)
2017-12-19 17:33 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (626.32 KB, patch)
2017-12-19 17:37 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (899.51 KB, application/zip)
2017-12-19 18:11 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.33 MB, application/zip)
2017-12-19 18:18 PST, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (909.17 KB, application/zip)
2017-12-19 18:35 PST, Build Bot
no flags Details
Patch (834.97 KB, patch)
2017-12-19 18:43 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (822.94 KB, patch)
2017-12-19 18:47 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.10 MB, application/zip)
2017-12-19 19:41 PST, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-elcapitan (1.88 MB, application/zip)
2017-12-19 19:48 PST, Build Bot
no flags Details
Patch (831.12 KB, patch)
2017-12-19 19:51 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (831.34 KB, patch)
2017-12-19 20:36 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (831.29 KB, patch)
2017-12-19 21:30 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (866.71 KB, patch)
2017-12-19 22:26 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (850.06 KB, patch)
2018-01-10 18:03 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (850.30 KB, patch)
2018-01-18 17:40 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Rebase after r227595 (464.01 KB, patch)
2018-01-25 10:42 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (11.75 MB, application/zip)
2018-01-25 12:43 PST, Build Bot
no flags Details
Rebase (419.39 KB, patch)
2018-04-20 12:03 PDT, Chris Nardi
no flags Details | Formatted Diff | Diff
Address comments by Simon (419.38 KB, patch)
2018-04-20 14:41 PDT, Chris Nardi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Nardi 2017-12-07 07:34:41 PST
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.
Comment 1 Radar WebKit Bug Importer 2017-12-07 19:34:38 PST
<rdar://problem/35926675>
Comment 2 Chris Nardi 2017-12-19 16:55:45 PST
Created attachment 329855 [details]
Patch
Comment 3 Chris Nardi 2017-12-19 17:00:31 PST
Created attachment 329856 [details]
Patch
Comment 4 Simon Fraser (smfr) 2017-12-19 17:01:18 PST
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.
Comment 5 Chris Nardi 2017-12-19 17:04:41 PST
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.
Comment 6 Chris Nardi 2017-12-19 17:05:37 PST
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.
Comment 7 Chris Nardi 2017-12-19 17:13:24 PST
Created attachment 329857 [details]
Patch
Comment 8 Simon Fraser (smfr) 2017-12-19 17:26:12 PST
(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).
Comment 9 Chris Nardi 2017-12-19 17:33:14 PST
Created attachment 329860 [details]
Patch
Comment 10 Chris Nardi 2017-12-19 17:37:10 PST
Created attachment 329861 [details]
Patch
Comment 11 Build Bot 2017-12-19 18:11:17 PST
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.
Comment 12 Build Bot 2017-12-19 18:11:19 PST
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
Comment 13 Build Bot 2017-12-19 18:18:14 PST
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.
Comment 14 Build Bot 2017-12-19 18:18:16 PST
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
Comment 15 Build Bot 2017-12-19 18:35:21 PST
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.
Comment 16 Build Bot 2017-12-19 18:35:22 PST
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
Comment 17 Chris Nardi 2017-12-19 18:43:48 PST
Created attachment 329872 [details]
Patch
Comment 18 Chris Nardi 2017-12-19 18:47:18 PST
Created attachment 329873 [details]
Patch
Comment 19 Build Bot 2017-12-19 19:01:16 PST
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.
Comment 20 Build Bot 2017-12-19 19:41:37 PST
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.
Comment 21 Build Bot 2017-12-19 19:41:38 PST
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
Comment 22 Build Bot 2017-12-19 19:48:05 PST
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.
Comment 23 Build Bot 2017-12-19 19:48:07 PST
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
Comment 24 Chris Nardi 2017-12-19 19:51:36 PST
Created attachment 329882 [details]
Patch
Comment 25 Chris Nardi 2017-12-19 20:36:31 PST
Created attachment 329890 [details]
Patch
Comment 26 Build Bot 2017-12-19 20:45:42 PST
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.
Comment 27 Chris Nardi 2017-12-19 21:30:45 PST
Created attachment 329895 [details]
Patch
Comment 28 Chris Nardi 2017-12-19 22:26:03 PST
Created attachment 329897 [details]
Patch
Comment 29 Chris Nardi 2017-12-21 20:52:29 PST
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.
Comment 30 Chris Nardi 2018-01-10 18:03:06 PST
Created attachment 331000 [details]
Patch
Comment 31 Chris Nardi 2018-01-15 13:02:15 PST
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.
Comment 32 Dean Jackson 2018-01-18 16:46:14 PST
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")
Comment 33 Chris Nardi 2018-01-18 17:29:34 PST
(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.
Comment 34 Chris Nardi 2018-01-18 17:40:17 PST
Created attachment 331679 [details]
Patch
Comment 35 Chris Nardi 2018-01-25 10:42:00 PST
Created attachment 332286 [details]
Rebase after r227595
Comment 36 Build Bot 2018-01-25 12:43:36 PST
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
Comment 37 Build Bot 2018-01-25 12:43:47 PST
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
Comment 38 Chris Nardi 2018-03-05 17:23:55 PST
Dean: any thoughts on the latest version of this patch?
Comment 39 Chris Nardi 2018-04-20 12:03:23 PDT
Created attachment 338448 [details]
Rebase
Comment 40 Simon Fraser (smfr) 2018-04-20 14:16:51 PDT
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
Comment 41 Chris Nardi 2018-04-20 14:41:39 PDT
Created attachment 338462 [details]
Address comments by Simon
Comment 42 Chris Nardi 2018-04-20 14:45:45 PDT
(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.
Comment 43 Build Bot 2018-04-20 14:59:34 PDT
Comment on attachment 338462 [details]
Address comments by Simon

Rejecting attachment 338462 [details] from commit-queue.

cnardi@chromium.org 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.
Comment 44 WebKit Commit Bot 2018-04-20 15:28:57 PDT
Comment on attachment 338462 [details]
Address comments by Simon

Clearing flags on attachment: 338462

Committed r230861: <https://trac.webkit.org/changeset/230861>
Comment 45 WebKit Commit Bot 2018-04-20 15:28:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 46 Simon Fraser (smfr) 2018-06-14 20:51:59 PDT
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.
Comment 47 Simon Fraser (smfr) 2018-06-14 20:55:49 PDT
I filed bug 186642.