1520770 - "Aa" from reader mode is not displayed if we select an option

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect, P2)

Description

[Sorina Florean [:sflorean]]

Environment:
Device: Samsung Galaxy Note 8 (Android 8.0.0);
Build: Nightly 66.0a1 (2019-01-13);

Steps to reproduce:

1. Go to Wikipedia -> English, or another article with content;
2. Tap on the Reader Mode icon on URL Bar;
3. Hide toolbar;
4. Tap on "Aa" option and choose "+";

Expected result:
"Aa" options are displayed.

Actual result:
"Aa" options are not displayed.

Notes:
See the video: https://drive.google.com/open?id=1TUGJM_aP47oVyPdOl9DiIzk2sL4EmjuM

Comment 1

[Stefan Deiac]

Hello, I found a regression range for this bug.
Device: Samsung Galaxy S8 (Android 8.0).
-Last good revision: 4728285e13e47b9c6dfee5af289c32a6f350a9ae
-First bad revision: 045c6ce06b3d2251809d6b51dc780ba7d7d9e156
-Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4728285e13e47b9c6dfee5af289c32a6f350a9ae&tochange=045c6ce06b3d2251809d6b51dc780ba7d7d9e156

Apparently, it is not 100% reproducible. I was able to reproduce this issue only 3/5 attempts.

Comment 2

[Petru-Mugurel Lingurar [:petru]]

Initially hoped this was a Java dialog but it is not.
Looked over it's implementation but found nothing wrong in the aboutReader family of files.

I see though that there are other open tickets referring to issues with that dialog, like bug 1525805.

Hiro, can you also check this?

Comment 3

[Hiroyuki Ikezoe (:hiro)]

I can't play the video in comment 0, so it's just a guess, bug 1525805 didn't fix this issue? There is no suspicious commits in the regression range, I suppose it might be bug 1423013 which landed just before the reporter opened this bug. And if bug 1525805 didn't fix this problem, I think it's bug 1527561.

Comment 4

[Petru-Mugurel Lingurar [:petru]]

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

I can't play the video in comment 0, so it's just a guess, bug 1525805 didn't fix this issue? There is no suspicious commits in the regression range, I suppose it might be bug 1423013 which landed just before the reporter opened this bug. And if bug 1525805 didn't fix this problem, I think it's bug 1527561.

Same video should be available to anyone with the link - https://drive.google.com/file/d/1MeUgl-mAWLLQs8M0KWcPVDk5Pwv-iDJ4

Just tested on the latest central which contains the patches for the above closed tickets and with just the default settings the issue still reproduces.

Basically, after pressing the font buttons ("+", "-", specific font name) of the reader dialog it will immediately be hidden.
This happens when:

• _setFontSize executes containerClasses.remove("font-size" + this._fontSize) [1]
• _setFontType executes bodyClasses.remove(this._fontType) [2]

[1] https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/toolkit/components/reader/AboutReader.jsm#315
[2] https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/toolkit/components/reader/AboutReader.jsm#617

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

I see this in Firefox 65 though it seems harder to trigger. Rarely the reader view options will close when the user taps on a item in the options. In 67 when it is in this state the options close on just about every tap.

Comment 6

[Neha Kochar [:neha]]

Hiro, do you have further thoughts considering the additional info in comment 4 and 5?

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Ok, I could reproduce the issue on my phone and debug it a little bit. The unexpected close comes from a mozvisualscroll event. Ccing Jan Henning since he is an expert of visual viewport stuff. I guess changing font-size triggeres a reflow and then the unexpected mozvisualscroll event happens.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

If you guys need more help, please NI to me. I think what we need to do is to ignore mozvisualscroll events during reflows triggered by the font-size changes.

Comment 9

[Jan Henning [:JanH]]

Re comment# 5, scroll anchoring most likely made this worse. With scroll anchoring, we try to preserve the scroll position relative to the text when zooming, which means that almost every zoom action will effectively also scroll the page. So somehow suppressing the resulting scroll event would indeed seem the way to go here.

Comment 10

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1520770 - Ignore `mozvisualscroll` immediately after setFontSize/setFontType in AboutReader.jsm; r?JanH

Changing font size or font type in reader mode would trigger a text reflow.
This coupled with text anchoring means any such action would trigger a scroll
on the page which fires our listener for mozvisualscroll where we close all
dropdowns and hide other visual elements, basically taking users the option to
do consecutive font related changes.

To prevent the font options dissapearing we will ignore any mozvisualscroll
events immediately after (speculative value - 500 ms) any setFontSize() or
setFontType() calls.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2921d542d88
Ignore `mozvisualscroll` immediately after setFontSize/setFontType in AboutReader.jsm; r=Gijs

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 13

[Eliza Balazs]

Hi!

I tested this on the latest version of Nightly 68.0a1 (2019-04-07) with HTC 10 (Android 8.0.0), Samsung Galaxy Note 8 (Android 9), Nexus 9 (Android 7.1.1, Tablet) and I wasn't able to reproduce the issue.

Due to my findings, I'll mark this issue as verified.

Thanks.