Closed Bug 1067191 Opened 6 years ago Closed 6 years ago
background-repeat not rendered correctly with page zoomed
12.00 KB, image/png
348 bytes, text/html
2.97 KB, patch
|Details | Diff | Splinter Review|
24.53 KB, patch
|Details | Diff | Splinter Review|
3.76 MB, video/mp4
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.
This must have regressed recently, maybe 4 weeks top.
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
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.
I'll take a look. Here's a smaller testcase.
Assignee: nobody → seth
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.)
(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.
Just ran the try build, and the result on the two test cases look fine to me on all zoom levels.
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.
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.
Attachment #8494181 - Attachment is obsolete: true
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
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.
Here's an updated version of the test. Same thing, just fuzzier.
Attachment #8494939 - Flags: review?(roc)
Here's a new try job that retests those orange reftests. Should be all green now. https://tbpl.mozilla.org/?tree=Try&rev=f16598aabaa1
Attachment #8494881 - Flags: review?(roc) → review+
Attachment #8494939 - Flags: review?(roc) → review+
(This should be uplifted once it lands on central, but I'll hold off on requesting until then.)
Seth - This bug has now been on m-c for a while. Can you please request uplift?
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?
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.
(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.
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
You need to log in before you can comment on or make changes to this bug.