Closed Bug 162252 Opened 23 years ago Closed 23 years ago

Implement CSS3 background-clip and background-origin properties

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.2beta

People

(Reporter: caillon, Assigned: caillon)

References

(
URL
)

Details

(Keywords: css3)

Attachments

(3 files, 6 obsolete files)

diff -uw nsCSSRendering.cpp (v1.3)
27.98 KB, patch
Details | Diff | Splinter Review
Patch v1.3 (updated against current trunk)
128.65 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Patch as it was checked in
133.89 KB, patch
Details | Diff | Splinter Review
I have 'background-clip', 'background-origin' working already against an older tree. I should merge these to trunk. They are implemented of course with the '-moz-' prefix in my tree, as these properties aren't yet finalized (though the spec looks solid and based on the comments in the document status, it appears the WG feels this way too so I don't expect major changes. David, correct me here if you feel I should hold off on these implementations. Also, I have background-size mostly working, but there is a few other issues and I feel it would be better to do that as a separate patch anyway.
Attached patch Patch v1 (obsolete) — — Splinter Review
So here it is. I removed a huge |else| block in nsCSSRendering::PaintBackgroundWithSC since it was unneeded. I will attach a diff -uw of only nsCSSRendering.cpp, to make reviewing changes to that file easier.
David, would you please review this?
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.2alpha
Comment on attachment 95190 [details] [diff] [review] Patch v1 >+ // background-clip: enum >+ if (eCSSUnit_Enumerated == colorData.mBackClip.GetUnit()) { >+ bg->mBackgroundClip = colorData.mBackClip.GetIntValue(); >+ } >+ >+ // background-origin: enum >+ if (eCSSUnit_Enumerated == colorData.mBackOrigin.GetUnit()) { >+ bg->mBackgroundOrigin = colorData.mBackOrigin.GetIntValue(); > } These need to handle eCSSUnit_Inherit, and eCSSUnit_Initial would be good too. More tomorrow...
Made the appropriate changes locally to the rule node and parser to handle inherit values. I didn't do those since the background WD doesn't define anything other than keywords as legal values. I'll wait for more tomorrow before attaching a new patch.
Comment on attachment 95190 [details] [diff] [review] Patch v1 >RCS file: /cvsroot/mozilla/content/shared/public/nsStyleStruct.h,v > > PRUint8 mBackgroundAttachment; // [reset] See nsStyleConsts.h >+ PRUint8 mBackgroundClip; // [reset] See nsStyleConsts.h > PRUint8 mBackgroundFlags; // [reset] See nsStyleConsts.h >+ PRUint8 mBackgroundOrigin; // [reset] See nsStyleConsts.h > PRUint8 mBackgroundRepeat; // [reset] See nsStyleConsts.h The only comment I have on this part (excluding nsCSSRendering.cpp, which I'll look at in the diff -uw) is that some of these should have field widths so you can avoid increasing the size of nsStyleBackground. You don't need more than 32-bits to represent these 4 things (and they're between two things that will be aligned at 32-bit boundaries), so it would be good to stick a field with (: 4) on two adjacent ones (perhaps the two new ones, moved next to each other).
Comment on attachment 95193 [details] [diff] [review] diff -uw nsCSSRendering.cpp >+ // The background is rendered over the 'background-clip' area. >+ nsRect bgClipArea(aBorderArea); >+ if (aColor.mBackgroundClip != NS_STYLE_BG_CLIP_BORDER) { NS_ASSERTION(aColor.mBackgroundClip == NS_STYLE_BG_CLIP_PADDING, "unexpected value"); >+ nsMargin border; >+ aBorder.GetBorder(border); >+ bgClipArea.Deflate(border); >+ } >@@ -2743,35 +2754,36 @@ > // rounded version of the border > if (!aBorder.mBorderColors) { > // XXXdwh Composite borders (with multiple colors per side) use their own border radius > // algorithm now, since the current one doesn't work right for small radii. > for(i=0;i<4;i++){ > if (borderRadii[i] > 0){ > PaintRoundedBackground(aPresContext,aRenderingContext,aForFrame,aDirtyRect, >- aBorderArea,aColor,aDX,aDY,borderRadii); >+ bgClipArea,aColor,aDX,aDY,borderRadii); > return; So where do we want PaintRoundedBackground to round the background? Wouldn't we want to reduce the radius appropriately if we inset the area? (My guess is that the right formula would be to reduce the border radius by the thickness of the offset, but I'm assuming there that the border radius is the radius of curvature at the outside edge of the border. Is it?) > } > } > } >- else { >+ else if (aColor.mBackgroundClip == NS_STYLE_BG_CLIP_BORDER) { >+ // XXX users of -moz-border-*-colors expect a transparent border-color to show >+ // the parent's background-color instead of its background-color. This seems >+ // wrong, but we handle that here by explictly clipping the background to the >+ // padding area. > nsMargin border; > aBorder.GetBorder(border); >- nsRect borderArea(aBorderArea); >- borderArea.Deflate(border); >- aRenderingContext.SetColor(aColor.mBackgroundColor); >- aRenderingContext.FillRect(borderArea); >- return; >+ bgClipArea.Deflate(border); > } > > aRenderingContext.SetColor(aColor.mBackgroundColor); >- aRenderingContext.FillRect(aBorderArea); >+ aRenderingContext.FillRect(bgClipArea); >+ } >+ return; Since you're doing the transformation of huge if blocks into early returns, how about doing the same for the |if (transparentBG)| above? If you're energetic, perhaps the non-image part could be refactored into a separate function that can be called from the two places in the image part that use FillRect (it's more relevant for backgrounds that aren't fully repeating, but pretty obscure in any case)? >- // Background images are tiled over the 'content' and 'padding' areas >- // only (not the 'border' area) >- nsRect paddingArea(aBorderArea); >+ // Background images are tiled over the area defined by 'background-origin' >+ nsRect bgOriginArea(aBorderArea); >+ if (aColor.mBackgroundOrigin != NS_STYLE_BG_ORIGIN_BORDER) { Hmm, no, the background images are tiled over the 'background-clip' area, but the origin of the tiling is given by the 'background-origin' property. I think the comment is wrong but the code is right, though it looks like the old code was implementing the somewhat odd but probably literal interpretation of CSS2 that the background color extended behind the border but background images do not. This interpretation seems to have been changed in the CSS3 background draft. Ian, any thoughts? > nsMargin border; >- > if (!aBorder.GetBorder(border)) { > NS_NOTYETIMPLEMENTED("percentage border"); > } >- paddingArea.Deflate(border); > >- // The actual dirty rect is the intersection of the padding area and the >- // dirty rect we were given >+ bgOriginArea.Deflate(border); >+ if (aColor.mBackgroundOrigin != NS_STYLE_BG_ORIGIN_PADDING) { >+ nsMargin padding; >+ if (!aPadding.GetPadding(padding)) { >+ NS_NOTYETIMPLEMENTED("percentage padding"); >+ } >+ >+ bgOriginArea.Deflate(padding); >+ if (aColor.mBackgroundOrigin != NS_STYLE_BG_ORIGIN_CONTENT) { >+ NS_NOTYETIMPLEMENTED("unknown background-origin"); >+ } This could just be a one-line NS_ASSERTION. >+ } >+ } >+ >+ // The actual dirty rect is the intersection of the 'background-origin' >+ // area and the dirty rect we were given > nsRect dirtyRect; > >- if (!dirtyRect.IntersectRect(paddingArea, aDirtyRect)) { >+ if (!dirtyRect.IntersectRect(bgOriginArea, aDirtyRect)) { This check should be against the bgClipArea, shouldn't it? Why can't it be much earlier in the function? (It seems odd to use 'background-origin' to determine the extent of background-image tiling, although I think it would be compatible with the CSS2 quirk I mentioned above.) > // Nothing to paint > return; > } > > // Get the anchor point >- ComputeBackgroundAnchorPoint(aColor, firstRootElementFrameArea, paddingArea, tileWidth, tileHeight, anchor); >+ ComputeBackgroundAnchorPoint(aColor, firstRootElementFrameArea, bgOriginArea, tileWidth, tileHeight, anchor); > } else { >- ComputeBackgroundAnchorPoint(aColor, paddingArea, paddingArea, tileWidth, tileHeight, anchor); >+ ComputeBackgroundAnchorPoint(aColor, bgOriginArea, bgOriginArea, tileWidth, tileHeight, anchor); Should one of these |bgOriginArea| lines be the bgClipArea? (Same caveat.) > } > } else { > // Otherwise, it is the normal case, and the background is >- // simply placed relative to the frame's padding area >- ComputeBackgroundAnchorPoint(aColor, paddingArea, paddingArea, tileWidth, tileHeight, anchor); >+ // simply placed relative to the frame's background-origin >+ ComputeBackgroundAnchorPoint(aColor, bgOriginArea, bgOriginArea, tileWidth, tileHeight, anchor); > } > } Same here. >@@ -3019,18 +3041,17 @@ > . . . . . . . . . .#. . . . .#. . . . > . | . . ########### . /|\ > . | . . . . | h > . . | . . . . . . . . . . . . . z . . \|/ > . | . . . . > |<-----nw------>| |<--w-->| I'm not reviewing any of the code from here to the bottom (although there isn't much left), since it seems like there are some issues that we should probably get clarified in the spec first...
Actually, I don't think we need spec clarification -- I think the spec is clear, since the change that image tiling should also cover the border area is in the CSS2 errata and in CSS 2.1. So I think the patch does need some changes to ensure that we're tiling over the 'background-clip', but using 'background-origin' as an origin, which, thanks to the CSS2 errata (and also CSS2.1), is the correct think to do for CSS2 as well, given the default values in the CSS3 background WD.
> it looks like the old code was implementing the somewhat odd but probably literal > interpretation of CSS2 that the background color extended behind the border but > background images do not. This interpretation seems to have been changed in the > CSS3 background draft. That's exactly what was going on; the spec has since been clarified and our implementation should be changed to match. (Default origin for background image is now the padding edge, and default extent for background image is now the border edge, iirc, although there are properties to control both, again iirc.)
What's the top left corner of the background area if you have a greatly rounded border-corner? Does the padding curve with the curved corner, or still describe a box that could (in theory) stick beyond the rounded corner? I'm just looking for clarification.
David, to answer your question in comment #8: >So where do we want PaintRoundedBackground to round the background? We want PaintRoundedBackground to paint the background whenever we have rounded borders. However, when we have -moz-border-*-colors set, we really _don't_ have rounded borders (yes, even if -moz-border-radius is set!). When we have a border-radius set, we actually have diagonal borders being painted (See nsCSSRendering::DrawCompositeSide() ). I'm not sure which behavior (painting a box-shaped border vs. painting a rounded border) is "better" given the fact that the border drawing algorithm just is wrong to begin with, and we'll draw the background outside the border in some cases for both instances, though it will be less visible if we just drew the rounded border. We _could_ use FillPolygon in a similar fashion to how DrawCompositeSide draws its borders, but I am inclined to not do that in this patch, since changing our behavior here can potentially break themes (Modern) and besides, we really should fix this the correct way if we are going to touch it at all. Also, thanks for clarifying the clip vs. origin issues. I based my patch in part on how I understood the CSS2 spec, and the current implementation and didn't look at the new 2.1 spec yet (I'll make sure to read the latest spec next time). I'll fix that as well as your other issues in the next patch which I should have uploaded later today. Eric, yes as far as our implementation is concerned the padding area is a box which does in theory extend beyond the curved border AFAICT, and the content area follows suit. That is probably a bug, but one that a different patch will fix.
In the first question you answered in the previous comment, my "where" really meant "with what radius of curvature".
Attached patch Patch v1.1 (obsolete) — — Splinter Review
* Corrects the background-image logic to paint over the clip area, not just the origin. * New function, nsCSSRendering::PaintBackgroundColor. * Fixes nsCSSRendering::PaintRoundedBackground to accomodate for background-clip. * Variable clarification in nsCSSRendering::ComputeBackgroundAnchorPoint * Handle inherit and initial. * Fix other issues raised by dbaron (thanks for a thorough review!) Once again, a diff -uw nsCSSRendering.cpp will follow.
Attachment #95190 - Attachment is obsolete: true
Attachment #95193 - Attachment is obsolete: true
Comment on attachment 95833 [details] [diff] [review] diff -uw nsCSSRendering.cpp (v1.1) Could you file a new bug on that edge of the rounded background issue that I mentioned in my second comment within comment 8? It's OK not to address it now, but we should have a bug on it. >+// except when painting on the canvas, in which case the relative bounds >+// should be the bounds of the root element's frame and the tiling bounds >+// should be the bounds of the canvas frame. You removed the terms "relative bounds" and "tiling bounds", so you shouldn't continue using them in comments. It would probably be clearer not to switch the order of the parameters (although it does look like you switched them in all the callers as well). >+ // Background images are tiled over the 'background-clip' >+ nsRect bgOriginArea(aBorderArea); The comment here is not about the variable being defined. Perhaps add "but the origin of the tiling is based on the 'background-origin' area"? >+ if (!aPadding.GetPadding(padding)) { >+ NS_NOTYETIMPLEMENTED("percentage padding"); >+ } Unfortunately, you can't get away with this, since percentage padding (unlike percentage border) really does exist. I'm not sure what the best way of handling this is. (I think CalcPaddingFor is deprecated, but I we don't want to construct a reflow state, either.) + // Move the background-clip area so that we can use the same logic for + // both the fixed and scrolling cases + bgClipArea.x = 0; + bgClipArea.y = 0; Shouldn't this be changing bgOriginArea? Try Ian's tests at http://www.hixie.ch/tests/evil/mixed/bgafixed.html . (Eek, that seems to have regressed in a current build even without your patch. Why?) Is Ian's canvas fixup code in PaintBackgroundWithSC (and the lack of modifications to it in this patch) causing these new properties not to be implemented for the root element's background? I think the rest of the math seems OK, but it might be good if Ian reviewed it as well.
Regarding http://www.hixie.ch/tests/evil/mixed/bgafixed.html , I don't think we regressed -- it looks like Ian broke the links to the background images when he moved the site to hixie.ch. So you'll have to download the page and substitute different background images, unless you can find the originals.
> Could you file a new bug on that edge of the rounded background issue that I mentioned in my second comment within comment 8? It's OK not to address it now, but we should have a bug on it. David, I addressed that in the patch. See the section in nsCSSRendering::PaintRoundedBackground() with my comment + // Adjust for background-clip I tested this with my patch in bug 45557 to allow transparent rounded borders, and it paints over the correct area. >You removed the terms "relative bounds" and "tiling bounds", so you shouldn't continue using them in comments. Whoops. I'll fix that. Good catch. > It would probably be clearer not to switch the order of the parameters I did make sure to update all the callers, and I made the switch because I thought it would be clearer to list the clip before the origin. I'll revert the order though since you commented on it. >Unfortunately, you can't get away with this, since percentage padding (unlike percentage border) really does exist. I'm not sure what the best way of handling this is. (I think CalcPaddingFor is deprecated, but I we don't want to construct a reflow state, either.) Yeah... good point. A fix for bug 106154 might help here, but I think in the meantime we should use CalcPaddingFor to make this work, and I'll add a comment about this. >+ // Move the background-clip area so that we can use the same logic for >+ // both the fixed and scrolling cases >+ bgClipArea.x = 0; >+ bgClipArea.y = 0; > Shouldn't this be changing bgOriginArea? Well, it really shouldn't be changing either clip or origin. That is pretty much just a hack so that we don't need to have different codepaths. Since the codepath we use calculates scroll backgrounds relative to the clip, that's why we need to force these to be 0, because we want fixed backgrounds relative to the scrolling ancestor instead. If you would prefer, I could remove those lines altogether and do something like this, which may make it clearer as to what is going on: if (repeat & NS_STYLE_BG_REPEAT_X) { // ... } else { // For scrolling attachment, the anchor is relative to the // background-clip. For fixed attachment, the anchor is // relative to the nearest scrolling ancestor (or the viewport) x0 = anchor.x; if (NS_STYLE_BG_ATTACHMENT_FIXED != aColor.mBackgroundAttachment) { x0 += bgClipArea.x; } x1 = x0 + tileWidth; }
I'm just attaching nsCSSRendering.cpp this time since the other files haven't really changed, barring some merge conflict fixups. This fixes David's concerns with the last patch.
Attachment #95833 - Attachment is obsolete: true
+ // background-clip: enum, inherit + // background-origin: enum, inherit Should we just list "initial" here too? Or stop listing "inherit, initial" on everything (per css3)? I lean toward the former, in the short term (that is, while all the other props have 'inherit' listed). It looks like the generation of "background" shorthands does not take these new properties into account. The computed style changes don't handle values of "inherit" or "-moz-initial". nsCSSProps::kBackgroundClipKTable and nsCSSProps::kBackgroundOriginKTable should try to use the same order for the enums, I think... ;) > // XXXldb It would make more sense to use > // |aRenderingContext.FillEllipse| here, but on at least GTK that > // doesn't draw a round enough circle. I believe this got fixed on gtk and xlib both... we should investigate fixing that (though not in this patch). > Index: layout/xul/base/src/nsSliderFrame.cpp Odd indentation in call to nsCSSRendering::PaintBackground + // XXX CalcPaddingFor is deprecated, but weneed it for percentage padding Hrm... I thought that nsIFrame's CalcBorderPadding might help us out here, but that looks like it's not that great either... I guess this is OK for now, but we really need to solve this problem at some point... And if nothing else, we should see whether CalcBorderPadding gives better results than CalcPaddingFor does. In any case, "we need" instead of "weneed". + ---- = the background clip area edge. The painting is done relative + to this area. If the background is positioned relative to the + viewport ('fixed') then this is the viewport edge. Should we be using the clip area edge here? Or the origin area edge? + // For scrolling attachment, the anchor is relative to the + // background-clip. same here? same for the vertical case. In PaintBackgroundColor: + // XXXdwh Composite borders (with multiple colors per side) use their own border radius + // algorithm now, since the current one doesn't work right for small radii. is this comment relevant here? > + aTheRadius[NS_SIDE_TOP] -= border.top; this assumes that moz-border-radius is the radius to the outer edge of the border. Is this correct?
> It looks like the generation of "background" shorthands does not take these new properties into account. That's correct. background: border padding; Which value is which? :-) The WD doesn't add define these properties on the shorthand, likely for this ambiguity. > The computed style changes don't handle values of "inherit" or "-moz-initial". The stylestructs shouldn't be storing inherit or initial values. The rule node should be storing the actual computed value into the stylestructs. >+ ---- = the background clip area edge. The painting is done relative >+ to this area. If the background is positioned relative to the >+ viewport ('fixed') then this is the viewport edge. > Should we be using the clip area edge here? Or the origin area edge? Clip. See dbaron's comments (8 and 9) and the errata'd CSS spec. Painting is done wrt the clip. >+ // For scrolling attachment, the anchor is relative to the >+ // background-clip. >same here? same for the vertical case. Same here. Same for the vertical case. ComputeBackgroundAnchorPoint took the origin into account already. This code is computing the area of the rect we want to paint our tiles within. A few lines below that, we will make sure that we get the offset positions correct when doing the actual drawing by removing the clip. >+ // XXXdwh Composite borders (with multiple colors per side) use their own border radius >+ // algorithm now, since the current one doesn't work right for small radii. >is this comment relevant here? Yeah, though the comment should probably be repositioned slightly (outside of the if?). >> + aTheRadius[NS_SIDE_TOP] -= border.top; >this assumes that moz-border-radius is the radius to the outer edge of the >border. Is this correct? Yes. I can and probably should comment about that.
> The stylestructs shouldn't be storing inherit or initial values. Add an assertion to that effect. > Clip. See dbaron's comments (8 and 9) and the errata'd CSS spec. Painting is > done wrt the clip. OK. I guess I was misled by the word "relative" in that comment; I can't figure out what that's supposed to mean... > Yeah, though the comment should probably be repositioned slightly (outside of > the if?). The reason I asked is that there are no composite borders involved; just a rounded one.
>> The stylestructs shouldn't be storing inherit or initial values. >Add an assertion to that effect. Actually, here "shouldn't be" means it's physically impossible for us to be storing inherit or initial since we're dealing with PRUint8s, not nsStyleCoords. >OK. I guess I was misled by the word "relative" in that comment; I can't figure out what that's supposed to mean... In this context it means "within". Want me to change that? > The reason I asked is that there are no composite borders involved; just a rounded one. Right, the comment still applies, just hyatt put it in a confusing place. I'll figure out a better home for it. I'm leaning towards // Rounded version of the border // XXXdwh Composite borders (with multiple colors per side) use their own border radius // algorithm now, since the current one doesn't work right for small radii. if (!aBorder.mBorderColors) {
> In this context it means "within". Want me to change that? Yes. If it means "within", say "within". The rest makes sense. I'm still working on fully understanding this code...
You are doing this as -moz-background-*, right? Please check you do not regress the background tests on hixie.ch, especially: http://www.hixie.ch/tests/adhoc/css/background/ ...and all other tests around there.
> You are doing this as -moz-background-*, right? Yep, see comment 0. > Please check you do not regress the background tests on hixie.ch I've run these and a few other test suites at one point and didn't notice any regressions. I'll run them all again before checking in.
One note from reading the comments above while going through my bugmail: we need to make sure that setting the background shorthand resets these properties, even if they aren't settable by it.
Attached patch Patch v1.3 (obsolete) — — Splinter Review
Addresses remaining comments from bz and dbaron. Barring any major issues, I'd like to try and get this patch reviewed and landed sometime soon, and move any remaining issues into separate bugs. Anyway, a run-through of the changes: * nsCSSParser.cpp - reset these to initial when setting the 'background'. I added a new static table like the one earlier in ParseBackground() since it seems clear we'll need to do the same thing for a few other properties down the line ('background-quantity', 'background-spacing'...) * nsCSSRendering.cpp - Clarified, fixed, moved, and added comments per bz. * nsStyleConsts.h, nsCSSProps.cpp - Switched the order of the constants so they match.
Attachment #95832 - Attachment is obsolete: true
comment 17: http://www.hixie.ch/tests/evil/mixed/bgafixed.html has now been fixed (by adding HTTP redirects for the two images). Thanks for the heads up.
Keywords: css3
Target Milestone: mozilla1.2alpha → mozilla1.2beta
Comment on attachment 100845 [details] [diff] [review] Patch v1.3 (updated against current trunk) r=dbaron
Attachment #100845 - Flags: review+
Why are the change hints in CSSProps for -clip and -origin "FRAMECHANGE"? VISUAL makes more sense and it's also what you use in CalcDifference. I can't believe dbaron let you get away with calling nsIStyleContext->GetStyleData(enum) instead of the overloaded ::GetStyleData(stylecontext, &styledata), but I'll let it slide too :-). If you fix the FRAMECHANGE hints, then sr=roc+moz
I merged this locally to the trunk and ran these background tests before landing: http://www.hixie.ch/tests/adhoc/css/background/ http://www.meyerweb.com/eric/css/tests/css2/ http://www.w3.org/Style/CSS/Test/CSS1/current/sec53.htm Our rendering with my patch is the same as the rendering on trunk linux build 2002100710 for every single page. The exception is http://www.hixie.ch/tests/adhoc/css/background/block/ but this is expected: in current trunk, background does not spill into the border. With my patch, it of course does. Everything is otherwise identical: our current bugs are still there, and everything we get correct is still correct. Everything looked good, so I checked in, with roc's comments addressed. Marking fixed.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: