1205234 - Remove Windows XP/Vista/7-specific toolbar button styling rules

Status: VERIFIED FIXED

Description

[Dão Gottwald [::dao]]

Attachment: patch

The toolbar button styling on Windows is pretty messy, because there are base rules used across the board and then there are Windows XP/Vista/7-specific rules, which makes it brittle to touch the base rules since you might break the others along the way. It's non-trivial dependency since the rules are fairly complex.

I think we can largely drop those XP/Vista/7-specific rules and basically just set the CSS variables to get the desired background, border and box-shadow. The differences for Win7 and older implemented by these legacy rules are often arbitrary if they make sense at all. There are some differences worth explaining:

- There was a 2px border radius, but the location and search bar use a 1px radius across the board, so it makes sense to have toolbar buttons follow suit.

- type="menu-button" toolbar buttons had an extra hover state for the nested buttons. We don't have this on Linux, and considering that we haven't implemented it for Windows 8 when it came out nor for 10, I think this is safe to drop.

- --toolbarbutton-checkedhover-backgroundcolor wasn't used on Windows 8 and 10. I moved the corresponding rule so it's used for all versions.

There's a try build here: http://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-808b62786775/try-win32/

Comment 1

[:Gijs (he/him)]

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

Can you upload screenshots that show the new look (esp where there have been changes) and get ui-review on that first?

Comment 2

[Dão Gottwald [::dao]]

There's no new look really, there are subtle differences here and there that are pretty hard to make out in screenshots. That's kind of my point: the rules are largely useless, as one would expect, because we don't have any replacement for them on 8 and 10.

Comment 3

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #2)
> There's no new look really, there are subtle differences here and there that
> are pretty hard to make out in screenshots.

(In reply to Dão Gottwald [:dao] from comment #0)
> - type="menu-button" toolbar buttons had an extra hover state for the nested
> buttons. We don't have this on Linux, and considering that we haven't
> implemented it for Windows 8 when it came out nor for 10, I think this is
> safe to drop.

This doesn't sound subtle, but it's hard to tell from just the comment.

Comment 4

[Dão Gottwald [::dao]]

It just means that the extra hover state is gone. The buttons behave like on 8 and 10.

Philipp, could you please give the try build a go and let me know if you see anything wrong?

Comment 5

[:Gijs (he/him)]

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

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

I finally made some time to look at this. r=me, though I think it'd be nice to get the menu-button stuff working everywhere + linux.

::: browser/themes/windows/browser.css
@@ +889,5 @@
>    transition-duration: 10ms;
>  }
>  
> +#nav-bar .toolbarbutton-1[checked]:not(:active):hover > .toolbarbutton-icon {
> +  background-color: var(--toolbarbutton-checkedhover-backgroundcolor);

This is the same as toolbarbutton-active-background, which is the background the icon gets without hover (for [checked] toolbarbuttons), so right now this doesn't do anything on win8, in my testing...

I'm fine with just filing a followup bug to fix that.

Comment 6

[Pulsebot]

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

Comment 7

[Dão Gottwald [::dao]]

> This is the same as toolbarbutton-active-background, which is the background
> the icon gets without hover (for [checked] toolbarbuttons), so right now
> this doesn't do anything on win8, in my testing...

filed bug 1205677

Comment 8

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/1f137ffa86e0

Comment 9

[Dão Gottwald [::dao]]

This may have had a positive perf impact according to some e-mail I got:

Improvement: Fx-Team-Non-PGO - Talos Page Switch - WINNT 6.1 (ix) - 4.67% decrease
Improvement: Fx-Team-Non-PGO - Customization Animation Tests - WINNT 6.1 (ix) (e10s) - 8.24% decrease
Improvement: Fx-Team-Non-PGO - TResize - WINNT 5.1 (ix) - 17% decrease
Improvement: Fx-Team-Non-PGO - TResize - WINNT 6.1 (ix) (e10s) - 12% decrease
Improvement: Fx-Team-Non-PGO - TResize - WINNT 6.1 (ix) - 12.5% decrease
Improvement: Fx-Team-Non-PGO - Customization Animation Tests - WINNT 6.1 (ix) - 6.08% decrease

Comment 10

[Cornel Ionce [:cornel_ionce], Desktop Release QA]

Verified fixed on Firefox 43 beta 9, build ID: 20151203163240.