1573014 - The page zooms out when scrolling in Reader view

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect, P2)

Description

[u611676]

User Agent: Mozilla/5.0 (Android 9; Mobile; rv:68.0) Gecko/68.0 Firefox/68.0

Steps to reproduce:

1. Open this page http://zguide.zeromq.org/page:all#Chapter-Reliable-Request-Reply-Patterns
2. Tap the Reader View icon, after loading the page looks fine
3. Scroll the page, the zoom changes, it's too small and you can't pinch zoom to fix it

Here is a video of the bug
https://drive.google.com/file/d/1-g-6cU69VgtTqQ8OAhb8hZRJd6EvKaoj/view?usp=drivesdk

This happens only with some pages.

Actual results:

As I scroll, the zoom became too small

Expected results:

The zoom shouldn't bee recalculated on scroll

Comment 1

[Laurentiu Apahidean]

Hello,

I have managed to reproduce your issue on Release 68.0, Beta 68.1b5 and Nightly 70.0a1 (2019-08-12) using a Samsung Galaxy S8+ (Android 8.0.0), I will mark this issue as new.

Comment 2

[Botond Ballo [:botond]]

(In reply to Laurentiu Apahidean from comment #1)

I have managed to reproduce your issue on Release 68.0, Beta 68.1b5 and Nightly 70.0a1 (2019-08-12)

Laurentiu, could you clarify which app you are testing -- e.g. Fennec, GVE, Firefox Preview -- and where you are getting the "Nightly 70.0a1" build from?

Comment 3

[Botond Ballo [:botond]]

In any case, this is a regression compared to Fennec 66.

Comment 4

[Botond Ballo [:botond]]

Regressed by bug 1519013.

Comment 5

[Botond Ballo [:botond]]

What's strange is why the zoom only decreases after scrolling. Generally, the effect of bug 1519013 is to decrease it as soon as the page finishes loading.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Yeah, it's weird. Also I can see user-scalable=0 there so that bug 1519013 should affect on the site, the user-scaleble=0 is injected after loading I guess and it causes this issue? Anyway I will debug into detail.

Comment 7

[Jan Henning [:JanH]]

I haven't used reader mode all that much, plus I've also enabled the Fennec/Gecko option to force-enable user scaling, however I dimly seem to remember that the default reader view configuration is indeed to prevent user scaling, too.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

This is somehow caused by interactions with the AndroidDynamicToolbar. When the toolbar hides, the widget size is changed, then the problem happens. The problematic change in bug 1519013 is this line. I supposed that IsResolutionUpdatedByApz returns true in such cases, but it seems not?

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)

This is somehow caused by interactions with the AndroidDynamicToolbar. When the toolbar hides, the widget size is changed, then the problem happens. The problematic change in bug 1519013 is this line. I supposed that IsResolutionUpdatedByApz returns true in such cases, but it seems not?

I was confused by resolution change. Actually in such cases the resolution hasn't changed by APZ.

Comment 11

[Botond Ballo [:botond]]

Another affected site (from bug 1574198): https://depletionmode.com/uefi-boot.html

Comment 12

[Botond Ballo [:botond]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)

This is somehow caused by interactions with the AndroidDynamicToolbar.

So, this problem only affects Fennec, and not Firefox Preview?

Comment 13

[Laurentiu Apahidean]

(In reply to Botond Ballo [:botond] from comment #2)

Laurentiu, could you clarify which app you are testing -- e.g. Fennec, GVE, Firefox Preview -- and where you are getting the "Nightly 70.0a1" build from?

The application I used to test is Firefox/Fennec, the Nightly build was downloaded from https://treeherder.mozilla.org/#/jobs?repo=mozilla-central.
I have also tested the Issue on Firefox Preview 1.3.0-rc. 1 (Thursday 8/15) and the issue does not occur.

Comment 14

[Botond Ballo [:botond]]

Thanks for clarifying.

Hiro: note that, since the issue only occurs in Fennec, it's only worth considering a fix if it's something we'd be able to uplift to 68esr.

Comment 15

[Botond Ballo [:botond]]

Perhaps we should just not do ShrinkToDisplaySizeIfNeeded at all in Reader Mode? I guess Reader Mode has its own logic for picking a zoom level that makes the text readable, and we don't want to step on its toes.

Comment 16

[Botond Ballo [:botond]]

Attachment: Bug 1573014 - Early-exit ShrinkToDisplaySizeIfNeeded() in reader mode. r=hiro

Comment 17

[Botond Ballo [:botond]]

I don't have a better idea than comment 15, so I implemented it.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa0fc354ee09
Early-exit ShrinkToDisplaySizeIfNeeded() in reader mode. r=hiro

Comment 19

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/fa0fc354ee09

Comment 20

[Botond Ballo [:botond]]

Comment on attachment 9087217 [details]
Bug 1573014 - Early-exit ShrinkToDisplaySizeIfNeeded() in reader mode. r=hiro

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Makes some pages unreadable in Reader Mode on Fennec. Fennec releases are made from the ESR branch to an ESR uplift is needed to get this fix to users.
• User impact if declined: On some pages (pages with some wide lines of text that cannot be wrapped to a smaller width, such as a long URL), Reader Mode zooms out to a level where the text is too small to read, and the zoom cannot be increased.
• Fix Landed on Version: 70 (note: not uplifted to 69 because this only affects Fennec)
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a small fix that only affects Reader Mode.
• String or UUID changes made by this patch: none

Comment 21

[Andrei Jolțan [:Andi]]

Hi, I've used a Samsung Galaxy Note 9 (Android 9) & a Samsung Galaxy S9 (Android 8) to test the above mentioned issue and it is :

1. Still reproducible on Firefox esr68.1b8
2. Verified as fixed on 70.0a7 from 27/08

Comment 22

[Julien Cristau [:jcristau]]

Could this affect desktop in some way or is the code path mobile-only?

FWIW my plan for now is to take this next week, so we can get it to users of fennec nightly and beta, not release directly.

Comment 23

[Botond Ballo [:botond]]

(In reply to Julien Cristau [:jcristau] from comment #22)

Could this affect desktop in some way or is the code path mobile-only?

It's mobile-only (modulo desktop zooming which is not enabled, and will certainly not be enabled on ESR).

FWIW my plan for now is to take this next week, so we can get it to users of fennec nightly and beta, not release directly.

SGTM.

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 9087217 [details]
Bug 1573014 - Early-exit ShrinkToDisplaySizeIfNeeded() in reader mode. r=hiro

reader mode fix for fennec 68.2

Comment 25

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

https://hg.mozilla.org/releases/mozilla-esr68/rev/e0386b59767f

Comment 26

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

https://hg.mozilla.org/releases/mozilla-esr68/rev/044e3567889e

Comment 27

[Diana Rus]

Hi, verified as fixed on Firefox Beta 68.2b1 with:

• Google Pixel 3 XL (Android 9)
• Samsung Galaxy Note 9 (Android 8.1.0)
• Sony Xperia Z5 (Android 7.0)
• Samsung Galaxy J7(2016) (Android 6.0.1)