Closed Bug 1143388 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect

Tracking

(firefox38 verified, firefox39 verified)

VERIFIED FIXED
Firefox 39
Tracking Status
firefox38 --- verified
firefox39 --- verified

People

(Reporter: ntim, Assigned: florian)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.
Flags: qe-verify+
Priority: -- → P2
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?
Flags: needinfo?(dolske)
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.
Changing the summary to match decision made in comment 2.
Flags: needinfo?(dolske)
Summary: Clicking reading list item from bookmarks menu should open it in reader view → Loading an item from the readinglist shouldn't make the user leave the reader mode if it was active
Attached patch PatchSplinter Review
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.
Assignee: nobody → florian
Status: NEW → ASSIGNED
Attachment #8582525 - Flags: review?(jaws)
Hi Florian, can you provide a point value.
Blocks: 1132074
Iteration: --- → 39.3 - 30 Mar
Flags: needinfo?(florian)
Flags: firefox-backlog+
Points: --- → 3
Flags: needinfo?(florian)
(In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) 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.
Flags: needinfo?(mmaslaney)
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 ?
Attachment #8582525 - Flags: review?(jaws) → feedback+
(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 on attachment 8582525 [details] [diff] [review]
Patch

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

Thanks!
Attachment #8582525 - Flags: feedback+ → review+
(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.
Updating the bug summary to reflect that I only took care of the sidebar case here.
Summary: Loading an item from the readinglist shouldn't make the user leave the reader mode if it was active → Loading an item from the readinglist sidebar shouldn't make the user leave the reader mode if it was active
https://hg.mozilla.org/mozilla-central/rev/992418b89168
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Flags: needinfo?(mmaslaney)
QA Contact: andrei.vaida
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.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.