907079 - The forward button is displayed after opening and closing the reader mode settings menu

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, defect, P5)

Description

[Adrian Tamas (:AdrianT)]

Samsung Galaxy Tab 2 (Android 4.1) / Asus Transformer TF101 (Android 4.0.4)
Nightly 26.0a1 2013-08-19 / Firefox Mobile 24 beta 4

Steps to reproduce:
1) Open any page in reader mode
2) From the reader toolbar open and close the reader settings menu ("Aa")

Expected results:
The menu is opened and closed but this does not affect the browser

Actual results:
The forward button is displayed although the current page is the last one in the tab history

Notes:
It seems that every time the menu is closed a new reader page is created with the selected font. Changing between fonts a few times will add multiple instances of the reader page in the tab history

Comment 1

[Aaron Train [:aaronmt]]

Which page were you testing with so we can take a look?

Comment 2

[:Margaret Leibovic]

Is this just a tablet issue, or does it happen on phones as well? Meaning, is the forward button in the menu enabled when you wouldn't expect it to be?

Comment 3

[Adrian Tamas (:AdrianT)]

I managed to reproduce this on the Asus Transformer TF101 (Android 4.0.4), LG Nexus 4 (Android 4.1.1) and the Samsung Galaxy R (Android 2.3.4) so it's not a tablet only issue. The page I used was the mozilla page from wikipedia.org but it is the same with any page. Please see the videocapture: http://youtu.be/ulV2erG8sxE

I don't know the reader code but maybe this line is the cause of this if I understood the code correctly: http://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#l776

Comment 4

[:Margaret Leibovic]

Not actively working on this, so unassinging.

I did start looking into this at one point, and it seemed tricky, since we're using history.pushState to make the back button close the menu, rather than leave the page. Fixing this will involve figuring out if there's a way to remove the fake history entry we pushed onto the stack when the user hits the back button, so that there's nowhere to navigate forward in the session.

I would recommend this bug for somebody who's comfortable with JS and DOM APIs.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 6

[:Margaret Leibovic]

This also affects desktop reader mode, so I'm just going to fix it.

Comment 7

[:Margaret Leibovic]

Attachment: MozReview Request: bz://907079/margaret

/r/3093 - Bug 907079 - Don't add unnecessary history state for reader mode style dropdown. r=mfinkle

Pull down this commit:

hg pull review -r 1124ae5c7e10e6d091c678b5be83b3561ef96310

Comment 8

[:Margaret Leibovic]

I took this opportunity to do some cleanup, mainly to make it easier to tell what's going on in here.

Since we only have one "dropdown" in the reader mode UI, I decided to replace the generic dropdown support with some more explicit logic.

I also found that using window.history.pushState/replaceState directly worked just fine, so we don't need that script-injection hack anymore (I'm not sure what would have fixed this, but the bug that comment points to is pretty old, and our code has also changed a lot in recent months).

With this patch, the forward button will never become active unless the user hits the back button themselves. I couldn't find any API to remove session history entries, so I think we need to live with this (and I think that's fine, the forward button becoming active makes a lot more sense if you've hit the back button first).

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8555926 [details]
MozReview Request: bz://907079/margaret

https://reviewboard.mozilla.org/r/3091/#review2465

Loks good to me. I'm glad we are removing the workaround. We can always add more general dropdown support back, if needed.

Consider the constant nit.

::: toolkit/components/reader/AboutReader.jsm
(Diff revision 1)
> +      win.history.replaceState({ dropdown: 0 }, "");

Could we use DROPDOWN_OPEN and DROPDOWN_CLOSED constants instead of the raw 1 and 0?

Comment 10

[:Margaret Leibovic]

After testing some more, I found this doesn't actually work in all cases :(

I've been banging my head against this for a while this morning, and I'm starting to be convinced that what we want to do here is just not possible with the history API. It seems like no matter what way I do things, it's always possible to end up in a situation where you either need an extra back press to go back to the previous page.

What we're doing is basically just a hack on top of the history API, since we don't actually want these things to appear in the user's history, but rather just intercept the back button click.

Question: Do we really need the back button to dismiss the popup? Currently you can tap on the toolbar to toggle the popup open/closed, and if that's enough, it would make things way simpler to just get rid of these history hacks.

I suppose the only unfortunate thing would be if the user hits the back button thinking it will dismiss the popup, but then that takes them back to the previous page. But that's recoverable, and that's a behavior that would happen on any website as well.

Comment 11

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to :Margaret Leibovic from comment #10)
> Question: Do we really need the back button to dismiss the popup? Currently
> you can tap on the toolbar to toggle the popup open/closed, and if that's
> enough, it would make things way simpler to just get rid of these history
> hacks.

Are you referring to the hardware back button? As in, do we need it to dismiss the pop-up controls?

> I suppose the only unfortunate thing would be if the user hits the back
> button thinking it will dismiss the popup, but then that takes them back to
> the previous page. But that's recoverable, and that's a behavior that would
> happen on any website as well.

Yeah, I think the original issue of this bug is what needs to be solved as well. I don't have my tablet on me right now, but I assume this means that it would trigger the -> button there too (in the toolbar).

What would make this MORE recoverable is if the page didn't "reload" every time the user comes out of Reader mode. This would be consistent with the work we're doing in bug 1114708 to improve the experience of going in and out of Reader mode.

The excessive waiting (when coming out of Reader mode) also makes me wonder if we could benefit from a system toast with "undo" here... But I think that's getting out of scope of this bug, heh.

Thoughts?

Comment 12

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #11)
> (In reply to :Margaret Leibovic from comment #10)
> > Question: Do we really need the back button to dismiss the popup? Currently
> > you can tap on the toolbar to toggle the popup open/closed, and if that's
> > enough, it would make things way simpler to just get rid of these history
> > hacks.
> 
> Are you referring to the hardware back button? As in, do we need it to
> dismiss the pop-up controls?

Yes, the hardware back button, the back button we use in our tablet UI, and the back button in the menu all do the same thing.

For a bit more context here: we decided we wanted the hardware back button to dismiss this popup, but the hardware back button is already hooked up to page navigation, so we decided to hack the window.history API to add history a history entry for the popup showing. While this does result in the back button dismissing the popup, it adds a history entry, which results in the UI back button entering the active state if it wasn't before, and more visible, it results in the forward button becoming active (and showing on desktop/tablets) after you hit that back button. Currently hitting the forward button in this case does nothing, so this is actually broken no matter what way you slice it.

> > I suppose the only unfortunate thing would be if the user hits the back
> > button thinking it will dismiss the popup, but then that takes them back to
> > the previous page. But that's recoverable, and that's a behavior that would
> > happen on any website as well.
> 
> Yeah, I think the original issue of this bug is what needs to be solved as
> well. I don't have my tablet on me right now, but I assume this means that
> it would trigger the -> button there too (in the toolbar).
> 
> What would make this MORE recoverable is if the page didn't "reload" every
> time the user comes out of Reader mode. This would be consistent with the
> work we're doing in bug 1114708 to improve the experience of going in and
> out of Reader mode.
> 
> The excessive waiting (when coming out of Reader mode) also makes me wonder
> if we could benefit from a system toast with "undo" here... But I think
> that's getting out of scope of this bug, heh.
> 
> Thoughts?

I don't think we can deal with the reload nature of coming in/out of reader mode in this bug. In theory, this shouldn't actually take too long if these pages are in your bf cache.

I would propose we just remove this back button behavior from the reader mode toolbar. You can tap on the toolbar button to toggle the popup, and I think that's a well-understood interaction. This is also consistent with what would happen if you were on a webpage.

I could add a telemetry probe to see how often users toggle this popup vs hit back, but I'm not sure it's worth the effort.

Comment 13

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to :Margaret Leibovic from comment #12)
> I would propose we just remove this back button behavior from the reader
> mode toolbar. You can tap on the toolbar button to toggle the popup, and I
> think that's a well-understood interaction. This is also consistent with
> what would happen if you were on a webpage.
> 
> I could add a telemetry probe to see how often users toggle this popup vs
> hit back, but I'm not sure it's worth the effort.

Yeah, I agree. It would be nice if we could figure out a solution so we don't have to insert a fake history entry. Since, having the ability to dismiss the control panel using the hardware back is quite nice cause it keeps consistent with things like our overflow menu.

That being said, from a UX point of view, I'd like to have it. But in the interim if this really just isn't possible, we could remove the ability to dismiss by hitting hardware back. Maybe file a follow up to get it working without a fake history entry?

Comment 14

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #13)
> (In reply to :Margaret Leibovic from comment #12)
> > I would propose we just remove this back button behavior from the reader
> > mode toolbar. You can tap on the toolbar button to toggle the popup, and I
> > think that's a well-understood interaction. This is also consistent with
> > what would happen if you were on a webpage.
> > 
> > I could add a telemetry probe to see how often users toggle this popup vs
> > hit back, but I'm not sure it's worth the effort.
> 
> Yeah, I agree. It would be nice if we could figure out a solution so we
> don't have to insert a fake history entry. Since, having the ability to
> dismiss the control panel using the hardware back is quite nice cause it
> keeps consistent with things like our overflow menu.
> 
> That being said, from a UX point of view, I'd like to have it. But in the
> interim if this really just isn't possible, we could remove the ability to
> dismiss by hitting hardware back. Maybe file a follow up to get it working
> without a fake history entry?

Okay, I can remove the logic here, then file a follow-up to try to hook up the back button to the popup in Fennec. Since this is privileged code, we could always add some logic to give ourselves a hook into the back button, although I worry that might become messy and difficult to maintain.

Comment 15

[:Margaret Leibovic]

Comment on attachment 8555926 [details]
MozReview Request: bz://907079/margaret

/r/3093 - Bug 907079 - Simplify reader mode style popup logic, including removing the use of the window.history API. r=mfinkle

Pull down this commit:

hg pull review -r bd274498812fc342e8e74cf137ae322407aff586

Comment 16

[:Margaret Leibovic]

I updated my patch here to just remove this problematic logic, and I filed bug 1130646 to come up with a better solution.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8555926 [details]
MozReview Request: bz://907079/margaret

https://reviewboard.mozilla.org/r/3091/#review2799

Ship It!

Comment 18

[:Margaret Leibovic]

https://reviewboard.mozilla.org/r/3091/#review2801

::: toolkit/components/reader/AboutReader.jsm
(Diff revision 2)
> -      win.history.back();

I realized I actually can't just get rid of this bit, I'll have to replace it with a call to hide the dropdown. I'll do that before landing this.

Comment 19

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/5bdff31fc6bd

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5bdff31fc6bd

Comment 21

[:Margaret Leibovic]

Note to QE: it's not possible to verify this on desktop yet, as I haven't landed the controls. But you can see a fix on Android and trust that it will also be fixed on desktop when the controls land :)

Comment 22

[Andrei Vaida [:avaida]]

I can confirm that this is verified fixed on desktop Nightly 39.0a1 (2015-03-08) and Aurora 38.0a2 (2015-03-08) as well, using Windows 7 (x64), Ubuntu 14.04 (x86) and Mac OS X 10.9.5.

Comment 24

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 907079 - Simplify reader mode style popup logic, including removing the use of the window.history API. r=mfinkle

Comment 25

[Chris Peterson [:cpeterson]]

Clearing the qe-verify? flag on this old flag. The flag should have been set to qe-verify+, but this is an old bug that doesn't need to be verified now.