1144675 - The Reading List button from the Location Bar should have a distinct icon for pages currently in the list

Status: VERIFIED FIXED

(Firefox Graveyard :: Reading List, defect)

Description

[Andrei Vaida [:avaida]]

Reproducible on:
Nightly 39.0a1 (2015-03-18)

Affected platforms:
Ubuntu 14.04 (x64), Windows 7 (x64), Mac OS X 10.9.5.

Preconditions:
* browser.readinglist.enabled = true

Steps to reproduce:
1. Launch Firefox.
2. Access a web page, (e.g.) https://www.mozilla.org/en-US/mission/.
3. Click the "Add page to Reading List" button from the Location Bar.

Expected result:
* Clicking the button successfully adds the example page to Reading List.
* The button icon turns from (+) to (-) after it has been clicked.

Actual result:
The button is only changing its icon in terms of color, gray to blue. 
I think it would be more intuitive and also consistent with the design used in Reader View's sidebar controls to change this.

Comment 1

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

Michael, the mockups imply this button is always a "+", with only the color changing - but clarkbw thought maybe a "-" was desired - what's your take?

Comment 2

[Tim Nguyen :ntim]

The fact that hovering the "checked" icon makes it black, is quite confusing, since when hovering, you can't really see whether the page is in the reading list or not.

Comment 3

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Mark Hammond [:markh] from comment #1)
> Michael, the mockups imply this button is always a "+", with only the color
> changing - but clarkbw thought maybe a "-" was desired - what's your take?

Yes, we need it to change to a delete button like the reader view.  This should fix bug 1144684, might need to dupe that here.

10:08 AM <clarkbw> mmaslaney: the url bar reading list button is supposed change to a (-) minus after adding something, right?
10:08 AM <clarkbw> that's the behavior of the reader view add/remove button currently
10:08 AM <clarkbw> https://projects.invisionapp.com/share/B21NGU7M3#/screens/59725441?maintainScrollPosition=false
10:10 AM <mmaslaney> clarkbw: Yes

Michael, can you provide the delete button icon?

Comment 4

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

(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #3)
> Michael, can you provide the delete button icon?

We are actually going to need it in our SVG - https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/readinglist/icons.svg - it may be easy for us to do it, but I'm not sure yet :/  If you know SVG, it would be great if you can tweak it.

Comment 5

[Florian Quèze [:florian]]

Attachment: Patch

This patch:
- Changes the icon to a '-' when the page has already been added.
- Makes the icon blue only in the active state.
- Hides the icon when the URL bar is neither focused nor hovered.
- moves the readinglist.inc.css file to the readinglist subfolder.

Comment 6

[Marco Mucci [:MarcoM]]

Hi Florian, can you provide a point value.

Comment 7

[Florian Quèze [:florian]]

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

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/89cb4fd9ee1d

Comment 9

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

The Reading List button from the Location Bar now acts and looks as specified in Comment 5.

Comment 10

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/69c5f5536e10

Comment 11

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