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.
<rdar://problem/35926675>
Created attachment 329855 [details] Patch
Created attachment 329856 [details] Patch
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.
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.
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.
Created attachment 329857 [details] Patch
(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).
Created attachment 329860 [details] Patch
Created attachment 329861 [details] Patch
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.
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 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.
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 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.
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
Created attachment 329872 [details] Patch
Created attachment 329873 [details] Patch
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 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.
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 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.
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
Created attachment 329882 [details] Patch
Created attachment 329890 [details] Patch
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.
Created attachment 329895 [details] Patch
Created attachment 329897 [details] Patch
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.
Created attachment 331000 [details] Patch
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 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")
(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.
Created attachment 331679 [details] Patch
Created attachment 332286 [details] Rebase after r227595
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
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
Dean: any thoughts on the latest version of this patch?
Created attachment 338448 [details] Rebase
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
Created attachment 338462 [details] Address comments by Simon
(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 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 on attachment 338462 [details] Address comments by Simon Clearing flags on attachment: 338462 Committed r230861: <https://trac.webkit.org/changeset/230861>
All reviewed patches have been landed. Closing bug.
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.
I filed bug 186642.