1133485 - [ReadingList] Open the Reading List sidebar when the button in the location bar is used to add the current page to Reading List

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect, P2)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

The ReaderMode controls have a "Add to Reading List" button - on desktop, this doesn't currently do anything. It probably should.

For v1, it should just add the current page. Bug  1133429 will provide an API to pass a tab/browser to (and it'll handle all the metadata stuff).

Comment 1

[Justin Dolske [:Dolske]]

I think we should also have this button open the Reading List sidebar the first time it's used. That would help with making it obvious that it's related to Reading List, that clicking it adds an item, and generally help introduce users to the feature. This is especially true for pages that don't work in Reader View, and so there's no book icon next to this button (i.e., the user may wonder why they have a mysterious (+) button in their URL bar).

Comment 2

[Marco Mucci [:MarcoM]]

Deprioritized during backlog review.  Removed from IT 39.2 priority backlog and not part of the initial Q2 campaign release.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Note that bug 1142217 landed basic support for the +- button, but I'm not resolving as a dupe as it didn't address automatically opening the sidebar.

Comment 4

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

Attachment: Patch

Comment 5

[Florian Quèze [:florian]]

Jared, are you sure the behavior implemented by this patch is wanted?

Comment 1 said "open the Reading List sidebar the first time it's used".

Yesterday I remember hearing mmaslaney say we should be opening the sidebar automatically when adding an item, except maybe for the first time experience.

Comment 6

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

(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> Jared, are you sure the behavior implemented by this patch is wanted?
> 
> Comment 1 said "open the Reading List sidebar the first time it's used".
> 
> Yesterday I remember hearing mmaslaney say we should be opening the sidebar
> automatically when adding an item, except maybe for the first time
> experience.

Oh, somehow I missed comment 1. I'll change the patch to only open it the first time it's used. (We discussed this in person and decided that a uitour/fte that opens the sidebar can count as that first opening since it will be doing the work of educating the user about the feature and how they are connected).

Comment 7

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

Attachment: Patch v2

This patch only opens it the if the sidebar hasn't been opened before. Note that it needs to be rebased after some of the fallout from bug 1131416 is fixed, so I based this on top of a backout of bug 1131416.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8580916 [details] [diff] [review]
Patch v2

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

::: browser/app/profile/firefox.js
@@ +1880,5 @@
>  #endif
>  
>  // Disable ReadingList browser UI by default.
>  pref("browser.readinglist.enabled", false);
> +pref("browser.readinglist.sidebarOpened", false);

sidebarOpened sounds a little like it should go back to false when it is closed - sidebarEverOpened? But I'm not too bothered

Comment 9

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

Landed with the pref renamed to sidebarEverOpened,
https://hg.mozilla.org/integration/fx-team/rev/eca6911ffb55

Comment 10

[Phil Ringnalda (:philor)]

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

Comment 11

[Andrei Vaida [:avaida]]

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

If the sidebar has never been enabled before, adding a web page to Reading List now automatically opens the sidebar and sets browser.readinglist.sidebarEverOpened to true.

Comment 12

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/54e9be44f35c

Comment 13

[Andrei Vaida [:avaida]]

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