1275966 - background-repeat:space doesn't fit an image that fits exactly

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

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

background-repeat: space doesn't fit an image that fits exactly.

In the attached testcase, you should see the image tiled 4 times horizontally (with spacing) and 2 times vertically (without spacing).

Firefox incorrectly shows only 1 vertical tile.  Chromium and Edge get it correct.

Comment 1

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

Attachment: testcase

Comment 2

[Ethan Lin[:ethlin]]

Attachment: Part1. Fix space checking.

I will write a test case for it.

Comment 3

[Ethan Lin[:ethlin]]

Attachment: fix sapce checking

Comment 4

[Markus Stange [:mstange]]

I'd like the repeat size to keep its meaning. You could add a bool outparam to ComputeSpacedRepeatSize that determines whether we should repeat in that dimension, and then you set the ExtendMode to the right value based on that, and also check the extend mode in nsLayoutUtils::DrawBackgroundImage.

Comment 5

[Ethan Lin[:ethlin]]

(In reply to Markus Stange [:mstange] from comment #4)
> I'd like the repeat size to keep its meaning. You could add a bool outparam
> to ComputeSpacedRepeatSize that determines whether we should repeat in that
> dimension, and then you set the ExtendMode to the right value based on that,
> and also check the extend mode in nsLayoutUtils::DrawBackgroundImage.

Okay, that wip is just one possible way to fix it. I will add a bool outparam for that.

Comment 6

[Ethan Lin[:ethlin]]

Attachment: Part1. fix repeat check

Add a bool parameter to determine whether the image is repeated.

Comment 7

[Ethan Lin[:ethlin]]

Attachment: Part2. Add reftests.

I added two test cases to check if space works fine while the image x or y fits exactly.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8757632 [details] [diff] [review]
Part2. Add reftests.

Review of attachment 8757632 [details] [diff] [review]:
-----------------------------------------------------------------

Did you forget to refresh this patch before you attached it? The tests don't look like they're testing this bug, and I think both reftests are the same.

Comment 9

[Ethan Lin[:ethlin]]

(In reply to Markus Stange [:mstange] from comment #8)
> Comment on attachment 8757632 [details] [diff] [review]
> Part2. Add reftests.
> 
> Review of attachment 8757632 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Did you forget to refresh this patch before you attached it? The tests don't
> look like they're testing this bug, and I think both reftests are the same.

I think the patch should be correct. For "background-repeat-space-9-ref.html", the background position size is 106x96, and the image is 32x32, so the image fix exactly on Y direction. And "background-repeat-space-10-ref.html" size is 96x106, which is for testing X direction. I will upload the snapshot of the test.

Comment 10

[Markus Stange [:mstange]]

Oh, nevermind then.

Comment 11

[Ethan Lin[:ethlin]]

Attachment: result.png

This is the before/after result of background-repeat-space-10-ref.html.

Comment 12

[Ethan Lin[:ethlin]]

try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8179e3d5306&selectedJob=21699567

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25fb10028068
Part 1. Fix wrong rendering of background-repeat:space. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/005a02a60fbd
Part 2. Add reftests for background-repeat: space. r=mstange

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/25fb10028068
https://hg.mozilla.org/mozilla-central/rev/005a02a60fbd

Comment 15

[Sebastian Zartner [:sebo]]

I've removed the note about this bug from https://developer.mozilla.org/en-US/docs/Web/CSS/background-repeat.

Sebastian