STR: 1) Open https://design.firefox.com/product-identity/firefox/firefox-stacked-lockup/firefox-logo-stacked-lockup.png 2) Zoom in 3) Scroll to bottom AR: Scroll resets to top-left when reaching bottom of image ER: Scroll reaches bottom and stays there It also happens if one scrolls right a bit, then down and back up. It might be the top bar reappearing which triggers it. Regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1bda868b03a366e0d7122a053477e2090bc789cf&tochange=61b5b9cf4cedb0fcc64003745dd097ad9d502e62 Eugen Sawin — Bug 1435290 - [1.0] Enable automatic image resizing for Fennec and custom tabs. r=kats
Randall, looks like the dynamic toolbar misses the scroll events. Any ideas?
Flags: needinfo?(esawin) → needinfo?(rbarker)
I don't think it is missing scroll events. I would guess that the new code that auto-resizes the image is some how no longer updating the toolbar frame metrics so that when we pinch zoom, the frame metrics that the toolbar is using are stale. I would look at how browser.enable_automatic_image_resizing is resizing the image. I hope it isn't bypassing APZ? Botond might know more.
Eugen, what are the next steps? Should we be backing out bug 1435290, or do we think we can fix this?
tracking-fennec: --- → ?
The issue is caused by the scroll position reset in ImageDocument::ShrinkToFit, which is applied unconditionally. We can fix this by verifying whether an image resize is actually required before applying it.
Assignee: nobody → esawin
Attachment #8950958 - Flags: review?(bzbarsky)
Comment on attachment 8950958 [details] [diff] [review] 0001-Bug-1437930-1.0-Avoid-redundant-image-shrinking-and-.patch Seems ok, but why is this not a problem on Firefox desktop?
Attachment #8950958 - Flags: review?(bzbarsky) → review+
> Seems ok, but why is this not a problem on Firefox desktop? The resize event chain is triggered by the dynamic toolbar. Generally, on desktop we return early at  when zoomed in. However, pinch-zooming on Fennec does not affect the page zoom level. Alternatively, we could extend the check  to something sensible for Fennec, but I'm not sure if that code path (adding "overflowVertical" to the class list) makes sense for the mobile case.  https://searchfox.org/mozilla-central/source/dom/html/ImageDocument.cpp#344
> However, pinch-zooming on Fennec does not affect the page zoom level. Aha. That explains things, thank you. I agree that if pinch-zooming is not supposed to affect page zoom, taking that path for it would be weird, since it would change how the letterboxing works which is not desired here.
tracking-fennec: ? → +
Priority: -- → P1
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/4eac0c22e526 [1.0] Avoid redundant image shrinking and scroll position resetting. r=bz
Unfortunately this doesn't seem to be fully fixed. Testing with the 2018-02-17 https://hg.mozilla.org/mozilla-central/rev/dde7eb1a589f nightly, it is indeed no longer reproducible on the example image from comment #0... in the portrait orientation. In landscape orientation I still see it. The obvious difference being whether the image is width-bound or height-bound when shrunk (Ι originally saw the problem on a vertically-long webcomic, so was initially confused when it was fixed on the example image but still reproducible where I first saw it).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Right, I shouldn't have missed that case. What we need to do here is to add an Android-specific check for pinch-zooming. Jan, I think you've refactored some zoom mechanics, do you know whether we can expose something through, e.g., nsPresContext that reveals the active zoom/resolution of the document as controlled through pinch-zooming?
Flags: needinfo?(esawin) → needinfo?(jh+bugzilla)
This is what the session store uses to store the current zoom level for later restoring: https://dxr.mozilla.org/mozilla-central/rev/ad133cd410a719c0b67e61b8d3b1c77a32fd80a9/mobile/android/components/SessionStore.js#936 Does that help you in some way?
Pinch-zooming only affects the resolution and no zoom levels. With this we mimic the zoom level check on desktop using the resolution on Android - if-def'ed to prevent regressions on other platforms. The previous patch doesn't work for height-bound image documents because of the vertical viewport dimension changes through the dynamic toolbar.
Attachment #8950958 - Attachment is obsolete: true
Comment on attachment 8952720 [details] [diff] [review] 0001-Bug-1437930-2.0-Don-t-shrink-image-document-when-res.patch See comment 13.
Comment on attachment 8952720 [details] [diff] [review] 0001-Bug-1437930-2.0-Don-t-shrink-image-document-when-res.patch Review of attachment 8952720 [details] [diff] [review]: ----------------------------------------------------------------- I'm not familiar with this code. Maybe qDot knows something about it if bz isn't available for a re-review?
Comment on attachment 8952720 [details] [diff] [review] 0001-Bug-1437930-2.0-Don-t-shrink-image-document-when-res.patch bz's review queue is blocked atm., qDot, is that something you could review? See comment 13.
Attachment #8952720 - Flags: review?(kyle)
Comment on attachment 8952720 [details] [diff] [review] 0001-Bug-1437930-2.0-Don-t-shrink-image-document-when-res.patch Review of attachment 8952720 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. I'm not horribly familiar with this code, but reading through the comments and tracing the event/code, the logic seems good.
Attachment #8952720 - Flags: review?(kyle) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7aa0c88ffbb0 [2.0] Don't shrink image document when resolution has changed, e.g., through pinch-zooming on Android. r=qdot
Status: REOPENED → RESOLVED
Closed: 2 years ago → 2 years ago
Resolution: --- → FIXED
Verified as fixed in build 60.0b3. Device Google Pixel(Android 8.1.0).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.