1131457 - [ReadingList] Add a button to the URLBar that allows adding the current page to the Reading List

Status: VERIFIED FIXED

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

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

We want a button in the urlbar that allows adding the current page to the Reading List.

Mockup: https://projects.invisionapp.com/share/NK1ZBQ6SY#/screens/59725441
Note that this bug is *just* for the button.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Note: Bug 1131461 adds a status display. I *think* we can use that as the aria-label for accessibility.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Open issues that need solutions/answers:
* The mockup shows the button only showing on hover. This is incompatible with touchscreens, accessibility, and discoverability.
* Need an image asset for the icon. Normal, hover, active. 1x, 2x DPI.
* What should show when a page is already in the ReadingList?

Comment 3

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

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #2)
> Open issues that need solutions/answers:
> * The mockup shows the button only showing on hover. This is incompatible
> with touchscreens, accessibility, and discoverability.

• The V2 design work will address much of the discoverability issues

> * Need an image asset for the icon. Normal, hover, active. 1x, 2x DPI.

• Would SVG fair better?

> * What should show when a page is already in the ReadingList?

• The highlighted Blue icon

Comment 4

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

Attachment: ReadingList-Readermode-Icons.zip

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Yea, much prefer SVG, TBH :) I'll massage these into one or two files. Thankyou!

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Mass change of ReadingList bugs, moving to their own component. Filter bugspam on the following quote:

“Reading is to the mind what exercise is to the body.”
― Joseph Addison

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: WiP v1

This adds a few APIs that help with all the other UI work. It also fixed bug 1133429 - I haven't split that out yet (it was easiest to do them at the same time).

No tests. Only Windows theme. And I think I have a color wrong in the SVG.

Migraine is getting in the way of getting this done, so upping it now.

Comment 8

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

Comment on attachment 8576425 [details] [diff] [review]
WiP v1

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

::: browser/base/content/browser-readinglist.js
@@ +90,5 @@
> +      if (this.listenerRegistered) {
> +        // This is safe to call if we're not currently registered, but we don't
> +        // want to forcibly load the normally lazy-loaded module on startup.
> +        ReadingList.removeListener(this);
> +        this.listenerRegistered = true;

should be false?

::: browser/components/readinglist/ReadingList.jsm
@@ +229,5 @@
>      return item;
>    }),
>  
>    /**
> +   * Add to the ReadingList the page that is loaded in a given browser.

TBH I'm not convinced this should be here - it makes the focus of this module a little blurry.  It looks like the only consumer is ReadingListUI() so having it there makes more sense to me.  Ditto for getMetadataFromBrowser().

@@ +301,5 @@
> +   * resolved URL.
> +   *
> +   * @param {nsIURI} uri - URI to match against. This will be normalized.
> +   */
> +  getItemForURL: Task.async(function* (uri) {

FTR, I stole this for bug 1142217 :)

@@ +357,5 @@
> +   *
> +   * @param {nsIURI} uri - URI to normalize.
> +   * @returns {nsIURI} Cloned and normalized version of the input URI.
> +   */
> +  normalizeURI(uri) {

FTR, I also stole this for bug 1142217, but added at the top:

    if (typeof uri == "string") {
      uri = Services.io.newURI(uri, "", null);
    }

So callers can call, eg, getItemForURL() with a string URL rather than an nsIURI.

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8576425 [details] [diff] [review]
WiP v1

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

Few nits about the SVG.

::: browser/themes/shared/readinglist/icons.svg
@@ +3,5 @@
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg"
> +     xmlns:xlink="http://www.w3.org/1999/xlink"
> +     x="0px" y="0px"

x and y attributes have no effect on the <svg> root element.

@@ +5,5 @@
> +<svg xmlns="http://www.w3.org/2000/svg"
> +     xmlns:xlink="http://www.w3.org/1999/xlink"
> +     x="0px" y="0px"
> +     viewBox="0 0 16 16"
> +     enable-background="new 0 0 16 16"

enable-background is deprecated and isn't supported by Firefox. It has no effect on SVG elements.

::: browser/themes/windows/jar.mn
@@ +109,5 @@
>          skin/classic/browser/session-restore.svg                     (../shared/incontent-icons/session-restore.svg)
>          skin/classic/browser/tab-crashed.svg                         (../shared/incontent-icons/tab-crashed.svg)
>          skin/classic/browser/welcome-back.svg                        (../shared/incontent-icons/welcome-back.svg)
>          skin/classic/browser/reader-mode-16.png                      (../shared/reader/reader-mode-16.png)
> +        skin/classic/browser/readinglist/icons.svg                   (../shared/readinglist/icons.svg)

I guess you know that, but this needs references on osx/linux jar.mn too.

Comment 10

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

Attachment: 0008-Bug-1131457-Add-a-button-to-the-URLBar-that-allows-a.patch

This is based on Blair's WIP and adds support for Mac and Linux.  It also addresses Tim's comments on the SVG.

All of the <browser> element support is back in browser-readinglist - I don't think it makes sense for ReadingList.jsm to know about browser elements.

It also has some changes so ReadingList.jsm supports an item's url attribute to be a URI or a string, particularly in "normal object" taken as input to addItem().  Drew, do you think this makes sense?

Key differences between the UX mockup and what is here:
* On hover a tooltip appears with "Add to reading list" - the URL bar does not change as the mock suggest (and it also doesn't change after hitting "add".
* There is no animation of the button to the bookmarks button.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Mark Hammond [:markh] from comment #10)
> Created attachment 8577783 [details] [diff] [review]

Thanks for looking at this while I wasn't able to :)


> All of the <browser> element support is back in browser-readinglist - I
> don't think it makes sense for ReadingList.jsm to know about browser
> elements.

Unfortunately, we need this in cases that aren't based on a browser window - see bug 1141476.

> It also has some changes so ReadingList.jsm supports an item's url attribute
> to be a URI or a string, particularly in "normal object" taken as input to
> addItem().  Drew, do you think this makes sense?

I like this.

> * On hover a tooltip appears with "Add to reading list" - the URL bar does
> not change as the mock suggest (and it also doesn't change after hitting
> "add".

Bug 1131461, punted to v2.

> * There is no animation of the button to the bookmarks button.

Bug 1131460.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8577783 [details] [diff] [review]
0008-Bug-1131457-Add-a-button-to-the-URLBar-that-allows-a.patch

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

::: browser/components/readinglist/ReadingList.jsm
@@ +888,5 @@
> +function normalizeURI(uri) {
> +  if (typeof uri == "string") {
> +    uri = Services.io.newURI(uri, "", null);
> +  }
> +  uri = uri.cloneIgnoringRef();

Oh, good catch - I keep forgetting about this method!

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Mark Hammond [:markh] from comment #10)
> It also has some changes so ReadingList.jsm supports an item's url attribute
> to be a URI or a string, particularly in "normal object" taken as input to
> addItem().  Drew, do you think this makes sense?

Apparently this broke test_ReadingList.js somehow.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1.2

*sigh* test_ReadingList.js doesn't have enough sanity checks nor enough logging to easily see what went wrong and where, and each part assumes a huge amount of state from the previous part that is never checked. And ReadingList.jsm makes assumptions that aren't obvious and aren't documented.

But AFAIK, I *think* the problem is that:
* ReadingList.addItem() relies on store.addItem() to throw, based on DB constraints
* ReadingList._itemFromObject() doesn't just do a conversion, it also modifies the state. So currently that can't happen before store.addItem()
* But we need _itemFromObject() to normalize the data that's put into the DB

I'm tired and too frustrated with this. Drew, could you look at it in the morning?

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Oh, and just FTR: After discussing with Mark, added addPageFromBrowser etc back to ReadingList.jsm, because they'll be needed for bug 1141476.

Comment 16

[Drew Willcoxon :adw]

Comment on attachment 8577970 [details] [diff] [review]
Patch v1.2

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

Sorry for the test trouble Blair, I think Mark and I have worked something out.

::: browser/components/readinglist/ReadingList.jsm
@@ +135,5 @@
> +   * @param {nsIURI} uri - URI to check.
> +   * @returns {Promise} Promise that is fulfilled with a boolean indicating
> +   *                    whether the URL is in the list or not.
> +   */
> +  containsURL: Task.async(function* (uri) {

Please call this containsURI since it takes an nsIURI, not a string URL.  (I'd prefer hasItemWithURI.)

@@ +148,5 @@
> +    }
> +
> +    // Then check if any cached items may have a different resolved URL
> +    // that matches.
> +    for (let itemWeakRef of this._itemsByURL.values()) {

This is a linear search so it won't scale well.  But I guess that the number of cached items should be fairly small in practice since consumers, hopefully, shouldn't be keeping a bunch of items in memory.

@@ +290,5 @@
> +   *
> +   * @param {<xul:browser>} browser - Browser element for the document.
> +   * @return {Promise} Promise that is fullfilled with the added item.
> +   */
> +  addPageFromBrowser: Task.async(function* (browser) {

addItemFromBrowser

@@ +304,5 @@
> +      itemData.exerpt = metadata.description;
> +    }
> +
> +    if (metadata.previews.length > 0) {
> +      itemData.image = metadata.previews[0];

There's no image property currently that I'm aware of, but it's OK with me if you want to leave this in for later.  Doesn't hurt anything, although Mark points out we should sanity-check the objects passed to addItem to make callers aware when they make a mistake.

@@ +318,5 @@
> +   *
> +   * @param {<xul:browser>} browser - Browser element for the document.
> +   * @returns {Promise} Promise that is fulfilled with an object describing the metadata.
> +   */
> +  getMetadataFromBrowser(browser) {

I don't think this needs to be a method on ReadingList (public or otherwise).  A helper function in the same file is fine with me.

Comment 17

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

Attachment: 0008-Bug-1131457-Add-a-button-to-the-URLBar-that-allows-a.patch

As we discussed

Comment 18

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

Attachment: 0008-Bug-1131457-Add-a-button-to-the-URLBar-that-allows-a.patch

oops - this is the correct one

Comment 19

[Drew Willcoxon :adw]

Comment on attachment 8578293 [details] [diff] [review]
0008-Bug-1131457-Add-a-button-to-the-URLBar-that-allows-a.patch

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

::: browser/components/readinglist/ReadingList.jsm
@@ +142,5 @@
> +    // This is used on every tab switch and page load of the current tab, so we
> +    // want it to be quick and avoid a DB query whenever possible.
> +
> +    // First check if any cached items have a direct match.
> +    if (this._itemsByURL.has(url)) {

Might be worth renaming this _itemsByNormalizedURL.  More verbose, but I can see someone (me!) sticking a non-normalized URL in there by accident.

Comment 20

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

(In reply to Drew Willcoxon :adw from comment #16)
> Please call this containsURI since it takes an nsIURI, not a string URL. 
> (I'd prefer hasItemWithURI.)

I left this as it was, but made it more obvious that either a URI or a URL can be passed - almost everywhere we accept a URL now works like this, so I felt using URI here was inconsistent.

But then I forgot about your preferred name - sorry about that - I agree hasItemWithURL is a better name, so let's fix this in a followup.

> This is a linear search so it won't scale well.  But I guess that the number
> of cached items should be fairly small in practice since consumers,
> hopefully, shouldn't be keeping a bunch of items in memory.

Agreed, but let's fix that later when we have a better feel for perf.

> addItemFromBrowser

Fixed.

> There's no image property currently that I'm aware of, but it's OK with me
> if you want to leave this in for later.  Doesn't hurt anything, although
> Mark points out we should sanity-check the objects passed to addItem to make
> callers aware when they make a mistake.

There's an entire other discussion going on about image, so I just left this as is for the time being (but yeah, I do think we should throw if someone passes in properties we don't understand as that implies the caller is confused)

> I don't think this needs to be a method on ReadingList (public or
> otherwise).  A helper function in the same file is fine with me.

Done.

(In reply to Drew Willcoxon :adw from comment #19)
> Might be worth renaming this _itemsByNormalizedURL.  More verbose, but I can
> see someone (me!) sticking a non-normalized URL in there by accident.

Yeah, let's do that in a followup too.

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

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/c4298f975b9a for bc1 orange:

https://treeherder.mozilla.org/logviewer.html#?job_id=2292904&repo=fx-team

Comment 22

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

Ack - sorry about that. Re-landed with a fix.

https://hg.mozilla.org/integration/fx-team/rev/79e1475e5b35

Comment 23

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

Backed out again: https://hg.mozilla.org/integration/fx-team/rev/8beb212cceb8

Comment 24

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

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=a590dfbc680b

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

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 26

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 39.0a1 (2015-03-18), using Ubuntu 14.04 (x64), Windows 7 (x64) and Mac OS X 10.9.5, with a few issues found and filed separately.

Comment 27

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

Mark, can you get this patch and its follow-ups uplifted to 38?

Comment 28

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

I'll be taking care of uplifts.

Comment 29

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/b2227f72c242

Comment 30

[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.