Closed Bug 1352643 Opened 7 years ago Closed 6 years ago

[css-images-4] Implement multi-position gradient color-stops syntax

Categories

(Core :: CSS Parsing and Computation, enhancement)

55 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: percyley, Assigned: rhunt, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36

Steps to reproduce:

Open testcase: http://dabblet.com/gist/d52773b90f2bf6d6304c95c86a858c23


Actual results:

The background is red


Expected results:

The background should be green

Spec: https://drafts.csswg.org/css-images-4/#color-stop-syntax

According to the new grammar, color stops can have two positions which is equivalent to two consecutive color stops with the same color and different positions. For example

    linear-gradient(45deg, #f06 25%, yellow 25%, yellow 50%, #f06 50%, #f06 75%, yellow 75%)

can be written as

    linear-gradient(45deg, #f06 25%, yellow 25% 50%, #f06 50% 75%, yellow 75%)

with this syntax. It's useful for all gradient types as it makes it easier and more readable to specify "bands" of color and should be relatively straightforward to implement since it's rather simple syntactic sugar.
Blocks: css-images-4
This is a pure CSS frontend issue, I expect, since the resulting gradient data structure needs to end up the same.
Status: UNCONFIRMED → NEW
Component: Layout: Images → CSS Parsing and Computation
Ever confirmed: true
I would like to work on this.
Flags: needinfo?(emilio)
Hi Jesper. Great, thank you!

Gradient parsing lives in servo/component/style/specified/values.rs. This should be an addition to the GradientItem / ColorStop parsing, which is called from:

  https://searchfox.org/mozilla-central/rev/ce86c6c0472d5021ef693cf99abaaa0644c89e55/servo/components/style/values/specified/image.rs#260

ColorStop parsing lives at:

  https://searchfox.org/mozilla-central/rev/ce86c6c0472d5021ef693cf99abaaa0644c89e55/servo/components/style/values/specified/image.rs#959

And right now only parses one position. Note that that function has a single caller too.

Well, the tl;dr: is that it needs to parse two positions.

There's three ways to go about this.

 * The easiest one to get the desired rendering on screen is generating two color stops in GradientItem::parse_comma_separated when we find a position after a ColorStop. But that means that we lose that information during serialization and such, which may be more complicated. I'd rather not do this, but you can try it to get something quick working if you want.

 * The second way to do this is converting the `position` field of `ColorStop` (whose definition lives in values/generics/image.rs) in something like:

    begin: Option<LengthOrPercentage>,
    end: Option<LengthOrPercentage>,

  Then change ColorStop::parse to also parse `end` if appropriate.

  You also need to add another field to the C++ representation of the stop, that is, split mLocation in mBegin and mEnd:

    https://searchfox.org/mozilla-central/rev/ce86c6c0472d5021ef693cf99abaaa0644c89e55/layout/style/nsStyleStruct.h#163

  And fix up the relevant conversion code.

  Afterwards you'd need to tweak nsCSSGradientRenderer to read both mBegin and mEnd if present, and append two ColorStops if mEnd is set:

    https://searchfox.org/mozilla-central/rev/ce86c6c0472d5021ef693cf99abaaa0644c89e55/layout/painting/nsCSSRenderingGradients.cpp#578

 * The third way is a mix of both that would preserve the serialization on the specified value, but not on the computed one. It'll save half of the work above, but it's worth to know what Chrome with the "Experimental web-platform features" flag enabled does. The process would splitting ColorStop into two types, one in the specified/image.rs module, and one in the computed one. The specified one would have begin / end, but the computed one would have just `position`, as now. Then the ToComputedValue implementation for Gradient would need to compute potentially a specified stop into two computed stops, and all the conversion code and such would remain untouched.

Regarding tests, there are tests that are about to be imported from https://github.com/w3c/web-platform-tests/pull/10274. You can run them with:

 ./mach test testing/web-platform/tests/css/css-images/gradient/color-stops-parsing.html

We should also probably add a few reftests and such, but that should be enough to get you started.

Lacking strong arguments to go for (1) or (3), I'd go with (2).

Let me know if you have any questions, or if you have any issues setting everything up / get stuck.

Thanks!
Mentor: emilio
Flags: needinfo?(emilio)
I've filed bug 1464589 with a cleanup that should make the C++ bits of this much more pleasurable :)
Thank you for the information and the links Emilio! I will give this a shot right away and keep you informed about the progress.
>  But that means that we lose that information during serialization

How does the CSS spec define serialization here?  Are color stops preserved as-is or canonicalized, now that there are multiple ways of writing the same stop information?
Flags: needinfo?(emilio)
I'm assuming that as-is is the only way to go. But looking at Chrome's implementation, they canonicalize them, which may not be backwards-compatible.

I'll ask for clarification here. The canonicalization would need to be defined I guess.
Flags: needinfo?(emilio)
Given the resolution in https://github.com/w3c/csswg-drafts/issues/2714, the change is even easier, and we should go with the first approach.
Summary: [css-images-4]Implement multi-position color-stops syntax → [css-images-4]Implement multi-position gradient color-stops syntax
Hi Jesper, are you actively working on this? Anything I can help out with?
Flags: needinfo?(jesper)
Hi Emilio! Sorry for my late response... I started working on this but had some trouble with my gcc version and compiling the source code. I will pick this up again this week and keep you posted about the progress. Thanks!
Flags: needinfo?(jesper)
WebKit just fixed this: https://bugs.webkit.org/show_bug.cgi?id=186154

We should probably follow suit sooner rather than later.
This commit adds the multi-position gradient color-stops syntax.

GradientItem::parse_comma_separated is extended to attempt to parse
a LengthOrPercent after each color stop. If it succeeds, it appends
an additional color stop with a duplicate color and the specified
position.

This change is only to the parsing, serialization is left unchanged.
I wrote a patch to do this.

Here's a try run [1].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bd8a8908bed6c2c88ed817ba40d5f958a5a4a8b
Assignee: nobody → rhunt
Comment on attachment 9013513 [details]
Bug 1352643 - Implement multi-position gradient color-stops syntax. r=emilio

Emilio Cobos Álvarez (:emilio) has approved the revision.
Attachment #9013513 - Flags: review+
Attachment #9013513 - Attachment description: Bug 1352643 - Implement multi-position gradient color-stops syntax. r?emilio → Bug 1352643 - Implement multi-position gradient color-stops syntax. r=emilio
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e85309ff67f4
Implement multi-position gradient color-stops syntax. r=emilio
Backed out changeset e85309ff67f4 (bug 1352643) for wpt failures at /css/css-images/gradient/color-stops-parsing.html 

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/1f808ca3a4b448677596c78909536c8c077275d7

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&searchStr=linux,x64,opt,web,platform,tests,with,e10s,test-linux64%2Fopt-web-platform-tests-e10s-6,w-e10s(wpt6)&selectedJob=202984983&revision=e85309ff67f47c2ee52944499f6ababe1278fabd

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=202984983&repo=mozilla-inbound&lineNumber=1674

task 2018-10-02T23:20:54.676Z] 23:20:54     INFO - TEST-PASS | /css/css-images/gradient/color-stops-parsing.html | linear-gradient(black 0%, 15%, green 50%, 60%, white 100%) [ parsable ] 
[task 2018-10-02T23:20:54.676Z] 23:20:54     INFO - TEST-UNEXPECTED-PASS | /css/css-images/gradient/color-stops-parsing.html | linear-gradient(black 0% 50%, white) [ parsable ] - expected FAIL
[task 2018-10-02T23:20:54.676Z] 23:20:54     INFO - TEST-INFO | expected FAIL
[task 2018-10-02T23:20:54.677Z] 23:20:54     INFO - 
[task 2018-10-02T23:20:54.677Z] 23:20:54     INFO - TEST-UNEXPECTED-PASS | /css/css-images/gradient/color-stops-parsing.html | linear-gradient(black 0% 50%, white 50% 100%) [ parsable ] - expected FAIL
[task 2018-10-02T23:20:54.678Z] 23:20:54     INFO - TEST-INFO | expected FAIL
[task 2018-10-02T23:20:54.678Z] 23:20:54     INFO - 
[task 2018-10-02T23:20:54.679Z] 23:20:54     INFO - TEST-UNEXPECTED-PASS | /css/css-images/gradient/color-stops-parsing.html | linear-gradient(black 0% 50%, green 25% 75%, white 50% 100%) [ parsable ] - expected FAIL
[task 2018-10-02T23:20:54.679Z] 23:20:54     INFO - TEST-INFO | expected FAIL
[task 2018-10-02T23:20:54.680Z] 23:20:54     INFO - 
[task 2018-10-02T23:20:54.680Z] 23:20:54     INFO - TEST-UNEXPECTED-PASS | /css/css-images/gradient/color-stops-parsing.html | linear-gradient(black 0% calc(100% / 5), 25%, green 30% 60%, calc(100% * 3 / 4), white calc(100% - 20%) 100%) [ parsable ] - expected FAIL
[task 2018-10-02T23:20:54.680Z] 23:20:54     INFO - TEST-INFO | expected FAIL
[task 2018-10-02T23:20:54.688Z] 23:20:54     INFO - 
[task 2018-10-02T23:20:54.689Z] 23:20:54     INFO - TEST-PASS | /css/css-images/gradient/color-stops-parsing.html | repeating-linear-gradient(black, white) [ parsable ] 
[task 2018-10-02T23:20:54.689Z] 23:20:54     INFO - TEST-PASS | /css/css-images/gradient/color-stops-parsing.html | repeating-linear-gradient(black 0, white) [ parsable ] 
[task 2018-10-02T23:20:54.689Z] 23:20:54     INFO - TEST-PASS | /css/css-images/gradient/color-stops-parsing.html | repeating-linear-gradient(black 0%, white) [ parsable ] 
[task 2018-10-02T23:20:54.690Z] 23:20:54     INFO - TEST-PASS | /css/css-images/gradient/color-stops-parsing.html | repeating-linear-gradient(black 0%, white 100%) [ parsable ] 
[task 2018-10-02T23:20:54.690Z] 23:20:54     INFO - TEST-PASS | /css/css-images/gradient/color-stops-parsing.html | repeating-linear-gradient(black, green, white) [ parsable ] 
[task 2018-10-02T23:20:54.690Z] 23:20:54     INFO - TEST-PASS | /css/css-images/gradient/color-stops-parsing.html | repeating-linear-gradient(black 0%, green 50%, white 100%) [ parsable ] 
[task 2018-10-02T23:20:54.691Z] 23:20:54     INFO - TEST-PASS | /css/css-images/gradient/color-stops-parsing.html | repeating-linear-gradient(black 50%, green 10%, white 100%) [ parsable ] 
[task 2018-10-02T23:20:54.691Z] 23:20:54     INFO - TEST-PASS | /css/css-images/gradient/color-stops-parsing.html | repeating-linear-gradient(black, 25%, white) [ parsable ] 
[task 2018-10-02T23:20:54.691Z] 23:20:54     INFO - TEST-PASS | /css/css-images/gradient/color-stops-parsing.html | repeating-linear-gradient(black 0%, 25%, white 100%) [ parsable ] 
[task 2018-10-02T23:20:54.691Z] 23:20:54     INFO - TEST-PASS | /css/css-images/gradient/color-stops-parsing.html | repeating-linear-gradient(black 0%, 15%, green 50%, 60%, white 100%) [ parsable ] 
[task 2018-10-02T23:20:54.692Z] 23:20:54     INFO - TEST-UNEXPECTED-PASS | /css/css-images/gradient/color-stops-parsing.html | repeating-linear-gradient(black 0% 50%, white) [ parsable ] - expected FAIL
[task 2018-10-02T23:20:54.693Z] 23:20:54     INFO - TEST-INFO | expected FAIL
[task 2018-10-02T23:20:54.695Z] 23:20:54     INFO - 
[task 2018-10-02T23:20:54.695Z] 23:20:54     INFO - TEST-UNEXPECTED-PASS | /css/css-images/gradient/color-stops-parsing.html | repeating-linear-gradient(black 0% 50%, white 50% 100%) [ parsable ] - expected FAIL
[task 2018-10-02T23:20:54.695Z] 23:20:54     INFO - TEST-INFO | expected FAIL
[task 2018-10-02T23:20:54.696Z] 23:20:54     INFO - 
[task 2018-10-02T23:20:54.696Z] 23:20:54     INFO - TEST-UNEXPECTED-PASS | /css/css-images/gradient/color-stops-parsing.html | repeating-linear-gradient(black 0% 50%, green 25% 75%, white 50% 100%) [ parsable ] - expected FAIL
[task 2018-10-02T23:20:54.697Z] 23:20:54     INFO - TEST-INFO | expected FAIL
[task 2018-10-02T23:20:54.697Z] 23:20:54     INFO - 
[task 2018-10-02T23:20:54.698Z] 23:20:54     INFO - TEST-UNEXPECTED-PASS | /css/css-images/gradient/color-stops-parsing.html | repeating-linear-gradient(black 0% calc(100% / 5), 25%, green 30% 60%, calc(100% * 3 / 4), white calc(100% - 20%) 100%) [ parsable ] - expected FAIL
[task 2018-10-02T23:20:54.698Z] 23:20:54     INFO - TEST-INFO | expected FAIL
[task 2018-10-02T23:20:54.703Z] 23:20:54     INFO - 
[task 2018-10-02T23:20:54.703Z] 23:20:54     INFO - TEST-PASS | /css/css-images/gradient/color-stops-parsing.html | radial-gradient(black, white) [ parsable ]
Flags: needinfo?(rhunt)
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13324 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR was closed without merging
Odd, I did do a try run including web-platform-tests and it didn't have this failure. Might be because it was OSX only? [1]

Either way, this looks like it just needs tests expectations fixed. Here's a try run with the fixup [2].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef88e24b3e8ee3031406a76c2b4823987bcfa673
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b13f175725c03d29010021ad6c96bf63e71da82f
Flags: needinfo?(rhunt)
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/984902ba6faf
Implement multi-position gradient color-stops syntax. r=emilio
https://hg.mozilla.org/mozilla-central/rev/984902ba6faf
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Note for documentation team:

I have added a note about this addition to the Fx64 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#CSS

Everything else still needs doing.
added multiple position color stop to linear gradient, radial gradient, repeating linear, repeating radial, and learning gradients pages.
Type: defect → enhancement
Summary: [css-images-4]Implement multi-position gradient color-stops syntax → [css-images-4] Implement multi-position gradient color-stops syntax
You need to log in before you can comment on or make changes to this bug.