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

VERIFIED FIXED in Firefox 43

Status

()

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

Trunk
Firefox 43
Unspecified
Windows
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox43 verified)

Details

Attachments

(1 attachment)

Posted patch patchSplinter Review
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/dgottwald@mozilla.com-808b62786775/try-win32/
Attachment #8661723 - Flags: review?(gijskruitbosch+bugs)
OS: Unspecified → Windows
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?
Attachment #8661723 - Flags: review?(gijskruitbosch+bugs)
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.
(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.
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?
Flags: needinfo?(philipp)
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.
Attachment #8661723 - Flags: review+
> 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
Flags: needinfo?(philipp)
https://hg.mozilla.org/mozilla-central/rev/1f137ffa86e0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
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
Flags: qe-verify+
Verified fixed on Firefox 43 beta 9, build ID: 20151203163240.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.