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.

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.

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.

(In reply to comment #2) > Using units is consistent with every other CSS property Except for border-image, right?

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.

fwiw, I agree with comment #2 and comment #4.

(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.

I think it's crazy for the parsing of a function "-moz-gradient" to depend on the *value* of its first parameter!

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.

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?

(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.

I think I prefer two separate functions. Is Ventnor confusing adding new properties (more expensive) with adding new values (much cheaper)?

I discussed both of the changes in comment #4 with Hyatt on IRC and he was in favour of them.

Or at least "fine" with them.

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.

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.

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.

(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?

(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.

(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.

void* and manual casts/refcounting.

Michael, are you going to be able to work on this anytime soon, or should I look for someone else to pick it up?

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.

(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.

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.

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.

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?

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?

I think it's fine to not have computed style for now.

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"?

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.

What you need are the [diff] options in http://pastebin.mozilla.org/658023 . (I recommend everything there, actually.)

(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.

(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.

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.

I'd really like to get this in for Firefox 3.6, which means we have a week to land it.

1 week? For 3.6? You must be joking...

And I do use queues, but I never though to use separate queues for style code and the rest.

Take a look at bug 339548 and bug 352093 :-).

+ 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!

(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.

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.

(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?

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.

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.

(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.

(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?

Yes.

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.

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 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.

Thanks. Would you know if dbaron would have time to do this before the freeze? (or would bz be better?)

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?

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.

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"?

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).

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).

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.

(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.

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.

(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.

(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).

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.

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.

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 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.

(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 :-)

Built on background-size.

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...

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.

(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 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.

Fixes the comments.

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

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.

More: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1249040192.1249043212.3838.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1249040190.1249043520.7283.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1249042939.1249043047.2021.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1249042939.1249043366.5533.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1249043206.1249043624.8292.gz backed out

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

(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.

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.

The first log of comment 80, I mean.

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

This should fix it.

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.)

OK, I'll do that, since I have nothing else to do. I also removed a redundant SetNull() call.

Except now you broke operator= if aOther is type null and this is not.

Oh yeah, you're right.

Without stupid mistake.

The tags slightly differ to the webkit, is this intentional? -webkit-gradient(radial,... -webkit-gradient(linear,... vs -moz-radial-gradient(... -moz-linear-gradient(...

Using the 2.2 patch, Linear displays fine, radial doesn't display at all.

(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.

(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.

I needed to make minor modifications to the reftests for them to pass on Mac. This should be fine now.

relanded: http://hg.mozilla.org/mozilla-central/rev/77a8228f2951 Still need to get some documentation.

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

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.

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.

background-repeat: no-repeat is what I meant.

(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".

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.

(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.

(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 ?

(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!

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...

(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.

...and I can't write anything to MDC because it won't load.

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.

If you send me the Web-tech post I can post it for you.

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.

Does Gradients support transparency? ie, fade from one color to a transparency?

Yes, e.g. background:-moz-linear-gradient(top left, bottom right, from(red), to(rgba(0,255,0,0)));

These are written up here: https://developer.mozilla.org/en/CSS/Gradients However, some reorganization work needs to be done on this content.

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.

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.

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.

Bug 513395 is about implementing the new syntax.

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?