986529 - Invert Tab Close Icon for background tabs on Windows Classic Theme

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Comment 1

[Blake Winton (:bwinton) (:☕️)]

Attachment: The first cut at a patch that fixes the issue.

Screenshot with patch applied at https://www.dropbox.com/s/de70gnxev4m3z1j/Screenshot%202014-03-21%2011.17.26.png

jaws: I needed to add the extra rules to the hover and active because the rule for the inverted close icon is way more specific, and so was overriding them.

Comment 2

[Blake Winton (:bwinton) (:☕️)]

(Armen first reported it, and so might be interested in the progress… :)

Comment 3

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

Comment on attachment 8394843 [details] [diff] [review]
The first cut at a patch that fixes the issue.

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

::: toolkit/themes/windows/global/global.css
@@ +338,5 @@
>  .close-icon:hover:active {
>    -moz-image-region: rect(0, 48px, 16px, 32px);
>  }
>  
> +.tabbrowser-tab:not([selected=true]) .close-icon:-moz-system-metric(windows-classic):not(:-moz-lwtheme) {

You should be able to drop the other rule additions if you change this to:
.tabbrowser-tab:not([selected=true]) .close-icon:-moz-system-metric(windows-clasic):not(:-moz-lwtheme):not(:hover))

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Attachment: The second version of the patch.

Fixed!  Nice catch!

Carrying forward ui-r=shorlander.

Thanks,
Blake.

Comment 5

[:Gijs (he/him)]

(In reply to Blake Winton (:bwinton) from comment #4)
> Created attachment 8394858 [details] [diff] [review]
> The second version of the patch.
> 
> Fixed!  Nice catch!
> 
> Carrying forward ui-r=shorlander.
> 
> Thanks,
> Blake.

Err, why is this rule in a toolkit stylesheet?

Comment 6

[Blake Winton (:bwinton) (:☕️)]

That's where the other rules were.

Comment 7

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

Comment on attachment 8394858 [details] [diff] [review]
The second version of the patch.

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

::: toolkit/themes/windows/global/global.css
@@ +336,5 @@
>  .close-icon:hover:active {
>    -moz-image-region: rect(0, 48px, 16px, 32px);
>  }
>  
> +.tabbrowser-tab:not([selected=true]) .close-icon:-moz-system-metric(windows-classic):not(:-moz-lwtheme):not(:hover) {

Yeah, this can't go in toolkit because we don't have any mentions of .tabbrowser-tab within toolkit.

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Attachment: The third version of the patch.

Okay, how about this one, then?
(Turns out we propagate selected down to the close icon, likely for this sort of thing.)

Comment 9

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

Comment on attachment 8394880 [details] [diff] [review]
The third version of the patch.

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

r=me

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Armen [:armenzg]]

You guys rock!
Are we planning to uplift all the way to beta?
I will test it on Nightly on my wife's computer after I disable Aero.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

I'll ask for approval once it lands on mozilla-central, and we'll see what they say.  :)
(It's css-only, so it'll probably be fine, but it's also in toolkit…)

Comment 13

[:Gijs (he/him)]

I hate to driveby again, but really, this should be in browser/themes/windows/browser.css, and it should depend on having tabsintitlebar and the close-icon being inside the tabbrowser. Right now, if you turn off tabsintitlebar, the close icon will be white on light grey. :-(

Comment 14

[:Gijs (he/him)]

(it can go inside the existing windows-classic media query block)

Comment 15

[:Gijs (he/him)]

Blake, can you put up a new patch, please? I'd do it myself but I need to run out for the evening. :-(

Comment 16

[Blake Winton (:bwinton) (:☕️)]

Gijs: Given that it's fixing a rule in toolkit, I don't see why it should be in browser/themes…  Unless you mean that we should move the whole .close-icon ruleset into browser/themes, in which case, as a Thunderbird contributor, I kinda disagree, since Thunderbird would also like this behaviour.

I can certainly look in to fixing the tabsintitlebar stuff, but would rather do it in a followup bug, if you're okay with that.

Comment 17

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

I backed out the push in https://hg.mozilla.org/integration/fx-team/rev/9f635bf9ee9c

Let's just get it right in one push, and I'd rather not introduce a different regression in tomorrow's Nightly if the follow-up wasn't fixed in time.

Comment 18

[Blake Winton (:bwinton) (:☕️)]

Attachment: A different version.

Comment 19

[:Gijs (he/him)]

Comment on attachment 8394970 [details] [diff] [review]
A different version.

(In reply to Blake Winton (:bwinton) from comment #16)
> Gijs: Given that it's fixing a rule in toolkit, I don't see why it should be
> in browser/themes…  Unless you mean that we should move the whole
> .close-icon ruleset into browser/themes, in which case, as a Thunderbird
> contributor, I kinda disagree, since Thunderbird would also like this
> behaviour.

No, so... this is a little bit more complex. We use the .close-icon class for lots of close buttons, including e.g. the sidebars that we have, and (I think) various other bits. Its basic 'normal', 'hover' and 'active' states are therefore defined in Toolkit. See bug 879921 and bug 851001 for history. We do not want to invert the icon in all those cases, just in the tabbar. But the Firefox tabbar isn't part of toolkit, and therefore its styles shouldn't live there, either. I agree that ideally we would share some styles with Thunderbird for these tabbars, but we currently don't, as far as I know (we also don't have the same front-end tabsintitlebar implementation, I suspect, at least on OS X). I'm not even sure if tabs there currently use the same close icon as we do or not.

The inverted styles were added by bug 879616. They were added specifically for tabs; other places like the sidebars never invert their icons. I added styles for LWTs at the time, and fixed luna blue, but classic still faded dark to light at the time, so it didn't make sense to invert the icons (I think? Maybe I just forgot...). Jared just fixed the Windows 8 dark windowframe color case over in bug 940393.

The luna-blue inverted styles are:

http://hg.mozilla.org/mozilla-central/annotate/199e65efd08b/browser/themes/windows/browser.css#l1779

1779   #main-window[tabsintitlebar]:not([inFullscreen]) .tab-close-button:not(:-moz-any(:hover,:-moz-lwtheme,[selected="true"])) {
1780     -moz-image-region: rect(0, 64px, 16px, 48px);
1781   }

For consistency, it'd make sense to use the same selector here. As a bonus, it also covers fullscreen and has one fewer descendant selector. :-)

I'm sorry if I've been all stop-energy so far on this bug... I wonder if classic should also be using different tab separators, but that's a question for a followup bug and/or Stephen.

Anyway, long story short, please adjust the selector as per above, and as a final nit, place it above the window border style for lightweight themes under which you put it (which puts it next to the other classic gradient tabbar fixes; the LWT top border seems unrelated).

Comment 20

[:Gijs (he/him)]

Attachment: invert tab close icons on windows classic,

Comment 21

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/3c45dcaa5b4f

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c45dcaa5b4f

Comment 23

[:Gijs (he/him)]

Comment on attachment 8395654 [details] [diff] [review]
invert tab close icons on windows classic,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: tab close icons on windows classic theme are hard to see
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): very low, small classic-specific CSS change
String or IDL/UUID changes made by this patch: none

Comment 24

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/cec221286be2

https://hg.mozilla.org/releases/mozilla-beta/rev/476831b2be87

Comment 25

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

Mozilla/5.0 (Windows NT 5.2; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Windows NT 5.2; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Windows NT 5.2; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0

Verified as fixed on Windows XP using Firefox 29 beta 3 (build ID:20140327113732), latest Aurora (build ID:20140328004001) and latest Nightly (build ID: 20140328030203).