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

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
mozilla40
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(p11+, firefox40 fixed)

Details

Attachments

(6 attachments, 11 obsolete attachments)

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
(Assignee)

Description

4 years ago
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)

Updated

4 years ago
Assignee: nobody → dholbert
(Assignee)

Comment 1

4 years ago
Created attachment 8564028 [details] [diff] [review]
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.
(Assignee)

Comment 2

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

Comment 3

4 years ago
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.
(Assignee)

Comment 4

4 years ago
Created attachment 8583604 [details] [diff] [review]
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".)
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.
(Assignee)

Comment 6

4 years ago
Created attachment 8583999 [details] [diff] [review]
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.
Attachment #8583999 - Flags: review?(dbaron)
(Assignee)

Comment 7

4 years ago
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
(Assignee)

Comment 8

4 years ago
Created 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)

(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+
Depends on: 1158383
Blocks: 1158383
No longer depends on: 1158383
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
Flags: needinfo?(dholbert)
(Assignee)

Comment 10

4 years ago
Created attachment 8599544 [details] [diff] [review]
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.
Attachment #8584000 - Attachment is obsolete: true
Attachment #8599544 - Flags: review+
(Assignee)

Comment 11

4 years ago
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.
(Assignee)

Comment 12

4 years ago
Created attachment 8599603 [details] [diff] [review]
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.
Flags: needinfo?(dholbert)
Attachment #8599603 - Flags: review?(hsteen)
(Assignee)

Updated

4 years ago
Attachment #8599603 - Attachment description: part 2, initial: import (and test) cssfixme code → part 2, initial stage: import (and test) cssfixme code
(Assignee)

Comment 13

4 years ago
Created attachment 8599604 [details] [diff] [review]
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.)
Attachment #8599603 - Attachment is obsolete: true
Attachment #8599603 - Flags: review?(hsteen)
Attachment #8599604 - Flags: review?(hsteen)
(Assignee)

Comment 14

4 years ago
Created attachment 8599606 [details]
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.)
(Assignee)

Comment 15

4 years ago
Created attachment 8599608 [details]
reference 1 (w/ unprefixed gradient expressions)

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

Updated

4 years ago
Attachment #8599608 - Attachment description: reference 1 → reference 1 (w/ unprefixed gradient expressions)
(Assignee)

Comment 16

4 years ago
Created attachment 8599692 [details] [diff] [review]
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.
Attachment #8599604 - Attachment is obsolete: true
Attachment #8599604 - Flags: review?(hsteen)
Attachment #8599692 - Flags: review?(hsteen)
(Assignee)

Comment 17

4 years ago
Created attachment 8599696 [details] [diff] [review]
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.
Attachment #8599696 - Flags: review?(hsteen)
(Assignee)

Comment 18

4 years ago
Created attachment 8599698 [details] [diff] [review]
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.
Attachment #8599698 - Flags: review?(hsteen)
(Assignee)

Comment 19

4 years ago
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.)
(Assignee)

Comment 20

4 years ago
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.)
(Assignee)

Comment 21

4 years ago
Created attachment 8599727 [details] [diff] [review]
part 2, "move tests" stage (incomplete): remove testing html file, & add to mochitest
(Assignee)

Comment 22

4 years ago
Created attachment 8599729 [details] [diff] [review]
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.)
(Assignee)

Updated

4 years ago
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 :)
(Assignee)

Comment 24

4 years ago
> > +        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.
(Assignee)

Updated

4 years ago
Attachment #8599692 - Attachment is obsolete: true
Attachment #8599692 - Flags: review?(hsteen)
(Assignee)

Updated

4 years ago
Attachment #8599696 - Attachment is obsolete: true
Attachment #8599696 - Flags: review?(hsteen)
(Assignee)

Comment 26

4 years ago
Created attachment 8601813 [details] [diff] [review]
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.)
Attachment #8599698 - Attachment is obsolete: true
Attachment #8599727 - Attachment is obsolete: true
Attachment #8599729 - Attachment is obsolete: true
Attachment #8601813 - Flags: review?(hsteen)
(Assignee)

Comment 27

4 years ago
Created attachment 8601814 [details] [diff] [review]
part 3: refactor imported code into CSSUnprefixingService{} scope & generateUnprefixedGradientValue() API
Attachment #8601814 - 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+
(Assignee)

Updated

4 years ago
Depends on: 1162319
(Assignee)

Comment 29

4 years ago
Created attachment 8602431 [details] [diff] [review]
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
(Assignee)

Updated

4 years ago
Blocks: 1162608
(Assignee)

Comment 31

4 years ago
Filed bug 1162608 on the -webkit-radial-gradient issue discussed in comment 29.
(Assignee)

Updated

4 years ago
Flags: in-testsuite+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1158383

Updated

3 years ago
tracking-p11: --- → +
(Assignee)

Updated

3 years ago
Blocks: 1213126
(Assignee)

Updated

3 years ago
No longer blocks: 1213126
(Assignee)

Updated

3 years ago
See Also: → bug 1217643
(Assignee)

Updated

3 years ago
See Also: → bug 1210575
Keywords: dev-doc-needed
I assume the actual implementation is done in bug 1210575, so I'm removing the dev-doc-needed again.


Sebastian
Keywords: dev-doc-needed
(Assignee)

Comment 35

2 years ago
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.