Closed
Bug 1205234
Opened 7 years ago
Closed 7 years ago
Remove Windows XP/Vista/7-specific toolbar button styling rules
Categories
(Firefox :: Theme, defect)
Tracking
()
VERIFIED
FIXED
Firefox 43
Tracking | Status | |
---|---|---|
firefox43 | --- | verified |
People
(Reporter: dao, Assigned: dao)
Details
Attachments
(1 file)
7.38 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter 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)
Assignee | ||
Updated•7 years ago
|
OS: Unspecified → Windows
Comment 1•7 years ago
|
||
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)
Assignee | ||
Comment 2•7 years ago
|
||
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•7 years ago
|
||
(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.
Assignee | ||
Comment 4•7 years ago
|
||
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 5•7 years ago
|
||
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+
Assignee | ||
Comment 7•7 years ago
|
||
> 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: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Assignee | ||
Comment 9•7 years ago
|
||
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
Updated•7 years ago
|
Flags: qe-verify+
Comment 10•7 years ago
|
||
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.
Description
•