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

Hi Dao, which team should take this bug - Visual or Structure?

Either could take it. Most of these items were added or changed as part of structure, though.

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.

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

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?

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.

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

Pushed by dwillcoxon@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/31c8cec38f46 Add Button States to Items in the Location Bar. r=dao

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.