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

VERIFIED FIXED in Firefox 57

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: abenson, Assigned: Gijs)

Tracking

57 Branch
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [reserve-photon-structure], )

Attachments

(1 attachment)

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
Whiteboard: [photon-visual] → [photon-visual] [triage]
Whiteboard: [photon-visual] [triage] → [photon-structure][triage]
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Priority: P3 → P1
Iteration: --- → 57.3 - Sep 19
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
Attachment #8909796 - Flags: ui-review?(abenson)
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!
Attachment #8909796 - Flags: review+
Attachment #8909796 - Flags: ui-review?(abenson)
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.
Attachment #8909796 - Flags: review?(adw) → review+
(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).
Pushed by gijskruitbosch@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/a8d0557f9fc2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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]
Status: RESOLVED → VERIFIED
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.