Last Comment Bug 479220 - Implement the CSS gradients proposal
: Implement the CSS gradients proposal
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- enhancement with 11 votes (vote)
: mozilla1.9.2a1
Assigned To: Michael Ventnor
:
: Jet Villegas (:jet)
Mentors:
https://developer.mozilla.org/en/CSS/...
Depends on: background-size 508116 509681 511617
Blocks: 113577 506826
  Show dependency treegraph
 
Reported: 2009-02-19 04:27 PST by Michael Ventnor
Modified: 2012-07-24 00:24 PDT (History)
31 users (show)
roc: wanted1.9.2+
dao+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (23.52 KB, patch)
2009-02-19 04:31 PST, Michael Ventnor
no flags Details | Diff | Splinter Review
Working code (35.33 KB, patch)
2009-05-07 06:42 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
Not working code (43.90 KB, patch)
2009-07-07 05:44 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
Now working patch (44.87 KB, patch)
2009-07-08 03:34 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
Parsing patch (58.10 KB, patch)
2009-07-14 21:17 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
The rest (25.89 KB, patch)
2009-07-14 21:17 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
Parsing patch 2 (59.06 KB, patch)
2009-07-15 20:11 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
The rest 2 (24.37 KB, patch)
2009-07-15 20:11 PDT, Michael Ventnor
roc: review+
roc: superreview+
Details | Diff | Splinter Review
The rest 2.1 (24.33 KB, patch)
2009-07-15 20:43 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
Parsing patch 3 (59.66 KB, patch)
2009-07-23 02:30 PDT, Michael Ventnor
dbaron: review-
Details | Diff | Splinter Review
Parsing patch 4 (63.23 KB, patch)
2009-07-30 03:21 PDT, Michael Ventnor
dbaron: review-
Details | Diff | Splinter Review
The rest rebased (24.19 KB, patch)
2009-07-30 04:00 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
Parsing patch 5 (63.56 KB, patch)
2009-07-30 18:32 PDT, Michael Ventnor
dbaron: review+
Details | Diff | Splinter Review
For checkin (91.11 KB, patch)
2009-07-31 02:56 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
For checkin 2 (91.15 KB, patch)
2009-07-31 17:51 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
For checkin 2.1 (91.12 KB, patch)
2009-07-31 18:26 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
For checkin 2.2 (91.13 KB, patch)
2009-07-31 19:18 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
Background Gradients Example (from webkit.org) (1.03 KB, text/html)
2009-07-31 22:25 PDT, mdew
no flags Details
For checkin 2.3 (91.23 KB, patch)
2009-08-01 00:24 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
radius bug? (94.88 KB, image/jpeg)
2009-08-01 16:04 PDT, mdew
no flags Details

Description Michael Ventnor 2009-02-19 04:27:56 PST
roc said it would be nice to have.

http://webkit.org/blog/175/introducing-css-gradients/

This feature allows you to use a gradient rule in lieu of an image URL. You can specify any number of stops and use either linear or radial gradients.
Comment 1 Michael Ventnor 2009-02-19 04:31:29 PST
Created attachment 363091 [details] [diff] [review]
WIP

This is what I have done so far. It can successfully parse any given gradient rule and put it into an nsStyleGradient object that can be read anywhere within layout.

Computed style doesn't work, and may not work for a while considering nsDOMCSSPrimitiveValue is frozen (and has no new NS equivalent).

No rendering is done yet, and no rule can use this. You'll have to add the VARIANT_GRADIENT flag to ParseVariant() in the parser on whatever rule you want. It can then parse the rule along with all the stops in an ordered array.
Comment 2 Michael Ventnor 2009-05-05 04:50:24 PDT
I'm having a slight doubt over a part of the spec that Webkit is proposing, namely the use of pure numbers to specify coordinates. I propose replacing this with the use of standard CSS units because:

a) Raw coordinates, while easy to pass to Cairo or CoreGraphics (which is no doubt why Webkit did this), have no place in CSS because CSS is not designed to scale or transform like SVG. Using units is consistent with every other CSS property, whereas this personally seems like another example of Webkit's "let's see how much SVG we can quickly cram into CSS" proposals.
b) Using units allows authors to adapt their gradients. They can use px, pt, fixed units, ch, rem and every other unit that CSS provides. I think this would be super-useful.
c) It would be easier for me to parse and implement with less code.

The downside is that we'd be incompatible with Webkit, but that's the case already because we use -moz and they use -webkit.

Thoughts? I really want to make this change.
Comment 3 Dão Gottwald [:dao] 2009-05-05 06:24:27 PDT
(In reply to comment #2)
> Using units is consistent with every other CSS property

Except for border-image, right?
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-05-05 08:53:51 PDT
The numbers in border-image are coordinates within the image so it makes no sense to allow units like "mm" there. The coordinates in gradients here are coordinates on the page so it makes sense to require units --- I agree with Michael.

The other thing we might want to change is the <type> parameter. It seems to me -moz-linear-gradient(<point>, <point> [, <stop>]*) and -moz-radial-gradient(<point>, <radius>, <point>, <radius> [, <stop>]*) makes more sense than a single -webkit-gradient function with 'type' as the first parameter, which forces you to make the radii optional, which is just weird.
Comment 5 Vladimir Vukicevic [:vlad] [:vladv] 2009-05-05 12:18:45 PDT
fwiw, I agree with comment #2 and comment #4.
Comment 6 Michael Ventnor 2009-05-05 17:13:11 PDT
(In reply to comment #4)
> The other thing we might want to change is the <type> parameter. It seems to me
> -moz-linear-gradient(<point>, <point> [, <stop>]*) and
> -moz-radial-gradient(<point>, <radius>, <point>, <radius> [, <stop>]*) makes
> more sense than a single -webkit-gradient function with 'type' as the first
> parameter, which forces you to make the radii optional, which is just weird.

I'm less keen about this. The radius is not optional on radial gradients, and causes a parse error when put on linear gradients. I'd rather have a single rule with a lot of functionality in it, just like what we have for inset/outset box-shadow, or the background shorthand.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-05-05 23:30:16 PDT
I think it's crazy for the parsing of a function "-moz-gradient" to depend on the *value* of its first parameter!
Comment 8 Michael Ventnor 2009-05-06 00:19:32 PDT
But I also think it's crazy to have two separate rules that do very similar things and take very similar parameters. CSS already has too many rules; I reckon one rule that changes is easier to work with in this particular case.
Comment 9 Michael Ventnor 2009-05-06 03:47:48 PDT
I've just done integrating this with the background image struct and am going to try the actual rendering code soon.

Should gradients be counted as background images in terms of printing etc? So if printing background images is turned off, gradients should be turned off too?
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-05-06 09:07:31 PDT
(In reply to comment #9)
> Should gradients be counted as background images in terms of printing etc? So
> if printing background images is turned off, gradients should be turned off
> too?

Yes.

(In reply to comment #8)
> But I also think it's crazy to have two separate rules that do very similar
> things and take very similar parameters. CSS already has too many rules; I
> reckon one rule that changes is easier to work with in this particular case.

You mean property values, not rules.

Currently you're adding three tokens: -moz-gradient, linear, and radial. My way you'd only add two tokens: -moz-linear-gradient and -moz-radial-gradient. I really think that pretending we can share syntax between these two functions is false simplicity.

I wonder what dbaron thinks.
Comment 11 David Baron :dbaron: ⌚️UTC-10 2009-05-06 09:19:40 PDT
I think I prefer two separate functions.  Is Ventnor confusing adding new properties (more expensive) with adding new values (much cheaper)?
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-05-06 15:08:51 PDT
I discussed both of the changes in comment #4 with Hyatt on IRC and he was in favour of them.
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-05-06 15:09:11 PDT
Or at least "fine" with them.
Comment 14 Michael Ventnor 2009-05-06 19:05:11 PDT
OK, after trying to make some testcases, and sleeping on this (amazing how that can change your mind on almost everything) I now agree with two separate functions.
Comment 15 Michael Ventnor 2009-05-07 06:42:06 PDT
Created attachment 376237 [details] [diff] [review]
Working code

This will render a -moz-linear-gradient or -moz-radial-gradient in background-image. There are a few points that I need some guidance on:

- For some reason this doesn't respect background-origin or background-clip, even though it seems like it should the way I read the code. I'm really confused about this.
- I'm also confused about radial gradients themselves. They're implemented and passed to Cairo but I don't know how to use them or test them. The documentation is a little hairy on this, or maybe I'm just tired.
- Computed style isn't implemented and since the file that specifies variant types is frozen, and with gradients not in the computed style spec, I don't know how to go about this. I wonder if Webkit implements computed style for gradients properly, I'll have to check sometime.

I won't have much time to work on this for a while. roc (and anyone else; dbaron maybe) if you could test this and maybe give some doorstop review comments that would be great.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-05-07 17:37:30 PDT
     nsCOMPtr<imgIRequest> mRequest;
-    PRBool mSpecified; // if false, mRequest is guaranteed to be null
+    nsRefPtr<nsStyleGradient> mGradient;

We could save memory by merging mRequest and mGradient in the same void* field with some manual refcounting/type-casting. Ugly perhaps, but maybe worth it --- nsStyleBackground is profilic.

+ConvertGradientValueToCoord(nsStyleCoord& aCoord,
+                            const nscoord& aFillLength,
+                            const nscoord& aAppUnitsPerPixel)

aCoord should be a const reference, but the other two parameters should just be 'nscoord' since nscoord is a simple type, just copying it is cheaper than passing a reference.

And the name shouldn't be ToCoord, since you don't return an nscoord but dev pixels.

+  ctx->Rectangle(areaToFill);

This should pass PR_TRUE to snap-to-pixels.

+  if (bottomImage.mType != eBackgroundImage_Null) {
+    if (!drawBackgroundImage) {
+      bottomImage.mRequest = nsnull;
+      bottomImage.mGradient = nsnull;
+      bottomImage.mType = eBackgroundImage_Null;
+      useFallbackColor = PR_TRUE;
+    }
+    if (bottomImage.mType == eBackgroundImage_Image &&
         !UseImageRequestForBackground(bottomImage.mRequest)) {
       bottomImage.mRequest = nsnull;
+      bottomImage.mGradient = nsnull;
+      bottomImage.mType = eBackgroundImage_Null;
+      useFallbackColor = PR_TRUE;

Why not just
    if (!drawBackgroundImage ||
        (bottomImage.mType == eBackgroundImage_Image &&
         !UseImageRequestForBackground(bottomImage.mRequest))
?

+    // Gradients don't have a size, so this will do.
+    // XXX This will need to change when we implement background-size.
+    imageSize = aForFrame->GetSize();

Webkit uses the background-origin rectangle here, so we should use bgOriginRect.Size().

+    nsCSSRendering::PaintGradient(aPresContext, aRenderingContext,
+        aLayer.mImage.mGradient, aDirtyRect, fillArea);

There's definitely a bug here; PaintGradient should take destArea as a parameter. The gradient background can be repeated with background-repeat; in particular if you set background-position to something nonzero and background-repeat:repeat, you should be able to see repeated tiles of the gradient. destArea is the rectangle that a single gradient tile should cover, fillArea is the total area we should draw.

Repeating gradient tiles is actually going to be a bit of a pain since just setting EXTEND_REPEAT won't give the correct results in general, I think --- the gradient will repeat along its radial or linear axis, instead of just repeating rectangular tiles as CSS wants. When more than one tile is visible you'll want to render one tile to a temporary surface and then render that as a repeated image.
Comment 17 Michael Ventnor 2009-05-07 20:38:26 PDT
(In reply to comment #16)
>      nsCOMPtr<imgIRequest> mRequest;
> -    PRBool mSpecified; // if false, mRequest is guaranteed to be null
> +    nsRefPtr<nsStyleGradient> mGradient;
> 
> We could save memory by merging mRequest and mGradient in the same void* field
> with some manual refcounting/type-casting. Ugly perhaps, but maybe worth it ---
> nsStyleBackground is profilic.

Eep, that's going to require many changes littered throughout the codebase. How much does an nsRefPtr take up, is it anymore than a regular pointer?


> +    // Gradients don't have a size, so this will do.
> +    // XXX This will need to change when we implement background-size.
> +    imageSize = aForFrame->GetSize();
> 
> Webkit uses the background-origin rectangle here, so we should use
> bgOriginRect.Size().

But isn't bgOriginRect calculated after, and calculated based on, imageSize?
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-05-07 20:47:57 PDT
(In reply to comment #17)
> (In reply to comment #16)
> >      nsCOMPtr<imgIRequest> mRequest;
> > -    PRBool mSpecified; // if false, mRequest is guaranteed to be null
> > +    nsRefPtr<nsStyleGradient> mGradient;
> > 
> > We could save memory by merging mRequest and mGradient in the same void* field
> > with some manual refcounting/type-casting. Ugly perhaps, but maybe worth it ---
> > nsStyleBackground is profilic.
> 
> Eep, that's going to require many changes littered throughout the codebase. How
> much does an nsRefPtr take up, is it anymore than a regular pointer?

No. With this patch you're just using two pointers where one would do.

You can create inlines member of Image, say nsIRequest* GetRequest() and nsStyleGradient* GetGradient() that handle the casting. The resulting code should be pretty clean.

> > +    // Gradients don't have a size, so this will do.
> > +    // XXX This will need to change when we implement background-size.
> > +    imageSize = aForFrame->GetSize();
> > 
> > Webkit uses the background-origin rectangle here, so we should use
> > bgOriginRect.Size().
> 
> But isn't bgOriginRect calculated after, and calculated based on, imageSize?

It's calculated after imageSize currently, but it doesn't use imageSize. It only looks at the frame.
Comment 19 Michael Ventnor 2009-05-10 00:38:28 PDT
(In reply to comment #18)
> No. With this patch you're just using two pointers where one would do.
> 
> You can create inlines member of Image, say nsIRequest* GetRequest() and
> nsStyleGradient* GetGradient() that handle the casting. The resulting code
> should be pretty clean.

That would work, but how would storage be best dealt with?
nsRefPtr<void*>?
There are so many types in XPCOM that I don't know what's best for what.
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-05-10 01:34:04 PDT
void* and manual casts/refcounting.
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-06-23 18:11:08 PDT
Michael, are you going to be able to work on this anytime soon, or should I look for someone else to pick it up?
Comment 22 Michael Ventnor 2009-06-23 18:16:09 PDT
I'll try and start work in the next few days; I still have exams to finish and I haven't been able to get my contract sorted out.
Comment 23 Michael Ventnor 2009-06-25 22:36:09 PDT
(In reply to comment #16)
> There's definitely a bug here; PaintGradient should take destArea as a
> parameter. The gradient background can be repeated with background-repeat; in
> particular if you set background-position to something nonzero and
> background-repeat:repeat, you should be able to see repeated tiles of the
> gradient. destArea is the rectangle that a single gradient tile should cover,
> fillArea is the total area we should draw.
> 
> Repeating gradient tiles is actually going to be a bit of a pain since just
> setting EXTEND_REPEAT won't give the correct results in general, I think ---
> the gradient will repeat along its radial or linear axis, instead of just
> repeating rectangular tiles as CSS wants. When more than one tile is visible
> you'll want to render one tile to a temporary surface and then render that as a
> repeated image.

This spec is so under-specified... should gradients even repeat at all? I was under the impression that gradients have a sort of "infinite" size but is clipped to whatever needs to be drawn. I'm not sure what Webkit does and can't really find out right now.
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-06-26 02:50:55 PDT
It's definitely underspecified, no-one's even written a draft spec as far as I know.

I think using the background-origin rectangle size as the size of the gradient image is the right thing to do. But when bug 189519 lands, authors will be able to use background-size to explicitly set the size of the image. So you could have

div { width:200px; height:200px; background-size: 100px 100px;
      background-image: linear-gradient(...); }

and you should get the linear gradient repeated as 4 tiles.
Comment 25 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-06-26 02:54:08 PDT
It might be a good idea to base your work on top of the patch in bug 189519. Pretty easy to do with Mercurial queues.
Comment 26 Michael Ventnor 2009-07-07 05:44:04 PDT
Created attachment 387181 [details] [diff] [review]
Not working code

I've made progress on this, but now I'm stuck with a crasher on startup that I can't figure out. It's to do with an imgIRequest in an nsStyleBackground::Image having a refcount of 0 when the images are being compared. But I have no idea how it gets a refcount of 0 in the first place. No passed in image gets a refcount less than 2 before I addref it, AFAICT.

roc, or anyone else, would you have any idea about this?
Comment 27 Michael Ventnor 2009-07-08 03:34:11 PDT
Created attachment 387415 [details] [diff] [review]
Now working patch

I figured out what the problem was, I had to implement a copy constructor. See kids, this is why if you're a C bloke you must never ever move to the high level baby languages that they teach in Uni nowadays, because they dumb you down from this sort of stuff.

So the core work is done. I want a tentative review for what I've done so far, then I'll start going into reftests.

A problem for us is computed style. Doing so requires adding something to a frozen interface, and creating a new file extending this interface is dangerous since this interface is W3C-defined, and any additions they make may clash with future additions the W3c may make. Should I worry about this before this gets checked in or after?
Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-09 22:43:09 PDT
I think it's fine to not have computed style for now.
Comment 29 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-09 22:53:02 PDT
Would you mind adding -p to your patches? I recommend adding to your ~/.hgrc
[defaults]
qdiff=-p -U 8
diff=-p -U 8

Also, can you split the patch into "style system changes" and "the rest"?
Comment 30 Michael Ventnor 2009-07-13 05:18:54 PDT
Are you sure that's right? I tried doing that but there was no change to the patch in .hg/patches after doing hg qrefresh.
Comment 31 David Baron :dbaron: ⌚️UTC-10 2009-07-13 10:40:21 PDT
What you need are the [diff] options in http://pastebin.mozilla.org/658023 .  (I recommend everything there, actually.)
Comment 32 David Baron :dbaron: ⌚️UTC-10 2009-07-13 10:43:27 PDT
(In reply to comment #27)
> A problem for us is computed style. Doing so requires adding something to a
> frozen interface, and creating a new file extending this interface is dangerous
> since this interface is W3C-defined, and any additions they make may clash with
> future additions the W3c may make. Should I worry about this before this gets
> checked in or after?

Why?  What I've done for a lot of the new complex properties is just represent their computed values as strings.  I really don't care much about the CSSValue interfaces to computed style.

It would probably be good if you got to the point of having tests in property_database.js before requesting review; the tests are likely to turn up some bugs.
Comment 33 Michael Ventnor 2009-07-14 03:56:40 PDT
(In reply to comment #32)
> What I've done for a lot of the new complex properties is just represent
> their computed values as strings.

Duhhhh, now why didn't I think of that?

Is there an easy way to split up patches like roc describes in comment 29? The stuff in layout/style separate from everything else.
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-14 12:17:08 PDT
You could just do it by hand with a text editor before you attach the patches.

In the long term it would be worth learning how to use Mercurial Queues to maintain your work as smaller patches locally.
Comment 35 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-14 19:08:53 PDT
I'd really like to get this in for Firefox 3.6, which means we have a week to land it.
Comment 36 Michael Ventnor 2009-07-14 21:17:28 PDT
Created attachment 388627 [details] [diff] [review]
Parsing patch

1 week? For 3.6? You must be joking...
Comment 37 Michael Ventnor 2009-07-14 21:17:56 PDT
Created attachment 388628 [details] [diff] [review]
The rest
Comment 38 Michael Ventnor 2009-07-14 21:21:46 PDT
And I do use queues, but I never though to use separate queues for style code and the rest.
Comment 39 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-14 21:36:10 PDT
Take a look at bug 339548 and bug 352093 :-).
Comment 40 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-14 22:25:34 PDT
+    gfxFloat gradRadius0 = aGradient->mStartRadius / appUnitsPerPixel;
+    gfxFloat gradRadius1 = aGradient->mEndRadius / appUnitsPerPixel;

These should be floating point divisions.

+  areaToFill = areaToFill - fillOrigin;

Instead of doing this, just use areaToFill - fillOrigin lower down. It's better to not overwrite locals if you don't need to.

+    // We're going to tile the gradients to fill up the entire area
+    nsRefPtr<gfxASurface> gradientSurface =
+      gfxPlatform::GetPlatform()->CreateOffscreenSurface(gfxIntSize(oneCellArea.Width(),
+                                                                    oneCellArea.Height()),
+                                                         gfxASurface::ImageFormatARGB32);

This is potentially going to be a very large surface. Imagine for example scrolling to the bottom of
<div style="height:10000px; width:10000px;
            background-image: -moz-linear-gradient(...);
            background-size:9999px 9999px;">

There is also the issue that oneCellArea.Width/Height might not be integers.

I suggest you call nsSVGUtils::ConvertToSurfaceSize to give yourself a usable surface size. Then, since the size of your surface might not match the oneCellArea size, you'll need to rescale your drawing into that surface. You'll also need to scale back out by setting a matrix in the pattern that you get from the surface. You probably should write some tests for this.

+    if (aStyleBackground->mLayers[i].mImage.GetType() == eBackgroundImage_Image)
+      image = aStyleBackground->mLayers[i].mImage.GetImageData();
     loaders = nsImageLoader::Create(aFrame, image, PR_FALSE, loaders);

I don't think you want to call nsImageLoader::Create for gradient images.

diff --git a/layout/base/nsStyleConsts.h b/layout/base/nsStyleConsts.h
--- a/layout/base/nsStyleConsts.h
+++ b/layout/base/nsStyleConsts.h

This belongs in the other patch.

Wouldn't it be easier to make your references SVG files instead of scripted? Maybe not, never mind.

You can test your tiling code before we land background-size using background-origin:padding and background-clip:border, with a thick dashed border.

Looks great, I really want this!
Comment 41 Michael Ventnor 2009-07-14 23:42:10 PDT
(In reply to comment #40)
> I suggest you call nsSVGUtils::ConvertToSurfaceSize to give yourself a usable
> surface size. Then, since the size of your surface might not match the
> oneCellArea size, you'll need to rescale your drawing into that surface. You'll
> also need to scale back out by setting a matrix in the pattern that you get
> from the surface. You probably should write some tests for this.

I can make this only run if needed by checking the resultOverflows out parameter, but the problem is when I try to make extremely large divs with a gradient background it sometimes causes crashes in pixman when I try to paint the tiles onto the actual page surface (@ fbCompositeSrc_8888x8888mmx) so I don't know about tests.

If I make this unconditional, then it all looks fine with a different background-origin and some tiling. But that would result in unnecessary work manipulating the matrices.
Comment 42 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-15 00:38:32 PDT
I think you could make it unconditional. The matrix work is minimal and it would simplify the code if it was unconditional.

There are some complex issues to do with the snapping we're doing and possibly sampling the beginning of the next tile (which shouldn't be seen) at the edge of a tile when there is subpixel positioning going on. But I'm wondering whether we should just redefine CSS image repetition for gradients so that the gradient repeats naturally along its axis instead of just horizontally/vertically. (The area to be filled would, of course, repeat as normal.) That would be much easier to implement --- this temporary surface stuff would go away --- and probably more what authors expect.

Maybe you could test how Webkit handles that. I'll try to talk to Hyatt tomorrow.
Comment 43 Michael Ventnor 2009-07-15 01:23:14 PDT
(In reply to comment #42)
> There are some complex issues to do with the snapping we're doing and possibly
> sampling the beginning of the next tile (which shouldn't be seen) at the edge
> of a tile when there is subpixel positioning going on. But I'm wondering
> whether we should just redefine CSS image repetition for gradients so that the
> gradient repeats naturally along its axis instead of just
> horizontally/vertically. (The area to be filled would, of course, repeat as
> normal.) That would be much easier to implement --- this temporary surface
> stuff would go away --- and probably more what authors expect.

That was what I originally proposed, wasn't it? Or do you mean basing the gradient coordinates on the oneCellArea but filling the entire fill area with the one gradient?
Comment 44 Michael Ventnor 2009-07-15 01:28:19 PDT
Actually, that sounds like a good idea since you could specify gradients that are larger than the cell size and they'd be rendered properly.
Comment 45 Michael Ventnor 2009-07-15 02:04:06 PDT
And as soon as I thought that was a good idea, I now think it might not be so... doing that would be completely inconsistent with background-size for images, and I actually doubt that your proposal would be what authors expect.

Webkit does the tiling like what were doing now.
Comment 46 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-15 16:58:56 PDT
 (In reply to comment #43)
> That was what I originally proposed, wasn't it?

Yes.

Right now there is no way to get a repeating diagonal or radial gradient. I think letting background-repeat:repeat/repeat-x/repeat-y enable natural repeating of the gradient would be pretty useful. Repeating the gradient as a tiled image is hardly ever going to be what authors actually want.
Comment 47 Michael Ventnor 2009-07-15 17:15:16 PDT
(In reply to comment #46)
>  (In reply to comment #43)
> > That was what I originally proposed, wasn't it?
> 
> Yes.
> 
> Right now there is no way to get a repeating diagonal or radial gradient. I
> think letting background-repeat:repeat/repeat-x/repeat-y enable natural
> repeating of the gradient would be pretty useful. Repeating the gradient as a
> tiled image is hardly ever going to be what authors actually want.

I still don't think I understand here. Do Cairo gradients naturally repeat like that? Do I set EXTEND_REPEAT on the gradient pattern?
Comment 48 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-15 18:25:26 PDT
Yes.
Comment 49 Michael Ventnor 2009-07-15 18:50:25 PDT
So I did that and tried this out:

background-image: -moz-radial-gradient(center center, 10px, center center, 80px, from(orange), to(red));
That's all folks!

Are you really sure this is the kind of effect authors expect? If they want a single radial gradient they'd have to set a transparent colour stop and the outer radius to no-man's land.
Comment 50 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-15 19:30:16 PDT
We discussed this on IRC. What I meant in comment #46 is that we should enable EXTEND_REPEAT on the gradient if background-repeat is repeat/repeat-x/repeat-y, and not enable EXTEND_REPEAT if background-repeat is no-repeat.
Comment 51 Michael Ventnor 2009-07-15 20:11:00 PDT
Created attachment 388859 [details] [diff] [review]
Parsing patch 2
Comment 52 Michael Ventnor 2009-07-15 20:11:34 PDT
Created attachment 388860 [details] [diff] [review]
The rest 2
Comment 53 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-15 20:29:39 PDT
Comment on attachment 388860 [details] [diff] [review]
The rest 2

+    gfxFloat gradRadius0 = aGradient->mStartRadius * 1.0 / appUnitsPerPixel;
+    gfxFloat gradRadius1 = aGradient->mEndRadius * 1.0 / appUnitsPerPixel;

just use double(aGradient->mStartRadius)/appUnitsPerDevPixel etc

aShouldGradientRepeat could be just 'aRepeat', there's no ambiguity about what's being repeated.
Comment 54 Michael Ventnor 2009-07-15 20:43:29 PDT
Created attachment 388865 [details] [diff] [review]
The rest 2.1

Thanks.

Would you know if dbaron would have time to do this before the freeze? (or would bz be better?)
Comment 55 Michael Ventnor 2009-07-15 21:05:59 PDT
Comment on attachment 388859 [details] [diff] [review]
Parsing patch 2

Whoever gets to it first, can (if its needed) you also set the sr flag?
Comment 56 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-16 16:46:47 PDT
I talked to Hyatt about our approach of using background-repeat to control whether the gradient repeats (and not tiling), and he thought it was good.
Comment 57 David Baron :dbaron: ⌚️UTC-10 2009-07-21 14:53:01 PDT
Why should the syntax for gradient points be stricter than the standard syntax for background-position and -moz-transform-origin?  (The only difference I see is that your gradient point parsing code doesn't accept left/right/top/bottom in non-X-Y order.)  It seems like it would be simple enough to accept the same syntax since we already have the code (factored already so it can be shared, in CSSParserImpl::ParseBoxPositionValues).  Does WebKit's gradients implementation support only "left top" or does it also support "top left"?
Comment 58 David Baron :dbaron: ⌚️UTC-10 2009-07-21 15:10:53 PDT
A few more review comments from jumping around the parts of the patch that might require some broader changes (i.e., header files defining data structures)... though I didn't find any big things other than the previous comment:


+struct nsCSSValueGradient {
...
+  PRPackedBool mIsRadial;
+  nsCSSValue mStartX;
+  nsCSSValue mStartY;
+  nsCSSValue mStartRadius;
+
+  nsCSSValue mEndX;
+  nsCSSValue mEndY;
+  nsCSSValue mEndRadius;

If some of these values are only valid when mIsRadial has a particular value, please say so in comments?  (I'm guessing that mStartRadius and mEndRadius are only meaningful when mIsRadial is true.)  Also comment that when mIsRadial is true the gradient is radial, and when it's false it's a linear gradient.

Furthermore, mStartX and mStartY should probably be condensed into nsCSSValuePair mStartPoint (and same for mEnd*), especially if you want to share the code I mentioned in the previous comment.

+protected:
+  nsrefcnt mRefCnt;

should be private.

nsCSSValueGradient should also probably have the obvious operator!= (i.e., return !(*this == aOther)).




+nsStyleBackground::Image::Image(const nsStyleBackground::Image& aOther)
+{
+  // We must overload this function so that copy-assignments will keep
+  // the refcount of the data in sync (otherwise we could
+  // end up releasing more than we addref when each Image is destroyed)

Is what this comment is trying to say just "We need our own copy constructor because we don't want to copy the reference count"?  If so, then that would be clearer and simpler.



nsStyleStruct.h:

nsStyleGradient:
+  nsStyleCoord mStartX; // percent or coord (in app units)

no need to say " (in app units)"; that's what coord values in nsStyleCoord mean.

+  nsStyleCoord mStartX; // percent or coord (in app units)
+  nsStyleCoord mStartY; // percent or coord
+  nscoord mStartRadius;
+
+  nsStyleCoord mEndX; // percent or coord
+  nsStyleCoord mEndY; // percent or coord
+  nscoord mEndRadius;

this may fit in less space or be aligned better if you put mStartRadius and mEndRadius next to each other.

+  nsStyleGradient();

declare the constructor first

+  struct Image {
+    public:
+      Image();

local style is to indent like:
   struct Image {
   public:
     Image();
(though I didn't catch you on it for nsCSSShadowArray).
Comment 59 David Baron :dbaron: ⌚️UTC-10 2009-07-21 15:12:20 PDT
And, to be clear, I'd like to see a new patch addressing comment 57 before I finish reviewing, unless there's a good reason that we we want to leave it as-is (and different from the other properties).
Comment 60 Jeff Walden [:Waldo] (remove +bmo to email) 2009-07-21 16:01:56 PDT
It would be good if the patch here could build on bug 189519's patch, since I made a number of changes to how the size, origin, and painting area of backgrounds are calculated so I could be sure the patch was doing the right thing, and I suspect merging my changes atop these is more difficult than doing it the other way around (not to mention more likely to inadvertently discover unintended interactions of gradients with the CSS3 backgrounds draft.
Comment 61 Michael Ventnor 2009-07-21 16:37:38 PDT
(In reply to comment #57)
> Does WebKit's gradients implementation
> support only "left top" or does it also support "top left"?

It supports only "left top" but if there is existing code to support both then I'll do it.
Comment 62 Michael Ventnor 2009-07-21 16:53:22 PDT
And problems with using ParseBoxPositionValues include accepting "inherit" and "initial" which I wouldn't know the best way to handle in a gradient rule, and setting the vertical position to 50% when only one value is specified, which I don't think makes sense for gradients.
Comment 63 David Baron :dbaron: ⌚️UTC-10 2009-07-21 17:40:03 PDT
(In reply to comment #62)
> And problems with using ParseBoxPositionValues include accepting "inherit" and
> "initial" which I wouldn't know the best way to handle in a gradient rule

Check for that case in the calling code and return a parse error.

> and
> setting the vertical position to 50% when only one value is specified, which I
> don't think makes sense for gradients.

The important use case there is that "left" means the middle of the left edge, "top" means the middle of the top edge, etc.
Comment 64 David Baron :dbaron: ⌚️UTC-10 2009-07-21 17:42:20 PDT
(In reply to comment #63)
> (In reply to comment #62)
> > And problems with using ParseBoxPositionValues include accepting "inherit" and
> > "initial" which I wouldn't know the best way to handle in a gradient rule
> 
> Check for that case in the calling code and return a parse error.

Actually, probably better to just make ParseBoxPositionValues take a boolean argument.


I think I do prefer that CSS doesn't grow multiple length-or-percentage point syntaxes, all slightly different, so I'd prefer to use the same rules (and code).
Comment 65 Michael Ventnor 2009-07-23 01:33:53 PDT
So, I don't think I can use nsCSSValuePair within nsCSSValue.h, unless I'm missing something:

../../layout/style/nsCSSValue.h:458: error: ‘nsCSSValuePair’ does not name a type

I could make a temporary nsCSSValuePair in the parsing function and just set the mXValue and mYValue there.
Comment 66 Michael Ventnor 2009-07-23 02:30:16 PDT
Created attachment 390186 [details] [diff] [review]
Parsing patch 3

Just for the record, there was no real reason why that function wasn't used, I just wasn't aware it existed. It's amazing how writing less code actually gives us _more_ freedom and flexibility in this case.

This is built on top of background-size. I'll upload a new "the rest" patch once this gets reviewed, since the reftests need updating and it also needs to be built off background-size.
Comment 67 Michael Ventnor 2009-07-29 05:42:56 PDT
Bug 189519 got nominated for blocking, and this feature is just as prominently mentioned for the next release so I'll nominate this.

dbaron, since the freeze is coming up very shortly would you be able to get the review done soon?
Comment 68 David Baron :dbaron: ⌚️UTC-10 2009-07-29 11:39:38 PDT
Comment on attachment 390186 [details] [diff] [review]
Parsing patch 3

+    // We have to handle enumerated values ourself here, since a recursive call
+    // will look at the CSS property, not the keyword table

That's not quite true, since you could make a recursive call and pass a
different property.  I think it would probably work if you did (without
the if/else pair):
  AppendCSSValueToString(eCSSProperty_background_position,
                         gradient->mStartX, aResult);
  aResult.AppendLiteral(" ");
  AppendCSSValueToString(eCSSProperty_background_position,
                         gradient->mStartY, aResult);
  aResult.AppendLiteral(" ");
but if that doesn't work for some reason I'm missing the current code is
ok if you fix the comment to explain the real reason it doesn't work.

Same for mEndX / mEndY.

In ParseGradientStop, every "return PR_FALSE" that comes after you've
parsed a "(" should be preceded by a "SkipUntil(')')".

In ParseGradientStop, the three:
+    if (!ExpectSymbol('(', PR_FALSE))
+      return PR_FALSE;
should really have an:
  NS_ABORT_IF_FALSE(PR_FALSE, "function token without (");
instead of the return PR_FALSE.  But make sure to use braces around the
macro.

In ParseGradientStop *and* ParseGradient, every call to ExpectSymbol
except for those expecting '(' should pass PR_TRUE instead of PR_FALSE,
and you should add some tests in property_database.js that have
whitespace in various places.  (In particular, this disallows whitespace
before the closing parenthesis, which is both very odd for CSS, and
asymmetric with allowing whitespace inside the opening paren, and
likewise with disallowing whitespace before a comma but allowing it
after.)

In ParseGradient, you likewise need appropriate SkipUntil(')') calls
(every false return except the first... which I'm sort of telling you to
eliminate in the next comment).

In ParseGradient, you also should NS_ABORT_IF_FALSE that the initial
ExpectSymbol never fails.

In ParseGradient, |cssGradient| should be an
nsRefPtr<nsCSSValueGradient>.

You accept a gradient that has no color stops.  What does that mean?
Should you?  I tend to think you shouldn't.  Either way, you should have
a test in property_database with an otherwise valid gradient containing
no stops.

Both ParseBoxPositionValues calls in ParseBackgroundItem should probably
pass PR_FALSE (it doesn't really matter since they'll never get inherit
or -moz-initial, but I think that's clearer).

The ParseBoxPositionValues call in ParseBackgroundPosition should pass
|!head| instead of |PR_TRUE| (the ! is important), and then you can
remove the code:
     if (inheritOrInitial && head) {
       // inherit and initial are only allowed on their own
       break;
     }

nsCSSValueGradient's constructor and destructor should NOT use the
MOZ_COUNT_CTOR and MOZ_COUNT_DTOR macros.  Instead, its AddRef and
Release methods should use the NS_LOG_ADDREF and NS_LOG_RELEASE macros
(like nsCSSValue::Array).  (And I think we should make the same change
to URL and Image.  Could you?)

nsCSSValueGradient should have a private-and-not-implemented
copy-constructor and operator=.  (See Array's copy constructor for an
example.)  (The auto-generated ones are not ok because of the reference
count, and given that we don't need them, we should just prevent them
from being used accidentally.)  And Image and URL should have the same,
really... and array should have the operator= too.  Could you fix those
while you're there?

In nsComputedDOMStyle::GetCSSGradientString, if you're going to use
temporary nsROCSSPrimitiveValue objects, at least just use one object
for the whole function and call it |tmp| or |tmpVal|.  And please have a
static function to do the work for appending a length, and another one
for radius, rather than copy-and-pasting code.

In nsComputedDOMStyle::GetBackgroundImage, please add a temporary variable:
  nsStyleBackground::Image &image = bg->mLayers[i].mImage;
so you don't have to keep writing "bg->mLayers[i].mImage", and rename
|image| to |req|.

+      NS_ENSURE_SUCCESS(rv, rv);
This leaks |valueList| in case of failure; you need to delete it.

nsRuleNode.cpp:

SetGradientCoord and SetGradient should return void rather than PRBool.
Every place inside them where you have something that could return false
(including literal PR_FALSE or where it returns the value of some other
function) should instead use NS_ABORT_IF_FALSE(result, "incorrect data
structure created by parsing code") or something similar.

+  // We don't use inheritance, so aParentCoord is meaningless
+  return SetCoord(aValue, aResult, aResult, SETCOORD_LP,
+                  aContext, aPresContext, aCanStoreInRuleTree);

Pass nsStyleCoord() for aParentCoord instead, and make the comment more
like the other places that do this, e.g.:
  // OK to pass bad aParentCoord since we're not passing SETCOORD_INHERIT

+    nsCSSValueGradientStop valueStop = gradient->mStops[i];

Probably better as a reference rather than a copy.

+    // inherit is not a valid color for stops, so we pass in a dummy
+    // parent color
You should assert that the color isn't inherit.

nsStyleGradient::operator!= should be inline.

In nsStyleGradient::operator==:
+    if (mStops[i].mPosition != aOther.mStops[i].mPosition ||
+        mStops[i].mColor != aOther.mStops[i].mColor)
This can just be
     if (mStops[i] != aOther.mStops[i])
since the autogenerated operator== and operator!= should be fine.

nsStyleGradient::operator== should start with:
  NS_ABORT_IF_FALSE(mIsRadial || (mStartRadius == 0 && mEndRadius == 0),
                    "incorrect unused values");
  NS_ABORT_IF_FALSE(aOther.mIsRadial ||
                    (aOther.mStartRadius == 0 && aOther.mEndRadius == 0),
                    "incorrect unused values");
since it depends on that.

nsStyleBackground::Image::GetType(), GetImageData(), and
GetGradientData() should be inline.

nsStyleGradient's AddRef and Release methods should use NS_LOG_ADDREF
and NS_LOG_RELEASE.

nsStyleGradient should have an inline private destructor to prevent it
from being deleted any way other than through Release.

nsStyleGradient should have a private-and-not-implemented
copy-constructor and operator=.

nsStyleBackground::Image's two constructors should use MOZ_COUNT_CTOR
and its destructor should use MOZ_COUNT_DTOR.

nsStyleBackground::Image's protected members should be private.



Now, a few comments on the non-spec that you're implementing (or is
there actually something written somewhere that I can read?):

I don't think gradient stop locations should accept float values -- it
should just be percentages.  Or does WebKit do otherwise?  (Fixing this
requires changing the parsing code and simplifying SetGradient in
nsRuleNode.cpp.)

I'm assuming you've tested the interactions with fixed backgrounds in
the other patch that I'm not reviewing.  (You should have reftests!)

My guess at what you're doing with the gradient stops is that you're
doing a stable sort of the gradient stop positions in order to figure
out the gradient.  You should probably document that the stops are
unsorted, but expected to be stably sorted later in nsStyleGradient
(where mStops is defined).  Is that what gfxPatterns do?  There should
be tests for this (out-of-order stops; multiple stops at the same
position in different orders causing different behavior).

You should also document in nsStyleGradient.h what happens outside the
bounds of the stops.  I really hope it does something sensible with
gradients that have only one stop.  I wouldn't want to have to reject
those, since then we'd likely also have to reject gradients with
multiple stops at the same position, which is impossible to check at
parse time given length computation.


review- because I'd like to look at the revised versions of
nsComputedDOMStyle::GetCSSGradientString, SetGradientCoord, and
SetGradient, plus maybe the changes if we disallow floats in
gradient stops.  But otherwise it looks good.

I'd also like to see a decent description of what you guys have decided
on for how this stuff should actually render.
Comment 69 Michael Ventnor 2009-07-30 03:21:50 PDT
Created attachment 391558 [details] [diff] [review]
Parsing patch 4

(In reply to comment #68)
> You accept a gradient that has no color stops.  What does that mean?
> Should you?  I tend to think you shouldn't.  Either way, you should have
> a test in property_database with an otherwise valid gradient containing
> no stops.

I think it should be valid, just do whatever Cairo thinks is best for a gradient pattern with no stops.

> In nsStyleGradient::operator==:
> +    if (mStops[i].mPosition != aOther.mStops[i].mPosition ||
> +        mStops[i].mColor != aOther.mStops[i].mColor)
> This can just be
>      if (mStops[i] != aOther.mStops[i])
> since the autogenerated operator== and operator!= should be fine.

It doesn't seem to be. Doing that causes a compile error where operator!= somehow expects two string arguments.

> Now, a few comments on the non-spec that you're implementing (or is
> there actually something written somewhere that I can read?):

Webkit's blog post in comment 0 is a good start, any changes we've made along with the reasoning behind it is mentioned in the first few comments of this bug.

> I don't think gradient stop locations should accept float values -- it
> should just be percentages.  Or does WebKit do otherwise?  (Fixing this
> requires changing the parsing code and simplifying SetGradient in
> nsRuleNode.cpp.)

Webkit does so. I'd prefer our deviations from Webkit are done to make our parsing more liberal and powerful rather than less.

Reftests are in the other patch.

> My guess at what you're doing with the gradient stops is that you're
> doing a stable sort of the gradient stop positions in order to figure
> out the gradient.  You should probably document that the stops are
> unsorted, but expected to be stably sorted later in nsStyleGradient
> (where mStops is defined).  Is that what gfxPatterns do?  There should
> be tests for this (out-of-order stops; multiple stops at the same
> position in different orders causing different behavior).

We don't do any sorting whatsoever ourselves, Cairo's gradient patterns have an API that adds a stop to a pattern so we just pass it there in the same order as the CSS rule is parsed. If two or more stops are specified at the same point then the gradient transitions to the first stop at that point, then immediately starts from the last stop at that point; that's Cairo's behaviour, not ours.

> You should also document in nsStyleGradient.h what happens outside the
> bounds of the stops.  I really hope it does something sensible with
> gradients that have only one stop.  I wouldn't want to have to reject
> those, since then we'd likely also have to reject gradients with
> multiple stops at the same position, which is impossible to check at
> parse time given length computation.

It seems to just do a solid fill of that colour. While pointless, I don't think it should be illegal.

> I'd also like to see a decent description of what you guys have decided
> on for how this stuff should actually render.

That's basically up to Cairo as all this bug does is expose Cairo's gradient pattern API into CSS. I plan to do a Web Tech blog post after checkin to show off some examples, though :-)
Comment 70 Michael Ventnor 2009-07-30 04:00:11 PDT
Created attachment 391563 [details] [diff] [review]
The rest rebased

Built on background-size.
Comment 71 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-30 04:37:44 PDT
It's not enough to just say "we do what cairo does" in the various edge cases. We at least need to document it and make sure that behaviour is reasonable, for the sake of authors as well as other implementors when we propose this as a spec.

I expect the cairo behaviour to be reasonable, though, so let's just document it. Maybe there already is some documentation we can lift...
Comment 72 Michael Ventnor 2009-07-30 04:47:03 PDT
I don't think putting that kind of documentation in the code will accomplish anything though; it's better off in a document somewhere where non-Mozilla-coders will read it.

As I said, I did plan on getting around to uploading examples after this is checked in, and I'll make sure to cover the edge cases in any description of this feature.
Comment 73 David Baron :dbaron: ⌚️UTC-10 2009-07-30 07:28:14 PDT
(In reply to comment #69)
> > I don't think gradient stop locations should accept float values -- it
> > should just be percentages.  Or does WebKit do otherwise?  (Fixing this
> > requires changing the parsing code and simplifying SetGradient in
> > nsRuleNode.cpp.)
> 
> Webkit does so. I'd prefer our deviations from Webkit are done to make our
> parsing more liberal and powerful rather than less.

More liberal isn't always better; CSS has to be readable in addition to being writable, and I think something that's a percent-of-the-way from one place to another is generally expressed as a percentage in CSS.  That said, in the world of math they're the same, so I suppose it's ok.

> > You should also document in nsStyleGradient.h what happens outside the
> > bounds of the stops.  I really hope it does something sensible with
> > gradients that have only one stop.  I wouldn't want to have to reject
> > those, since then we'd likely also have to reject gradients with
> > multiple stops at the same position, which is impossible to check at
> > parse time given length computation.
> 
> It seems to just do a solid fill of that colour. While pointless, I don't think
> it should be illegal.

That's actually exactly what I expected.

So if you have a gradient from red at 30% to green at 40%, what you get is red from 0% to 30%, the gradient from 30% to 40%, and green past 40%?

> That's basically up to Cairo as all this bug does is expose Cairo's gradient
> pattern API into CSS. I plan to do a Web Tech blog post after checkin to show
> off some examples, though :-)

We can't just standardize "whatever cairo does".  And there are some pretty easy things we can do to change that... for example, if what cairo does with stops at the same position depends on the order they're added, we may decide we want to add the stops in forwards or reverse order.

I would tend to say for things like that we ought to be consistent with SVG if possible.  In particular, the "some notes on gradients" section at the end of section 13.2.4 Gradient Stops:
http://www.w3.org/TR/SVG11/pservers.html#GradientStops
or its equivalent in http://www.w3.org/TR/SVGMobile12/painting.html#StopElement

It sounds like that's probably the case, but we ought to have reftests testing that we match that for:
 * no stops
 * one stop
 * multiple stops at the same position
Comment 74 David Baron :dbaron: ⌚️UTC-10 2009-07-30 11:22:27 PDT
Comment on attachment 391558 [details] [diff] [review]
Parsing patch 4

+    if (!ExpectSymbol('(', PR_FALSE)) {
+      SkipUntil(')');

You don't want a SkipUntil if you haven't hit a (, and it's not relevant
anyway since it's followed by NS_ABORT_IF_FALSE.

But you do need SkipUntil on the following two early returns.

Plus the equivalent changes to the other two cases.

(The SkipUntil calls in ParseGradient look right, though.)

> nsCSSValue::Image::~Image()
>+{ }

Put the braces each on a line.

+  // TRUE if gradient is radial, FALSE if it is linear
...
+  // Only meaningful if mIsRadial is TRUE

If you're going to use all-caps, call them PR_TRUE and PR_FALSE.
Otherwise use lowercase English true and false.

+  nsStyleGradient(const nsStyleGradient& aOther);
+  nsStyleGradient& operator=(const nsStyleGradient& aOther);

Need a comment that these are not implemented and not supported.

+  ~nsStyleGradient() {};

no trailing ;


review- so I can look at ParseGradientStop again.

Other than that this patch looks fine, although I think you should at 
least add a comment explaining that the stops in nsStyleGradient::mStops
are in the order specified in the style sheet.
Comment 75 Michael Ventnor 2009-07-30 18:32:54 PDT
Created attachment 391769 [details] [diff] [review]
Parsing patch 5

Fixes the comments.
Comment 76 David Baron :dbaron: ⌚️UTC-10 2009-07-30 18:52:15 PDT
Comment on attachment 391769 [details] [diff] [review]
Parsing patch 5

>+    nsCSSValue stopFloat;
>+    if (!ParseVariant(stopFloat, VARIANT_PERCENT | VARIANT_NUMBER, nsnull)) {
>+      SkipUntil(')');
>+      return PR_FALSE;
>+    }
>+
>+    // Check for a sane position value, and round it if it isn't

The word you want is "clamp" rather than "round".

>+    float stopFloatValue;
>+    if (stopFloat.GetUnit() == eCSSUnit_Percent)
>+      stopFloatValue = stopFloat.GetPercentValue();
>+    else
>+      stopFloatValue = stopFloat.GetFloatValue();
>+
>+    if (stopFloatValue > 1.0)
>+      stopFloat.SetPercentValue(1.0);
>+    else if (stopFloatValue < 0.0)
>+      stopFloat.SetPercentValue(0.0);

You might want to keep the current unit here; it seems inconsistent to change the unit when you're clamping but keep it otherwise.

r=dbaron
Comment 77 Michael Ventnor 2009-07-31 02:56:42 PDT
Created attachment 391839 [details] [diff] [review]
For checkin
Comment 78 Dão Gottwald [:dao] 2009-07-31 04:35:28 PDT
http://hg.mozilla.org/mozilla-central/rev/c086fca6fc55
Comment 79 Dão Gottwald [:dao] 2009-07-31 05:12:52 PDT
OS X 10.5.2 leak test just crashed:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1249040192.1249041497.16783.gz

Anyone care to quickly provide a minimal fix? Otherwise I'll have to back this out.
Comment 81 Michael Ventnor 2009-07-31 05:46:28 PDT
I doubt this is the fault of the gradients patch. From the log:

JavaScript error: chrome://browser/content/browser.js, line 1476: gPrefService.getBranch is not a function
Comment 82 Dão Gottwald [:dao] 2009-07-31 06:01:37 PDT
(In reply to comment #81)
> I doubt this is the fault of the gradients patch. From the log:
> 
> JavaScript error: chrome://browser/content/browser.js, line 1476:
> gPrefService.getBranch is not a function

That wouldn't cause a crash though, and it doesn't seem to appear in the other logs. Also, there were no other checkins when this started.

I've filed bug 507623 on that error, btw.
Comment 83 Michael Ventnor 2009-07-31 06:07:20 PDT
Could you paste in the relevant parts of the log? We don't have real internet in this country and it's taking too long to load the other log.
Comment 84 Michael Ventnor 2009-07-31 06:12:50 PDT
The first log of comment 80, I mean.
Comment 85 Dão Gottwald [:dao] 2009-07-31 06:16:30 PDT
From <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1249040560.1249045782.31741.gz>:

> NOISE: __FAILbrowser crash (code 256)__FAIL
> NOISE: Found crashdump: /var/folders/Xr/Xr--yJnSEY0U11ET5NZuMU+++TM/-Tmp-/tmp2FiW7t/profile/minidumps/2A39B39E-D9AA-4056-A2CE-5C2C5BF3D6D3.dmp
> Operating system: Mac OS X
>                   10.5.2 9C7010
> CPU: x86
>      GenuineIntel family 6 model 15 stepping 2
>      2 CPUs
> 
> Crash reason:  EXC_BAD_ACCESS / KERN_PROTECTION_FAILURE
> Crash address: 0x12ca413
> 
> Thread 0 (crashed)
>  0  XUL!nsStyleBackground::Image::SetNull() [nsStyleStruct.h:c086fca6fc55 : 161 + 0x0]
>     eip = 0x012ca413   esp = 0xbfffc3c0   ebp = 0xbfffc3f8   ebx = 0x012bbc5f
>     esi = 0x00000000   edi = 0x0a3df228   eax = 0x00000002   ecx = 0x00000001
>     edx = 0x00000000   efl = 0x00010246
>  1  XUL!nsStyleBackground::Image::DoCopy(nsStyleBackground::Image const&) [nsStyleStruct.cpp:c086fca6fc55 : 1460 + 0x7]
>     eip = 0x012cd0ee   esp = 0xbfffc400   ebp = 0xbfffc418
>  2  XUL + 0x3582b3
>     eip = 0x012cd2b4   esp = 0xbfffc420   ebp = 0xbfffc458
>  3  XUL!nsRuleNode::ComputeBackgroundData(void*, nsCSSStruct const&, nsStyleContext*, nsRuleNode*, nsRuleNode::RuleDetail, int) [nsRuleNode.cpp:c086fca6fc55 : 4128 + 0xd]
>     eip = 0x012bbca7   esp = 0xbfffc460   ebp = 0xbfffc5d8
>  4  XUL!nsRuleNode::WalkRuleTree(nsStyleStructID, nsStyleContext*, nsRuleData*, nsCSSStruct*) [nsStyleStructList.h:c086fca6fc55 : 79 + 0x33]
>     eip = 0x012c1800   esp = 0xbfffc5e0   ebp = 0xbfffc688
>  5  XUL!nsRuleNode::GetBackgroundData(nsStyleContext*) [nsRuleNode.cpp:c086fca6fc55 : 1517 + 0x10]
>     eip = 0x012c31f9   esp = 0xbfffc690   ebp = 0xbfffc748
>  6  XUL!nsRuleNode::GetStyleBackground(nsStyleContext*, int) [nsStyleStructList.h:c086fca6fc55 : 79 + 0x7e]
>     eip = 0x012c32e7   esp = 0xbfffc750   ebp = 0xbfffc778
>  7  XUL!nsCachedStyleData::Destroy(unsigned int, nsPresContext*) [nsStyleStructList.h:c086fca6fc55 : 79 + 0x16]
>     eip = 0x012c4b67   esp = 0xbfffc780   ebp = 0xbfffc798
>  8  XUL!nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsIFrame*, nsFrameItems&) [nsCSSFrameConstructor.cpp:c086fca6fc55 : 5503 + 0x6]
>     eip = 0x01177db7   esp = 0xbfffc7a0   ebp = 0xbfffc7e8
>  9  XUL!nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsIFrame*, nsFrameItems&) [nsCSSFrameConstructor.cpp:c086fca6fc55 : 9455 + 0x20]
>     eip = 0x01177f91   esp = 0xbfffc7f0   ebp = 0xbfffc828
> 10  XUL!nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsIFrame*, int, nsFrameItems&, int) [nsCSSFrameConstructor.cpp:c086fca6fc55 : 9568 + 0x29]
>     eip = 0x011751a5   esp = 0xbfffc830   ebp = 0xbfffca38
> 11  XUL!nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsIFrame*, nsFrameItems&) [nsCSSFrameConstructor.cpp:c086fca6fc55 : 3923 + 0x4a]
>     eip = 0x01177b0c   esp = 0xbfffca40   ebp = 0xbfffcaf8
> 12  XUL!nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsIFrame*, nsFrameItems&) [nsCSSFrameConstructor.cpp:c086fca6fc55 : 5527 + 0x23]
>     eip = 0x01177dee   esp = 0xbfffcb00   ebp = 0xbfffcb48
> 13  XUL!nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsIFrame*, nsFrameItems&) [nsCSSFrameConstructor.cpp:c086fca6fc55 : 9455 + 0x20]
>     eip = 0x01177f91   esp = 0xbfffcb50   ebp = 0xbfffcb88
> 14  XUL!nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsIFrame*, int, nsFrameItems&, int) [nsCSSFrameConstructor.cpp:c086fca6fc55 : 9568 + 0x29]
>     eip = 0x011751a5   esp = 0xbfffcb90   ebp = 0xbfffcd98
> 15  XUL!nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsIFrame*, nsFrameItems&) [nsCSSFrameConstructor.cpp:c086fca6fc55 : 3923 + 0x4a]
>     eip = 0x01177b0c   esp = 0xbfffcda0   ebp = 0xbfffce58
> 16  XUL!nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsIFrame*, nsFrameItems&) [nsCSSFrameConstructor.cpp:c086fca6fc55 : 5527 + 0x23]
>     eip = 0x01177dee   esp = 0xbfffce60   ebp = 0xbfffcea8
> 17  XUL!nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsIFrame*, nsFrameItems&) [nsCSSFrameConstructor.cpp:c086fca6fc55 : 9455 + 0x20]
>     eip = 0x01177f91   esp = 0xbfffceb0   ebp = 0xbfffcee8
> 18  XUL!nsCSSFrameConstructor::ContentAppended(nsIContent*, int) [nsCSSFrameConstructor.cpp:c086fca6fc55 : 6380 + 0x12]
>     eip = 0x0117fa48   esp = 0xbfffcef0   ebp = 0xbfffd008
> 19  XUL!PresShell::ContentAppended(nsIDocument*, nsIContent*, int) [nsPresShell.cpp:c086fca6fc55 : 5019 + 0x15]
>     eip = 0x011ba487   esp = 0xbfffd010   ebp = 0xbfffd038
> 20  XUL!nsNodeUtils::ContentAppended(nsIContent*, int) [nsNodeUtils.cpp:c086fca6fc55 : 131 + 0x19]
>     eip = 0x0138d1b2   esp = 0xbfffd040   ebp = 0xbfffd078
> 21  XUL!nsGenericElement::doInsertChildAt(nsIContent*, unsigned int, int, nsIContent*, nsIDocument*, nsAttrAndChildArray&) [nsGenericElement.cpp:c086fca6fc55 : 3219 + 0x11]
>     eip = 0x013820d4   esp = 0xbfffd080   ebp = 0xbfffd138
> 22  XUL!nsGenericElement::InsertChildAt(nsIContent*, unsigned int, int) [nsGenericElement.cpp:c086fca6fc55 : 3150 + 0x24]
>     eip = 0x01382522   esp = 0xbfffd140   ebp = 0xbfffd168
> 23  XUL!nsGenericElement::doReplaceOrInsertBefore(int, nsIDOMNode*, nsIDOMNode*, nsIContent*, nsIDocument*, nsIDOMNode**) [nsGenericElement.cpp:c086fca6fc55 : 3934 + 0x29]
>     eip = 0x01382f0a   esp = 0xbfffd170   ebp = 0xbfffd268
> 24  XUL!nsGenericElement::InsertBefore(nsIDOMNode*, nsIDOMNode*, nsIDOMNode**) [nsGenericElement.cpp:c086fca6fc55 : 3469 + 0x28]
>     eip = 0x01383823   esp = 0xbfffd270   ebp = 0xbfffd298
> 25  XUL!nsXULElement::AppendChild(nsIDOMNode*, nsIDOMNode**) [nsGenericElement.h:c086fca6fc55 : 499 + 0x20]
>     eip = 0x01666b2a   esp = 0xbfffd2a0   ebp = 0xbfffd2b8
> 26  XUL!nsIDOMNode_AppendChild [dom_quickstubs.cpp : 3510 + 0x12]
>     eip = 0x0100b663   esp = 0xbfffd2c0   ebp = 0xbfffd3e8
> 27  libmozjs.dylib!js_Interpret [jsinterp.cpp:c086fca6fc55 : 5190 + 0x1c]
>     eip = 0x001946f7   esp = 0xbfffd3f0   ebp = 0xbfffd728
> 28  libmozjs.dylib!js_Invoke [jsinterp.cpp:c086fca6fc55 : 1379 + 0xa]
>     eip = 0x0019a65e   esp = 0xbfffd730   ebp = 0xbfffd848
> 29  libmozjs.dylib!js_InternalInvoke [jsinterp.cpp:c086fca6fc55 : 1451 + 0x1c]
>     eip = 0x0019aef7   esp = 0xbfffd850   ebp = 0xbfffd888
> 30  libmozjs.dylib!js_InternalGetOrSet [jsinterp.cpp:c086fca6fc55 : 1521 + 0x2f]
>     eip = 0x0019b1d0   esp = 0xbfffd890   ebp = 0xbfffd8d8
> 31  libmozjs.dylib!js_SetPropertyHelper [jsscope.h:c086fca6fc55 : 585 + 0x36]
>     eip = 0x001ad0e7   esp = 0xbfffd8e0   ebp = 0xbfffd968
> 32  libmozjs.dylib!js_Interpret [jsinterp.cpp:c086fca6fc55 : 4833 + 0x27]
>     eip = 0x0018a7df   esp = 0xbfffd970   ebp = 0xbfffdca8
> 33  libmozjs.dylib!js_Invoke [jsinterp.cpp:c086fca6fc55 : 1379 + 0xa]
>     eip = 0x0019a65e   esp = 0xbfffdcb0   ebp = 0xbfffddc8
> 34  libmozjs.dylib!js_InternalInvoke [jsinterp.cpp:c086fca6fc55 : 1451 + 0x1c]
>     eip = 0x0019aef7   esp = 0xbfffddd0   ebp = 0xbfffde08
> 35  libmozjs.dylib!JS_CallFunctionValue [jsapi.cpp:c086fca6fc55 : 5176 + 0x2f]
>     eip = 0x0014318e   esp = 0xbfffde10   ebp = 0xbfffde38
> 36  XUL!nsXBLProtoImplAnonymousMethod::Execute(nsIContent*) [nsXBLProtoImplMethod.cpp:c086fca6fc55 : 332 + 0x2c]
>     eip = 0x014c9d51   esp = 0xbfffde40   ebp = 0xbfffdea8
> 37  XUL!nsXBLBinding::ExecuteAttachedHandler() [nsXBLBinding.cpp:c086fca6fc55 : 979 + 0x11]
>     eip = 0x014bd1aa   esp = 0xbfffdeb0   ebp = 0xbfffdec8
> 38  XUL!nsXBLBinding::ExecuteAttachedHandler() [nsXBLBinding.cpp:c086fca6fc55 : 976 + 0xa]
>     eip = 0x014bd18c   esp = 0xbfffded0   ebp = 0xbfffdee8
> 39  XUL!nsBindingManager::ProcessAttachedQueue(unsigned int) [nsBindingManager.cpp:c086fca6fc55 : 1017 + 0xa]
>     eip = 0x014d48d0   esp = 0xbfffdef0   ebp = 0xbfffdf38
> 40  XUL!PresShell::InitialReflow(int, int) [nsPresShell.cpp:c086fca6fc55 : 2726 + 0x18]
>     eip = 0x011b9faf   esp = 0xbfffdf40   ebp = 0xbfffdf98
> 41  XUL!nsXULDocument::StartLayout() [nsXULDocument.cpp:c086fca6fc55 : 2063 + 0xf]
>     eip = 0x014df3a3   esp = 0xbfffdfa0   ebp = 0xbfffe008
> 42  XUL!nsXULDocument::DoneWalking() [nsXULDocument.cpp:c086fca6fc55 : 3207 + 0xa]
>     eip = 0x014e31ec   esp = 0xbfffe010   ebp = 0xbfffe0a8
> 43  XUL!nsXULDocument::ResumeWalk() [nsXULDocument.cpp:c086fca6fc55 : 3156 + 0x7]
>     eip = 0x014e7fc4   esp = 0xbfffe0b0   ebp = 0xbfffe138
> 44  XUL!nsXULDocument::OnStreamComplete(nsIStreamLoader*, nsISupports*, unsigned int, unsigned int, unsigned char const*) [nsXULDocument.cpp:c086fca6fc55 : 3607 + 0xa]
>     eip = 0x014e856f   esp = 0xbfffe140   ebp = 0xbfffe1b8
> 45  XUL!nsStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) [nsStreamLoader.cpp:c086fca6fc55 : 108 + 0x2a]
>     eip = 0x0103cb86   esp = 0xbfffe1c0   ebp = 0xbfffe1f8
> 46  XUL!nsJARChannel::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) [nsJARChannel.cpp:c086fca6fc55 : 879 + 0x22]
>     eip = 0x010e21fb   esp = 0xbfffe200   ebp = 0xbfffe228
> 47  XUL!nsInputStreamPump::OnStateStop() [nsInputStreamPump.cpp:c086fca6fc55 : 576 + 0x1f]
>     eip = 0x01020ade   esp = 0xbfffe230   ebp = 0xbfffe258
> 48  XUL!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) [nsInputStreamPump.cpp:c086fca6fc55 : 401 + 0x7]
>     eip = 0x01021628   esp = 0xbfffe260   ebp = 0xbfffe288
> 49  XUL!nsInputStreamReadyEvent::Run() [nsStreamUtils.cpp:c086fca6fc55 : 111 + 0xe]
>     eip = 0x01a5346d   esp = 0xbfffe290   ebp = 0xbfffe2a8
> 50  XUL!nsThread::ProcessNextEvent(int, int*) [nsThread.cpp:c086fca6fc55 : 527 + 0x9]
>     eip = 0x01a70e27   esp = 0xbfffe2b0   ebp = 0xbfffe2f8
> 51  XUL!NS_ProcessPendingEvents_P(nsIThread*, unsigned int) [nsThreadUtils.cpp : 180 + 0x13]
>     eip = 0x01a313e7   esp = 0xbfffe300   ebp = 0xbfffe348
> 52  XUL!nsBaseAppShell::NativeEventCallback() [nsBaseAppShell.cpp:c086fca6fc55 : 121 + 0x1a]
>     eip = 0x019e3572   esp = 0xbfffe350   ebp = 0xbfffe378
> 53  XUL!nsAppShell::ProcessGeckoEvents(void*) [nsAppShell.mm:c086fca6fc55 : 413 + 0x7]
>     eip = 0x019add5e   esp = 0xbfffe380   ebp = 0xbfffe458
> 54  CoreFoundation + 0x7262d
>     eip = 0x9390462e   esp = 0xbfffe460   ebp = 0xbfffea18
> 55  CoreFoundation + 0x72d17
>     eip = 0x93904d18   esp = 0xbfffea20   ebp = 0xbfffea58
> 56  HIToolbox + 0x3069f
>     eip = 0x9128d6a0   esp = 0xbfffea60   ebp = 0xbfffea98
> 57  HIToolbox + 0x304b8
>     eip = 0x9128d4b9   esp = 0xbfffeaa0   ebp = 0xbfffeb28
> 58  HIToolbox + 0x3032c
>     eip = 0x9128d32d   esp = 0xbfffeb30   ebp = 0xbfffeb48
> 59  AppKit + 0x407d8
>     eip = 0x908137d9   esp = 0xbfffeb50   ebp = 0xbfffeeb8
> 60  AppKit + 0x4008d
>     eip = 0x9081308e   esp = 0xbfffeec0   ebp = 0xbffff1b8
> 61  AppKit + 0x390c4
>     eip = 0x9080c0c5   esp = 0xbffff1c0   ebp = 0xbffff278
> 62  XUL!nsAppShell::Run() [nsAppShell.mm:c086fca6fc55 : 766 + 0x7f]
>     eip = 0x019ad8fa   esp = 0xbffff280   ebp = 0xbffff308
> 63  XUL!nsAppStartup::Run() [nsAppStartup.cpp:c086fca6fc55 : 193 + 0x7]
>     eip = 0x0183f137   esp = 0xbffff310   ebp = 0xbffff328
> 64  XUL!XRE_main [nsAppRunner.cpp:c086fca6fc55 : 3386 + 0xa]
>     eip = 0x00f7efe9   esp = 0xbffff330   ebp = 0xbffff8a8
> 65  firefox-bin!main [nsBrowserApp.cpp:c086fca6fc55 : 156 + 0x18]
>     eip = 0x00002cb8   esp = 0xbffff8b0   ebp = 0xbffff908
> 66  firefox-bin + 0x1541
>     eip = 0x00002542   esp = 0xbffff910   ebp = 0xbffff948
> 67  firefox-bin + 0x1468
>     eip = 0x00002469   esp = 0xbffff950   ebp = 0xbffff960
> 68  0x4
>     eip = 0x00000005   esp = 0xbffff968   ebp = 0x00000000
Comment 86 Michael Ventnor 2009-07-31 17:51:24 PDT
Created attachment 392018 [details] [diff] [review]
For checkin 2

This should fix it.
Comment 87 David Baron :dbaron: ⌚️UTC-10 2009-07-31 18:02:59 PDT
For the record, the difference is:

$ interdiff <(vs 'https://bugzilla.mozilla.org/attachment.cgi?id=391839') <(vs 'https://bugzilla.mozilla.org/attachment.cgi?id=392018')
diff -u b/layout/style/nsStyleStruct.cpp b/layout/style/nsStyleStruct.cpp
--- b/layout/style/nsStyleStruct.cpp
+++ b/layout/style/nsStyleStruct.cpp
@@ -1442,6 +1442,7 @@
   // We need our own copy constructor because we don't want
   // to copy the reference count
   MOZ_COUNT_CTOR(nsStyleBackground::Image);
+  mType = eBackgroundImage_Null;
   DoCopy(aOther);
 }
 
which looks good.

(Though I would note that it might also make sense to put the this != aOther check in the operator= rather than DoCopy, since the copy-constructor doesn't need it.)
Comment 88 Michael Ventnor 2009-07-31 18:26:29 PDT
Created attachment 392025 [details] [diff] [review]
For checkin 2.1

OK, I'll do that, since I have nothing else to do. I also removed a redundant SetNull() call.
Comment 89 David Baron :dbaron: ⌚️UTC-10 2009-07-31 18:48:26 PDT
Except now you broke operator= if aOther is type null and this is not.
Comment 90 Michael Ventnor 2009-07-31 19:17:11 PDT
Oh yeah, you're right.
Comment 91 Michael Ventnor 2009-07-31 19:18:30 PDT
Created attachment 392034 [details] [diff] [review]
For checkin 2.2

Without stupid mistake.
Comment 92 mdew 2009-07-31 22:20:42 PDT
The tags slightly differ to the webkit, is this intentional?

-webkit-gradient(radial,...
-webkit-gradient(linear,...

vs

-moz-radial-gradient(...
-moz-linear-gradient(...
Comment 93 mdew 2009-07-31 22:25:32 PDT
Created attachment 392047 [details]
Background Gradients Example (from webkit.org)

Using the 2.2 patch, Linear displays fine, radial doesn't display at all.
Comment 94 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-31 22:49:16 PDT
(In reply to comment #92)
> The tags slightly differ to the webkit, is this intentional?
> 
> -webkit-gradient(radial,...
> -webkit-gradient(linear,...
> 
> vs
> 
> -moz-radial-gradient(...
> -moz-linear-gradient(...

Yes, see comment #4.
Comment 95 Michael Ventnor 2009-08-01 00:22:51 PDT
(In reply to comment #93)
> Created an attachment (id=392047) [details]
> Background Gradients Example (from webkit.org)
> 
> Using the 2.2 patch, Linear displays fine, radial doesn't display at all.

Webkit accepts numbers only, we use units.
Comment 96 Michael Ventnor 2009-08-01 00:24:59 PDT
Created attachment 392053 [details] [diff] [review]
For checkin 2.3

I needed to make minor modifications to the reftests for them to pass on Mac. This should be fine now.
Comment 97 David Baron :dbaron: ⌚️UTC-10 2009-08-01 11:09:17 PDT
relanded: http://hg.mozilla.org/mozilla-central/rev/77a8228f2951

Still need to get some documentation.
Comment 98 mdew 2009-08-01 16:04:05 PDT
Created attachment 392119 [details]
radius bug?

not sure if my end or not.. just "ported" one of the radius examples. Looks rather broken compared to the webkit version

orginally off:

http://trac.webkit.org/browser/trunk/LayoutTests//fast/gradients/simple-gradients.html?format=raw
Comment 99 Michael Ventnor 2009-08-01 16:05:57 PDT
See roc, I told you it wouldn't be what authors expect. :-)

You need to set no-repeat on the gradient layer, otherwise the gradient will continuously repeat.
Comment 100 mdew 2009-08-01 16:26:56 PDT
So something like this?
background: -moz-radial-gradient(45px 45px, 10px, 52px 50px, 30px, from(#A7D30C), to(rgba(1,159,98,0)), no-repeat, color-stop(90%, #019F62)),

also implementing both webkit and moz gradient strings in the same css background seems to break, I was assuming Mozilla would ignore the -webkit strings and still process the -moz- ones.
Comment 101 Michael Ventnor 2009-08-01 16:30:28 PDT
background-repeat: no-repeat is what I meant.
Comment 102 Dão Gottwald [:dao] 2009-08-01 16:39:01 PDT
(In reply to comment #99)
> See roc, I told you it wouldn't be what authors expect. :-)

Well, it's a ported webkit testcase. That doesn't necessarily show what an unbiased author would expect.

(In reply to comment #100)
> also implementing both webkit and moz gradient strings in the same css
> background seems to break, I was assuming Mozilla would ignore the -webkit
> strings and still process the -moz- ones.

A value is either valid as a whole or invalid as a whole. There's nothing in between, especially no "drop invalid chunks and try to treat the rest as valid".
Comment 103 mdew 2009-08-01 18:05:29 PDT
unrelated/related to a new bug, but in webkit.. most likely a color profile issue with webkit. From mozilla stand point, it looks fine. The background color in webkit is off white.

https://bugs.webkit.org/show_bug.cgi?id=27926

(In reply to comment #102)
> A value is either valid as a whole or invalid as a whole. There's nothing in
> between, especially no "drop invalid chunks and try to treat the rest as
> valid".

In the end I set 2 css background: tags, one for webkit and one for firefox. I assumed if I had everything under the one background tag, Firefox would execute the right tags and ignore the webkit tags.
Comment 104 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-01 20:17:09 PDT
(In reply to comment #99)
> See roc, I told you it wouldn't be what authors expect. :-)

I agree that this is a small problem. But I think it's better than the alternatives.
Comment 105 philippe (part-time) 2009-08-01 22:53:55 PDT
(In reply to comment #101)
> background-repeat: no-repeat is what I meant.

Is this intentional that this does _not_ work with the background shorthand notation ?

Compare the rendering of the 2 boxes (port of the WebKit example)
http://dev.l-c-n.com/css3/gradient-background1.html

Note that I did get interesting effects by using the background shorthand, but _only_ when using only one -moz-radial-gradient() for the background rule.
http://dev.l-c-n.com/css3/gradient-background1.html

---
Is there a draft spec published anywhere ? Or is the only document available at this point the Webkit blog post ?
Comment 106 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-01 23:15:04 PDT
(In reply to comment #105)
> (In reply to comment #101)
> > background-repeat: no-repeat is what I meant.
> 
> Is this intentional that this does _not_ work with the background shorthand
> notation ?

It does, but you have to specify no-repeat for each image, and your example doesn't.

> Is there a draft spec published anywhere ? Or is the only document available at
> this point the Webkit blog post ?

Michael's going to write some right away!
Comment 107 Michael Ventnor 2009-08-01 23:16:59 PDT
I have a web tech post ready to put up, but I'm having problems logging into the web tech blog and resetting my password doesn't work.

I could work on the gradients MDC article that just got started, though...
Comment 108 philippe (part-time) 2009-08-01 23:38:40 PDT
(In reply to comment #106)
> (In reply to comment #105)
> > (In reply to comment #101)
> > > background-repeat: no-repeat is what I meant.
> > 
> > Is this intentional that this does _not_ work with the background shorthand
> > notation ?
> 
> It does, but you have to specify no-repeat for each image, and your example
> doesn't.

Oh my, got it now:
http://dev.l-c-n.com/css3/gradient-background1.html

I should have re-read the CSS3 background & borders module.
Comment 109 Michael Ventnor 2009-08-02 00:03:11 PDT
...and I can't write anything to MDC because it won't load.
Comment 110 Michael Ventnor 2009-08-02 02:42:50 PDT
I've done as much as I can think of so far for an MDC document:

https://developer.mozilla.org/en/CSS/Gradients

Hopefully it provides enough guidance, and its explanations are clear enough.
Comment 111 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-02 02:56:57 PDT
If you send me the Web-tech post I can post it for you.
Comment 112 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-02 03:11:30 PDT
Although if it's OK with you, I may want to edit it. In particular it's worth calling out the differences from Webkit's gradients and explaining why we changed them.
Comment 113 mdew 2009-08-04 21:22:17 PDT
Does Gradients support transparency? ie, fade from one color to a transparency?
Comment 114 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-04 21:33:34 PDT
Yes, e.g.
background:-moz-linear-gradient(top left, bottom right, from(red), to(rgba(0,255,0,0)));
Comment 115 Eric Shepherd [:sheppy] 2009-08-25 09:01:32 PDT
These are written up here:

https://developer.mozilla.org/en/CSS/Gradients

However, some reorganization work needs to be done on this content.
Comment 116 Eric Shepherd [:sheppy] 2009-09-02 13:45:11 PDT
I've removed the existing page and replaced it with:

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

This makes this documentation match up with other CSS documentation stylistically.
Comment 117 Kroc Camen 2009-09-07 10:16:29 PDT
Pardon my ignorance, but this syntax doesn’t appear to allow me to create a gradient that starts at the bottom of an element and proceeds until 100px *from* the bottom (i.e. bottom-100px). You can only specify units absolute from the top of the element so therefore as the user increases the height, the gradient would stretch.
Comment 118 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-07 14:25:16 PDT
You can, like this:

body {
  background-image:-moz-linear-gradient(top, bottom, from(blue), to(lime));
  background-attachment:fixed;
  background-repeat:no-repeat;
  -moz-background-size:100% 100px;
  background-position:bottom;
}

Note that the CSS WG is discussing a gradients proposal using quite different syntax.
Comment 119 David Baron :dbaron: ⌚️UTC-10 2009-10-15 18:31:45 PDT
Bug 513395 is about implementing the new syntax.
Comment 120 patrickjdempsey 2009-10-26 16:10:14 PDT
I do not see any mention in documentation here for support of start and end.  Currently, there is no way to automatically create RTL backgrounds for Firefox, et al.  Using start and end in addition to left and right would create the possibility of RTL sensitive backgrounds.  Are there any plans for this?

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