1173743 - Update toolbar button hover/pressed states on Windows 10

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P3)

Description

[Dão Gottwald [:dao]]

spec is in http://people.mozilla.org/~shorlander/mockups/Windows-10/Windows-10.html

Comment 1

[Tim Nguyen :ntim]

needinfo for the hsla/rgba colors as we talked about on IRC

Comment 2

[Tim Nguyen :ntim]

Sharing some of the research I've done :

:root {
  --toolbarbutton-hover-background: rgba(0,0,0,0.1);
  --toolbarbutton-hover-bordercolor: transparent;
  --toolbarbutton-hover-boxshadow: 0 0 0 2px rgba(0,0,0,0.2);
  --toolbarbutton-active-background: rgba(0,0,0,0.15);
  --toolbarbutton-active-bordercolor: transparent;
  --toolbarbutton-active-boxshadow: 0 0 0 2px rgba(0,0,0,0.3)
}

This works, but there are some issues :
- Forward button looks broken with this style
- The toolbar buttons have more height than the mockup
- The split buttons (bookmarks button) don't look right

Comment 3

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Just a quick note since I've just looked at this: the buttons have a 1px border radius in the mockup, so let's include that as well.

Comment 4

[Tim Nguyen :ntim]

This could work for the bookmarks button :
<div>
<button>F</button>
<button>G</button>
</div>

<style>
button {
  background: rgba(0,0,0,0.1);
  border: none;
  float: left;
  padding: 6px 10px;
}
button:first-child {
  box-shadow: -1.5px 0 0 2px rgba(0,0,0,0.2);

}
button:last-child {
  box-shadow: 1.5px 0 0 2px rgba(0,0,0,0.2);
}
</style>

If this doesn't, we'll have to use an actual 2px border.

Comment 5

[Lawrence Mandel [:lmandel] (use needinfo)]

Tracking 40 as Philipp flagged this as a bug that we need for Windows 10 support in this release.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Unfortunately, this is too late for 40. We have time to take a fix for 41 if it is available soon.

Comment 7

[Justin Dolske [:Dolske]]

-> P3, and a candidate for being in a UI polish clusterbug.

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Dolske, Dao: I am following up on FF41 tracked bugs. Since this is win10 related and was not fixed in FF40, it would be nice if we fixed this for FF41. What do you think?

Comment 9

[Justin Dolske [:Dolske]]

Yes, that's why it's on our list of things we'd like to fix in 41 (bug 1192839).

Comment 10

[Dão Gottwald [:dao]]

Attachment: patch

Comment 11

[Tim Nguyen :ntim]

Comment on attachment 8657613 [details] [diff] [review]
patch

The split toolbar buttons should only have a 1px separator on hover: http://people.mozilla.org/~shorlander/mockups/Windows-10/images/Windows10-Specs-01-Main-Window-04.png

Comment 12

[Tim Nguyen :ntim]

Also, I'm not quite sure if we want this on Windows 8.

Comment 13

[Dão Gottwald [:dao]]

I don't want Windows 8 to be the odd one out. We should let it follow the style for XP/Vista/7 or that for 10, and I think 10 probably makes more sense.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8657613 [details] [diff] [review]
patch

Review of attachment 8657613 [details] [diff] [review]:
-----------------------------------------------------------------

This adds an awkward splitter on menu-buttons, http://screencast.com/t/OZyi6z95Jt

Comment 15

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14)
> Comment on attachment 8657613 [details] [diff] [review]
> patch
> 
> Review of attachment 8657613 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This adds an awkward splitter on menu-buttons,
> http://screencast.com/t/OZyi6z95Jt

It's just two adjacent borders, doesn't look that awkward to me. If this is the only issue, we should get this landed and fine-tune in a separate bug.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

I disagree. This is tracking firefox41 and we would want to uplift this but not with that regression which will appear in the default set.

Comment 17

[Dão Gottwald [:dao]]

Not targeting 41 then.

Comment 18

[Dão Gottwald [:dao]]

Stephen, Philipp, do we still want the 2px thick border on hover? We had this for the location and search bars but then reverted it to 1px.

Comment 19

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Dão Gottwald [:dao] from comment #18)
> Stephen, Philipp, do we still want the 2px thick border on hover? We had
> this for the location and search bars but then reverted it to 1px.

I think we should use the 2px border on buttons only on the pressed state. We can use the same transition of the inner shadow as we do on the URL bar.
(Unless Stephen has a strong opinion in doing something different)

Comment 20

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 8659744 [details] [diff] [review]
patch v2

changing reviewer since Jared is "mostly away" for a couple of days

Comment 22

[:Gijs (out for now; he/him)]

Comment on attachment 8659744 [details] [diff] [review]
patch v2

Review of attachment 8659744 [details] [diff] [review]:
-----------------------------------------------------------------

r=me but see the question below.

::: browser/themes/windows/browser.css
@@ +50,5 @@
>  
>    --urlbar-separator-color: hsla(0,0%,16%,.2);
>  }
>  
> +#nav-bar[brighttext] {

Shouldn't this happen for the other toolbars as well?

Comment 23

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #22)
> Comment on attachment 8659744 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8659744 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me but see the question below.
> 
> ::: browser/themes/windows/browser.css
> @@ +50,5 @@
> >  
> >    --urlbar-separator-color: hsla(0,0%,16%,.2);
> >  }
> >  
> > +#nav-bar[brighttext] {
> 
> Shouldn't this happen for the other toolbars as well?

We don't use that custom toolbar button style on other toolbars, except for the find bar but I don't think we have the brighttext attribute there...

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/fd3764cfdcc5

Comment 25

[Phil Ringnalda (:philor)]

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

Comment 26

[Tim Nguyen :ntim]

Comment on attachment 8659744 [details] [diff] [review]
patch v2

Review of attachment 8659744 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/windows/browser.css
@@ +50,5 @@
>  
>    --urlbar-separator-color: hsla(0,0%,16%,.2);
>  }
>  
> +#nav-bar[brighttext] {

This will also apply on the devedition dark theme, not sure if that's wanted.

Comment 27

[:Gijs (out for now; he/him)]

(In reply to Tim Nguyen [:ntim] from comment #26)
> Comment on attachment 8659744 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8659744 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/windows/browser.css
> @@ +50,5 @@
> >  
> >    --urlbar-separator-color: hsla(0,0%,16%,.2);
> >  }
> >  
> > +#nav-bar[brighttext] {
> 
> This will also apply on the devedition dark theme, not sure if that's wanted.

Please file a separate bug if it actually ends up not looking as desired.

Comment 28

[Dão Gottwald [:dao]]

Not going to bother uplifting this.

Comment 29

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 10.0; rv:43.0) Gecko/20100101 Firefox/43.0
Verified this fix on Windows 10 32-bit using latest Nightly, build ID: 20150917030229

Filled bug 1206083 and bug 1206087 for some issues spotted while testing this, so we can track them separately.