594935 - support calc() on gradient stop positions

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

We might want to support calc() on the gradient stop positions in the -moz-*-gradient functions in the background-image property.  (This is different from the gradient endpoint positions, which are covered in bug 594934.)

This is work that I did not do in bug 363249 / bug 585715, and am not currently planning to do for Mozilla 2 / Firefox 4.

Comment 2

[Lea Verou]

Btw, in case you need a use case for this, it's extremely useful when mixing percentages with absolute units in color stop positions. For example, you want to make a middle stripe with thickness 1in regardless of container size. The gradient for that would be 

linear-gradient(white calc(50%-.5in), red calc(50%-.5in), red calc(50%+.5in), white calc(50%+.5in))

I'm pretty sure there are similar cases with "normal" gradients too, but this is the case that usually has me wishing for calc() in color stop positions.

Comment 3

[Brandon Frohs]

Attachment: Testcase showing 2 divs (1 using calc(), 1 using px) (should look identical)

Minimal testcase containing one div using calc() and another using pixels. Both should look identical once calc() is allowed.

Comment 4

[Brandon Frohs]

Attachment: Testcase showing 2 divs (1 using calc(), 1 using px) (should look identical)

Comment 5

[Thomasy]

(In reply to Brandon Frohs from comment #4)
> Created attachment 597565 [details]
> Testcase showing 2 divs (1 using calc(), 1 using px) (should look identical)

Testcase need to remove "45deg, " in order to look identical. Since, "div#two" have no "45deg, " style sheet.

Comment 6

[Thomasy]

Attachment: Update testcase remove "45deg" from style sheet

Testcase showing 2 divs (1 using calc(), 1 using px) (should look identical). Update testcase remove "45deg" from style sheet

Comment 7

[Thomasy]

Attachment: Add calc to parser and do calc in PaintGradient

1. Handle eStyleUnit_Calc type in nsCSSRendering::PaintGradient
2. Add VARIANT_CALC to ParseVariant @ CSSParserImpl::ParseColorStop
3. Add SETCOORD_STORE_CALC to SetCoord @ SetGradient
4. Update comment @ layout/style/nsStyleStruct.h

Comment 8

[Thomasy]

Attachment: Add calc to parser and do calc in PaintGradient

1. Handle eStyleUnit_Calc type in nsCSSRendering::PaintGradient
2. Add VARIANT_CALC to ParseVariant @ CSSParserImpl::ParseColorStop
3. Add SETCOORD_STORE_CALC to SetCoord @ SetGradient
4. Update comment @ layout/style/nsStyleStruct.h

Comment 9

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

Comment on attachment 662614 [details] [diff] [review]
Add calc to parser and do calc in PaintGradient

>diff --git a/layout/base/nsCSSRendering.cpp b/layout/base/nsCSSRendering.cpp
>--- a/layout/base/nsCSSRendering.cpp
>+++ b/layout/base/nsCSSRendering.cpp
>@@ -2041,6 +2041,7 @@
>     for (uint32_t i = 0; i < aGradient->mStops.Length(); ++i) {
>       const nsStyleGradientStop& stop = aGradient->mStops[i];
>       double position;
>+      nsStyleCoord::Calc *calc;
>       switch (stop.mLocation.GetUnit()) {
>       case eStyleUnit_None:
>         if (i == 0) {

Instead of declaring variables outside the switch, put {} inside the case, like here:
http://hg.mozilla.org/mozilla-central/file/0c8ac138706e/layout/style/nsStyleAnimation.cpp#l390

>@@ -2068,6 +2069,11 @@
>         position = lineLength < 1e-6 ? 0.0 :
>             stop.mLocation.GetCoordValue() / appUnitsPerPixel / lineLength;
>         break;
>+      case eStyleUnit_Calc:
>+        calc = stop.mLocation.GetCalcValue();
>+        position = calc->mPercent +
>+            NSAppUnitsToFloatPixels(calc->mLength, appUnitsPerPixel) / lineLength;
>+        break;

This presumably needs the same test of lineLength being very small that the eStyleUnit_Percent case has.

(It might also be worth sharing the code if you can see a good way to do it, but I don't.)

You should also add tests to layout/style/test/property_database.js (where there are other gradient tests), and make sure that the mochitests in layout/style/ still pass after you do.

Comment 10

[Thomasy]

Attachment: Update acorrding to Comment 9

Add tests to layout/style/test/property_database.js

Comment 11

[Thomasy]

(In reply to David Baron [:dbaron] from comment #9)
> Comment on attachment 662614 [details] [diff] [review]
> Add calc to parser and do calc in PaintGradient
> 
> >diff --git a/layout/base/nsCSSRendering.cpp b/layout/base/nsCSSRendering.cpp
> >--- a/layout/base/nsCSSRendering.cpp
> >+++ b/layout/base/nsCSSRendering.cpp
> >@@ -2041,6 +2041,7 @@
> >     for (uint32_t i = 0; i < aGradient->mStops.Length(); ++i) {
> >       const nsStyleGradientStop& stop = aGradient->mStops[i];
> >       double position;
> >+      nsStyleCoord::Calc *calc;
> >       switch (stop.mLocation.GetUnit()) {
> >       case eStyleUnit_None:
> >         if (i == 0) {
> 
> Instead of declaring variables outside the switch, put {} inside the case,
> like here:
> http://hg.mozilla.org/mozilla-central/file/0c8ac138706e/layout/style/
> nsStyleAnimation.cpp#l390
> 
> >@@ -2068,6 +2069,11 @@
> >         position = lineLength < 1e-6 ? 0.0 :
> >             stop.mLocation.GetCoordValue() / appUnitsPerPixel / lineLength;
> >         break;
> >+      case eStyleUnit_Calc:
> >+        calc = stop.mLocation.GetCalcValue();
> >+        position = calc->mPercent +
> >+            NSAppUnitsToFloatPixels(calc->mLength, appUnitsPerPixel) / lineLength;
> >+        break;
> 
> This presumably needs the same test of lineLength being very small that the
> eStyleUnit_Percent case has.
> 
I do not see any related tests in 

https://bugzilla.mozilla.org/show_bug.cgi?id=594935
http://hg.mozilla.org/mozilla-central/rev/32e1ceb06e44

> (It might also be worth sharing the code if you can see a good way to do it,
> but I don't.)
> 
> You should also add tests to layout/style/test/property_database.js (where
> there are other gradient tests), and make sure that the mochitests in
> layout/style/ still pass after you do.

Get it. Thanks

Comment 12

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

I mean that your code needs to test whether lineLength < 1e-6, just like the code above does.

Comment 13

[Thomasy]

Attachment: Add calc to parser and do calc in PaintGradient

Update prevent lineLength too small or devided-by-zero

Comment 14

[Mozilla RelEng Bot]

Try run for 0d31e2c87860 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0d31e2c87860
Results (out of 14 total builds):
    exception: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0d31e2c87860

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 662837 [details] [diff] [review]
Add calc to parser and do calc in PaintGradient

Adding the "-p -U 8" flags to your diff command would be a good idea.  It would definitely make this easier to review!

r=me

Comment 16

[Thomasy]

(In reply to Boris Zbarsky (:bz) from comment #15)
> Comment on attachment 662837 [details] [diff] [review]
> Add calc to parser and do calc in PaintGradient
> 
> Adding the "-p -U 8" flags to your diff command would be a good idea.  It
> would definitely make this easier to review!
> 
> r=me

Get it.

Green on try.

https://tbpl.mozilla.org/?tree=Try&rev=58b4680ecb51

Comment 17

[Ryan VanderMeulen [:RyanVM]]

This appears to have some intermittent reftest failures on WinXP. Please investigate.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Happens on Win7 too.

Comment 19

[Thomasy]

(In reply to Ryan VanderMeulen from comment #18)
> Happens on Win7 too.

It always crashes at 
nsStyleCoord::HashValue(unsigned int) [nsStyleCoord.cpp:174f70187687 : 106 + 0x6]

Comment 20

[Thomasy]

Attachment: Add stop point to HasCalc()

Add add stop point to HasCalc() and it seem have no crash on Windows https://tbpl.mozilla.org/?tree=Try&rev=559d028d4aa2

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 663786 [details] [diff] [review]
Add stop point to HasCalc()

Please remove the "try" bits from the checkin message and r=me.

Sorry for the lag here; I hadn't realized this was just a small change from the previous diff.  :(

Comment 22

[Thomasy]

Attachment: Add calc to parser and do calc in PaintGradient

Update checkin message.

Comment 23

[Thomasy]

Attachment: Add calc to parser and do calc in PaintGradient

Update check-in message.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 667776 [details] [diff] [review]
Add calc to parser and do calc in PaintGradient

r=me.  Really, no need for extra review on a nit like this.

By the way, r=bzbarsky is just fine.  ;)

Comment 25

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/9fd65b219801

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9fd65b219801

Comment 27

[Jean-Yves Perrier [:teoli]]

Added a small note on:
https://developer.mozilla.org/en-US/docs/Firefox_19_for_developers
and
https://developer.mozilla.org/en-US/docs/CSS/calc