support calc() on gradient stop positions

RESOLVED FIXED in mozilla18

Status

()

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

People

(Reporter: dbaron, Assigned: Thomasy)

Tracking

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

Trunk
mozilla18
css3, dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

(Reporter)

Description

7 years ago
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.
Duplicate of this bug: 647488

Comment 2

6 years ago
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

6 years ago
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

6 years ago
Created attachment 597565 [details]
Testcase showing 2 divs (1 using calc(), 1 using px) (should look identical)
Attachment #597563 - Attachment is obsolete: true

Updated

6 years ago
Attachment #597565 - Attachment mime type: text/plain → text/html
Blocks: 741643
(Assignee)

Updated

5 years ago
Assignee: nobody → thomas
(Assignee)

Comment 5

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

Comment 6

5 years ago
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
Attachment #597565 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
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
Attachment #662544 - Flags: review?(bzbarsky)
(Assignee)

Comment 8

5 years ago
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
Attachment #662544 - Attachment is obsolete: true
Attachment #662544 - Flags: review?(bzbarsky)
Attachment #662614 - Flags: review?(bzbarsky)
(Reporter)

Comment 9

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

Comment 10

5 years ago
Created attachment 662791 [details] [diff] [review]
Update acorrding to Comment 9

Add tests to layout/style/test/property_database.js
Attachment #662614 - Attachment is obsolete: true
Attachment #662614 - Flags: review?(bzbarsky)
Attachment #662791 - Flags: review?(bzbarsky)
(Assignee)

Comment 11

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

Comment 12

5 years ago
I mean that your code needs to test whether lineLength < 1e-6, just like the code above does.
(Assignee)

Comment 13

5 years ago
Created attachment 662837 [details] [diff] [review]
Add calc to parser and do calc in PaintGradient

Update prevent lineLength too small or devided-by-zero
Attachment #662791 - Attachment is obsolete: true
Attachment #662791 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #662837 - Flags: review?(bzbarsky)

Comment 14

5 years ago
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 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
Attachment #662837 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 16

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

Updated

5 years ago
Keywords: checkin-needed
This appears to have some intermittent reftest failures on WinXP. Please investigate.
Keywords: checkin-needed
Happens on Win7 too.
(Assignee)

Comment 19

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

Comment 20

5 years ago
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
Attachment #662837 - Attachment is obsolete: true
Attachment #663786 - Flags: review?(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.  :(
Attachment #663786 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 22

5 years ago
Created attachment 667775 [details] [diff] [review]
Add calc to parser and do calc in PaintGradient

Update checkin message.
Attachment #663786 - Attachment is obsolete: true
Attachment #667775 - Flags: review?(bzbarsky)
(Assignee)

Comment 23

5 years ago
Created attachment 667776 [details] [diff] [review]
Add calc to parser and do calc in PaintGradient

Update check-in message.
Attachment #667775 - Attachment is obsolete: true
Attachment #667775 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #667776 - Flags: review?(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.  ;)
Attachment #667776 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/9fd65b219801
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9fd65b219801
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Updated

5 years ago
Keywords: dev-doc-needed
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.