Add Button States to Items in the Location Bar

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: shorlander, Assigned: adw)

Tracking

(Blocks: 2 bugs)

57 Branch
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(2 attachments)

Posted image Location Bar States
Buttons in the Location Bar (Action Buttons) need feedback for hover and pressed states.

Specs at the bottom of this link:

https://mozilla.invisionapp.com/share/PYC5LJJXG#/229940647_Toolbars
Priority: P1 → --
Whiteboard: [photon-visual] → [photon-visual] [triage]

Updated

2 years ago
Duplicate of this bug: 1388163

Updated

2 years ago
Blocks: 1352697
Whiteboard: [photon-visual] [triage] → [photon-visual] [triage][reserve-photon-structure]
Flags: qe-verify+
Whiteboard: [photon-visual] [triage][reserve-photon-structure] → [reserve-photon-visual][reserve-photon-structure]
Hi Dao, which team should take this bug - Visual or Structure?
Flags: needinfo?(dao+bmo)
Either could take it. Most of these items were added or changed as part of structure, though.
Flags: needinfo?(dao+bmo)
Priority: -- → P3
QA Contact: gwimberly
Whiteboard: [reserve-photon-visual][reserve-photon-structure] → [reserve-photon-structure]

Comment 4

2 years ago
Note on sizing: icons should have 6px horizontal padding and no gap between them and the top/bottom of the location bar, and then the colors from the attachment.
Depends on: 1389740
(In reply to :Gijs from comment #4)
> Note on sizing: icons should have 6px horizontal padding

My patch in bug 1389740 fixes this. (Feel free to steal the review if you have time today.)
Priority: P3 → P4

Updated

2 years ago
Duplicate of this bug: 1393629
(Assignee)

Updated

2 years ago
Assignee: nobody → adw
Status: NEW → ASSIGNED
Iteration: --- → 57.2 - Aug 29
Priority: P4 → P1
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19
Comment hidden (mozreview-request)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8902478 [details]
Bug 1388589 - Add Button States to Items in the Location Bar.

https://reviewboard.mozilla.org/r/174056/#review179548

::: browser/themes/shared/urlbar-searchbar.inc.css:182
(Diff revision 1)
> +  /* 30x30 box - 16x16 image = 14x14 padding, 7 on each side */
> +  padding: 7px;
>  }
>  
>  .urlbar-icon:hover {
> -  fill-opacity: 0.8;
> +  background-color: var(--toolbarbutton-hover-background);

This doesn't work with dark lightweight themes. Try e.g. Space Fantasy offered in customize mode.

Anyway, according to <http://design.firefox.com/people/shorlander/photon/Mockups/windows-10.html>, this is also not the right color for the default theme. Can you use the same color as the identity block?
Attachment #8902478 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 years ago
Stephen says the colors in the patch, which are the colors in the Invision spec, are right, and the color in his mockup is wrong.
(In reply to Drew Willcoxon :adw from comment #10)
> Stephen says the colors in the patch, which are the colors in the Invision
> spec, are right, and the color in his mockup is wrong.

Did he also say specifically that the identity block and urlbar icons should use different hover effects? This seems a bit weird.

Comment 12

2 years ago
mozreview-review
Comment on attachment 8902478 [details]
Bug 1388589 - Add Button States to Items in the Location Bar.

https://reviewboard.mozilla.org/r/174056/#review180520

::: browser/themes/shared/urlbar-searchbar.inc.css:8
(Diff revision 2)
>   * 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/. */
>  %endif
>  
> +:root {
> +  --urlbar-icon-hover-background: var(--toolbarbutton-hover-background);

This is a neat trick to avoid the problem from comment 8, but it creates a similar problem for dark OS themes such as dark Gtk themes or High Contrast Dark on Windows. --toolbarbutton-hover-background isn't designed to be used like this, as it's supposed to change based on the color of the toolbar.

Btw, I noticed that identity block's hover effect is broken with dark OS themes too. The --arrowpanel-dimmed* colors don't have this problem: http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/browser/themes/windows/browser.css#28-30
I'd suggest copying that here.
Attachment #8902478 - Flags: review?(dao+bmo)
(Assignee)

Comment 13

2 years ago
> adw: shorlander: dao says that the bg color of the identity block doesn't match the spec'ed bg color of the urlbar buttons. true? what do you think? etc.
> shorlander: adw: Looks pretty close, I would have to inspect the actual values to tell. It does look like the icons themselves are getting darker, which we discussed the other day already.
> adw: shorlander: but the bg colors in the spec for the urlbar buttons is correct, and if the identity icon's bg currently differs from that, then that's wrong, correct?
> adw: ... or it's ok that the two bg's are slightly different
> shorlander: adw: this is correct  https://cl.ly/2x1k2R2Q0h1z
> shorlander: And they should all be the same
> adw: ok, thank you
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 years ago
The `:root:not(:-moz-lwtheme) toolbar[brighttext]` rule fixes the high-contrast dark theme problem.

If there's an obvious better way to do all this, please let me know.
(In reply to Drew Willcoxon :adw from comment #15)
> The `:root:not(:-moz-lwtheme) toolbar[brighttext]` rule fixes the
> high-contrast dark theme problem.
> 
> If there's an obvious better way to do all this, please let me know.

Technically, an OS theme could make the toolbar dark and text fields much lighter, so this is still a bit fragile. I wouldn't be too surprised if some Gtk themes do this. Can you pick a hsla(0,0%,80%,.X) value that ends up looking like hsla(240,5%,5%,.1) under average circumstances? This avoids the need to switch colors and you can get rid of the CSS variables too.
Comment hidden (mozreview-request)
(Assignee)

Comment 18

2 years ago
I ran some numbers.  I tested hsla(0,0%,80%,X) for various values of X.

> * below, right-hand-side column values are Y in measured rgb(Y, Y, Y)
> * "ref" value is Y for the spec'ed color
> 
> default theme
> -------------
> 0.5: 223
> 0.4: 230
> 0.3: 236
> 0.2: 242
> --------
> ref: 224
> 
> dark theme
> ----------
> 0.5: 118
> 0.4: 106
> 0.3: 93
> 0.2: 79
> -------
> ref: 89
> 
> high contrast black
> -------------------
> 0.5: 83
> 0.4: 65
> 0.3: 47
> 0.2: 31
> -------
> ref: 38
> 
> avg diff, unweighted
> --------------------
> 0.5: (/ (+ (abs (- 223 224)) (abs (- 118 89)) (abs (- 83 38))) 3.0)
> 25.0
> 0.4: (/ (+ (abs (- 230 224)) (abs (- 106 89)) (abs (- 65 38))) 3.0)
> 16.666666666666668
> 0.3: (/ (+ (abs (- 236 224)) (abs (- 93 89)) (abs (- 47 38))) 3.0)
> 8.333333333333334
> 0.2: (/ (+ (abs (- 242 224)) (abs (- 79 89)) (abs (- 31 38))) 3.0)
> 11.666666666666666
> 
> avg diff, weighted: default=2.0, dark=1.5, high contrast=1.0
> ------------------------------------------------------------
> 0.5: (/ (+ (* 2.0 (abs (- 223 224))) (* 1.5 (abs (- 118 89))) (* 1.0 (abs (- 83 38)))) 4.5)
> 20.11111111111111
> 0.4: (/ (+ (* 2.0 (abs (- 230 224))) (* 1.5 (abs (- 106 89))) (* 1.0 (abs (- 65 38)))) 4.5)
> 14.333333333333334
> 0.3: (/ (+ (* 2.0 (abs (- 236 224))) (* 1.5 (abs (- 93 89))) (* 1.0 (abs (- 47 38)))) 4.5)
> 8.666666666666666
> 0.2: (/ (+ (* 2.0 (abs (- 242 224))) (* 1.5 (abs (- 79 89))) (* 1.0 (abs (- 31 38)))) 4.5)
> 12.88888888888889
> 
> avg diff, weighted: default=4.0, dark=1.0, high contrast=1.0
> ------------------------------------------------------------
> 0.5: (/ (+ (* 4.0 (abs (- 223 224))) (* 1.0 (abs (- 118 89))) (* 1.0 (abs (- 83 38)))) 6.0)
> 13.0
> 0.4: (/ (+ (* 4.0 (abs (- 230 224))) (* 1.0 (abs (- 106 89))) (* 1.0 (abs (- 65 38)))) 6.0)
> 11.333333333333334
> 0.3: (/ (+ (* 4.0 (abs (- 236 224))) (* 1.0 (abs (- 93 89))) (* 1.0 (abs (- 47 38)))) 6.0)
> 10.166666666666666
> 0.2: (/ (+ (* 4.0 (abs (- 242 224))) (* 1.0 (abs (- 79 89))) (* 1.0 (abs (- 31 38)))) 6.0)
> 14.833333333333334
> 
> avg diff, weighted: default=10.0, dark=1.0, high contrast=1.0
> -------------------------------------------------------------
> 0.5: (/ (+ (* 10.0 (abs (- 223 224))) (* 1.0 (abs (- 118 89))) (* 1.0 (abs (- 83 38)))) 12.0)
> 7.0
> 0.4: (/ (+ (* 10.0 (abs (- 230 224))) (* 1.0 (abs (- 106 89))) (* 1.0 (abs (- 65 38)))) 12.0)
> 8.666666666666666
> 0.3: (/ (+ (* 10.0 (abs (- 236 224))) (* 1.0 (abs (- 93 89))) (* 1.0 (abs (- 47 38)))) 12.0)
> 11.083333333333334
> 0.2: (/ (+ (* 10.0 (abs (- 242 224))) (* 1.0 (abs (- 79 89))) (* 1.0 (abs (- 31 38)))) 12.0)
> 16.416666666666668

X~=0.3 is a good choice but only when the avg diffs are unweighted.  Unweighted avg diffs are misleading because we should put more weight on the default theme since most people will stick with it.  Subjectively, X=0.3 is just too faint in the default theme.

I chose X=0.4.  Anything less is too faint in the default theme, as is apparent when the default theme is weighted heavily.  Overall it's an OK balance.

Comment 19

2 years ago
mozreview-review
Comment on attachment 8902478 [details]
Bug 1388589 - Add Button States to Items in the Location Bar.

https://reviewboard.mozilla.org/r/174056/#review181254
Attachment #8902478 - Flags: review?(dao+bmo) → review+

Comment 20

2 years ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/31c8cec38f46
Add Button States to Items in the Location Bar. r=dao
https://hg.mozilla.org/mozilla-central/rev/31c8cec38f46
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Updated

2 years ago
Depends on: 1397278

Updated

2 years ago
Depends on: 1397395
Depends on: 1397903
No longer depends on: 1397903
Depends on: 1397583
(Assignee)

Updated

2 years ago
Depends on: 1399235
I have reproduced the issue mentioned in comment 0 using an affected Firefox 57.0a1 build (BuildId:20170808100226).

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.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.