Support unprefixed linear-gradient & radial-gradient

RESOLVED FIXED in mozilla16

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: teoli, Assigned: emk)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla16
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-kilimanjaro:+)

Details

(Whiteboard: [qa+])

Attachments

(13 attachments, 14 obsolete attachments)

8.10 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.32 KB, patch
emk
: review+
Details | Diff | Splinter Review
5.17 KB, patch
emk
: review+
Details | Diff | Splinter Review
6.88 KB, patch
emk
: review+
Details | Diff | Splinter Review
6.46 KB, patch
emk
: review+
Details | Diff | Splinter Review
3.72 KB, patch
emk
: review+
Details | Diff | Splinter Review
4.88 KB, patch
emk
: review+
Details | Diff | Splinter Review
5.72 KB, patch
emk
: review+
Details | Diff | Splinter Review
5.15 KB, patch
emk
: review+
Details | Diff | Splinter Review
18.20 KB, patch
emk
: review+
Details | Diff | Splinter Review
9.93 KB, patch
emk
: review+
Details | Diff | Splinter Review
5.62 KB, patch
emk
: review+
Details | Diff | Splinter Review
95.67 KB, patch
emk
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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)
(Reporter)

Updated

5 years ago
OS: Mac OS X → All
Hardware: x86 → All
Summary: Drop prefix for linear-gradients() → Drop prefix for linear-gradient()
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.
Saw you had a patch on try; hope you don't mind me assigning the bug to you.
Assignee: nobody → VYV03354
(Assignee)

Comment 3

5 years ago
Created attachment 621428 [details] [diff] [review]
Implement unprefixed gradients
Attachment #621428 - Flags: review?(dbaron)
(Assignee)

Comment 4

5 years ago
Created attachment 621429 [details] [diff] [review]
Tests
Attachment #621429 - Flags: review?(dbaron)
(Assignee)

Comment 5

5 years ago
This patch also contains an implementation of unprefixed radial-gradient().
Summary: Drop prefix for linear-gradient() → Drop prefix for gradients
(Assignee)

Comment 6

5 years ago
Win7 debug mochitest-4 output Exceeds 5 MB again...
https://tbpl.mozilla.org/php/getParsedLog.php?id=11775874&tree=Try
blocking on making a decision on whether we'll drop the prefix in time for k9o
blocking-kilimanjaro: ? → +
(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?
(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

5 years ago
Yes we should unprefix gradients. The reasoning in Comment 0 (description) is correct.
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==.
(Assignee)

Comment 12

5 years ago
Created attachment 638365 [details] [diff] [review]
Part 1: Separate color stops parsing
Attachment #621428 - Attachment is obsolete: true
Attachment #621428 - Flags: review?(dbaron)
Attachment #638365 - Flags: review?(dbaron)
(Assignee)

Comment 13

5 years ago
Created attachment 638366 [details] [diff] [review]
Part 2: Separate legacy gradient line parsing
Attachment #638366 - Flags: review?(dbaron)
(Assignee)

Comment 14

5 years ago
Created attachment 638367 [details] [diff] [review]
Part 3: Separate linear gradient parsing and radial gradient parsing

This is just copy & paste. Actual changes are included in following patches.
Attachment #638367 - Flags: review?(dbaron)
(Assignee)

Comment 15

5 years ago
Created attachment 638368 [details] [diff] [review]
Part 4: Remove aIsRadial parameter
Attachment #638368 - Flags: review?(dbaron)
(Assignee)

Comment 16

5 years ago
Created attachment 638369 [details] [diff] [review]
Part 5: Remove "to" corner parsing from ParseRadialGradient
Attachment #638369 - Flags: review?(dbaron)
(Assignee)

Comment 17

5 years ago
Created attachment 638370 [details] [diff] [review]
Part 6: Rename mIsToCorner to mIsLegacySyntax
Attachment #638370 - Flags: review?(dbaron)
(Assignee)

Comment 18

5 years ago
Created attachment 638371 [details] [diff] [review]
Part 7: Add aIsLegacy parameter to parsing functions
Attachment #638371 - Flags: review?(dbaron)
(Assignee)

Comment 19

5 years ago
Created attachment 638372 [details] [diff] [review]
Part 8: Implement unprefixed linear-gradient parsing
Attachment #638372 - Flags: review?(dbaron)
(Assignee)

Comment 20

5 years ago
Created attachment 638373 [details] [diff] [review]
Part 9: Implement unprefixed radial-gradient parsing
Attachment #638373 - Flags: review?(dbaron)
(Assignee)

Comment 21

5 years ago
Created attachment 638374 [details] [diff] [review]
Part 10: Implement redering unprefixed gradients
Attachment #638374 - Flags: review?(dbaron)
(Assignee)

Comment 22

5 years ago
Created attachment 638375 [details] [diff] [review]
Part 11: Implement serializing unprefixed gradients
Attachment #638375 - Flags: review?(dbaron)
(Assignee)

Comment 23

5 years ago
Created attachment 638376 [details] [diff] [review]
Part 12: Implement serializing unprefixed computed values
Attachment #638376 - Flags: review?(dbaron)
(Assignee)

Comment 24

5 years ago
Created attachment 638377 [details] [diff] [review]
Tests
Attachment #621429 - Attachment is obsolete: true
Attachment #621429 - Flags: review?(dbaron)
Attachment #638377 - Flags: review?(dbaron)
Attachment #638365 - Flags: review?(dbaron) → review+
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
Attachment #638366 - Flags: review?(dbaron) → review+
Attachment #638367 - Flags: review?(dbaron) → review+
Attachment #638368 - Flags: review?(dbaron) → review+
Attachment #638369 - Flags: review?(dbaron) → review+
Attachment #638370 - Flags: review?(dbaron) → review+
(Assignee)

Comment 26

5 years ago
(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 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)
Attachment #638371 - Flags: review?(dbaron) → review+
(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.)
(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.)
Attachment #638372 - Flags: review?(dbaron) → review+
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
Attachment #638373 - Flags: review?(dbaron) → review+
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
Attachment #638374 - Flags: review?(dbaron) → review+
Attachment #638375 - Flags: review?(dbaron) → review+
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
Attachment #638376 - Flags: review?(dbaron) → review+
Attachment #638377 - Flags: review?(dbaron) → review+
(Assignee)

Comment 33

5 years ago
Created attachment 639882 [details] [diff] [review]
Part 1: Separate color stops parsing
Attachment #638365 - Attachment is obsolete: true
Attachment #639882 - Flags: review+
(Assignee)

Comment 34

5 years ago
Created attachment 639883 [details] [diff] [review]
Part 2: Separate legacy gradient line parsing
Attachment #638366 - Attachment is obsolete: true
Attachment #639883 - Flags: review+
(Assignee)

Comment 35

5 years ago
Created attachment 639884 [details] [diff] [review]
Part 3: Separate linear gradient parsing and radial gradient parsing
Attachment #638367 - Attachment is obsolete: true
Attachment #639884 - Flags: review+
(Assignee)

Comment 36

5 years ago
Created attachment 639885 [details] [diff] [review]
Part 4: Remove aIsRadial parameter
Attachment #638368 - Attachment is obsolete: true
Attachment #639885 - Flags: review+
(Assignee)

Comment 37

5 years ago
Created attachment 639887 [details] [diff] [review]
Part 5: Remove "to" corner parsing from ParseRadialGradient
Attachment #638369 - Attachment is obsolete: true
Attachment #639887 - Flags: review+
(Assignee)

Comment 38

5 years ago
Created attachment 639888 [details] [diff] [review]
Part 6: Rename mIsToCorner to mIsLegacySyntax
Attachment #638370 - Attachment is obsolete: true
Attachment #639888 - Flags: review+
(Assignee)

Comment 39

5 years ago
Created attachment 639889 [details] [diff] [review]
Part 7: Add aIsLegacy parameter to parsing functions
Attachment #638371 - Attachment is obsolete: true
Attachment #639889 - Flags: review+
(Assignee)

Comment 40

5 years ago
Created attachment 639890 [details] [diff] [review]
Part 8: Implement unprefixed linear-gradient parsing
Attachment #638372 - Attachment is obsolete: true
Attachment #639890 - Flags: review+
(Assignee)

Comment 41

5 years ago
Created attachment 639891 [details] [diff] [review]
Part 9: Implement unprefixed radial-gradient parsing
Attachment #638373 - Attachment is obsolete: true
Attachment #639891 - Flags: review+
(Assignee)

Comment 42

5 years ago
Created attachment 639892 [details] [diff] [review]
Part 10: Implement rendering unprefixed gradients
Attachment #638374 - Attachment is obsolete: true
Attachment #639892 - Flags: review+
(Assignee)

Comment 43

5 years ago
Created attachment 639893 [details] [diff] [review]
Part 12: Implement serializing unprefixed computed values
Attachment #638376 - Attachment is obsolete: true
Attachment #639893 - Flags: review+
(Assignee)

Comment 44

5 years ago
Created attachment 639894 [details] [diff] [review]
Tests
Attachment #638377 - Attachment is obsolete: true
Attachment #639894 - Flags: review+
(Assignee)

Comment 45

5 years ago
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.
Keywords: checkin-needed
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
Flags: in-testsuite+
Keywords: checkin-needed

Updated

5 years ago
Whiteboard: [qa+]
Target Milestone: --- → mozilla16
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Depends on: 771975

Updated

5 years ago
Depends on: 591600

Updated

5 years ago
Blocks: 772342

Updated

5 years ago
Blocks: 775235

Updated

5 years ago
Depends on: 776225

Updated

5 years ago
No longer depends on: 776225

Comment 48

5 years ago
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.
(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

5 years ago
Calculating your annoyance against bug triager's convenience and accuracy is an unsolved equation.
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.
(Assignee)

Updated

5 years ago
Duplicate of this bug: 712474
(Reporter)

Updated

5 years ago
Keywords: dev-doc-needed → dev-doc-complete

Updated

5 years ago
Blocks: 823673

Updated

5 years ago
No longer blocks: 772342
(Assignee)

Updated

4 years ago
Depends on: 941300
--> Updating summary to make this bug more clearly distinguishable from bug 1176496 (which is where we actually drop support for prefixed gradients).
Summary: Drop prefix for gradients → Support unprefixed linear-gradient & radial-gradient
You need to log in before you can comment on or make changes to this bug.