1143388 - Loading an item from the readinglist sidebar shouldn't make the user leave the reader mode if it was active

Status: VERIFIED FIXED

(Firefox Graveyard :: Reading List, defect, P2)

Comment 1

[Florian Quèze [:florian]]

There doesn't seem to be consensus here. I thought we wanted to do this, but when I mentioned this, about have the people in the room here were surprised. If there was a decision to do this, do you remember who wanted it?

Comment 2

[Michael Maslaney [:mmaslaney] ([e-mail])]

Interactions Rules:

1.) From Default View: 
If a user clicks on a Reading List Item, then the new page will load in the Default View.

2.) From Reader View:
If a user clicks on a Reading List Item, then the new page will load in the Reader View. 

3.) From Reader View
If a user clicks on a link, then the new page will load in a new tab in default view.

Comment 3

[Florian Quèze [:florian]]

Changing the summary to match decision made in comment 2.

Comment 4

[Florian Quèze [:florian]]

Attachment: Patch

This doesn't implement point 3 from comment 2. I think we can either handle that part in a separate bug, or ignore it until we have a feel for whether it's actually an issue when using the UI.

Comment 5

[Marco Mucci [:MarcoM]]

Hi Florian, can you provide a point value.

Comment 6

[Justin Dolske [:Dolske]]

(In reply to Michael Maslaney [:mmaslaney] ([e-mail]) from comment #2)
> Interactions Rules:
> 
> 1.) From Default View: 
> If a user clicks on a Reading List Item, then the new page will load in the
> Default View.

Can you expand on the reasoning here? My understanding was that we ideally wanted to always load Reading List items in reading view, since the features are so closely specialized / related to each other. If the user just wanted the regular page, bookmarking the site would have sufficed.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8582525 [details] [diff] [review]
Patch

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

::: browser/base/content/content.js
@@ +518,5 @@
>          if (content.document.body) {
>            // Update the toolbar icon to show the "reader active" icon.
>            sendAsyncMessage("Reader:UpdateReaderButton");
>            new AboutReader(global, content, this._articlePromise);
> +          this._articlePromise = null;

Why is this change needed (on this bug)?

::: browser/components/readinglist/sidebar.js
@@ +303,5 @@
>                             .getInterface(Ci.nsIDOMWindow);
> +
> +    let currentUrl = mainWindow.gBrowser.currentURI.spec;
> +    if (currentUrl.startsWith("about:reader"))
> +      url = "about:reader?url=" + encodeURIComponent(url);

How should this handle pages that don't render in reader mode, such as about:reader?http://www.msuja.ws ?

Comment 8

[Florian Quèze [:florian]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)

> ::: browser/base/content/content.js
> @@ +518,5 @@
> >          if (content.document.body) {
> >            // Update the toolbar icon to show the "reader active" icon.
> >            sendAsyncMessage("Reader:UpdateReaderButton");
> >            new AboutReader(global, content, this._articlePromise);
> > +          this._articlePromise = null;
> 
> Why is this change needed (on this bug)?

Without this change, loading a page in reader mode while this._articlePromise is non-null will cause the code to attempt to re-use an article from a previous page. The check at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/AboutReader.jsm?rev=cf65e2ae03f9#573 will detect that something went awry and will just reload the page normally.

> ::: browser/components/readinglist/sidebar.js
> @@ +303,5 @@
> >                             .getInterface(Ci.nsIDOMWindow);
> > +
> > +    let currentUrl = mainWindow.gBrowser.currentURI.spec;
> > +    if (currentUrl.startsWith("about:reader"))
> > +      url = "about:reader?url=" + encodeURIComponent(url);
> 
> How should this handle pages that don't render in reader mode, such as
> about:reader?http://www.msuja.ws ?

When about:reader encounters a page where it can't find any article, it just reloads the page normally: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/AboutReader.jsm?rev=cf65e2ae03f9#576

This seems reasonable to me :). It's unfortunate that we leave reader mode at that point, but it wouldn't really make sense to stay in it if the user has started browsing non-readerable pages.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8582525 [details] [diff] [review]
Patch

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

Thanks!

Comment 10

[Florian Quèze [:florian]]

(In reply to Justin Dolske [:Dolske] from comment #6)

> Can you expand on the reasoning here? My understanding was that we ideally
> wanted to always load Reading List items in reading view, since the features
> are so closely specialized / related to each other. If the user just wanted
> the regular page, bookmarking the site would have sufficed.

This was also my understanding after our conversation at the beginning of last week.

But when I started talking about implementing this behavior, people in the room [including at least Jared and Gijs] were very surprised. After some more discussion with Michael, it seems preventing the user from accidentally exiting the reader mode is something everybody can agree is wanted.

If after some more discussion it turns out we want to open reading list items in reader mode all the time, making that change will be just a one line removal (I suggest doing this in a separate bug).

Another thing that we may want to do in a follow-up is ensuring that clicking on a readinglist item from the bookmark menu/panel doesn't make the user exit reader mode.

Comment 11

[Florian Quèze [:florian]]

Updating the bug summary to reflect that I only took care of the sidebar case here.

Comment 12

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/992418b89168

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/992418b89168

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/57a010d48a47

Comment 15

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 39.0a1 (2015-03-29) and Aurora 38.0a2 (2015-03-29), using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.

If the next item from the Reading List is also compatible with Reader View, it will now be displayed in Reader View as well.