Closed Bug 1275957 Opened 3 years ago Closed 3 years ago

background-repeat: space doesn't work with gradients

Categories

(Core :: Web Painting, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: dbaron, Assigned: ethlin)

References

Details

(Keywords: dev-doc-complete)

Attachments

(6 files, 4 obsolete files)

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.
Attached file testcase
Attachment #8756912 - Attachment mime type: text/plain → text/html
Flags: needinfo?(ethlin)
Sorry for missing this in my review.
Attached file testcase2.html
In testcase2, the Edge rendering the effect is most in line with expectations.

Chrome and Safari have some problems more or less.
Attached image Edge.png
Attached image Chrome.png
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.
Assignee: nobody → ethlin
Flags: needinfo?(ethlin)
Attached patch wip (obsolete) — Splinter Review
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.
Attachment #8757701 - Attachment is obsolete: true
Attachment #8757767 - Flags: review?(mstange)
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".
Attachment #8757768 - Flags: review?(mstange)
@Ethan Lin I think you can open another bug width background-size(The height is scaled proportionally to keep the original aspect ratio).
See Also: → 1276582
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?
Attachment #8757767 - Flags: review?(mstange)
Attachment #8757768 - Flags: review?(mstange) → review+
(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.
Attachment #8757767 - Attachment is obsolete: true
Attachment #8758100 - Flags: review?(mstange)
Attachment #8758100 - Flags: review?(mstange) → review+
Is this ready to land?
Flags: needinfo?(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.
Flags: needinfo?(ethlin)
Keywords: checkin-needed
Attachment #8759051 - 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)
> 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.
Attachment #8758100 - Attachment is obsolete: true
Attachment #8759051 - Attachment description: Part2. Add test cases for graident with background-repeat: round/space (carry r+: mstange) → Part2. Add test cases for gradient with background-repeat: round/space (carry r+: mstange)
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
Keywords: checkin-needed
See Also: → 1277746
https://hg.mozilla.org/mozilla-central/rev/f795d90a2deb
https://hg.mozilla.org/mozilla-central/rev/f4fd6197c80a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(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
Flags: needinfo?(dbaron)
Can we take this discussion to bug 1277746?
Flags: needinfo?(dbaron)
(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
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.