Closed
Bug 788526
Opened 13 years ago
Closed 13 years ago
Reader Mode toolbar is hardly triggered
Categories
(Firefox for Android Graveyard :: Toolbar, defect, P1)
Tracking
(firefox16 affected, firefox17 affected, firefox18 verified)
VERIFIED
FIXED
Firefox 18
People
(Reporter: xti, Assigned: lucasr)
Details
Attachments
(1 file, 1 obsolete file)
3.27 KB,
patch
|
cwiiis
:
review+
cwiiis
:
feedback+
akeybl
:
approval-mozilla-aurora-
akeybl
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•13 years ago
|
Severity: normal → major
Comment 1•13 years ago
|
||
I can reproduce only after rotation and only if the toolbar is visible prior.
Assignee | ||
Comment 2•13 years ago
|
||
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?
Assignee | ||
Updated•13 years ago
|
Keywords: regressionwindow-wanted
Priority: -- → P1
Assignee | ||
Comment 3•13 years ago
|
||
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
Assignee | ||
Updated•13 years ago
|
Component: Graphics, Panning and Zooming → General
Assignee | ||
Comment 4•13 years ago
|
||
Kats, any ideas?
Comment 5•13 years ago
|
||
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.
Assignee | ||
Comment 6•13 years ago
|
||
(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.
Assignee | ||
Updated•13 years ago
|
Component: General → Graphics, Panning and Zooming
Comment 7•13 years ago
|
||
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...)
Assignee | ||
Comment 8•13 years ago
|
||
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
Comment 9•13 years ago
|
||
Sounds good. Cwiiis is probably the best person to review this.
Updated•13 years ago
|
Attachment #658951 -
Flags: review?(chrislord.net)
Comment 10•13 years ago
|
||
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+
Comment 11•13 years ago
|
||
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•13 years ago
|
||
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+
Comment 14•13 years ago
|
||
Comment 15•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Updated•13 years ago
|
Updated•13 years ago
|
Keywords: regressionwindow-wanted
Reporter | ||
Updated•13 years ago
|
Attachment #658951 -
Flags: approval-mozilla-beta?
Attachment #658951 -
Flags: approval-mozilla-aurora?
Reporter | ||
Comment 16•13 years ago
|
||
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•13 years ago
|
||
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-
Updated•5 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•