Closed Bug 1132748 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
p11 + ---
firefox40 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 11 obsolete files)

11.02 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
1.09 KB, text/html
Details
868 bytes, text/html
Details
10.03 KB, patch
hsteen
: review+
Details | Diff | Splinter Review
9.20 KB, patch
hsteen
: review+
Details | Diff | Splinter Review
5.75 KB, patch
Details | Diff | Splinter Review
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.)
Assignee: nobody → dholbert
Attached patch wip patch (obsolete) — Splinter Review
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.
Note to self: technically I need to rev the uuid in the .idl file, too (since I'm adding a new function to it).
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.
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".)
Attachment #8564028 - Attachment is obsolete: true
(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.
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.
Attachment #8583999 - Flags: review?(dbaron)
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.)
Attachment #8583604 - Attachment is obsolete: true
(reposting to fix a typo in a comment. I've also now added a commit message.)
Attachment #8583999 - Attachment is obsolete: true
Attachment #8583999 - Flags: review?(dbaron)
Attachment #8584000 - Flags: review?(dbaron)
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
Attachment #8584000 - Flags: review?(dbaron) → review+
Blocks: 1158383
No longer depends on: 1158383
Status: NEW → ASSIGNED
Flags: needinfo?(dholbert)
Here's part 1 again, with comment 9 addressed.
Attachment #8584000 - Attachment is obsolete: true
Attachment #8599544 - Flags: review+
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.
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.
Flags: needinfo?(dholbert)
Attachment #8599603 - Flags: review?(hsteen)
Attachment #8599603 - Attachment description: part 2, initial: import (and test) cssfixme code → part 2, initial stage: 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.)
Attachment #8599603 - Attachment is obsolete: true
Attachment #8599603 - Flags: review?(hsteen)
Attachment #8599604 - Flags: review?(hsteen)
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.)
...and here's a live reference case for comparison, with the unprefixed output generated by cssfixme.
Attachment #8599608 - Attachment description: reference 1 → reference 1 (w/ unprefixed gradient expressions)
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.
Attachment #8599604 - Attachment is obsolete: true
Attachment #8599604 - Flags: review?(hsteen)
Attachment #8599692 - Flags: review?(hsteen)
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.
Attachment #8599696 - Flags: review?(hsteen)
This patch is just a straight cut-and-paste to move code from tmp-gradient.js (the staging file) into CSSUnprefixingService.js.
Attachment #8599698 - Flags: review?(hsteen)
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.)
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.)
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.)
Attachment #8599729 - Attachment description: rollup version of part 2 stages folded together → rollup version of part 2 stages folded together (if it helps for review)
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 :)
> > +        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?
Attachment #8599698 - Flags: review?(hsteen) → review+
> 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.
Attachment #8599692 - Attachment is obsolete: true
Attachment #8599692 - Flags: review?(hsteen)
Attachment #8599696 - Attachment is obsolete: true
Attachment #8599696 - Flags: review?(hsteen)
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.)
Attachment #8599698 - Attachment is obsolete: true
Attachment #8599727 - Attachment is obsolete: true
Attachment #8599729 - Attachment is obsolete: true
Attachment #8601813 - Flags: review?(hsteen)
Attachment #8601813 - Flags: review?(hsteen) → review+
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!
Attachment #8601814 - Flags: review?(hsteen) → review+
Depends on: 1162319
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
Blocks: 1162608
Filed bug 1162608 on the -webkit-radial-gradient issue discussed in comment 29.
Flags: in-testsuite+
tracking-p11: --- → +
Blocks: 1170789
Blocks: 1213126
No longer blocks: 1213126
See Also: → 1217643
See Also: → 1210575
I assume the actual implementation is done in bug 1210575, so I'm removing the dev-doc-needed again.


Sebastian
Keywords: dev-doc-needed
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)
You need to log in before you can comment on or make changes to this bug.