Closed Bug 788526 Opened 8 years ago Closed 8 years ago

Reader Mode toolbar is hardly triggered

Categories

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

16 Branch
ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 18
Tracking Status
firefox16 --- affected
firefox17 --- affected
firefox18 --- verified

People

(Reporter: xti, Assigned: lucasr)

Details

Attachments

(1 file, 1 obsolete file)

Firefox 16.0b2 (2012-09-04)
Device: Galaxy Nexus
OS: Android 4.1.1

Steps to reproduce:
1. Open Fennec
2. Go to news.google.com and open any news from the list
3. Tap on Reader Mode icon from URL Bar
4. Scroll down the article
5. Tap to invoke the Reader Mode toolbar
6. Rotate the device to landscape and repeat step 5

Expected result:
Reader Mode toolbar is triggered instantly after the tap was performed after step 5 and 6.

Actual result:
Reader Mode toolbar is triggered just after a couple of taps in portrait and even several taps in landscape as you can see here: http://youtu.be/QkdXmtQO1u4?hd=1

Note:
On Galaxy Note (4.0.4), after the Reader Mode toolbar is dismissed it cannot be triggered again, unless you exit and go back to Reader Mode.
Severity: normal → major
I can reproduce only after rotation and only if the toolbar is visible prior.
I noticed that scrolling fixes the problem (i.e. makes the toolbar reappear). This really seems to be a bug on position:fixed elements when we rotate the device. I suspect that the toolbar is being moved offscreen when the window size changes.

I also noticed that the problem is slightly different between Nightly, Aurora, and Beta. My impression is that it's slightly harder to reproduce this in Aurora. Aaron, Cristian, what do you think?

Chris, any clue about the cause of this bug?
Priority: -- → P1
Just confirmed that when the toolbar is placed on top, it's still visible after rotating the device but it keeps the width from the previous orientation.

I noticed that setScrollPositionClampingScrollPortSize() is being called on rotation. Only when you scroll.
Component: Reader Mode → Graphics, Panning and Zooming
Component: Graphics, Panning and Zooming → General
Kats, any ideas?
Just to clarify, any time the window size changes, setScrollPositionClampingScrollPortSize *needs* to be called - preferably in sync with the resize, as it'll cause a reflow of any position:fixed elements.

There's code in browser.js to do this, but I assume that something has broken it recently/semi-recently.
(In reply to Lucas Rocha (:lucasr) from comment #3)
> I noticed that setScrollPositionClampingScrollPortSize() is being called on
> rotation. Only when you scroll.

Err... I meant *not* being called.
Component: General → Graphics, Panning and Zooming
Attached patch potential fix (obsolete) — Splinter Review
Yeah, I agree. I think we need to be calling setScrollPositionClampingScrollPortSize from the updateViewportSize code as well.

:lucasr, could you try the following patch and see if it fixes the problem? (I haven't tried it at all, there may be syntax problems...)
Attached patch Fixed patchSplinter Review
Kats, your patch seems to fix the problem. it had a small error in it (fixed in this new patch). I think this patch should be uplifted to Aurora and Beta btw.
Attachment #658929 - Attachment is obsolete: true
Sounds good. Cwiiis is probably the best person to review this.
Comment on attachment 658951 [details] [diff] [review]
Fixed patch

Review of attachment 658951 [details] [diff] [review]:
-----------------------------------------------------------------

This is an expensive function to call - Can you either test, or add code to setScrollClampingSize so that this only triggers when the scroll-port actually changes?
Attachment #658951 - Flags: feedback+
Why do you say it's an expensive function to call? It looks like the code in nsPresShell already guards against setting it to the same value - see http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#9039

And anyway, we were already calling it unconditionally in setViewport, which gets called often while panning around. The new call to the code should only be called when the viewport actually changes size (and maybe when meta-viewport tags get added/removed). It seems like the new calls to this should be insignificant compared to what we were already doing; if anything we should guard the existing call with a check to see if it changed.
Comment on attachment 658951 [details] [diff] [review]
Fixed patch

Review of attachment 658951 [details] [diff] [review]:
-----------------------------------------------------------------

kats is right, I didn't realise the check was done native-side... My mistake, this is fine.
Attachment #658951 - Flags: review?(chrislord.net) → review+
Land it?
Uplift it?
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/07f04ff0b782
Assignee: nobody → lucasr.at.mozilla
Flags: in-testsuite-
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/07f04ff0b782
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Status: RESOLVED → VERIFIED
Attachment #658951 - Flags: approval-mozilla-beta?
Attachment #658951 - Flags: approval-mozilla-aurora?
Reader Mode still works slowly on the Galaxy Note. The toolbar is hardly to be triggered and the Layout Options button is not responsive. Sometimes it's needed one or two Firefox restarts until it's useable. 

On Nightly the Reader Mode works a little bit faster than on the latest Beta build.

--
Firefox 16.0b4 (2012-09-20)
Device: Galaxy Note
OS: Android 4.0.4
Comment on attachment 658951 [details] [diff] [review]
Fixed patch

This is missing a risk evaluation and does not appear to be critical for release. Please re-nominate for Aurora uplift (and Beta if you disagree) with a risk evaluation.
Attachment #658951 - Flags: approval-mozilla-beta?
Attachment #658951 - Flags: approval-mozilla-beta-
Attachment #658951 - Flags: approval-mozilla-aurora?
Attachment #658951 - Flags: approval-mozilla-aurora-
You need to log in before you can comment on or make changes to this bug.