1437930 - Scrolling on images broken since bug 1435290

Status: VERIFIED FIXED

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

Description

[Ian Moody [:Kwan] (UTC+1)]

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

Comment 1

[Eugen Sawin [:esawin]]

Randall, looks like the dynamic toolbar misses the scroll events. Any ideas?

Comment 2

[Randall Barker [: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.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Eugen, what are the next steps?  Should we be backing out bug 1435290, or do we think we can fix this?

Comment 4

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1437930-1.0-Avoid-redundant-image-shrinking-and-.patch

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.

Comment 5

[Boris Zbarsky [: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?

Comment 6

[Eugen Sawin [: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

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4eac0c22e526
[1.0] Avoid redundant image shrinking and scroll position resetting. r=bz

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/4eac0c22e526

Comment 10

[Ian Moody [:Kwan] (UTC+1)]

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).

Comment 11

[Eugen Sawin [:esawin]]

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?

Comment 12

[Jan Henning [:JanH]]

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?

Comment 13

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1437930-2.0-Don-t-shrink-image-document-when-res.patch

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.

Comment 14

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1437930-2.0-Don-t-shrink-image-document-when-res.patch

Comment 15

[Eugen Sawin [:esawin]]

Comment on attachment 8952720 [details] [diff] [review]
0001-Bug-1437930-2.0-Don-t-shrink-image-document-when-res.patch

See comment 13.

Comment 16

[Andrew McCreight [:mccr8]]

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 17

[Eugen Sawin [:esawin]]

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.

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7aa0c88ffbb0

Comment 21

[Sorina Florean [:sflorean]]

Verified as fixed in build 60.0b3.
Device Google Pixel(Android 8.1.0).