Last Comment Bug 986529 - Invert Tab Close Icon for background tabs on Windows Classic Theme
: Invert Tab Close Icon for background tabs on Windows Classic Theme
Status: VERIFIED FIXED
[Australis:P3+]
:
Product: Firefox
Classification: Client Software
Component: Theme (show other bugs)
: Trunk
: All Windows XP
-- normal (vote)
: Firefox 31
Assigned To: :Gijs
:
: Dão Gottwald [:dao]
Mentors:
Depends on:
Blocks: australis-tabs
  Show dependency treegraph
 
Reported: 2014-03-21 08:21 PDT by Stephen Horlander [:shorlander]
Modified: 2014-03-28 07:42 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
verified


Attachments
The first cut at a patch that fixes the issue. (1.35 KB, patch)
2014-03-21 08:32 PDT, Blake Winton (:bwinton) (:☕️)
jaws: review-
shorlander: ui‑review+
Details | Diff | Splinter Review
The second version of the patch. (986 bytes, patch)
2014-03-21 08:51 PDT, Blake Winton (:bwinton) (:☕️)
jaws: review-
bwinton: ui‑review+
Details | Diff | Splinter Review
The third version of the patch. (970 bytes, patch)
2014-03-21 09:27 PDT, Blake Winton (:bwinton) (:☕️)
jaws: review-
bwinton: ui‑review+
Details | Diff | Splinter Review
A different version. (1.24 KB, patch)
2014-03-21 11:58 PDT, Blake Winton (:bwinton) (:☕️)
gijskruitbosch+bugs: feedback+
Details | Diff | Splinter Review
invert tab close icons on windows classic, (1.35 KB, patch)
2014-03-24 07:23 PDT, :Gijs
jaws: review+
sledru: approval‑mozilla‑aurora+
sledru: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image Stephen Horlander [:shorlander] 2014-03-21 08:21:16 PDT

    
Comment 1 User image Blake Winton (:bwinton) (:☕️) 2014-03-21 08:32:33 PDT
Created attachment 8394843 [details] [diff] [review]
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 User image Blake Winton (:bwinton) (:☕️) 2014-03-21 08:45:45 PDT
(Armen first reported it, and so might be interested in the progress… :)
Comment 3 User image Jared Wein [:jaws] (please needinfo? me) 2014-03-21 08:46:44 PDT
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 User image Blake Winton (:bwinton) (:☕️) 2014-03-21 08:51:48 PDT
Created attachment 8394858 [details] [diff] [review]
The second version of the patch.

Fixed!  Nice catch!

Carrying forward ui-r=shorlander.

Thanks,
Blake.
Comment 5 User image :Gijs 2014-03-21 08:56:11 PDT
(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 User image Blake Winton (:bwinton) (:☕️) 2014-03-21 08:57:46 PDT
That's where the other rules were.
Comment 7 User image Jared Wein [:jaws] (please needinfo? me) 2014-03-21 09:18:14 PDT
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 User image Blake Winton (:bwinton) (:☕️) 2014-03-21 09:27:09 PDT
Created attachment 8394880 [details] [diff] [review]
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 User image Jared Wein [:jaws] (please needinfo? me) 2014-03-21 09:31:54 PDT
Comment on attachment 8394880 [details] [diff] [review]
The third version of the patch.

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

r=me
Comment 10 User image Ryan VanderMeulen [:RyanVM] 2014-03-21 10:46:04 PDT
https://hg.mozilla.org/integration/fx-team/rev/675945ee2609
Comment 11 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-03-21 11:00:53 PDT
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 User image Blake Winton (:bwinton) (:☕️) 2014-03-21 11:05:37 PDT
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 User image :Gijs 2014-03-21 11:10:51 PDT
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 User image :Gijs 2014-03-21 11:11:19 PDT
(it can go inside the existing windows-classic media query block)
Comment 15 User image :Gijs 2014-03-21 11:15:06 PDT
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 User image Blake Winton (:bwinton) (:☕️) 2014-03-21 11:18:44 PDT
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 User image Jared Wein [:jaws] (please needinfo? me) 2014-03-21 11:57:51 PDT
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 User image Blake Winton (:bwinton) (:☕️) 2014-03-21 11:58:56 PDT
Created attachment 8394970 [details] [diff] [review]
A different version.
Comment 19 User image :Gijs 2014-03-21 16:02:59 PDT
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 User image :Gijs 2014-03-24 07:23:21 PDT
Created attachment 8395654 [details] [diff] [review]
invert tab close icons on windows classic,
Comment 22 User image Carsten Book [:Tomcat] 2014-03-25 06:09:45 PDT
https://hg.mozilla.org/mozilla-central/rev/3c45dcaa5b4f
Comment 23 User image :Gijs 2014-03-26 02:18:47 PDT
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 25 User image Cornel Ionce [QA] (:cornel_ionce) 2014-03-28 07:42:54 PDT
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).

Note You need to log in before you can comment on or make changes to this bug.