Last Comment Bug 594935 - support calc() on gradient stop positions
: support calc() on gradient stop positions
Status: RESOLVED FIXED
: css3, dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal with 9 votes (vote)
: mozilla18
Assigned To: Thomasy
:
Mentors:
: 647488 (view as bug list)
Depends on:
Blocks: css3-values
  Show dependency treegraph
 
Reported: 2010-09-09 14:05 PDT by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2012-11-12 02:49 PST (History)
16 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase showing 2 divs (1 using calc(), 1 using px) (should look identical) (388 bytes, text/plain)
2012-02-15 14:36 PST, Brandon Frohs
no flags Details
Testcase showing 2 divs (1 using calc(), 1 using px) (should look identical) (388 bytes, text/html)
2012-02-15 14:41 PST, Brandon Frohs
no flags Details
Update testcase remove "45deg" from style sheet (381 bytes, text/html)
2012-09-19 06:24 PDT, Thomasy
no flags Details
Add calc to parser and do calc in PaintGradient (5.59 KB, patch)
2012-09-19 07:11 PDT, Thomasy
no flags Details | Diff | Review
Add calc to parser and do calc in PaintGradient (5.36 KB, patch)
2012-09-19 10:58 PDT, Thomasy
no flags Details | Diff | Review
Update acorrding to Comment 9 (6.12 KB, patch)
2012-09-19 20:49 PDT, Thomasy
no flags Details | Diff | Review
Add calc to parser and do calc in PaintGradient (6.17 KB, patch)
2012-09-20 00:11 PDT, Thomasy
bzbarsky: review+
Details | Diff | Review
Add stop point to HasCalc() (8.96 KB, patch)
2012-09-22 22:55 PDT, Thomasy
bzbarsky: review+
Details | Diff | Review
Add calc to parser and do calc in PaintGradient (8.94 KB, patch)
2012-10-03 19:12 PDT, Thomasy
no flags Details | Diff | Review
Add calc to parser and do calc in PaintGradient (8.93 KB, patch)
2012-10-03 19:20 PDT, Thomasy
bzbarsky: review+
Details | Diff | Review

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-09-09 14:05:30 PDT
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 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-04-02 20:46:47 PDT
*** Bug 647488 has been marked as a duplicate of this bug. ***
Comment 2 Lea Verou 2011-04-30 07:37:38 PDT
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 2012-02-15 14:36:32 PST
Created attachment 597563 [details]
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 2012-02-15 14:41:37 PST
Created attachment 597565 [details]
Testcase showing 2 divs (1 using calc(), 1 using px) (should look identical)
Comment 5 Thomasy 2012-09-17 22:43:33 PDT
(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 2012-09-19 06:24:03 PDT
Created attachment 662528 [details]
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 2012-09-19 07:11:57 PDT
Created attachment 662544 [details] [diff] [review]
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 2012-09-19 10:58:53 PDT
Created attachment 662614 [details] [diff] [review]
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-7 (review requests must explain patch) 2012-09-19 17:01:22 PDT
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 2012-09-19 20:49:21 PDT
Created attachment 662791 [details] [diff] [review]
Update acorrding to Comment 9

Add tests to layout/style/test/property_database.js
Comment 11 Thomasy 2012-09-19 21:00:46 PDT
(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-7 (review requests must explain patch) 2012-09-19 21:49:46 PDT
I mean that your code needs to test whether lineLength < 1e-6, just like the code above does.
Comment 13 Thomasy 2012-09-20 00:11:35 PDT
Created attachment 662837 [details] [diff] [review]
Add calc to parser and do calc in PaintGradient

Update prevent lineLength too small or devided-by-zero
Comment 14 Mozilla RelEng Bot 2012-09-20 01:00:28 PDT
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/thomas@thomasy.tw-0d31e2c87860
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-09-20 19:54:45 PDT
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 2012-09-20 19:57:19 PDT
(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] 2012-09-21 17:34:14 PDT
This appears to have some intermittent reftest failures on WinXP. Please investigate.
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-09-21 20:14:23 PDT
Happens on Win7 too.
Comment 19 Thomasy 2012-09-22 05:03:29 PDT
(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 2012-09-22 22:55:06 PDT
Created attachment 663786 [details] [diff] [review]
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 [:bz] (Out June 25-July 6) 2012-10-03 19:04:38 PDT
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 2012-10-03 19:12:44 PDT
Created attachment 667775 [details] [diff] [review]
Add calc to parser and do calc in PaintGradient

Update checkin message.
Comment 23 Thomasy 2012-10-03 19:20:37 PDT
Created attachment 667776 [details] [diff] [review]
Add calc to parser and do calc in PaintGradient

Update check-in message.
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-03 19:57:37 PDT
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] 2012-10-03 22:33:45 PDT
Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/9fd65b219801
Comment 26 Ed Morley [:emorley] 2012-10-04 08:55:26 PDT
https://hg.mozilla.org/mozilla-central/rev/9fd65b219801

Note You need to log in before you can comment on or make changes to this bug.