788526 - Reader Mode toolbar is hardly triggered

Status: VERIFIED FIXED

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

Description

[Cristian Nicolae (:xti)]

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.

Comment 1

[Aaron Train [:aaronmt]]

I can reproduce only after rotation and only if the toolbar is visible prior.

Comment 2

[Lucas Rocha (:lucasr)]

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?

Comment 3

[Lucas Rocha (:lucasr)]

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.

Comment 4

[Lucas Rocha (:lucasr)]

Kats, any ideas?

Comment 5

[Chris Lord [:cwiiis]]

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.

Comment 6

[Lucas Rocha (:lucasr)]

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

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: potential fix

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

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: Fixed patch

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.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Sounds good. Cwiiis is probably the best person to review this.

Comment 10

[Chris Lord [:cwiiis]]

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?

Comment 11

[Kartikaya Gupta (email:[e-mail])]

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 12

[Chris Lord [:cwiiis]]

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.

Comment 13

[Aaron Train [:aaronmt]]

Land it?
Uplift it?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/07f04ff0b782

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/07f04ff0b782

Comment 16

[Cristian Nicolae (:xti)]

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 17

[Alex Keybl [:akeybl]]

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.