752187 - Support unprefixed linear-gradient & radial-gradient

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jean-Yves Perrier [:teoli]]

Now that CSS3 Images has reached CR, we should ASAP drop prefix on linear-gradient().

1) Our implementation is fairly close to the final syntax and semantic (mainly need a change in angle direction, I think)
2) We pushed for it at the CSSWG (and accepted heart-breaking decisions like postponing element() to CSS4 Images). We need to act in a coherent way: would be sad to have done this, which impairs our fight against -webkit-box-reflect, and not harvest the positive side of it.
3) k9o-related evangelism need this to be successful. 

(There are so many k9o-related meta bugs that I don't know which one this one should block)

Comment 1

[Jason Smith [:jsmith]]

Gradients are on the list of CSS properties that have high evidence in top sites of webkit-only use (example: foursquare). Nominating for kilimanjaro, as this affects top sites.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Saw you had a patch on try; hope you don't mind me assigning the bug to you.

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: Implement unprefixed gradients

Comment 4

[Masatoshi Kimura [:emk]]

Attachment: Tests

Comment 5

[Masatoshi Kimura [:emk]]

This patch also contains an implementation of unprefixed radial-gradient().

Comment 6

[Masatoshi Kimura [:emk]]

Win7 debug mochitest-4 output Exceeds 5 MB again...
https://tbpl.mozilla.org/php/getParsedLog.php?id=11775874&tree=Try

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

blocking on making a decision on whether we'll drop the prefix in time for k9o

Comment 8

[Jason Smith [:jsmith]]

(In reply to Brad Lassey [:blassey] from comment #7)
> blocking on making a decision on whether we'll drop the prefix in time for
> k9o

Tantek - Do you think we would be able to drop the prefix for gradients in time for k9o?

Comment 9

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #8)
> (In reply to Brad Lassey [:blassey] from comment #7)
> > blocking on making a decision on whether we'll drop the prefix in time for
> > k9o
> 
> Tantek - Do you think we would be able to drop the prefix for gradients in
> time for k9o?

Or better question - Should we do this?

Comment 10

[Tantek Çelik]

Yes we should unprefix gradients. The reasoning in Comment 0 (description) is correct.

Comment 11

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

Comment on attachment 621428 [details] [diff] [review]
Implement unprefixed gradients

This would have been much easier to review if it had been a patch queue;
the reorganization should not have been in the same patch as the
functional changes.  I keep thinking I'm going to get through it, but I
think it might actually make more sense to try to split the patch up
first.

However, a few comments that I've already noticed:

I don't see the point of nsCSSValueGradient::mRadiusX if it's always a
reference to mRadialSize.  It's just a wasted word of storage.  If you
want something with two names, use a getter function.

It also looks like mRadiusY is also redundant, since you use either
mRadiusY or mRadialShape in the operator==.

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Separate color stops parsing

Comment 13

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Separate legacy gradient line parsing

Comment 14

[Masatoshi Kimura [:emk]]

Attachment: Part 3: Separate linear gradient parsing and radial gradient parsing

This is just copy & paste. Actual changes are included in following patches.

Comment 15

[Masatoshi Kimura [:emk]]

Attachment: Part 4: Remove aIsRadial parameter

Comment 16

[Masatoshi Kimura [:emk]]

Attachment: Part 5: Remove "to" corner parsing from ParseRadialGradient

Comment 17

[Masatoshi Kimura [:emk]]

Attachment: Part 6: Rename mIsToCorner to mIsLegacySyntax

Comment 18

[Masatoshi Kimura [:emk]]

Attachment: Part 7: Add aIsLegacy parameter to parsing functions

Comment 19

[Masatoshi Kimura [:emk]]

Attachment: Part 8: Implement unprefixed linear-gradient parsing

Comment 20

[Masatoshi Kimura [:emk]]

Attachment: Part 9: Implement unprefixed radial-gradient parsing

Comment 21

[Masatoshi Kimura [:emk]]

Attachment: Part 10: Implement redering unprefixed gradients

Comment 22

[Masatoshi Kimura [:emk]]

Attachment: Part 11: Implement serializing unprefixed gradients

Comment 23

[Masatoshi Kimura [:emk]]

Attachment: Part 12: Implement serializing unprefixed computed values

Comment 24

[Masatoshi Kimura [:emk]]

Attachment: Tests

Comment 25

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

Comment on attachment 638366 [details] [diff] [review]
Part 2: Separate legacy gradient line parsing

I don't think you actually need the |return false| behavior here at all; if there's an error the later could should catch it as well.

So I'd prefer to make the return value here be what you have in the |haveGradientLine| out parameter, make the default: case just be "break;", and rename the function to IsGradientLine.  (I don't see how it's specific to being a legacy gradient line.)

I think you should also rename the function's parameters to begin with "a" followed by a capital letter.  (The same for the previous patch as well.)

r=dbaron with that

Comment 26

[Masatoshi Kimura [:emk]]

(In reply to David Baron [:dbaron] from comment #25)
> (I don't see how it's specific
> to being a legacy gradient line.)
Non-legacy syntax parsing will be added in part 8 & 9. The latest gradient-line syntax is different and incompatible from legacy one, so I divided the code.

Comment 27

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

Comment on attachment 638371 [details] [diff] [review]
Part 7: Add aIsLegacy parameter to parsing functions

>+    nsAutoString ident = tk->mIdent;
>+    bool isLegacy = false;
>+    if (StringBeginsWith(ident, NS_LITERAL_STRING("-moz-"))) {
>+      ident.Cut(0, 5);
>+      isLegacy = true;
>+    }
>+    bool isRepeating = false;
>+    if (StringBeginsWith(ident, NS_LITERAL_STRING("repeating-"))) {
>+      ident.Cut(0, 10);
>+      isRepeating = true;
>+    }
>+
>+    if (ident.LowerCaseEqualsLiteral("linear-gradient"))
>+      return ParseLinearGradient(aValue, isRepeating, isLegacy);
>+
>+    if (ident.LowerCaseEqualsLiteral("radial-gradient"))
>+      return ParseRadialGradient(aValue, isRepeating, isLegacy);

Rather than mutating strings, I think this is better to do with dependent strings, like this:

    nsDependentString tmp1(tk->mIdent, 0);
    bool isLegacy = false;
    if (StringBeginsWith(tmp1, NS_LITERAL_STRING("-moz-"))) {
      tmp1.Rebind(tk->mIdent, 5);
      isLegacy = true;
    }

    nsDependentString tmp2(tmp1, 0);
    bool isRepeating = false;
    if (StringBeginsWith(tmp2, NS_LITERAL_STRING("repeating-"))) {
      tmp2.Rebind(tmp1, 10);
      isRepeating = true;
    }

    if (tmp2.LowerCaseEqualsLiteral("linear-gradient"))
       ...

also, please brace (put {} around) the one-line if bodies at the end of the quote above given that you're rewriting them.

r=dbaron with that (if it works, at least, otherwise ok to leave it)

Comment 28

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

(I think it's actually ok to use a single temporary rather than 2, after looking a little more closely at nsTDependentString_CharT::Rebind and nsTSubstring_CharT::Finalize.)

Comment 29

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

(In reply to Masatoshi Kimura [:emk] from comment #26)
> (In reply to David Baron [:dbaron] from comment #25)
> > (I don't see how it's specific
> > to being a legacy gradient line.)
> Non-legacy syntax parsing will be added in part 8 & 9. The latest
> gradient-line syntax is different and incompatible from legacy one, so I
> divided the code.

OK, then leave the function name.  (I still need to understand part 8.)

Comment 30

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

Comment on attachment 638373 [details] [diff] [review]
Part 9: Implement unprefixed radial-gradient parsing

Instead of calling the new constant table kRadialGradientExtentKTable,
rename the old one to kRadialGradientLegacySizeKTable and reuse the
old name (switching all uses).

>+  cssGradient->GetRadialShape().SetNoneValue();

>+  cssGradient->GetRadialSize().SetNoneValue();

you don't need to do these explicitly; it's already none from the constructor


>+    haveSize =
>+      ParseNonNegativeVariant(cssGradient->GetRadiusX(), VARIANT_LP, nsnull);
>+    if (haveSize) {
>+      // vertical extent is optional
>+      bool haveYSize =
>+        ParseNonNegativeVariant(cssGradient->GetRadiusY(), VARIANT_LP, nsnull);
>+      if (!haveShape) {
>+        haveShape = ParseVariant(cssGradient->GetRadialShape(), VARIANT_KEYWORD,
>+                                 nsCSSProps::kRadialGradientShapeKTable);

This overwrites the RadiusX with the RadialShape.  However, you don't
need to store the shape value here -- you only need to check that it's
'circle' if haveYSize is false and 'ellipse' if haveYSize is true.  So
you should put it in a different variable.

>+      if (!haveYSize &&
>+            (cssGradient->GetRadiusX().GetUnit() == eCSSUnit_Percent ||
>+             shape == NS_STYLE_GRADIENT_SHAPE_ELLIPTICAL) ||
>+          haveYSize && shape == NS_STYLE_GRADIENT_SHAPE_CIRCULAR) {

I think this would actually be easier to read with a ternary operator:

      if (haveYSize
            ? shape == NS_STYLE_GRADIENT_SHAPE_CIRCULAR
            : cssGradient->GetRadiusX().GetUnit() == eCSSUnit_Percent ||
              shape == NS_STYLE_GRADIENT_SHAPE_ELLIPTICAL) {


r=dbaron with those things fixed

Comment 31

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

Comment on attachment 638374 [details] [diff] [review]
Part 10: Implement redering unprefixed gradients

nsRuleNode.cpp:

>+    SetCoord(gradient->GetRadiusX(), aResult.mRadiusX, nsStyleCoord(),
>+             SETCOORD_LPO | SETCOORD_STORE_CALC,
>+             aContext, aPresContext, aCanStoreInRuleTree);
>+    if (gradient->GetRadiusY().GetUnit() != eCSSUnit_None) {
>+      SetCoord(gradient->GetRadiusY(), aResult.mRadiusY, nsStyleCoord(),
>+               SETCOORD_LPO | SETCOORD_STORE_CALC,
>+               aContext, aPresContext, aCanStoreInRuleTree);
>+      aResult.mShape = NS_STYLE_GRADIENT_SHAPE_ELLIPTICAL;
>+    } else {
>+      aResult.mShape = NS_STYLE_GRADIENT_SHAPE_CIRCULAR;
>+    }
>+    aResult.mSize = NS_STYLE_GRADIENT_SIZE_EXPLICIT_SIZE;

These should both use SETCOORD_LP rather than SETCOORD_LPO.  You
don't need to handle eCSSUnit_None.

You also need to set aResult.mRadiusY to mRadiusX when the Y unit is 
none.  (You should add a test that catches the resulting misrendering.)

r=dbaron with those things fixed

Comment 32

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

Comment on attachment 638376 [details] [diff] [review]
Part 12: Implement serializing unprefixed computed values

>+    } else if (aGradient->mBgPosX.GetUnit() != eStyleUnit_Percent ||
>+        aGradient->mBgPosX.GetPercentValue() != 0.5f ||
>+        aGradient->mBgPosY.GetUnit() != eStyleUnit_Percent ||
>+        aGradient->mBgPosY.GetPercentValue() != (isRadial ? 0.5f : 1.0f)) {

I'm a little scared of adding these conditions, but it is the right thing to do, and they look correct, so I guess it's best to keep them.

Fix the indent here too.

r=dbaron with that

Comment 33

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Separate color stops parsing

Comment 34

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Separate legacy gradient line parsing

Comment 35

[Masatoshi Kimura [:emk]]

Attachment: Part 3: Separate linear gradient parsing and radial gradient parsing

Comment 36

[Masatoshi Kimura [:emk]]

Attachment: Part 4: Remove aIsRadial parameter

Comment 37

[Masatoshi Kimura [:emk]]

Attachment: Part 5: Remove "to" corner parsing from ParseRadialGradient

Comment 38

[Masatoshi Kimura [:emk]]

Attachment: Part 6: Rename mIsToCorner to mIsLegacySyntax

Comment 39

[Masatoshi Kimura [:emk]]

Attachment: Part 7: Add aIsLegacy parameter to parsing functions

Comment 40

[Masatoshi Kimura [:emk]]

Attachment: Part 8: Implement unprefixed linear-gradient parsing

Comment 41

[Masatoshi Kimura [:emk]]

Attachment: Part 9: Implement unprefixed radial-gradient parsing

Comment 42

[Masatoshi Kimura [:emk]]

Attachment: Part 10: Implement rendering unprefixed gradients

Comment 43

[Masatoshi Kimura [:emk]]

Attachment: Part 12: Implement serializing unprefixed computed values

Comment 44

[Masatoshi Kimura [:emk]]

Attachment: Tests

Comment 45

[Masatoshi Kimura [:emk]]

All review comments are resolved.

(In reply to David Baron [:dbaron] from comment #31)
> You also need to set aResult.mRadiusY to mRadiusX when the Y unit is 
> none.  (You should add a test that catches the resulting misrendering.)
Modified radial-shape-*-1b.html to catch this error.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9704644a486f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5df0233b74e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae0a58ffa3f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cbef9a8469d
https://hg.mozilla.org/integration/mozilla-inbound/rev/6aa473d9aa47
https://hg.mozilla.org/integration/mozilla-inbound/rev/58c902db06f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f3a325d58a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/d228cf703d19
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f569b4103b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/abebbd5a16ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/f31675524ba3
https://hg.mozilla.org/integration/mozilla-inbound/rev/27241e3ecff2
https://hg.mozilla.org/integration/mozilla-inbound/rev/8acd1c0b2383

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9704644a486f
https://hg.mozilla.org/mozilla-central/rev/f5df0233b74e
https://hg.mozilla.org/mozilla-central/rev/ae0a58ffa3f8
https://hg.mozilla.org/mozilla-central/rev/5cbef9a8469d
https://hg.mozilla.org/mozilla-central/rev/6aa473d9aa47
https://hg.mozilla.org/mozilla-central/rev/58c902db06f2
https://hg.mozilla.org/mozilla-central/rev/1f3a325d58a4
https://hg.mozilla.org/mozilla-central/rev/d228cf703d19
https://hg.mozilla.org/mozilla-central/rev/2f569b4103b6
https://hg.mozilla.org/mozilla-central/rev/abebbd5a16ba
https://hg.mozilla.org/mozilla-central/rev/f31675524ba3
https://hg.mozilla.org/mozilla-central/rev/27241e3ecff2
https://hg.mozilla.org/mozilla-central/rev/8acd1c0b2383

Comment 48

[j.j.]

Dão, why did you remove dependency to bug 776225?
It's somewhat common to do this. E.g. is makes it easy to find the right bug to resolve future duplicates.

Comment 49

[Dão Gottwald [:dao]]

(In reply to j.j. (inactive in 2012) from comment #48)
> Dão, why did you remove dependency to bug 776225?

Because that bug is invalid anyway and the extra bugmail due to the resolved/reopened fights is annoying and unnecessary.

Comment 50

[j.j.]

Calculating your annoyance against bug triager's convenience and accuracy is an unsolved equation.

Comment 51

[Dão Gottwald [:dao]]

I don't see the accuracy. The bug is invalid, tracking it as a dependency isn't useful. Feel free to add it to the "blocks" list, though.

Comment 53

[Daniel Holbert [:dholbert]]

--> Updating summary to make this bug more clearly distinguishable from bug 1176496 (which is where we actually drop support for prefixed gradients).