1132748 - Add support for emulating legacy "-webkit-gradient" syntax, via CSS Unprefixing Service

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

As discussed in the second half of bug 1107378 comment 31, I'm deferring "-webkit-gradient" support to a followup bug -- this bug here is that followup bug.

(For context, see bug 1107378 comment 0.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: wip patch

Here's a wip patch -- it's missing the actual JS logic (right now it just turns everything into a radial gradient from lime to green), but I think everything's hooked up on the nsCSSParser.cpp side.

This layers on top of the patches in bug 1107378.

Comment 2

[Daniel Holbert [:dholbert]]

Note to self: technically I need to rev the uuid in the .idl file, too (since I'm adding a new function to it).

Comment 3

[Daniel Holbert [:dholbert]]

I spent a chunk of today trying to import the generalized gradient-unprefixing code from cssfixme, but I ran into some issues with it:
 https://github.com/hallvors/css-fixme/issues/4
 https://github.com/hallvors/css-fixme/issues/5
 https://github.com/hallvors/css-fixme/issues/6

So I might hold off on this for the time being, until the cssfixme gradient code is a bit more hardened/tested & suitable for importing.  (Alternately, I can write some of my own, but it seems wasteful to write two separate independent gradient-translation implementations -- so as long as we're maintaining the CSSFixme one, it seems worth improving that & then importing it.)

I'll describe the API I'm planning on using here (which falls naturally out of how the CSS parser processes these while parsing e.g. a CSS Background).  I'm hoping this API overview will help in making the cssfixme code more directly-importable, as it gets improved.

The JS implementation function-signature will look like this:
  generateUnprefixedGradientValue: function(aPrefixedFuncName,
                                            aPrefixedFuncBody,
                                            aUnprefixedFuncName, /*[out]*/
                                            aUnprefixedFuncBody /*[out]*/) {
    [...]
  }

Input parameters:
* aPrefixedFuncName will be e.g. "-webkit-gradient","-webkit-linear-gradient", or "-webkit-radial-gradient",
* aPrefixedFuncBody will be everything inside the gradient function-body -- everything inside the parens. (This handles nested functions like rgb() correctly; the CSS parser has code to find a close-paren while skipping over nested parenthesized things.)

Outparams:
* aUnprefixedFuncName will be "linear-gradient" or "radial-gradient". (Alternately, this could really be an enum; I'm just having it be a string for symmetry w/ the input-params. This just tells the C++ code in nsCSSParser.cpp whether to call ParseLinearGradient or ParseRadialGradient.)
* aPrefixedFuncBody is the (unparenthesized) function-body of the resulting standardized gradient expression.

Return value: bool -- true if we think we succeeded, false if we hit something unrecognized/un-convertable.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: wip v2: CSS Parser C++ code, calling into (empty) JS api

Here's an updated WIP with everything that we need on the C++ side here (I think), and with a stub JS function to implement the API that I described in my previous comment.

(Note: my previous attached WIP just detected "-webkit-gradient" in C++. This version detects "-webkit-gradient", "-webkit-linear-gradient", and "-webkit-radial-gradient".)

Comment 5

[Hallvord R. M. Steen [:hallvors]]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> I spent a chunk of today trying to import the generalized
> gradient-unprefixing code from cssfixme, but I ran into some issues with it:
>  https://github.com/hallvors/css-fixme/issues/4
>  https://github.com/hallvors/css-fixme/issues/5
>  https://github.com/hallvors/css-fixme/issues/6

All fixed.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: part 1: Add the API and the C++ code to call into it (with stub always-failing JS impl for now)

Thanks, Hallvord!

I'm going to get the review process going on the C++ parts of this, since the JS part (and its cssfixme source) may still evolve a bit.

The JS impl will come in a "part 2" here.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8583604 [details] [diff] [review]
wip v2: CSS Parser C++ code, calling into (empty) JS api

(obsoleting "wip v2" -- it's the same as my just-attached "part 1", except that it's missing a uuid-bump.)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: part 1 v2: Add the API and the C++ code to call into it (with stub always-failing JS impl for now)

(reposting to fix a typo in a comment. I've also now added a commit message.)

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8584000 [details] [diff] [review]
part 1 v2: Add the API and the C++ code to call into it (with stub always-failing JS impl for now)

>+ // ... Only call if
>+  // ShouldUseUnprefixingService() returns true.

How about an assertion at the start of the function that it's true?

r=dbaron with that

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: part 1 v3: Add the API and the C++ code to call into it (with stub always-failing JS impl for now)

Here's part 1 again, with comment 9 addressed.

Comment 11

[Daniel Holbert [:dholbert]]

The JS implementation will be split into 3 stages: "initial", "cleanup", "finalize" (which I'll fold together when landing). I think this 3-part strategy will make review easier -- "cleanup" will be the only non-trivial bit.

* "initial stage": straight copypaste of gradient-related functions from cssfixme into a standalone temporary JS file. Also create a standalone "test.html" which exercises this JS to ensure it works.

* "cleanup stage": refactor the functionality to fit the API that we're exposing in the IDL in "part 1" here.

* "finalize stage": Delete the temporary files, moving the logic into CSSUnprefixingService.js & mochitests. (Just a cut-and-paste of code between files, with a bit of massaging for the mochitests.)

hallvors, I'll tag you for review on these, if that's all right with you.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: part 2, initial stage: import (and test) cssfixme code

As noted above: this initial patch creates two temporary files in layout/style, as an easy staging/testing ground for the imported JS code.

(Currently, I'm not intending for these temporary files to ever exist in mozilla-central; I'm planning on squashing all of part 2 together when landing.)

FWIW: for each of my testcases in the included "tmp-test-gradient.html", I verified that Firefox Nightly's rendering of the unprefixed version (generated by the imported unprefixing code) looks like Chrome 44's rendering of the prefixed version.  When testing this, I just used 100x100 divs, with the "background" property set to the tested value.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: part 2, initial stage, v2: import (and test) cssfixme code

(reposting to add one tweak that I forgot to qref in -- a brief comment at the top of tmp-gradient.js.)

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (w/ prefixes)

For demonstration purposes, here's a live testcase with all the prefixed gradients that I included in the test file, in "part 2 initial stage".  (cssfixme can successfully unprefix all of these.)

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: reference 1 (w/ unprefixed gradient expressions)

...and here's a live reference case for comparison, with the unprefixed output generated by cssfixme.

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: part 2, initial stage, v3: directly import (and test) cssfixme code

I submitted a cssfixme pull request to do some minor cleanup:
 https://github.com/hallvors/css-fixme/pull/7
and rebased my import patch to grab the post-cleanup cssfixme code.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: part 2, refactor stage: tweak imported code to fit the CSSUnprefixingService API

This tweaks the gradient-unprefixing code to fit CSSUnprefixingService's API (and makes similar tweaks to the test HTML file, to keep exercising the code).

In particular: this patch:
 (1) Adds a "fake" CSSUnprefixingService {} closure around the code (so that the next patch can just be a straight cut-and-paste)
 (2) Refactors the code to handle the gradient function-name separately from the function-body.  nsCSSParser handles the function-name & the function-body separately, so the service needs to support handling them separately. So, this patch tweaks the cssfixme code to conform to that use case.

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: part 2, "move code" stage: move code from tmp file into CSSUnprefixingService.js

This patch is just a straight cut-and-paste to move code from tmp-gradient.js (the staging file) into CSSUnprefixingService.js.

Comment 19

[Daniel Holbert [:dholbert]]

At this point in the patch queue -- with the "part 2" stages so far -- i verified that we render the formerly-missing gradients on hao123.com (per top of bug 1107378 comment 28) and on www.daily.co.jp with the prefixing whitelist disabled & spoofing a mobile UA (per dependent bug 1158383).

Just one more stage left -- replacing my dummy test HTML file with mochitest.  (After I post that, I'll post a roll-up as well, to make the actual intended-to-land patch clearer.)

Comment 20

[Daniel Holbert [:dholbert]]

I don't have the mochitest ready quite yet -- it's a bit trickier than I thought, because the existing unprefixing service mochitest assumes (for simplicity) that the unprefixed value will serialize to the same thing in the specified style & computed style -- and a lot of the values I've been testing don't actually behave that way[1] -- which is fine, but it just means the mochitest may need to be extended a bit, to be able to test these values (& fully exercise the imported unprefixing code).

So for now, I'm posting a partial mochitest patch, & the rollup patch to assist w/ review, and I'll finish off the mochitest before landing.

[1] (if anyone's curious: many gradient expressions serialize to different specified values vs. computed values in part because we always convert colors to rgb() in computed style, and in part because there's some flexibility in the modern gradient syntax about whether a default <position> needs to be specified or not, and we apparently make different choices about that expressiveness in specified style vs. computed style.)

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: part 2, "move tests" stage (incomplete): remove testing html file, & add to mochitest

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: rollup version of part 2 stages folded together (if it helps for review)

Here's the rollup of all the "part 2" stages, folded together.

Sorry if this seems overly complex. :) To be clear, the idea here is that the only *real* review needed is:
 (1) sanity-checking that the code in the "initial stage" patch seems fine to import (copypasting from github)
 (2) sanity-checking the changes to that imported code, in the "refactor stage" patch.

(The "refactor stage" patch is the only patch with non-copypaste code changes.)

Comment 23

[Hallvord R. M. Steen [:hallvors]]

Comment on attachment 8599729 [details] [diff] [review]
rollup version of part 2 stages folded together (if it helps for review)

Review of attachment 8599729 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/CSSUnprefixingService.js
@@ +162,5 @@
> +      if (aPrefixedFuncName == "-webkit-gradient") {
> +        // Create expression for oldGradientParser:
> +        var oldGradientExpression = aPrefixedFuncName + "(" + aPrefixedFuncBody + ")";
> +        // extracting the values..
> +        var parts = this.oldGradientParser(oldGradientExpression), type;

This could be simplified a bit further. It should work to just do 

var parts = this.oldGradientParser(aPrefixedFuncBody);

and do 

type = parts[0].name;

I think..

@@ +181,5 @@
> +                if(rxPercTest.test(points[0]) || points[0] == 0){
> +                    var startX = parseInt(points[0]), startY = parseInt(points[1]), endX = parseInt(points[2]), endY = parseInt(points[3]);
> +                    newValue +=  ((Math.atan2(endY- startY, endX - startX)) * (180 / Math.PI)+90) + 'deg';
> +                }else{
> +                    if(points[1] === points[3]){ // both 'top' or 'bottom, this linear gradient goes left-right

This logic may be wrong - wonder if  "left, top, top, right" is valid and used?

::: layout/style/test/unprefixing_service_iframe.html
@@ +161,5 @@
> +  { decl: "background-image: -webkit-gradient(linear, left center, right center, from(rgb(0, 0, 0)), color-stop(30%, rgb(255, 0, 0)), color-stop(60%, rgb(0, 255, 0)), to(rgb(0, 0, 255)))",
> +    targetPropName: "background-image",
> +    targetPropVal: "linear-gradient(to right, rgb(0, 0, 0) 0%, rgb(255, 0, 0) 30%, rgb(0, 255, 0) 60%, rgb(0, 0, 255) 100%)"},
> +
> +  // XXXdholbert Test -webkit-gradient(radial, ...), and -webkit-[linear|radial]-gradient

I suggest also testing background: with gradients as part of a shorthand expression with prefixed and suffixed parts.. just in case :)

Comment 24

[Daniel Holbert [:dholbert]]

> > +        var oldGradientExpression = aPrefixedFuncName + "(" + aPrefixedFuncBody + ")";
> > +        // extracting the values..
> > +        var parts = this.oldGradientParser(oldGradientExpression), type;
> 
> This could be simplified a bit further. It should work to just do 
> 
> var parts = this.oldGradientParser(aPrefixedFuncBody);
> 
> and do 
> 
> type = parts[0].name;
> 
> I think..

Good point -- that removes the need for the loop over "i", too.  Though, the code below this consistently uses "parts[i].args[1]" etc. which assume "parts" is a parsing of the top-level gradient expression -- all of those lines (with "parts[i]") would need changing, too (to "parts[1" in this case).

Rather than making all of those changes locally here, I'm going to submit a pull request to refactor the parts-handling to use a helper-function, so that this simplification here can be more minimal.  (That way, cssfixme & CSSUnprefixingService can be kept in-sync more easily, going forward, which is important.)

> @@ +181,5 @@
> > +                    if(points[1] === points[3]){ // both 'top' or 'bottom, this linear gradient goes left-right
> 
> This logic may be wrong - wonder if  "left, top, top, right" is valid and
> used?

I don't believe that style is valid. From my testing with Chrome at least, right|left has to consistently come first, and top|bottom has to consistently come second. So e.g. Chrome renders this:
 <div style="background: -webkit-gradient(linear, left top, right bottom,
             from(white), to(black))"></div>

...but won't render this (flipping "right bottom"):
 <div style="background: -webkit-gradient(linear, left top, bottom right,
             from(white), to(black))"></div>

I think that answers your question? Or if not, could you clarify?

Comment 25

[Hallvord R. M. Steen [:hallvors]]

> I think that answers your question? Or if not, could you clarify?

It does indeed. Thanks for testing :)

I've merged the PR on GitHub.

Comment 26

[Daniel Holbert [:dholbert]]

Attachment: part 2: import cssfixme code directly into CSSUnprefixingService.js

New plan (different from comment 11): now I'm just importing the cssfixme code (post-github-PR discussed above) directly into CSSUnprefixingService.js as "part 2", and I'll bring it inside the CSSUnprefixingService {} scope in "part 3", and extend the mochitest in "part 4".  I won't fold these parts together when landing. (This should hopefully make it easier to port cssfixme changes to CSSUnprefixingService.js, since the differences-since-import will be more clearly tracked.)

(I initially thought that directly importing the code might break CSSUnprefixingService.js somehow, but it seems to be fine -- my mochitest "test_unprefixing_service.html" still passes at each stage.)

Comment 27

[Daniel Holbert [:dholbert]]

Attachment: part 3: refactor imported code into CSSUnprefixingService{} scope & generateUnprefixedGradientValue() API

Comment 28

[Hallvord R. M. Steen [:hallvors]]

Comment on attachment 8601814 [details] [diff] [review]
part 3: refactor imported code into CSSUnprefixingService{} scope & generateUnprefixedGradientValue() API

Review of attachment 8601814 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

Comment 29

[Daniel Holbert [:dholbert]]

Attachment: part 4: mochitests

Thanks for the reviews! Here's the completed mochitests patch (based on top of bug 1162319, which fixes the broken assumption that was causing complications described in comment 20). I included a testcase that uses a mix of prefixed/unprefixed gradients in a single declaration, per end of comment 23.

(Also, I ran across some issues with -webkit-radial-gradient unprefixing and filed https://github.com/hallvors/css-fixme/issues/9 . I'm not gating this on that fix for the moment, since I don't know that any of the sites where we're unprefixing actually run up against that bug anyway.)

Not bothering with review on these tests, but feedback is welcome.

Try run (with the test patch mis-numbered in commit message; fixed that locally): https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca8c7cec6304

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/522d010bd1ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc88f4422a6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f5726cc7249
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e3204c8bfc6

Comment 31

[Daniel Holbert [:dholbert]]

Filed bug 1162608 on the -webkit-radial-gradient issue discussed in comment 29.

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/522d010bd1ff
https://hg.mozilla.org/mozilla-central/rev/cc88f4422a6f
https://hg.mozilla.org/mozilla-central/rev/7f5726cc7249
https://hg.mozilla.org/mozilla-central/rev/7e3204c8bfc6

Comment 34

[Sebastian Zartner [:sebo]]

I assume the actual implementation is done in bug 1210575, so I'm removing the dev-doc-needed again.


Sebastian

Comment 35

[Daniel Holbert [:dholbert]]

Correct; best not to try to add documentation here, since this bug's implementation was only ever enabled as a targeted hack for a select whitelist of sites. (and will soon be obsoleted in Firefox 49 by the better implementation on bug 1210575)