872965 - Exit Reader Mode icon takes user back, but not necessarily to original article

Status: RESOLVED FIXED

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

Description

[Adrian Tamas (:AdrianT)]

Aurora 23.0a2 2013-05-15/ Nightly 24.0a1 2013-05-15
HTC Desire Z (Android 2.3.3) / Samsung Galaxy S2 (Android 4.0.3)

Steps to reproduce:
1) Add an item to the reading list
2) Open a new tab and load any page in it
3) Tap on the Awesomebar, go to the reading list and load the reading list entry created at step 1
4) Tap on the exit reader mode icon

Expected results:
At step 3 the Exit Reader Mode icon should not be displayed just like when opening the reading list entry in a new tab

Actual results:
At step 3 the Exit Reader Mode icon is displayed
At step 4 tapping on the icon has the effect of "Back" instead of opening the original article

Notes:
Either the icon should not be displayed at all for any item that has been opened from the reading list or should be present all the time and when the user taps it it should just strip the reader mode formating of the URL and load it in the tab - the reverse of the "open reader mode"

Comment 1

[:Margaret Leibovic]

I feel like we should always have the icon there as a way out, but I agree that it should just load the page in "normal mode" instead of reader mode. Ian, do you agree with this?

Comment 2

[:Margaret Leibovic]

I'm updating the summary to make it more clear what the issue is here. We are explicitly calling tab.doBack() when the exit reader mode icon is tapped:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserToolbar.java#325

Doing something different to figure out what URL we should go to kinda sucks because it wouldn't take advantage of the bfcache, but it would provide a more consistent experience.

Comment 3

[Ian Barlow (:ibarlow)]

(In reply to :Margaret Leibovic from comment #1)
> I feel like we should always have the icon there as a way out, but I agree
> that it should just load the page in "normal mode" instead of reader mode.
> Ian, do you agree with this?

Yes.

Comment 4

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

(In reply to :Margaret Leibovic from comment #2)
> I'm updating the summary to make it more clear what the issue is here. We
> are explicitly calling tab.doBack() when the exit reader mode icon is tapped:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> BrowserToolbar.java#325
> 
> Doing something different to figure out what URL we should go to kinda sucks
> because it wouldn't take advantage of the bfcache, but it would provide a
> more consistent experience.

Would this "consistent experience" work differently than the normal "BACK" buttons?

Comment 5

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> (In reply to :Margaret Leibovic from comment #2)
> > I'm updating the summary to make it more clear what the issue is here. We
> > are explicitly calling tab.doBack() when the exit reader mode icon is tapped:
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> > BrowserToolbar.java#325
> > 
> > Doing something different to figure out what URL we should go to kinda sucks
> > because it wouldn't take advantage of the bfcache, but it would provide a
> > more consistent experience.
> 
> Would this "consistent experience" work differently than the normal "BACK"
> buttons?

Well, yes. The "exit reader mode" icon would take you out of reader mode and into the normal web view for that article. The BACK button would just take you back wherever you came from.

But thinking about this bug more, I wonder if we should just hide "exit reader mode" icon if you didn't use that icon to get into reader mode in the first place. But users may be confused if this icon is only sometimes there. So maybe we should get rid of the icon altogether, since the BACK button will always do the same thing anyway. Ian, do you remember why this "exit reader mode" icon was added in the first place? This feels like a tricky issue to solve.

Comment 6

[Ian Barlow (:ibarlow)]

> But thinking about this bug more, I wonder if we should just hide "exit
> reader mode" icon if you didn't use that icon to get into reader mode in the
> first place. But users may be confused if this icon is only sometimes there.
> So maybe we should get rid of the icon altogether, since the BACK button
> will always do the same thing anyway. Ian, do you remember why this "exit
> reader mode" icon was added in the first place? This feels like a tricky
> issue to solve.

I don't think we should ever hide the icon, since it's not a substitute for "back", rather it's a way to toggle between our Reader view and the web. It shouldn't matter where you came in from, you should always be able to flip back and forth between the two views. 

There are cases where the back button and this icon will do the same thing, but that won't be the case in every scenario. For example, once we land the ability to navigate from one reading list article to the next without going back to your reading list, pressing "back" would presumably take you back an article, where the reader icon would still let you quickly flip back to the full website version of the article you are on.

Comment 7

[:Margaret Leibovic]

Attachment: Toggle reader mode when tapping the reader mode icon

Comment 8

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da52b9dee503

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Lucas Rocha (:lucasr)]

Margaret, ReaderModeUtils already had a getUrlFromAboutReader() to do exactly what implemented with the new getUrlForAboutReader(). Am I missing something?

Comment 11

[Lucas Rocha (:lucasr)]

Also: one problem I see with how this bug was fixed is that it will grow the session history indefinitely if you keep toggling (which was something we wanted to avoid iirc).

Comment 12

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #10)
> Margaret, ReaderModeUtils already had a getUrlFromAboutReader() to do
> exactly what implemented with the new getUrlForAboutReader(). Am I missing
> something?

No, I obviously missed something when writing this patch. I'll file a follow-up to fix that. Sorry about that, and hanks for pointing that out!

(In reply to Lucas Rocha (:lucasr) from comment #11)
> Also: one problem I see with how this bug was fixed is that it will grow the
> session history indefinitely if you keep toggling (which was something we
> wanted to avoid iirc).

I wonder if it would be worth the added complexity to see if we can go back in session history if the previous page matches the un-reader-moded URL. I can look into how we might do this. If not, I think that growing the session history is a cost worth paying for predictable behavior.