Closed Bug 1388589 Opened 7 years ago Closed 7 years ago

Add Button States to Items in the Location Bar

Categories

(Firefox :: Theme, defect, P1)

57 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.3 - Sep 19
Tracking Status
firefox57 --- verified

People

(Reporter: shorlander, Assigned: adw)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(2 files)

Attached 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]
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]
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
Assignee: nobody → adw
Status: NEW → ASSIGNED
Iteration: --- → 57.2 - Aug 29
Priority: P4 → P1
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19
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)
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 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)
> 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
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.
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 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+
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1397278
Depends on: 1397395
Depends on: 1397903
No longer depends on: 1397903
Depends on: 1397583
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
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.