1397790 - URL bar icons that have menus need an 'active' state

Status: VERIFIED FIXED

(Firefox :: Menus, enhancement, P1)

Description

[Aaron Benson]

URL bar icons (Page Action Menu, Bookmarks, Pocket, Send to Device) currently have a hover state. When a menu is opened from one of those items, there should be an "active" state which essentially means it has a background color much like the hover state. 

See the very bottom of this spec for details on color: https://mozilla.invisionapp.com/share/PYC5LJJXG#/229940647_Toolbars

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1397790 - add 'open' active state to urlbar buttons, use a more distinct background colour for it,

Review commit: https://reviewboard.mozilla.org/r/181290/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181290/

Comment 2

[:Gijs (he/him)]

I've deliberately made the active state a bit darker because it was hard to distinguish it from the 'hover' state. I looked at the comments in bug 1388589 and I think this works, but please let me know if you have concerns.

(In reply to Aaron Benson from comment #0)
> URL bar icons (Page Action Menu, Bookmarks, Pocket, Send to Device)
> currently have a hover state. When a menu is opened from one of those items,
> there should be an "active" state which essentially means it has a
> background color much like the hover state. 
> 
> See the very bottom of this spec for details on color:
> https://mozilla.invisionapp.com/share/PYC5LJJXG#/229940647_Toolbars

So, we can't use those exact colours directly for the reasons given in bug 1388589. Aaron, can you check if the colours look OK to you and if not, suggest changes that you check with the browser toolbox (note dark/light/default/"space fantasy" theme to get a good idea of how it looks in themes where either everything is dark, light or where the toolbar is dark but the input box is light).

Trypush with builds in a few minutes:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f665faad0da

Comment 3

[:Gijs (he/him)]

Linux build:

https://queue.taskcluster.net/v1/task/f7rw3ktoS5eNQzPB4eJWaQ/runs/0/artifacts/public/build/target.tar.bz2

OSX:

https://queue.taskcluster.net/v1/task/XIJjfgNGSJyM71voEgTr8w/runs/0/artifacts/public/build/target.dmg

Windows:

https://queue.taskcluster.net/v1/task/HWltRWJ0RpWWnbthjnoLNw/runs/0/artifacts/public/build/target.zip

Comment 4

[Aaron Benson]

Comment on attachment 8909796 [details]
Bug 1397790 - add 'open' active state to urlbar buttons, use a more distinct background colour for it,

https://reviewboard.mozilla.org/r/181290/#review186546

LGTM!

Comment 5

[Drew Willcoxon :adw]

Comment on attachment 8909796 [details]
Bug 1397790 - add 'open' active state to urlbar buttons, use a more distinct background colour for it,

https://reviewboard.mozilla.org/r/181290/#review186668

Nice, thanks!  I wish it were possible to set this open attribute in one place instead of three places.  Or at least in one place within browser-pageActions.js (i.e., excluding browser-places.js).  We could probably factor out a method for opening a panel on a button, whether the panel is the main panel or the activated-action panel, and whether the button is the main button or an action button.  Just thinking out loud, probably not worth it, at least right now.

Thanks too for making the active color darker.  I meant to file a bug about that.

Might be worth updating browser_page_action_menu.js and/or browser_PageActions.js to check the open attribute?  Should be two simple one-line checks in a single test task for checking both its presence and absence.  But not a big deal.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8909796 [details]
Bug 1397790 - add 'open' active state to urlbar buttons, use a more distinct background colour for it,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181290/diff/1-2/

Comment 7

[:Gijs (he/him)]

(In reply to Drew Willcoxon :adw from comment #5)
> Might be worth updating browser_page_action_menu.js and/or
> browser_PageActions.js to check the open attribute?  Should be two simple
> one-line checks in a single test task for checking both its presence and
> absence.  But not a big deal.

Good idea! I added a check in 3 places in browser_page_action_menu.js to check all of the bookmark star, the standalone send to device panel, and the main panel button. Pocket was already setting the attribute, so I hope/assume that has its own test (and in any case, it's not tested in these tests I think, so I didn't want to try to fix that here).

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8d0557f9fc2
add 'open' active state to urlbar buttons, use a more distinct background colour for it, r=abenson+572682,adw

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 10

[Almas Hossain[:mAlmas]]

I have reproduced this bug with Nightly 57.0a1 (2017-09-07) in Windows 10 (64-bit).

This bug's fix is verified with latest Nightly 58.0a1 (64-bit).

Build ID   :   20170921220243
User Agent :   Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0

[bugday 20170920]

Comment 11

[Emil Ghitta, QA [:emilghitta]]

I have reproduced the issue mentioned in comment 0 using an affected Firefox 57.0a1 build (BuildId:20170907220212).

I have verified that the issue is not reproducible using Firefox 57.0b7 (Build Id:20171009192146) on Windows 10 64bit, macOS 10.11.6 and Ubuntu 16.04 64bit.