1275957 - background-repeat: space doesn't work with gradients

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

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

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.

Comment 1

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

Attachment: testcase

Comment 2

[Markus Stange [:mstange]]

Sorry for missing this in my review.

Comment 3

[yisibl]

Attachment: testcase2.html

Comment 4

[yisibl]

In testcase2, the Edge rendering the effect is most in line with expectations.

Chrome and Safari have some problems more or less.

Comment 5

[yisibl]

Attachment: Edge.png

Comment 6

[yisibl]

Attachment: Chrome.png

Comment 7

[yisibl]

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 }

Comment 8

[Ethan Lin[:ethlin]]

I will have a patch for gradients case.

Comment 9

[Ethan Lin[:ethlin]]

Attachment: wip

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.

Comment 10

[Markus Stange [:mstange]]

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

Comment 11

[Ethan Lin[:ethlin]]

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

Comment 12

[Ethan Lin[:ethlin]]

Attachment: Part1. Apply repeat size to graident

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.

Comment 13

[Ethan Lin[:ethlin]]

Attachment: Part2. Add test cases for graident with background-repeat: round/space

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

Comment 14

[yisibl]

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

Comment 15

[Markus Stange [:mstange]]

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?

Comment 16

[Ethan Lin[:ethlin]]

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

Comment 17

[Ethan Lin[:ethlin]]

Attachment: Part1. Apply repeat size to graident

Fix the repeat size from boarder image.

Comment 18

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

Is this ready to land?

Comment 19

[Ethan Lin[:ethlin]]

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

Comment 20

[Ethan Lin[:ethlin]]

Attachment: Part2. Add test cases for gradient with background-repeat: round/space (carry r+: mstange)

Rebase to master.

Comment 21

[Ethan Lin[:ethlin]]

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

Comment 22

[Sebastian Zartner [:sebo]]

> 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

Comment 23

[Sebastian Zartner [:sebo]]

> Part1. Apply repeat size to graident

Same here. s/graident/gradient/

Sebastian

Comment 24

[Ethan Lin[:ethlin]]

Attachment: Part1. Apply repeat size to gradient (carry r+: mstange)

Fix description.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
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

Comment 26

[Carsten Book [:Tomcat]]

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

Comment 27

[Sebastian Zartner [:sebo]]

(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

Comment 28

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

Can we take this discussion to bug 1277746?

Comment 29

[Sebastian Zartner [:sebo]]

(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