background-repeat not rendered correctly with page zoomed

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
5 years ago
10 months ago

People

(Reporter: me, Assigned: seth)

Tracking

({regression})

34 Branch
mozilla35
All
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox32 unaffected, firefox33 unaffected, firefox34+ fixed, firefox35+ fixed, b2g-v2.1 verified, b2g-v2.2 verified)

Details

Attachments

(5 attachments, 2 obsolete attachments)

Reporter

Description

5 years ago
STR:

1) Go to https://wiki.mozilla.org/Main_Page
2) Zoom in more than 100%
3) Notice background on the tabs on top doesn't repeat on the whole element.
Reporter

Comment 1

5 years ago
This must have regressed recently, maybe 4 weeks top.
Reporter

Comment 2

5 years ago
Was able to narrow it down a bit to somewhere between beta and aurora. Only tested on OSX so far.
[Tracking Requested - why for this release]: serious Web functionality regression when page zoomed
Reporter

Comment 4

5 years ago
Couldn't reproduce this on Windows, so I guess this is OS X specific. I'm on Yosemite Beta.
Summary: Regression: background-repeat not rendered correctly with page zoomed → background-repeat not rendered correctly with page zoomed
Version: Trunk → 34 Branch
Last good revision: 3488976ecf0f
First bad revision: 3d51132a0099
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3488976ecf0f&tochange=3d51132a0099

Was able to reproduce on OS X 10.9.5.
Sounds like a regression from bug 1043560
Blocks: 1043560
Flags: needinfo?(seth)
Assignee

Comment 7

5 years ago
Posted file tiling-bug.html
I'll take a look. Here's a smaller testcase.
Assignee: nobody → seth
Flags: needinfo?(seth)
Assignee

Comment 8

5 years ago
OK, I think I've tracked down the problem. Bug 1043560 added code to ComputeSnappedImageDrawingParameters() that computed a snapped destination rect, which we use for image size calculations. It also used the snapped destination rect when computing the transformation between image space and device pixel space; previously we had used the unsnapped destination rect. The snapping seems to be problem; essentially we are rounding the scale component of the transform, which means that it doesn't always scale the image enough. This patch returns us to using the unsnapped destination rect, which seems to fix the problem.

Before requesting review, I'd like to see the result of this try job:

https://tbpl.mozilla.org/?tree=Try&rev=44804a4acb6a

(Also upcoming: a test. This is something we should've had a test for.)
Assignee

Comment 9

5 years ago
(In reply to Seth Fowler [:seth] from comment #8)
> essentially we are rounding the scale component of the transform,

What's actually happening is that we're rounding an input that we use to compute the scale component of the transform, in case that's not clear. It has the effect of making the scale component smaller than it should be when the destination rect wasn't pixel aligned.
Reporter

Comment 10

5 years ago
Just ran the try build, and the result on the two test cases look fine to me on all zoom levels.
Assignee

Comment 11

5 years ago
I'm glad that this patch fixes the problem, but unfortunately it needs more debugging before it can be used. That try run shows that it introduces some other failures.
Assignee

Comment 12

5 years ago
Alright, I investigated the failures and it looks like the problem is straightforward. The previous version of the patch led us to use an unsnapped destination rect when computing transforms. What it failed to do was scale that dest rect by the scale factors of the matrix on the gfxContext. After making that change, it looks like the failures in the previous try run are fixed.

I still want to see green try results before requesting review, though.
Assignee

Updated

5 years ago
Attachment #8494181 - Attachment is obsolete: true
Assignee

Comment 13

5 years ago
This patch adds a test that would have caught this failure. It checks the combination of tiling images that are one pixel wide or tall (which is not required to make the bug manifest, but makes it *much* easier to see) with reftest-zoom.

Here's a try job which includes part 1 and these tests:

https://tbpl.mozilla.org/?tree=Try&rev=510a4715ea25
Assignee

Updated

5 years ago
Blocks: 1068881
Assignee

Comment 14

5 years ago
So there's a lot of orange there, but it's all on the new test I added. Looks like it just needs some fuzz.

I'll upload a revised test patch once all of the oranges are in, but I think we're at the point where I can ask for review. Looks like the actual patch works just fine now.
Assignee

Updated

5 years ago
Attachment #8494881 - Flags: review?(roc)
Assignee

Updated

5 years ago
Attachment #8494882 - Flags: review?(roc)
Assignee

Comment 15

5 years ago
Here's an updated version of the test. Same thing, just fuzzier.
Attachment #8494939 - Flags: review?(roc)
Assignee

Updated

5 years ago
Attachment #8494882 - Attachment is obsolete: true
Attachment #8494882 - Flags: review?(roc)
Assignee

Comment 16

5 years ago
Here's a new try job that retests those orange reftests. Should be all green now.

https://tbpl.mozilla.org/?tree=Try&rev=f16598aabaa1
Assignee

Comment 18

5 years ago
(This should be uplifted once it lands on central, but I'll hold off on requesting until then.)
https://hg.mozilla.org/mozilla-central/rev/0ec600141872
https://hg.mozilla.org/mozilla-central/rev/c3eda3844838
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Seth - This bug has now been on m-c for a while. Can you please request uplift?
Flags: needinfo?(seth)
Assignee

Comment 21

5 years ago
Comment on attachment 8494881 [details] [diff] [review]
(Part 1) - Use an unsnapped destination rect when computing transforms for image drawing

Sure, let's get this uplifted. Looks like we need it for Aurora only.

Approval Request Comment
[Feature/regressing bug #]: 1043560
[User impact if declined]: Tiled images (such as CSS background images) could be rendered incorrectly when full-page zoom is used.
[Describe test coverage new/current, TBPL]: On m-c for a week, no issues.
[Risks and why]: Low risk. The patch just changes when we apply rounding when rendering images.
[String/UUID change made/needed]: None.
Attachment #8494881 - Flags: approval-mozilla-aurora?
Flags: needinfo?(seth)
Comment on attachment 8494881 [details] [diff] [review]
(Part 1) - Use an unsnapped destination rect when computing transforms for image drawing

Aurora+
Attachment #8494881 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/27ac8a26be7d
https://hg.mozilla.org/releases/mozilla-aurora/rev/da372ef9b8ef

Note that this had to be rebased slightly around bug 1064855. If you end up wanting to uplift that, some of the logic from this patch will need to be ported to that uplift as well.
Flags: in-testsuite+
Assignee

Comment 24

5 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #23)
> Note that this had to be rebased slightly around bug 1064855. If you end up
> wanting to uplift that, some of the logic from this patch will need to be
> ported to that uplift as well.

Thanks Ryan! I took a quick look at the rebased code and it looks fine. It might indeed be a good idea to uplift bug 1064855, though! I'll request it.
Depends on: 1080770
This issue has been verified successfully on Flame 2.1, 2.2.
See attachment: 1414.MP4
Reproducing rate: 0/5

Step:
1) Launch Browser, then go to https://wiki.mozilla.org/Main_Page
2) Zoom in more than 100%
3) Notice background on the tabs.

Actual result:
The background repeat on the whole element.

Flame 2.1 version:
Gaia-Rev        dbaf3e31c9ba9c3436e074381744f2971e15c7bf
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-2g34_v2_1/rev/ebce587d2194
Build-ID        20141203001205
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141203.034907
FW-Date         Wed Dec  3 03:49:18 EST 2014
Bootloader      L1TC00011880

Flame 2.2 version:
Gaia-Rev        725685831f5336cf007e36d9a812aad689604695
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/2c9781c3e9b5
Build-ID        20141203040207
Version         37.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141203.072513
FW-Date         Wed Dec  3 07:25:25 EST 2014
Bootloader      L1TC00011880

Updated

10 months ago
Product: Core → Core Graveyard

Updated

10 months ago
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.