background-repeat: space doesn't work with gradient backgrounds. See attached testcase, which should show spacing between the tiles, at least at most window sizes. Compare to Chrome and Edge, which show the spacing.

Sorry for missing this in my review.

In testcase2, the Edge rendering the effect is most in line with expectations. Chrome and Safari have some problems more or less.

And I found background-size: 50px parsing and other browsers are not the same, the specification is written in this way: In the following example, the background is shown with a width of approximately 3em: scaled so that it fits a whole number of times in the width of the background. The height is scaled proportionally to keep the original aspect ratio: div { background-image: url(image3.png); background-repeat: round repeat; background-size: 3em auto } In the following example, the background image is shown with a width of 3em and a height that is either the height corresponding to that width at the original aspect ratio or slightly less: div { background-image: url(image4.png); background-repeat: repeat round; background-size: 3em auto } In the following example, the background image is shown with a height of approximately 4em: scaled slightly so that it fits a whole number of times in the background height. The width is the approximately the width that correspond to a 4em height at the original aspect ratio: scaled slightly so that it fits a whole number of times in the background width. div { background-image: url(image5.png); background-repeat: round; background-size: auto 4em }

I will have a patch for gradients case.

Apply repeat size to gradient function. There is a resize problem. After resizing browser, the original area doesn't update and the new area is blur.

You may need to make a change to nsStyleImageLayers::Layer::RenderingMightDependOnPositioningAreaSizeChange.

(In reply to Markus Stange [:mstange] from comment #10) > You may need to make a change to > nsStyleImageLayers::Layer::RenderingMightDependOnPositioningAreaSizeChange. Thanks! That's what I am looking for.

Send the repeatSize to PaintGradient for space property. I think I should also handle eStyleImageType_Element in another bug. There is another problem shown in testcase2.html while the gradient background-size is "33.33% auto". We seem to take it as "33.33% 100%" while chrome and edge take it as "33.33% 33.33%". I haven't checked the spec. I can open another bug for it if necessary.

I append gradient tests in the original test. Basically the tests are similar, other than I need to define pixel number for gradient but not "auto" in background-repeat-round-2.html and background-repeat-round-3.html. And I use smaller area to test in background-repeat-space-8-ref.html since I don't know how to create the same gradient image without "space".

@Ethan Lin I think you can open another bug width background-size(The height is scaled proportionally to keep the original aspect ratio).

Comment on attachment 8757767 [details] [diff] [review] Part1. Apply repeat size to graident Review of attachment 8757767 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSRendering.cpp @@ +2829,5 @@ > gfxMatrix ctm = ctx->CurrentMatrix(); > bool isCTMPreservingAxisAlignedRectangles = ctm.PreservesAxisAlignedRectangles(); > > // xStart/yStart are the top-left corner of the top-left tile. > + nscoord xRepeatSize = aRepeatSize.width ? aRepeatSize.width : aDest.width; When would aRepeatSize be 0? @@ +2843,3 @@ > // The coordinates of the tile > gfxRect tileRect = nsLayoutUtils::RectToGfxRect( > nsRect(x, y, aDest.width, aDest.height), Shouldn't aDest.width and aDest.height be replaced with xRepeatSize and yRepeatSize here too?

(In reply to Markus Stange [:mstange] from comment #15) > Comment on attachment 8757767 [details] [diff] [review] > Part1. Apply repeat size to graident > > Review of attachment 8757767 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/nsCSSRendering.cpp > @@ +2829,5 @@ > > gfxMatrix ctm = ctx->CurrentMatrix(); > > bool isCTMPreservingAxisAlignedRectangles = ctm.PreservesAxisAlignedRectangles(); > > > > // xStart/yStart are the top-left corner of the top-left tile. > > + nscoord xRepeatSize = aRepeatSize.width ? aRepeatSize.width : aDest.width; > > When would aRepeatSize be 0? The is a rendering path from DrawBorderImage (DrawBorderImage -> DrawBorderImageComponent -> Draw -> PaintGradient). I think this is a bug. I should pass the destTile size as the repeat size here[1]. [1] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp#5545 > > @@ +2843,3 @@ > > // The coordinates of the tile > > gfxRect tileRect = nsLayoutUtils::RectToGfxRect( > > nsRect(x, y, aDest.width, aDest.height), > > Shouldn't aDest.width and aDest.height be replaced with xRepeatSize and > yRepeatSize here too? Maybe not, the RepeatSize includes the gap size. I think tileRect should be the rendering size. If I use RepeatSize here, the gradient will fill the background position.

Fix the repeat size from boarder image.

Is this ready to land?

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #18) > Is this ready to land? Yes, I'll upload a rebased patch and try server result.

Rebase to master.

try result: https://bugzilla.mozilla.org/show_bug.cgi?id=1275957

> Attachment #8759051 [details] [diff] - Attachment description: Part2. Add test cases for graident with background-repeat: round/space → Part2. Add test cases for graident with background-repeat: round/space (carry r+: mstange) s/graident/gradient/ Sebastian

> Part1. Apply repeat size to graident Same here. s/graident/gradient/ Sebastian

Fix description.

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f795d90a2deb Part 1. Apply space property to gradient. r=mstange https://hg.mozilla.org/integration/mozilla-inbound/rev/f4fd6197c80a Part 2. Add reftests for gradient image's background-repeat: round/space. r=mstange

https://hg.mozilla.org/mozilla-central/rev/f795d90a2deb https://hg.mozilla.org/mozilla-central/rev/f4fd6197c80a

(In reply to Ethan Lin[:ethlin] (pto until 6/10) from comment #12) > There is another problem shown in testcase2.html while the gradient > background-size is "33.33% auto". We seem to take it as "33.33% 100%" while > chrome and edge take it as "33.33% 33.33%". I haven't checked the spec. I > can open another bug for it if necessary. (In reply to percyley from comment #14) > @Ethan Lin I think you can open another bug width background-size(The height > is scaled proportionally to keep the original aspect ratio). This was not discussed anymore, so I had a look at the specifications. "A gradient is drawn into a box with the dimensions of the concrete object size, referred to as the gradient box. However, *the gradient itself has no intrinsic dimensions*." And the CSS Backgrounds and Borders Module says[2] this: "An ‘auto’ value for one dimension is resolved by using the image's intrinsic ratio and the size of the other dimension, or failing that, using the image's intrinsic size, or failing that, *treating it as 100%*." So, I think the patch is correct here regarding the spec. I.e. the height of the gradients in the second and third example is treated as 100% of the background positioning area, and by that Chrome and Edge (and also Opera and IE; can't test Safari) are wrong. Is my interpretation correct? Regarding the documentation, I've removed this bug from the Firefox compatibility note here: https://developer.mozilla.org/en-US/docs/Web/CSS/background-repeat Sebastian [1] https://drafts.csswg.org/css-images/#gradients [2] https://drafts.csswg.org/css-backgrounds-3/#the-background-size

Can we take this discussion to bug 1277746?

(In reply to David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) (vacation June 4-12) from comment #28) > Can we take this discussion to bug 1277746? Sure. Just missed that bug when writing my previous comment. Marking this as dev-doc-complete, then. Sebastian