513395 - Implement less awkward/more logical CSS gradient syntax

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Kornel]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_8; en-us) AppleWebKit/532.0+ (KHTML, like Gecko) Version/4.0.3 Safari/531.9
Build Identifier: 

Current WebKit-inspired syntax for gradients is too verbose, needlessly abuses functions, is hard to understand, hard to memorize and is pain to use.

There is a very nice, solid proposal (which I've discovered after writing nearly identical one myself :)

http://www.xanthir.com/:4bhipd

that's simpler to use than current syntax, especially in most common cases.



Reproducible: Always

Actual Results:  
eye-sore!

linear-gradient(top left, bottom left, from(#111), to(#333), color-stop(0.5,#222));

Expected Results:  
linear-gradient(#111, #222, #333);

Comment 1

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

Yeah, the document above is the syntax under discussion in the working group.  I think we should switch to it, for 3.6 if possible.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Who's going to do it? We're on a very short schedule for this. Me? You? Zack? Michael?

Comment 3

[Zack Weinberg (:zwol)]

I could probably get it done in a couple days if all that was required was parser changes.  Is that so?  Just how little time do we have?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's not entirely parser changes, but it mostly is. If you do the parser (and nsStyleGradient) changes you, I or someone else can finish it off pretty easily.

Comment 5

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

Are there gradient features that we have now that we wouldn't have if we switched to the new proposal without also doing bug 522607?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Nothing related to 522607 that I know of.

The new spec doesn't support repeating gradients. Maybe we can make background-repeat (used in combination with background-size) do something reasonable though.

Comment 7

[Zack Weinberg (:zwol)]

I don't *think* so, but I could be wrong.  If I understand the old syntax right, it positioned the gradient starting point the same way the new syntax does, so both have the same limitations.  The new syntax defines the endpoint in terms of an angle and a distance to a box edge, instead of explicitly; it might actually be easier to do things with the endpoint in the new syntax, without bug 522607.

Comment 8

[Zack Weinberg (:zwol)]

Attachment: WIP patch v1

This does the parser and style data structure changes, but I haven't fixed up the rendering or any of the tests.  I'll come back to this tomorrow, but if someone wants to work on it meantime be my guest.

Comment 9

[Zack Weinberg (:zwol)]

I forgot to mention: it compiles, but only because I #ifdefed out the entire body of nsCSSRendering::PaintGradient.

Comment 10

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

Attachment: Linear gradients tests

Updates for the linear gradients tests.

Comment 11

[Zack Weinberg (:zwol)]

Attachment: WIP v2 (merging test updates)

Thanks, Ms2ger!  I have merged your patch into mine and made some further updates to property_database.js; layout/style mochitests now pass, and I added a whole bunch of should-be-valid cases to the tests..  I'm going to throw this at the try server while I have lunch.  (Reftests will fail regardless, since gradient rendering is totally disabled, but at least we can find out whether there are any more issues with the parsing and roundtripping.)

Comment 12

[Zack Weinberg (:zwol)]

Tryserver builds are at
http://build.mozilla.org/tryserver-builds/[e-mail]-try-5dbe28c8854a
(to remind everyone, gradients will not be drawn *at all* in these builds, but please do try to break the new parser)

The Linux and Windows unit tests appear to have blown up for unrelated reasons. The Mac unit test log, which I might actually believe, has 7 reftest and 21 mochitest failures.  Some of those are probably also unrelated (test_scrolling.html, crashtests/28613-1.html) and the majority of the reftest failures are of course because there is no gradient drawing.  However, it also looks like I may have broken some assumptions about rounding in the -moz-transform code when I moved the normalize-to-radians-in-(-pi,pi) function to nsCSSValue.  Will look at that on Monday.

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1255725297.1255733951.27937.gz

Comment 13

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

(In reply to comment #12)
> failures are of course because there is no gradient drawing.  However, it also
> looks like I may have broken some assumptions about rounding in the
> -moz-transform code when I moved the normalize-to-radians-in-(-pi,pi) function
> to nsCSSValue.  Will look at that on Monday.

Beware of moving that normalization earlier: it can be bad for animation.  (We actually might need to move it later for animation, but that's another story.)  I think we don't want to normalize that in nsCSSValue since they're not actually equivalent values for purposes of animation.

Comment 14

[Zack Weinberg (:zwol)]

It's done at the same time it always was; I just promoted the CSSToRadians helper function in nsStyleTransformMatrix.cpp to a nsCSSValue getter method (and, apparently, broke the floating point).  nsCSSValue stores un-normalized values.

Comment 15

[Zack Weinberg (:zwol)]

Attachment: WIP v3: small fixes

This small revision fixes some of the mochitest failures.  We now round transformation matrix values exactly as we used to, and recognize 'grad' units properly.

layout/base/test/test_scrolling.html still fails - this appears to be because it makes use of gradients, so is broken by the lack of rendering.  I fixed the gradient syntax in scrolling_helper.html.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I have linear gradient rendering working, at least for basic cases. The syntax for the tests in css-gradients is still incorrect.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Linear gradient rendering

On top of WIP v2. There's a few style fixes in here too. I've fixed up some of the css-gradients reftests to pass. A few still don't work. We need a whole lot more testscases for the features of the new syntax. They seem to work pretty well here though.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix quartz gradient bug

I discovered a small bug in cairo-quartz gradients. If you have multiple stops at position 0, then cairo-quartz pads with the *last* stop at position 0, instead of the first stop at position 0. This patch fixes that.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Gradient rendering v2

Updated with some bug fixes and a lot more tests. Now all the tests pass except for radial.html, which is because I haven't (re)implemented radial gradients yet. We need the cairo-quartz patch I just attached to pass twostops-1a and twostops-1b on Mac.

Zack, it would be great if you could merge this into your latest patch.

Comment 20

[Zack Weinberg (:zwol)]

Attachment: v4: linear gradients rendered

Merged roc's patch with mine and updated for dbaron's changes in bug 522852 (nsStyleCoord no longer has a color alternative or an IsNull() method).

Comment 21

[Zack Weinberg (:zwol)]

For clarity: I did not merge in the cairo-quartz patch.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Render radial gardients

Applies on top of v4. Has tests. Works.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That patch also fixes a bug in linear gradient rendering where we weren't handling the starting point + angle case correctly in some cases.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Add support for repeating gradients

Applies on top of the previous patch.

This adds support for -moz-repeating-linear and -moz-repeating-radial gradients, with tests.

It also fixes subtle issues with how negative stops are handled in radial gradients. We were setting negative stop offsets to 0 too early. That fixup needs to be applied after stops without explicit offsets get their offsets computed. I added a test for that. We also need to handle cases like radial-gradient(black -25px, white 25px) by creating a new stop at position 0 that's halfway between black and white. I haven't got a reftest for that interpolation because I don't know how to write a good one...

Pushed to the tryservers...

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If the tryserver tests look OK, then we should merge these patches together and go for review of the combined patch.

Comment 26

[Zack Weinberg (:zwol)]

All Windows tryserver builds are busted:

e:/builds/moz2_slave/win32-unit/mozilla/layout/base/nsCSSRendering.cpp(1625) : error C2065: 'M_PI' : undeclared identifier
e:/builds/moz2_slave/win32-unit/mozilla/layout/base/nsCSSRendering.cpp(1720) : error C2065: 'M_SQRT2' : undeclared identifier

http://msdn.microsoft.com/en-us/library/4hwaceh6(VS.80).aspx says that one should #define _USE_MATH_DEFINES before including <cmath> / <math.h>, if one wants those constants.  I shall make that change and retry.

Do you think a single giant patch would be easier to review, or several patches (split as syntax, implementation, tests, perhaps?)

Comment 27

[Zack Weinberg (:zwol)]

Attachment: v5: complete patch with build and unit test fixes

Here's a unified patch with the aforementioned Windows build failure (hopefully) fixed, plus some fixes for unit tests:

 - the mIsRepeating check was inverted in nsCSSDeclaration::AppendCSSValueToString, causing a bunch of failures in test_value_storage;
 - several places in nsComputedDOMStyle::GetCSSGradientString were checking for eStyleUnit_Null when they should have been checking for eStyleUnit_None, causing a flood of assertions from test_value_storage; also, one of the conditionals had a stray ! causing yet more test_value_storage failures;
 - the file reftests/css-gradients/radial-2-ref.html was missing, causing reftests to hang (filed bug 523864, they shouldn't hang for that).  I am not sure I wrote the correct reference.

Pushed to try server.  I fat-fingered and sent it twice, once without the new radial-2-ref.html; the build with tip revision http://hg.mozilla.org/try/rev/89caaae7381c is the one to watch.  Unfortunately, trunk is not exactly stable right now, so there may be noise.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: patch to add missing radial-2-ref.html

sorry

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

oh, it's actually the same as yours. Great :-) Thanks!!

Comment 30

[Zack Weinberg (:zwol)]

Oh good, I wasn't sure I had translated it correctly.

twostops-1c.html is failing on Linux: <http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1256232392.1256240803.6855.gz>.  I saw that on my own machine but assumed it was yet another DPI problem.

WinCE and WinMo builds are blowing up on nsCSSRendering.obj:
CE <http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1256232390.1256235636.13263.gz>
Mo <http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1256232390.1256235069.6519.gz>

Looks like #define _USE_MATH_DEFINES is not good enough on these platforms to get the M_PI and M_SQRT2 constants -- or 'hypot', which is C99 but not C90.  MSDN mentions _hypot as an alternative, but plain hypot was working on NT, and I can't find anything specific about CE/Mo math functions.  I leave this to someone who knows from Windows.

The Windows unit tests also failed, but it looks like we got bit by a combination of the "timed out waiting for onload to fire" rando-orange and unrelated tree bustage.  The T-FAIL L-FAIL and "Exited with code -1073741819 during test run" diagnoses also appear for the next build in the try server tinderboxpushlog, which is totally unrelated.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #30)
> Looks like #define _USE_MATH_DEFINES is not good enough on these platforms to
> get the M_PI and M_SQRT2 constants -- or 'hypot', which is C99 but not C90. 
> MSDN mentions _hypot as an alternative, but plain hypot was working on NT, and
> I can't find anything specific about CE/Mo math functions.  I leave this to
> someone who knows from Windows.

Can you just add NS_PI, NS_SQRT2 and NS_hypot to nsMathUtils.h and push again? That would really be more efficient than trying to delegate the problem.

Comment 32

[Zack Weinberg (:zwol)]

Attachment: math fixes for wince/winmo

Yeah sure.  If this isn't good enough for CE, though, I don't know what to do.  hypot is nontrivial - see http://svn.FreeBSD.org/viewvc/base/head/lib/msun/src/e_hypot.c?view=markup&pathrev=177758 for the gory details.

Comment 33

[Zack Weinberg (:zwol)]

Attachment: math fixes with no hypotf

*sigh* WinCE/Mo don't seem to have [_]hypotf at all.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

 /*
+ * constants - names and values match the BSD/XOpen M_ constants.
+ * N.B. they have enough digits of mantissa for IEEE quad 'long double',
+ * but their type is 'double'.
+ */

You might want to mention that we have these here because of WinMo/WinCE limitations...

We don't really need the super-accurate version of hypot, as far as I know, but I suppose we may as well use it where available.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #26)
> Do you think a single giant patch would be easier to review, or several patches
> (split as syntax, implementation, tests, perhaps?)

I think probably two patches: syntax + syntax tests, rendering + reftests

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

twostops-1c.html failed on Linux and Windows. It's just a difference of exactly where the transition between blue and red happens. On Mac the blue and red halves are exactly 150px height each, which seems right... On Linux and Windows the blue half (the first half) is 151px high, the red half is 149px high. That's probably a cairo issue. We should just disable this test on Linux and Windows. Other than that, we're good to go for review.

Actually if you post the refactored patches, I should add some extra comments to the rendering patch.

Comment 37

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 407257 [details] [diff] [review]
fix quartz gradient bug

Using a negative value here to ensure that the first stop color is selected scares me a little bit. What approach does Webkit use to ensure this?

Comment 38

[Zack Weinberg (:zwol)]

(In reply to comment #34)
>  /*
> + * constants - names and values match the BSD/XOpen M_ constants.
> + * N.B. they have enough digits of mantissa for IEEE quad 'long double',
> + * but their type is 'double'.
> + */
> 
> You might want to mention that we have these here because of WinMo/WinCE
> limitations...

Done.  I changed it around again; it tries harder to get the M_ constants out of Windows <math.h> and if they still aren't available it defines them with the M_ names.  This avoids introducing yet more renames of things that are standardized.

> We don't really need the super-accurate version of hypot, as far as I know, but
> I suppose we may as well use it where available.

Seems to be moot since the WinCE builds are happy using _hypot, but I added a note to the header saying that falling back to sqrt(x*x + y*y) would be acceptable if it became necessary.

(In reply to comment #36)
> twostops-1c.html failed on Linux and Windows. It's just a difference of exactly
> where the transition between blue and red happens. On Mac the blue and red
> halves are exactly 150px height each, which seems right... On Linux and Windows
> the blue half (the first half) is 151px high, the red half is 149px high.
> That's probably a cairo issue. We should just disable this test on Linux and
> Windows. Other than that, we're good to go for review.

I was able to reproduce the problem with canvas gradients.  I filed bug 524173, added a couple more variants of the problem test, and marked them fails-if(MOZ_WIDGET_TOOLKIT!="cocoa").

> Actually if you post the refactored patches, I should add some extra comments
> to the rendering patch.

Coming up.

Comment 39

[Zack Weinberg (:zwol)]

Attachment: split patch 1/2: syntax

Here's part 1 of the split patch for review.  I split it strictly by file, so you need both pieces to compile.  The nsMathUtils change is in here, as nsCSSValue needs some of that.

Comment 40

[Zack Weinberg (:zwol)]

Attachment: split patch 2/2: rendering

Part 2.

roc, you get to pick reviewers. :)

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #37)
> (From update of attachment 407257 [details] [diff] [review])
> Using a negative value here to ensure that the first stop color is selected
> scares me a little bit. What approach does Webkit use to ensure this?

I don't know. It's entirely possible they don't care.

Why does this scare you?

Comment 42

[Zack Weinberg (:zwol)]

In the tryserver cycle on the above "split patch"es, twostops-1c.html failed on Mac.  This is still bug 524173, but apparently it affects Mac as well.  I'm now inclined to mark twostops-1c through 1e as random on all targets.  Thoughts?

(canvas/linear-gradient-1b.html also failed, but that test belongs to bug 524173 and will not be part of the checkin for this bug, so it can be ignored.)

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #42)
> In the tryserver cycle on the above "split patch"es, twostops-1c.html failed on
> Mac.  This is still bug 524173, but apparently it affects Mac as well.  I'm now
> inclined to mark twostops-1c through 1e as random on all targets.  Thoughts?

Sounds reasonable.

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 2: rendering (v2)

Added a bunch of comments to the rendering patch. I think this is ready for review.

Comment 45

[Michael Ventnor]

+  double leftDistance = abs(aLineStart->x);
+  double rightDistance = abs(aBoxSize.width - aLineStart->x);
+  double topDistance = abs(aLineStart->y);
+  double bottomDistance = abs(aBoxSize.height - aLineStart->y);

Is this the standard math library abs? If so, this isn't available in wince. You'll have to use PR_ABS().

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Didn't this build on WinCE on the try servers?

Comment 47

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

Comment on attachment 408085 [details] [diff] [review]
split patch 1/2: syntax

I did both of these reviews on an airplane, referring to an out-of-date
copy of Tab's spec.  That means some of the comments may not make sense
with respect to the current spec.  I'll review these again tomorrow
morning (I hope) with respect to the current spec, send comments on it
to www-style, and probably update this bug as well.  But I figured I'd
give you the comments now, as some of them (especially on this patch)
require a bit of work.

nsCSSDeclaration.cpp:

You introduce a bunch of 80th-column violations.

Also, in the final loop over stops, you might stop using the loop
termination condition part of the for(), but instead put a break;
in the middle of the loop, to avoid checking the condition twice.

nsCSSParser.cpp:

+  PRBool ParseGradient(nsCSSValue& aValue, PRBool aIsRadial, PRBool aIsRepeatin

please wrap after the second comma (80th-column violation)

In ParseColorStop:

+  aGradient->mStops.AppendElement(nsCSSValueGradientStop(position, color));

I'd slightly prefer if you used the 0-argument AppendElement() at the
start of the function, checked it for null (set mErrorCode and return
false), and then parse directly into it.  This avoids copying each
nsCSSValue twice.

In ParseGradient:

+  ParseBoxPositionValues(bgPosition, PR_FALSE); // don't care if it succeeds
+  ParseVariant(angle, VARIANT_ANGLE, nsnull);   // ditto
+
+  if (bgPosition.HasValue() || angle.GetUnit() != eCSSUnit_Null) {

You absolutely do care if they succeed, for two reasons:
 * you need to reject invalid values (as opposed to missing ones)
 * they sometimes touch their outparams even when they return false;
   it's the return value that's authoritative.

In order to do the former correctly, you need to either:
 * decide which (if any) to call based on what the next token is (this
   is what we do in all similar cases, and probably preferable)
 * add a mechanism for determining if they consumed any tokens (and
   audit them to make sure they unget tokens where they need to); I'm
   not entirely sure this would work, but I could imagine a token
   counter in the scanner that could be compared.
I'd prefer the former.

Additionally, you also need to accept the angle before the position;
that's what "||" in CSS property syntax descriptions means.  (Unless the
spec has changed here; I'm looking at a slightly old copy.)

And you should add tests that would have caught all of these issues.

+    ParseVariant(shape, VARIANT_KEYWORD, nsCSSProps::kGradientShapeKTable);
+    ParseVariant(size, VARIANT_KEYWORD, nsCSSProps::kGradientSizeKTable);
+
+    if (shape.GetUnit() != eCSSUnit_Null || size.GetUnit() != eCSSUnit_Null) {

All the same issues here.

+  if (!ParseColorStop(cssGradient)
+      || !ExpectSymbol(',', PR_TRUE)
+      || !ParseColorStop(cssGradient)) {

Mozilla style puts the || at the end of the previous line, not start
of the next.

nsCSSProps.cpp/h:
  kGradientShapeKTable, etc., should be renamed with
  s/kGradient/kRadialGradient/.

  Also, in nsCSSProps::kGradientSizeKTable, you should add a comment
  about the duplicate values (and that the canonical version in
  serialization is the one listed first).

nsCSSValue.cpp:

-#include "nsIPrincipal.h"
+#include "nsIPrincipal.h"

No need to reorder the existing headers.  (Maybe a result of merging?)

nsCSSValue.h (and cpp):
  
+  // Return value is in radians and reduced to the range (-pi, pi).
+  float GetNormalizedAngleValue() const;

nsCSSValue shouldn't do angle normalization.  (It's bad for animation; I
need to get the spec changed in this regard as well.)  I also suspect
you generally don't need any normalization since you're going to end up
passing angles to trig functions that don't need it.

(This basically means replacing the last 2 lines of the implementation
with just |return angle|, and renaming the function, perhaps to
GetAngleValueInRadians().)

nsRuleNode.cpp:

+  if (!SetCoord(aValue, aResult, nsStyleCoord(), SETCOORD_LP,
+                aContext, aPresContext, aCanStoreInRuleTree)) {
+    // Non-length/percent values are stored as None.
+    aResult.SetNoneValue();
+  }

if SetCoord fails, you should assert that aValue.GetUnit() is eCSSUnit_Null.

+  if (gradient->mAngle.IsAngularUnit()) {
+    aResult.mAngle.SetAngleValue(gradient->mAngle.GetNormalizedAngleValue());
+  } else {
+    aResult.mAngle.SetNoneValue();

In the else, you should assert that gradient->mAngle.GetUnit() is
eCSSUnit_Null.

+    SetGradientCoord(valueStop.mLocation, aPresContext, aContext,
+                     stop.mLocation, aCanStoreInRuleTree);

If you're going to reuse SetGradientCoord for this, you should at least
assert that the location doesn't have an enumerated value before you do.

nsStyleCoord.h:

+  eStyleUnit_Angle        = 12,     // (float) angle normalized to (-pi, pi)

Angles in nsStyleCoord should not be normalized; it breaks animation
based on computed values (including transitions).

+  float       GetAngleValue(void) const;
+  void  SetAngleValue(float aValue);

Should say what the units are (i.e., radians).

nsStyleStruct.h:

+  PRUint8 mSize;   // NS_STYLE_GRADIENT_SIZE_*; not used for SHAPE_LINEAR

You should say that it has to be NS_STYLE_GRADIENT_SIZE_FARTHEST_CORNER
for linear.

nsStyleTransformMatrix.cpp:

You should undo a bunch (although not all) of these changes per
my earlier comments about not normalizing to a range.  (You can still
refactor the conversion to radians.)

property_database.js:

  STILL NEED TO REVIEW THESE (waiting until I have the newer spec in
  front of me)

Comment 48

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

Comment on attachment 408193 [details] [diff] [review]
part 2: rendering (v2)

I did both of these reviews on an airplane, referring to an out-of-date
copy of Tab's spec.  That means some of the comments may not make sense
with respect to the current spec.  And, in the case of this patch, I
actually didn't finish the review since I wanted to read the spec before
going over the key drawing part.  I'll review these again tomorrow
morning (I hope) with respect to the current spec, send comments on it
to www-style, and post the rest of the comments on this patch to this
bug.  But I figured I'd give you the comments now.


I think the xpcom/ds/nsMathUtils.h changes in the other patch actually
belong in this one, don't they?


> static gfxFloat
> ConvertGradientValueToPixels(const nsStyleCoord& aCoord,
>-                             nscoord aFillLength,
>+                             gfxFloat aFillLength,
>                              nscoord aAppUnitsPerPixel)

I know you're not changing this bit, but nscoord seems like an unusual
unit for aAppUnitsPerPixel.  The function you're now passing it to takes
a float.  nsPresContext::AppUnitsPerDevPixel returns a PRInt32.  This
should probably be one or the other, and the appUnitsPerPixel value in
nsCSSRendering::PaintGradient should match it.


Given the changes I suggested to the previous patch,
ComputeFarthestCornerForAngle needs to normalize the angle.

I'd also suggest calling it ComputeCornerInDirectionOfAngle.

But, really, I think there's a rather simpler way to do this:  instead
of interpreting the spec literally, handle the angle-only case in
ComputeLinearGradientLine by computing the gradient end as though you
were computing for a start position of center combined with that angle,
and then make the gradient start be the reflection of that point about
the center.  This gives an gradient line equivalent to the one in the
spec (i.e., it and the one in the spec are two of the sides of a
rectangle).  Then you can just get rid of the second call to
ComputeFarthestCornerFromAngle.

+// and a starting point for the graident line aStart, find the endpoint of

s/graident/gradient/

This is your current ComputeGradientLineEndFromAngle:
>+  gfxPoint farthestCorner = ComputeFarthestCornerForAngle(aAngle, aBoxSize);
>+  gfxPoint delta = farthestCorner - aStart;
>+  double dx = cos(aAngle);
>+  double dy = -sin(aAngle);
>+  double u = delta.x*dy - delta.y*dx;
>+  return farthestCorner + gfxPoint(-u*dy, u*dx);

Now that ComputeFarthestCornerForAngle has just one caller, you can
incorporate ComputeFarthestCornerForAngle (and make it not require
normalization) by changing it to:

+double dx = cos(aAngle);
+double dy = -sin(aAngle);
+gfxPoint farthestCorner(dx > 0 ? aBoxSize.width : 0,
+                        dy > 0 ? aBoxSize.height : 0);
+gfxPoint delta = farthestCorner - aStart;
+double u = delta.x*dy - delta.y*dx;
+return farthestCorner + gfxPoint(-u*dy, u*dx);

It might also make sense to use the cos(-aAngle) / sin(-aAngle)
formulation here rather than cos(aAngle)/-sin(aAngle)

(That said... do |cos| and |sin| not handle large angles well?  I'd
think they'd normalize internally if needed, but maybe they don't.  If
so, we still might want to do normalization to [-pi,pi) somewhere in
this code, but it shouldn't be in the style code.)


COME BACK TO CHECKING THE END OF THIS FUNCTION

in ComputeLinearGradientLine *and* ComputeRadialGradientLine:

>+    *aLineStart = gfxPoint(
>+      ConvertGradientValueToPixels(aGradient->mBgPosX, aBoxSize.width, appUnits
>+      ConvertGradientValueToPixels(aGradient->mBgPosY, aBoxSize.height, appUnit

80th-column violations

nsCSSRendering::PaintGradient needs to handle lineLength being 0; a
linear gradient starting at center, or from X to X, does that.  (I think
the spec says that (and a bunch of other radial cases I mention below)
makes it a solid color.)

+      // maintain aspec ratio
+      radiusX = offsetX*M_SQRT2;
+      radiusY = offsetY*M_SQRT2;

s/aspec/aspect/

COME BACK TO THIS: IS THIS EVEN RIGHT?


You need to have a mechanism for ComputeRadialGradientLine to propagate
failure, and do the same solid color thing.  In particular, the somewhat
dated draft of the spec I'm looking at says:

# In certain circumstances it is impossible to determine an appropriate
# ending-shape from the given parameters; in these instances the
# gradient image is just a solid color, defined by the last color-stop
# in the list. The following combinations of values will trigger this:
# closest-side  if the starting-point is on a box edge, closest-corner
# if the starting-point is on a box corner, ellipse and closest-corner
# if the starting-point is on a box-edge, and finally ellipse  and
# either closest-corner or closest-side if the starting-point is outside
# of the box.

Your code produces negative or 0 radii in some of these cases, although
not the very last one.  (Maybe the spec is wrong there?)  

So it might be sufficient not to bother with error propagation but just
use both radii >= 0 as the success condition (and do the solid color in
that case).

Both FARTHEST_* cases should call PR_ABS on all the distances before
using them.

PaintGradient:

>+  double lineLength = NS_hypot(lineEnd.x - lineStart.x,

Maybe call it gfxFloat since it's a length in pixels?

>+  // Eliminate negative-position stops if the gradient is radial.

SKIPPED FROM HERE TO END OF FUNCTION until I get the updated spec.

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #48)
> I think the xpcom/ds/nsMathUtils.h changes in the other patch actually
> belong in this one, don't they?

Yeah, could be. I didn't read the other patch.

> > static gfxFloat
> > ConvertGradientValueToPixels(const nsStyleCoord& aCoord,
> >-                             nscoord aFillLength,
> >+                             gfxFloat aFillLength,
> >                              nscoord aAppUnitsPerPixel)
> 
> I know you're not changing this bit, but nscoord seems like an unusual
> unit for aAppUnitsPerPixel.  The function you're now passing it to takes
> a float.  nsPresContext::AppUnitsPerDevPixel returns a PRInt32.  This
> should probably be one or the other, and the appUnitsPerPixel value in
> nsCSSRendering::PaintGradient should match it.

nscoord is actually a good unit for it, I think, since it's a number of appunits. I'll change it to a PRInt32, though.

> Given the changes I suggested to the previous patch,
> ComputeFarthestCornerForAngle needs to normalize the angle.

Yep.

> I'd also suggest calling it ComputeCornerInDirectionOfAngle.

Sure OK.

> But, really, I think there's a rather simpler way to do this:  instead
> of interpreting the spec literally, handle the angle-only case in
> ComputeLinearGradientLine by computing the gradient end as though you
> were computing for a start position of center combined with that angle,
> and then make the gradient start be the reflection of that point about
> the center.  This gives an gradient line equivalent to the one in the
> spec (i.e., it and the one in the spec are two of the sides of a
> rectangle).  Then you can just get rid of the second call to
> ComputeFarthestCornerFromAngle.

Good idea!

> This is your current ComputeGradientLineEndFromAngle:
> >+  gfxPoint farthestCorner = ComputeFarthestCornerForAngle(aAngle, aBoxSize);
> >+  gfxPoint delta = farthestCorner - aStart;
> >+  double dx = cos(aAngle);
> >+  double dy = -sin(aAngle);
> >+  double u = delta.x*dy - delta.y*dx;
> >+  return farthestCorner + gfxPoint(-u*dy, u*dx);
> 
> Now that ComputeFarthestCornerForAngle has just one caller, you can
> incorporate ComputeFarthestCornerForAngle (and make it not require
> normalization) by changing it to:
> 
> +double dx = cos(aAngle);
> +double dy = -sin(aAngle);
> +gfxPoint farthestCorner(dx > 0 ? aBoxSize.width : 0,
> +                        dy > 0 ? aBoxSize.height : 0);
> +gfxPoint delta = farthestCorner - aStart;
> +double u = delta.x*dy - delta.y*dx;
> +return farthestCorner + gfxPoint(-u*dy, u*dx);
> 
> It might also make sense to use the cos(-aAngle) / sin(-aAngle)
> formulation here rather than cos(aAngle)/-sin(aAngle)

Looks great.

> (That said... do |cos| and |sin| not handle large angles well?  I'd
> think they'd normalize internally if needed, but maybe they don't.  If
> so, we still might want to do normalization to [-pi,pi) somewhere in
> this code, but it shouldn't be in the style code.)

I think they should be OK. If not, I blame the compiler/library.

> nsCSSRendering::PaintGradient needs to handle lineLength being 0; a
> linear gradient starting at center, or from X to X, does that.  (I think
> the spec says that (and a bunch of other radial cases I mention below)
> makes it a solid color.)

I think cairo already handles that, although I should add tests for these cases.

> # In certain circumstances it is impossible to determine an appropriate
> # ending-shape from the given parameters; in these instances the
> # gradient image is just a solid color, defined by the last color-stop
> # in the list. The following combinations of values will trigger this:
> # closest-side  if the starting-point is on a box edge, closest-corner
> # if the starting-point is on a box corner, ellipse and closest-corner
> # if the starting-point is on a box-edge, and finally ellipse  and
> # either closest-corner or closest-side if the starting-point is outside
> # of the box.
> 
> Your code produces negative or 0 radii in some of these cases, although
> not the very last one.  (Maybe the spec is wrong there?)

The spec changed in response to my feedback. We made all these be defined according to the definition already given in the spec (interpreting "the sides of the box" to mean lines extended infinitely beyond the corners of the box).

The patch does not ever produce negative radii, as far as I can see.
+  double leftDistance = abs(aLineStart->x);
+  double rightDistance = abs(aBoxSize.width - aLineStart->x);
+  double topDistance = abs(aLineStart->y);
+  double bottomDistance = abs(aBoxSize.height - aLineStart->y);
So those are all non-negative, and then we just do MINs, MAXs, hypots, and *M_SQRT2 operations on them.

> PaintGradient:
> 
> >+  double lineLength = NS_hypot(lineEnd.x - lineStart.x,
> 
> Maybe call it gfxFloat since it's a length in pixels?

Sure.

Comment 50

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

About half the spec issues I noticed were already fixed in the last two weeks; I sent the rest as http://lists.w3.org/Archives/Public/www-style/2009Oct/0303.html


(In reply to comment #49)
> The spec changed in response to my feedback. We made all these be defined
> according to the definition already given in the spec (interpreting "the sides
> of the box" to mean lines extended infinitely beyond the corners of the box).
> 
> The patch does not ever produce negative radii, as far as I can see.

Not sure how I managed to miss that one.

In any case, we need to be sure that you handle both 0-radius and 0-gradient-line-length correctly, as the spec says.  If cairo happens to do what we want, we still want both tests and code comments pointing out that it does what we want in that case.

Comment 51

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

(In reply to comment #49)
> > nsCSSRendering::PaintGradient needs to handle lineLength being 0; a
> > linear gradient starting at center, or from X to X, does that.  (I think
> > the spec says that (and a bunch of other radial cases I mention below)
> > makes it a solid color.)
> 
> I think cairo already handles that, although I should add tests for these
> cases.

Er, cairo might handle it fine, but your code will do a divide-by-zero in one of the cases.

Comment 52

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

Comments on the rest of the rendering patch:

In theory, both InterpolateColor and what we pass to cairo ought to 
handle the 'color-interpolation' property, I think.  But we don't handle
that for SVG gradients (or animations!) yet either.

>+      // So to strecth the ellipse by factor of P vertically, we scale

stretch

And does this really handle radiusX == 0 correctly?

And in the ellipse/circle case, it's probably wirth asserting that 
firstStop >= 0.

In the tiling code, I'm having trouble understanding where the origin of
the tiling comes from.  Do you have testcases for various 
background-origins (and background-repeats) and tiling of gradients?

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #52)
> In theory, both InterpolateColor and what we pass to cairo ought to 
> handle the 'color-interpolation' property, I think.  But we don't handle
> that for SVG gradients (or animations!) yet either.

Added a comment for that.

> And does this really handle radiusX == 0 correctly?

No, I'll make the stretch code conditional on both radii being > 0.

> And in the ellipse/circle case, it's probably wirth asserting that 
> firstStop >= 0.

Yes, added.

> In the tiling code, I'm having trouble understanding where the origin of
> the tiling comes from.  Do you have testcases for various 
> background-origins (and background-repeats) and tiling of gradients?

I have one testcase for background-position for each of linear and radial. I'll add some more, and a comment there.

(In reply to comment #51)
> Er, cairo might handle it fine, but your code will do a divide-by-zero in one
> of the cases.

Ah, right. I'll add a test and set coordinate stop positions to 0.0 instead of dividing by zero in that case.

Comment 54

[Zack Weinberg (:zwol)]

Attachment: part 1: syntax (v2)

This revised patch has been compiled, but not tested, and I have not
made any changes to the tests themselves.

(In reply to comment #47)
> nsCSSDeclaration.cpp:
> You introduce a bunch of 80th-column violations.

Fixed throughout the patch; sorry about this, the sheer number of
existing violations of this rule causes me to get sloppy from time to time.

> Also, in the final loop over stops, you might stop using the loop
> termination condition part of the for(), but instead put a break;
> in the middle of the loop, to avoid checking the condition twice.

Done.

> In ParseColorStop:
>
> +  aGradient->mStops.AppendElement(nsCSSValueGradientStop(position, color));
>
> I'd slightly prefer if you used the 0-argument AppendElement() at the
> start of the function, checked it for null (set mErrorCode and return
> false), and then parse directly into it.  This avoids copying each
> nsCSSValue twice.

Done.  I had to change the nsCSSValueGradientStop constructor.  I also
did the same thing in ParseGradient, except that it's not possible to
put a nsCSSValuePair in a nsCSSValueGradient, so we are still copying
the background position.

> In ParseGradient:
>
> +  ParseBoxPositionValues(bgPosition, PR_FALSE); // don't care if it succeeds
> +  ParseVariant(angle, VARIANT_ANGLE, nsnull);   // ditto
> +
> +  if (bgPosition.HasValue() || angle.GetUnit() != eCSSUnit_Null) {
>
> You absolutely do care if they succeed, for two reasons:
>  * you need to reject invalid values (as opposed to missing ones)
>  * they sometimes touch their outparams even when they return false;
>    it's the return value that's authoritative.
>
> In order to do the former correctly, you need to either:
>  * decide which (if any) to call based on what the next token is (this
>    is what we do in all similar cases, and probably preferable)
>  * add a mechanism for determining if they consumed any tokens (and
>    audit them to make sure they unget tokens where they need to); I'm
>    not entirely sure this would work, but I could imagine a token
>    counter in the scanner that could be compared.
> I'd prefer the former.

Well, I've done this, but man, that was no fun, and the result is not
fun to read, either.  I think it would be better, going forward, if
ParseVariant and friends could be trusted not to consume any tokens or
touch their output if they return PR_FALSE.

> Additionally, you also need to accept the angle before the position;
> that's what "||" in CSS property syntax descriptions means.  (Unless the
> spec has changed here; I'm looking at a slightly old copy.)

I assumed || just meant concatenation because anything else seemed
mind-bogglingly ugly.  But sure enough, §1.4.2.1 says "one or more, in
any order."  Blech.  Fixed.

> And you should add tests that would have caught all of these issues.

Not done yet; will get to it tomorrow.

> +    ParseVariant(shape, VARIANT_KEYWORD, nsCSSProps::kGradientShapeKTable);
> +    ParseVariant(size, VARIANT_KEYWORD, nsCSSProps::kGradientSizeKTable);
>
> All the same issues here.

Also fixed.

> nsCSSProps.cpp/h:
>   kGradientShapeKTable, etc., should be renamed with
>   s/kGradient/kRadialGradient/.
>

Fixed.

>   Also, in nsCSSProps::kGradientSizeKTable, you should add a comment
>   about the duplicate values (and that the canonical version in
>   serialization is the one listed first).

Will "// synonyms" before the _contain and _cover entries do?

> -#include "nsIPrincipal.h"
> +#include "nsIPrincipal.h"
>
> No need to reorder the existing headers.  (Maybe a result of merging?)

Fixed.  Not sure how that happened.

>
> nsCSSValue.h (and cpp):
>
> +  // Return value is in radians and reduced to the range (-pi, pi).
> +  float GetNormalizedAngleValue() const;
>
> nsCSSValue shouldn't do angle normalization.  (It's bad for animation; I
> need to get the spec changed in this regard as well.)  I also suspect
> you generally don't need any normalization since you're going to end up
> passing angles to trig functions that don't need it.
>
> (This basically means replacing the last 2 lines of the implementation
> with just |return angle|, and renaming the function, perhaps to
> GetAngleValueInRadians().)

Done.  This might break rounding assumptions in
nsStyleTransformMatrix.cpp, however.  (I'm not sure why that file is
doing everything in |float| anyway...)

> nsRuleNode.cpp:
>
> +  if (!SetCoord(aValue, aResult, nsStyleCoord(), SETCOORD_LP,
> +                aContext, aPresContext, aCanStoreInRuleTree)) {
> +    // Non-length/percent values are stored as None.
> +    aResult.SetNoneValue();
> +  }
>
> if SetCoord fails, you should assert that aValue.GetUnit() is eCSSUnit_Null.

What I've done instead is have the parser set the angle &c to
eCSSUnit_None instead of _Null when not present, changed to
SETCOORD_LPO (which had to be added), and then I just NS_RUNTIMEERROR
if SetCoord fails.

>
> +  if (gradient->mAngle.IsAngularUnit()) {
> +    aResult.mAngle.SetAngleValue(gradient->mAngle.GetNormalizedAngleValue());
> +  } else {
> +    aResult.mAngle.SetNoneValue();
>
> In the else, you should assert that gradient->mAngle.GetUnit() is
> eCSSUnit_Null.

Done (modulo _None instead of _Null) and also cleaned up the radial
shape and size similarly.

> +    SetGradientCoord(valueStop.mLocation, aPresContext, aContext,
> +                     stop.mLocation, aCanStoreInRuleTree);
>
> If you're going to reuse SetGradientCoord for this, you should at least
> assert that the location doesn't have an enumerated value before you do.

Switched to plain SetCoord.

>
> nsStyleCoord.h:
>
> +  eStyleUnit_Angle        = 12,     // (float) angle normalized to (-pi, pi)
>
> Angles in nsStyleCoord should not be normalized

Changed to // (float) angle in radians.  No normalization was being
done by nsStyleCoord itself.

> +  float       GetAngleValue(void) const;
> +  void  SetAngleValue(float aValue);
>
> Should say what the units are (i.e., radians).

Will the comment on eStyleUnit_Angle suffice there?

> nsStyleStruct.h:
>
> +  PRUint8 mSize;   // NS_STYLE_GRADIENT_SIZE_*; not used for SHAPE_LINEAR
>
> You should say that it has to be NS_STYLE_GRADIENT_SIZE_FARTHEST_CORNER
> for linear.

Done.

> nsStyleTransformMatrix.cpp:
>
> You should undo a bunch (although not all) of these changes per
> my earlier comments about not normalizing to a range.  (You can still
> refactor the conversion to radians.)

This was normalizing before I changed anything.  For the now, I've had
it use .GetAngleValueInRadians and not normalize, as it appears always
to pass the value directly to <math.h> trig functions that will reduce
their input anyway, but again, this may change rounding behavior and
break existing test cases (this code seems to be very sensitive to
rounding).

> I think the xpcom/ds/nsMathUtils.h changes in the other patch actually
> belong in this one, don't they?

The first patch makes nsCSSValue.cpp use M_PI, so it needs at least
that part, and nsMathUtils.h is included all over the place so I
thought it best to change it only once.

Comment 55

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: rendering (v3)

Updated.

twostops-1c.html isn't passing for me on Mac anymore, and I can't figure out why, we seem to be doing the right thing all the way down to the call to CGDrawShading. But since this doesn't pass on the other platforms either, I've just marked it as failing.

It seems that for radial gradients with zero inner and outer radii, cairo (by which I mean pixman, since we fall back to pixman on Mac in this case) actually fills with the first color stop color. This may be a cairo/pixman bug, but I'm working around it here by only adding one color stop (the last color stop) for repeating or radial gradients with all color stops at the same point.

Comment 56

[Zack Weinberg (:zwol)]

Attachment: part 1: syntax (v2a)

Added test cases as requested, and fixed several parser bugs exposed by the new test cases.  Also, reintroduced the #if 0 around the entire body of nsCSSRendering::PaintGradient so that this patch can be compiled without the rendering component.

Comment 57

[Zack Weinberg (:zwol)]

Attachment: part 2: rendering (v3a)

Just rediffed this on top of syntax-v2a.

Some of the layout mochitests are failing due to what appear to be rounding issues in nsStyleTransformMatrix.cpp.  I would appreciate any advice on how to tackle those.

Should the quartz gradient patch maybe get its own bug?

Comment 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #57)
> Some of the layout mochitests are failing due to what appear to be rounding
> issues in nsStyleTransformMatrix.cpp.  I would appreciate any advice on how to
> tackle those.

What are the issues?

> Should the quartz gradient patch maybe get its own bug?

I've asked Jeff to review it today.

Comment 59

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 407257 [details] [diff] [review]
fix quartz gradient bug

r+ with the comment additions discussed on irc.

Comment 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: quartz gradient fix (v2)

Updated with superior comments, ready for landing.

Comment 61

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: quartz gradient fix (v3) (Checkin: comment #79)

Oops. Updated with cairo patch info, now REALLY ready for checkin.

Comment 62

[Zack Weinberg (:zwol)]

(In reply to comment #58)
> (In reply to comment #57)
> > Some of the layout mochitests are failing due to what appear to be rounding
> > issues in nsStyleTransformMatrix.cpp.  I would appreciate any advice on how to
> > tackle those.
> 
> What are the issues?

test_bug435293-skew.html:

failed | Test4: Skew mixing deg and rad - 
           got "matrix(1, 1, 1.74846e-7, 1, 0px, 0px)", 
   expected "matrix(1, 1, 0, 1, 0px, 0px)"

failed | Test11: Skew with more infinite numbers - 
           got "matrix(1, -10000, -10000, 1, 0px, 0px)", 
   expected "matrix(1, -10000, 10000, 1, 0px, 0px)"

This is almost certainly because dbaron had me take the angle normalization out, but I don't know what to do about it.

Comment 63

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You're just going to have to debug it, I guess.

Comment 64

[Zack Weinberg (:zwol)]

Attachment: kludge for 435293-skew failures

(In reply to comment #63)
> You're just going to have to debug it, I guess.

It's not that I can't debug it; attached is an additional patch that makes the failures go away.  It's that what happened is not strictly speaking a bug, but a change in behavior away from nice tidy numbers and toward more mathematically accurate numbers.  We're using lousy floating point precision (i.e. "float" rather than "double").   The math library, internally, does very high precision argument reduction for sin and cos, which reveals that (in test 4) the sine of 6.283185 is not zero, and (in test 11) the cosine of 300grad = 4.712389 is on the other side of 3π/2 than we expected it to be.  So I'm not exactly happy with the patch.

What do you say to making nsCSSValue store doubles rather than floats?  As a separate bug.

Note that there are also changes to the test case (the "pane" stuff) whose aim is to make it not hide the failure messages under a sea of green anymore, when you load it directly into the browser.

Comment 65

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #64)
> It's not that I can't debug it; attached is an additional patch that makes the
> failures go away.

Ah, OK, great! This patch looks good to me. Thanks.

> What do you say to making nsCSSValue store doubles rather than floats?  As a
> separate bug.

Would that actually solve the problem? I'm not sure. I'm not sure if we store a lot of nsCSSValues for long.

Comment 66

[Zack Weinberg (:zwol)]

(In reply to comment #65)
> > What do you say to making nsCSSValue store doubles rather than floats?  As a
> > separate bug.
> 
> Would that actually solve the problem? I'm not sure. I'm not sure if we store a
> lot of nsCSSValues for long.

Fundamentally, the issue here (as I see it - I'd like dbaron to weigh in if possible, didn't he implement the transforms stuff) is that values in 'rad' units are being rounded to 7 decimal digits at parse time, leaving us fairly far away from the true value of pi.

I had an alternative idea over dinner: we could add a new CSS unit - call it "pirad", perhaps - that is scaled by pi.  So 1pirad == 3.14159...rad.  Then we could just use more precision in internal calculations and leave nsCSSValue alone.

Comment 67

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

Maybe the common unit we convert to should be degrees, since they can store the important values exactly?  Then we can convert to radians after converting to doubles at the end?

Changing things to use doubles might be a little more involved than we want right now.

Comment 68

[Zack Weinberg (:zwol)]

Attachment: better fix for angle issues (NOT FOR COMMITTING)

I came up with a better way of dealing with the angle issues this morning.  What we need here is:

1) Preserve angles in their as-written form until the point of use: i.e. nsStyleCoord needs to distinguish deg/rad/grad as well as nsCSSValue.
2) When we do ultimately convert to radians for use, do that calculation in double precision and keep the intermediates in double precision until they get fed to trig functions.  (roc's rendering code already does this, mostly; nsStyleTransformMatrix didn't.
3) Don't use radian units in tests where the exact value matters.
4) test_bug435293-skew.html should accept a value that is very close to, but not exactly, zero as tan(360deg).  It already had code for imprecise floating point comparisons, so I recycled that.

If we do these things we don't need to carry values around as 'double' in nsCSSValue and nsStyleCoord, and we don't need any more rounding kludges in nsStyleTransformMatrix.  Double precision for CSS dimension values may still be a good idea, but we can do it later.

This patch is a collection of my changes for ease of review, but I have actually rolled them into the syntax and rendering patches, which I will repost next.

Comment 69

[Zack Weinberg (:zwol)]

Attachment: part 1: syntax (v4)

Latest revision of syntax patch, incorporating appropriate bits of the angle changes.

Comment 70

[Zack Weinberg (:zwol)]

Attachment: part 2: rendering (v4)

rendering update with angle adjustments.

This stack has been pushed to the try server.

Comment 71

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

Comment on attachment 409381 [details] [diff] [review]
part 1: syntax (v4)

nsCSSDeclaration.cpp:

>+      if (i == gradient->mStops.Length()-1) {

May as well move the ++i from the for() to here as well, and drop the
-1.  (Otherwise, put spaces around the "-".)

nsCSSParser.cpp:

>+        // if we didn't already get an angle, we might have one now
>+        if (!haveAngle) {
>+          if (ParseVariant(cssGradient->mAngle, VARIANT_ANGLE, nsnull)) {
>+            // now we better have a comma
>+            if (!ExpectSymbol(',', PR_TRUE)) {
>+              SkipUntil(')');
>+              return PR_FALSE;
>+            }
>+          } else {
>+            SkipUntil(')');
>+            return PR_FALSE;
>+          }
>+        } else {
>+          SkipUntil(')');
>+          return PR_FALSE;
>+        }

This can be simplified to:

+        // if we didn't already get an angle, we might have one now,
+        // otherwise it's an error
+        if (haveAngle ||
+            !ParseVariant(cssGradient->mAngle, VARIANT_ANGLE, nsnull) ||
+            // now we better have a comma
+            !ExpectSymbol(',', PR_TRUE)) {
+          SkipUntil(')');
+          return PR_FALSE;
+        }

and perhaps even unified with what's outside it to:

+      if (!ExpectSymbol(',', PR_TRUE) &&
+          // if we didn't already get an angle, we might have one now,
+          // otherwise it's an error
+          (haveAngle ||
+           !ParseVariant(cssGradient->mAngle, VARIANT_ANGLE, nsnull) ||
+           // now we better have a comma
+           !ExpectSymbol(',', PR_TRUE))) {
+        SkipUntil(')');
+        return PR_FALSE;
+      }

>+    if (!GetToken(PR_TRUE)) {
>+      return PR_FALSE;
>+    }
>+
>+    if (mToken.mType == eCSSToken_Ident) {
>+      nsCSSKeyword kw = nsCSSKeywords::LookupKeyword(mToken.mIdent);
>+      if (kw > eCSSKeyword_UNKNOWN) {
>+        PRInt32 val;
>+        if (nsCSSProps::FindKeyword(kw,
>+                nsCSSProps::kRadialGradientShapeKTable, val)) {
>+          cssGradient->mRadialShape.SetIntValue(val, eCSSUnit_Enumerated);
>+        } else if (nsCSSProps::FindKeyword(kw,
>+                nsCSSProps::kRadialGradientSizeKTable, val)) {
>+          cssGradient->mRadialSize.SetIntValue(val, eCSSUnit_Enumerated);
>+        }
>+
>+        if (!GetToken(PR_TRUE)) {
>+          return PR_FALSE;
>+        }
>+        if (mToken.mType == eCSSToken_Ident) {
>+          kw = nsCSSKeywords::LookupKeyword(mToken.mIdent);
>+          if (kw <= eCSSKeyword_UNKNOWN) {
>+            SkipUntil(')');
>+            return PR_FALSE;
>+          }
>+          if (nsCSSProps::FindKeyword(kw,
>+                  nsCSSProps::kRadialGradientShapeKTable, val)) {
>+            if (cssGradient->mRadialShape.GetUnit() == eCSSUnit_None) {
>+              cssGradient->mRadialShape.SetIntValue(val, eCSSUnit_Enumerated);
>+            } else {
>+              SkipUntil(')');
>+              return PR_FALSE;
>+            }
>+          } else if (nsCSSProps::FindKeyword(kw,
>+                         nsCSSProps::kRadialGradientSizeKTable, val)) {
>+            if (cssGradient->mRadialSize.GetUnit() == eCSSUnit_None) {
>+              cssGradient->mRadialSize.SetIntValue(val, eCSSUnit_Enumerated);
>+            } else {
>+              SkipUntil(')');
>+              return PR_FALSE;
>+            }
>+          }
>+        } else {
>+          UngetToken();
>+        }
>+
>+        if (!ExpectSymbol(',', PR_TRUE)) {
>+          SkipUntil(')');
>+          return PR_FALSE;
>+        }
>+      } else {
>+        UngetToken();
>+      }
>+    } else {
>+      UngetToken();
>+    }

Since ParseVariant is known to be safe in terms of putting back what
it doesn't parse correctly, I think this can be simplified to:

+    PRBool haveShape =
+      ParseVariant(cssGradient->mRadialShape, VARIANT_KEYWORD,
+                   nsCSSProps::kRadialGradientShapeKTable);
+    PRBool haveSize =
+      ParseVariant(cssGradient->mRadialSize, VARIANT_KEYWORD,
+                   nsCSSProps::kRadialGradientSizeKTable);
+    // Could be in either order:
+    if (!haveShape) {
+      haveShape =
+        ParseVariant(cssGradient->mRadialShape, VARIANT_KEYWORD,
+                     nsCSSProps::kRadialGradientShapeKTable);
+    }
+    if ((haveShape || haveSize) && !ExpectSymbol(',', PR_TRUE)) {
+      SkipUntil(')');
+      return PR_FALSE;
+    }

nsStyleTransformMatrix.cpp:

>-/* Computes tan(theta).  For values of theta such that
>- * tan(theta) is undefined or arbitrarily large, SafeTangent
>- * returns a managably large or small value of the correct sign.
>+/* SafeTangent computes tan(aTheta) but limits the size of the result,
>+ * and is defined for all values of aTheta.

I think the old comment was clearer.

r=dbaron with those fixed

Comment 72

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

Also, could you add invalid_values tests for various valid gradients with the commas missing in various places and with extra commas (",," instead of ",") and also for some of the invalid cases that I suspect your first patch accepted, like:

-moz-linear-gradient(left left blue, red)

Comment 73

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

Comment on attachment 409382 [details] [diff] [review]
part 2: rendering (v4)

>++  PRBool IsAngleValue(void) const {
>++    return mUnit >= eStyleUnit_Grad && mUnit <= eStyleUnit_Radian;
>++  }

I tend to like writing these with the values from least to greatest, i.e.:

eStyleUnit_Grad <= mUnit && mUnit <= eStyleUnit_Radian


r=dbaron

Comment 74

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I will try landing these tomorrow.

Comment 75

[Zack Weinberg (:zwol)]

Before you land anything, be aware that a bunch of gradient reftests failed on the try server.  I was going to look into that first thing Monday morning (PDT) and didn't say anything because I didn't expect dbaron to review the patches so quickly.

Comment 76

[Zack Weinberg (:zwol)]

unit test logs:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1256928506.1256937810.11863.gz (Linux)
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1256928506.1256941117.17119.gz (OSX)
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1256929896.1256945156.30636.gz (Windows)

There were no failures other than these 13 or 17 reftests.  You can ignore canvas/linear-gradient-1b.html, it's from an extra patch I forgot to take out of my tree before pushing.

Comment 77

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The reftest problem is this:
+  PRBool IsAngleValue(void) const {
+    return mUnit >= eStyleUnit_Grad && mUnit <= eStyleUnit_Radian;
+  }

But the units are
  eStyleUnit_Degree       = 12,     // (float) angle in degrees
  eStyleUnit_Grad         = 13,     // (float) angle in radians
  eStyleUnit_Radian       = 14,     // (float) angle in radians

So IsAngleValue returns false for degrees...

I'll let you fix that and apply dbaron's comments...

Comment 78

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

(In reply to comment #77)
>   eStyleUnit_Grad         = 13,     // (float) angle in radians

Also please fix the comment on this line.

Comment 79

[Zack Weinberg (:zwol)]

I pushed the cairo-quartz patch by itself: http://hg.mozilla.org/mozilla-central/rev/4e88b5896a2c  Remainder to follow in about an hour.

Comment 80

[Zack Weinberg (:zwol)]

Attachment: part 1: syntax (v5) (Checkin: comment #82)

to-be-checked-in, incorporating dbaron's last set of comments

Comment 81

[Zack Weinberg (:zwol)]

Attachment: part 2: rendering (v5) (Checkin: comment #82)

to-be-checked-in, incorporates dbaron's comments and the fix for the IsAngleValue bug

Comment 82

[Zack Weinberg (:zwol)]

http://hg.mozilla.org/mozilla-central/rev/c2261a741da8 (part 1)
http://hg.mozilla.org/mozilla-central/rev/32e1ceb06e44 (part 2)

Comment 83

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Angle-related reftests failed on Mac:

REFTEST TEST-UNEXPECTED-FAIL |
file:///builds/slave/mozilla-central-macosx-unittest-everythingelse/build/reftest/tests/layout/reftests/transform/translatex-1b.html 
REFTEST TEST-UNEXPECTED-FAIL |
file:///builds/slave/mozilla-central-macosx-unittest-everythingelse/build/reftest/tests/layout/reftests/transform/translatex-1b.html 
REFTEST TEST-UNEXPECTED-FAIL |
file:///builds/slave/mozilla-central-macosx-unittest-everythingelse/build/reftest/tests/layout/reftests/transform/translate-1b.html |

Since they're visually OK and this patch is time critical, I disabled the tests:
http://hg.mozilla.org/mozilla-central/rev/905bcb1d08c1
and filed bug 526075.

Comment 84

[Dão Gottwald [:dao]]

Note that gradients are used in different places on 1.9.2 than on trunk. See <http://mxr.mozilla.org/mozilla1.9.2/search?string=-moz-linear-gradient&find=pinstripe&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla1.9.2>. Also note that the syntax fix for shared.inc on trunk was incorrect (bug 526145).

Comment 85

[Zack Weinberg (:zwol)]

Attachment: 1.9.2 backport part 1: Cairo/Quartz changes

Part 1 of the 1.9.2 backport.  It turns out that we need to bring over several Cairo/Quartz patches, not just the one developed for this patch.

This would land as a separate commit from the backport proper, but in the same push.

Comment 86

[Zack Weinberg (:zwol)]

Attachment: 1.9.2 backport part 2: substantive change

This is a rollup of the v5 syntax and rendering changes + the fix in bug 526075 + the equivalent of the fix in bug 526145.  Pushed to try server.

Comment 87

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

In layout/style/nsCSSValue.h:
240     NS_ASSERTION(eCSSUnit_Degree <= mUnit &&
241                  eCSSUnit_Radian >=-mUnit, "not an angle value");

The |-mUnit| should probably be just |mUnit|.

Comment 88

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 409956 [details] [diff] [review]
1.9.2 backport part 2: substantive change

a192=beltzner

Comment 89

[Zack Weinberg (:zwol)]

(In reply to comment #87)
> In layout/style/nsCSSValue.h:
> 240     NS_ASSERTION(eCSSUnit_Degree <= mUnit &&
> 241                  eCSSUnit_Radian >=-mUnit, "not an angle value");
> 
> The |-mUnit| should probably be just |mUnit|.

Nice catch, will commit the obvious fix to m-c shortly & incorporate it into the patch for 1.9.2.

Comment 90

[Zack Weinberg (:zwol)]

http://hg.mozilla.org/mozilla-central/rev/c73de6f65dec -- this change:

--- a/layout/style/nsCSSValue.h
+++ b/layout/style/nsCSSValue.h
@@ -236,6 +236,6 @@ public:
   float GetAngleValue() const
   {
     NS_ASSERTION(eCSSUnit_Degree <= mUnit &&
-                 eCSSUnit_Radian >=-mUnit, "not an angle value");
+                 mUnit <= eCSSUnit_Radian, "not an angle value");
     return mValue.mFloat;
   }

per dbaron's earlier suggestion to put the variable in the middle for this kind of range check. Also incorporated into the patch I'll push for 1.9.2. Local debug-build reftests were green before commit.

Meantime, the 1.9.2 patch scored orange unit tests on the try server on all three platforms - will investigate after lunch.

Comment 91

[Zack Weinberg (:zwol)]

Windows and Linux failures were another manifestation of bug 523864 -- the trunk patch renamed some reftests, and applying it to branch with "patch" didn't pick that up.

Something else is going on on OSX --

25542 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.gradient.radial.cone.behind.html | pixel 1,1 is 255,0,0,255; expected 0,255,0,255 +/- 0

is typical.  I'm going to have to do my own OSX 1.9.2 build, which will take a couple hours.

Comment 92

[Zack Weinberg (:zwol)]

Attachment: 1.9.2 backport: Cairo/Quartz: final

The patches required some small tweaks to pass the try server.  The only change in this patch is to disable three canvas reftests that were disabled on trunk when these cairo patches were applied there.  (Those were the ones that failed on OSX yesterday.)

Comment 93

[Zack Weinberg (:zwol)]

Attachment: 1.9.2 backport: substantive change: final

And the only change here is to pick up some file renames in reftests/css-gradients/ that got lost because I applied a git diff with plain patch(1).  That was the cause of the reftest hang on Linux and Windows (the reftest driver doesn't handle missing test files well).

Comment 94

[Zack Weinberg (:zwol)]

& I don't think we should land this until we resolve bug 526402 on trunk, at least.

Comment 95

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #94)
> & I don't think we should land this until we resolve bug 526402 on trunk, at
> least.

Why? Do you mean that you think bug 562402 blocks this one, and we shouldn't land this on 1.9.2 without also landing that one? Or do you mean that you need a decision in terms of how to parse something there?

We want to get the new syntax in front of our beta audience ASAP, so I'm asking the question in terms of what's delaying that goal.

Comment 96

[Zack Weinberg (:zwol)]

(In reply to comment #95)
> (In reply to comment #94)
> > & I don't think we should land this until we resolve bug 526402 on trunk, at
> > least.
> 
> Why? Do you mean that you think bug 562402 blocks this one, and we shouldn't
> land this on 1.9.2 without also landing that one?

Exactly.  The syntax is not right without that fix.

We have a patch there that's ready to land - just need dbaron to look over the test cases.  So I still think we're on track to put the whole thing to bed by tomorrow.

Comment 97

[Zack Weinberg (:zwol)]

I suppose this should be FIXED, as it is on trunk.

Comment 98

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #96)
> Exactly.  The syntax is not right without that fix.

Actually, this doesn't need to be perfect before we land it on branch. We should land this on branch ASAP. If bug 562402 is ready already, great.

Comment 99

[Zack Weinberg (:zwol)]

Pushed to releases/1.9.2 together with bug 562402:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ebbeedee7a9a
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a1d8d0e6b6c7

Comment 100

[Michael Ventnor]

Is the syntax about to change again, from what I read on www-style?

Comment 101

[Eric Shepherd [:sheppy]]

Does the document at http://www.xanthir.com/:4bhipd accurately describe the current implementation? I need to update the docs on MDC.

Comment 102

[Zack Weinberg (:zwol)]

(In reply to comment #100)
> Is the syntax about to change again, from what I read on www-style?

The specification might change again, but the syntax that Firefox 3.6 supports is not going to change at this point.

(In reply to comment #101)
> Does the document at http://www.xanthir.com/:4bhipd accurately describe the
> current implementation? I need to update the docs on MDC.

Yes, except that we do not support the full <bg-position> syntax from the current Backgrounds and Borders module (see bug 522607) which means some of the examples in that document cannot be replicated.

The MDC docs should point that out, and should also mention that the spec might change again, in which case Firefox 3.7 may have different behavior.

Comment 103

[Zack Weinberg (:zwol)]

For situations like this and -moz-border-image (which was also drastically changed after we froze our implementation) I wonder if it makes sense to start putting revision numbers on the end of our vendor-prefixed implementation names.  So, in this case, -moz-*-gradient() would keep the syntax and semantics that they have now, since we shipped that behavior, but -moz-*-gradient-v2() would adopt whatever changes come out of the current discussion.  We desupport all of the vendor prefixes at the same time (one or two releases after the spec goes to CR).

Comment 104

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I would prefer to avoid multiple versions of vendor-prefix syntax, if we can, and just jump straight to the final syntax when we remove the vendor prefix.

Right now it's not clear to me how much the gradient syntax is going to change. I suspect the "no angle or start position specified" and "only start position specified" cases may end up unchanged.

Comment 105

[philippe (part-time)]

(In reply to comment #103)
> I wonder if it makes sense to start
> putting revision numbers on the end of our vendor-prefixed implementation
> names.

Please don't. That would make it even more confusing for authors. They'll never remember that -moz(v1) applies to Gecko 1.9.something, -moz(v2) is for Gecko1.9.something-else-more-recent. etc.

Comment 106

[Tab Atkins Jr.]

In my estimation (as owner of that section of the spec), linear gradient syntax will change little or none.  Worst case we may drop the angle+point and gain a double-point version.  The no-point, just-angle, and just-point versions have no real chance of being changed.  Color stops won't change at all.

Radial gradient *may* change, but I'm not certain of that.  The syntax for radials didn't get a lot of review the first time around, unfortunately - I think it works well, but my authoring practices may not generalize as well as I would wish.

Comment 107

[Eric Shepherd [:sheppy]]

Documentation is updated:

https://developer.mozilla.org/en/CSS/-moz-radial-gradient
https://developer.mozilla.org/en/CSS/-moz-linear-gradient

Comment 108

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You had the comment "about Gecko honors background-repeat by repeating the gradient in the natural way..." which is no longer true, so I removed it.