940133 - Australis: Invert icons in the tabstrip and menubar on Windows Classic

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Alice0775 White]

Attachment: screenshot

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/f2adb62d07eb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131118094134

Steps To Reproduce
0. Make sure setting Windows7 Classic visual style from control panel
1. Start Australis build
2. Open many tabs 
   or
   Customize and put icon in Tabbar

Actual Results:
Toolbuttons are almost invisible

Comment 2

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

Bumping to P3 to match the bug that got duped to this one.

Comment 3

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

Attachment: Patch

Comment 4

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

Attachment: WIP Patch v2

Comment 5

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

Attachment: Patch v2

Comment 6

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

Comment on attachment 8385393 [details] [diff] [review]
Patch v2

I didn't add a comment next to the windows-classic overrides yet. What do you think about this:

"Show icons in the TabsToolbar and #menubar as inverted when in Classic mode for improved contrast."

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8385393 [details] [diff] [review]
Patch v2

This doesn't seem to take into account that drawing in the title bar may be disabled.

Comment 8

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

Attachment: WIP Patch v4

Comment 9

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

Attachment: WIP Patch v4 (qref'd)

Comment 10

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

Attachment: Patch v5

Tested on my WinXP vm.

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8385618 [details] [diff] [review]
Patch v5

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

You forgot to not do this with LWT for some rules.

::: browser/themes/windows/browser.css
@@ +14,5 @@
>  %define navbarTextboxCustomBorder border-color: rgba(0,0,0,.32);
>  %define forwardTransitionLength 150ms
>  %define conditionalForwardWithUrlbar window:not([chromehidden~="toolbar"]) #urlbar-container
>  %define conditionalForwardWithUrlbarWidth 27
> +%define nestedButtons #zoom-out-button, #zoom-in-button, #cut-button, #copy-button, #paste-button

Nit: I would rather this move to browser/themes/shared/browser.inc so we don't need to move it again if OS X and Linux need it.

@@ +1651,5 @@
>  }
>  
> +@media (-moz-windows-classic) {
> +  #main-window[tabsintitlebar]:not([inFullscreen]) .tabbrowser-arrowscrollbox > .scrollbutton-up,
> +  #main-window[tabsintitlebar]:not([inFullscreen]) .tabbrowser-arrowscrollbox > .scrollbutton-down {

This could be cleaner by combining it with the ruleset above and using :-moz-system-metric(windows-classic)

@@ +1703,5 @@
>  
> +@media (-moz-windows-classic) {
> +  #main-window[tabsintitlebar]:not([inFullscreen]) .tabs-newtab-button,
> +  #main-window[tabsintitlebar]:not([inFullscreen]) #TabsToolbar > #new-tab-button,
> +  #main-window[tabsintitlebar]:not([inFullscreen]) #TabsToolbar > toolbarpaletteitem > #new-tab-button {

ditto

@@ +1721,5 @@
>    list-style-image: url("chrome://browser/skin/toolbarbutton-dropdown-arrow-inverted.png");
>  }
>  
> +@media (-moz-windows-classic) {
> +  #main-window[tabsintitlebar]:not([inFullscreen]) #alltabs-button {

ditto

Comment 12

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

Attachment: Patch v6

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8385641 [details] [diff] [review]
Patch v6

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

Thanks!

Nit: Including the menubar in your commit message

Comment 15

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

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

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/532818bf6b7a

Comment 17

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

Comment on attachment 8385641 [details] [diff] [review]
Patch v6

[Approval Request Comment]
Bug caused by (feature/regressing bug #): issue since beginning of australis
User impact if declined: poor contrast with icons customized to menubar or tabs toolbar
Testing completed (on m-c, etc.): locally and on m-c
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

There is a follow-up bug to also invert the text for the Zoom controls when placed in the menubar or tabs bar, but I don't think that bug should block this getting merged to Aurora.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/61d66f52f24e

Comment 19

[Ioana (away)]

Verified as fixed on the latest Firefox 29 beta and Firefox 30 Aurora on Windows XP 32bit.