Closed Bug 1437930 Opened 2 years ago Closed 2 years ago

Scrolling on images broken since bug 1435290

Categories

(Firefox for Android :: General, defect, P1)

Unspecified
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 60
Tracking Status
fennec + ---
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- verified

People

(Reporter: Kwan, Assigned: esawin)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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
Flags: needinfo?(esawin)
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.
Flags: needinfo?(rbarker)
Eugen, what are the next steps?  Should we be backing out bug 1435290, or do we think we can fix this?
Flags: needinfo?(esawin)
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
Flags: needinfo?(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+
Flags: needinfo?(esawin)
> 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 [1] when zoomed in. However, pinch-zooming on Fennec does not affect the page zoom level.

Alternatively, we could extend the check [1] 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.

[1] https://searchfox.org/mozilla-central/source/dom/html/ImageDocument.cpp#344
Flags: needinfo?(esawin)
> 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 esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4eac0c22e526
[1.0] Avoid redundant image shrinking and scroll position resetting. r=bz
https://hg.mozilla.org/mozilla-central/rev/4eac0c22e526
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
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
Flags: needinfo?(esawin)
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?
Flags: needinfo?(jh+bugzilla)
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.
Attachment #8952720 - Flags: review?(continuation)
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?
Attachment #8952720 - Flags: review?(continuation)
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 esawin@mozilla.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
https://hg.mozilla.org/mozilla-central/rev/7aa0c88ffbb0
Status: REOPENED → RESOLVED
Closed: 2 years ago2 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.