Invert Tab Close Icon for background tabs on Windows Classic Theme

VERIFIED FIXED in Firefox 29

Status

()

Firefox
Theme
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: shorlander, Assigned: Gijs)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 31
All
Windows XP
Points:
---

Firefox Tracking Flags

(firefox29 verified, firefox30 verified, firefox31 verified)

Details

(Whiteboard: [Australis:P3+])

Attachments

(3 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

4 years ago
Whiteboard: [Australis:M?] → [Australis:P3+]
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.
Attachment #8394843 - Flags: ui-review?(shorlander)
Attachment #8394843 - Flags: review?(jaws)
(Reporter)

Updated

4 years ago
Attachment #8394843 - Flags: ui-review?(shorlander) → ui-review+
(Armen first reported it, and so might be interested in the progress… :)
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))
Attachment #8394843 - Flags: review?(jaws) → review-
Created attachment 8394858 [details] [diff] [review]
The second version of the patch.

Fixed!  Nice catch!

Carrying forward ui-r=shorlander.

Thanks,
Blake.
Attachment #8394843 - Attachment is obsolete: true
Attachment #8394858 - Flags: ui-review+
Attachment #8394858 - Flags: review?(jaws)
(Assignee)

Comment 5

4 years ago
(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?
That's where the other rules were.
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.
Attachment #8394858 - Flags: review?(jaws) → review-
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.)
Attachment #8394858 - Attachment is obsolete: true
Attachment #8394880 - Flags: ui-review+
Attachment #8394880 - Flags: review?(jaws)
Comment on attachment 8394880 [details] [diff] [review]
The third version of the patch.

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

r=me
Attachment #8394880 - Flags: review?(jaws) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/675945ee2609
Keywords: checkin-needed
Whiteboard: [Australis:P3+] → [Australis:P3+][fixed-in-fx-team]

Comment 11

4 years ago
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.
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…)
(Assignee)

Comment 13

4 years ago
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. :-(
(Assignee)

Comment 14

4 years ago
(it can go inside the existing windows-classic media query block)
(Assignee)

Comment 15

4 years ago
Blake, can you put up a new patch, please? I'd do it myself but I need to run out for the evening. :-(
Flags: needinfo?(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.
Flags: needinfo?(bwinton)
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.
Status: NEW → ASSIGNED
Whiteboard: [Australis:P3+][fixed-in-fx-team] → [Australis:P3+]
Attachment #8394880 - Flags: review+ → review-
Created attachment 8394970 [details] [diff] [review]
A different version.
Attachment #8394970 - Flags: review?(gijskruitbosch+bugs)
Blocks: 732583
Summary: Invert Tab Close Icon on Windows Classic Theme → Invert Tab Close Icon for background tabs on Windows Classic Theme
(Assignee)

Comment 19

4 years ago
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).
Attachment #8394970 - Flags: review?(gijskruitbosch+bugs) → feedback+
Assignee: bwinton → nobody
(Assignee)

Comment 20

4 years ago
Created attachment 8395654 [details] [diff] [review]
invert tab close icons on windows classic,
Attachment #8395654 - Flags: review?(jaws)
(Assignee)

Updated

4 years ago
Assignee: nobody → gijskruitbosch+bugs
Attachment #8395654 - Flags: review?(jaws) → review+
(Assignee)

Comment 21

4 years ago
remote:   https://hg.mozilla.org/integration/fx-team/rev/3c45dcaa5b4f
status-firefox29: --- → affected
status-firefox30: --- → affected
Whiteboard: [Australis:P3+] → [Australis:P3+][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/3c45dcaa5b4f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3+][fixed-in-fx-team] → [Australis:P3+]
Target Milestone: --- → Firefox 31
(Assignee)

Comment 23

4 years ago
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
Attachment #8395654 - Flags: approval-mozilla-beta?
Attachment #8395654 - Flags: approval-mozilla-aurora?
status-firefox31: --- → fixed
Attachment #8395654 - Flags: approval-mozilla-beta?
Attachment #8395654 - Flags: approval-mozilla-beta+
Attachment #8395654 - Flags: approval-mozilla-aurora?
Attachment #8395654 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/cec221286be2

https://hg.mozilla.org/releases/mozilla-beta/rev/476831b2be87
status-firefox29: affected → fixed
status-firefox30: affected → fixed
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).
Status: RESOLVED → VERIFIED
status-firefox29: fixed → verified
status-firefox30: fixed → verified
status-firefox31: fixed → verified
You need to log in before you can comment on or make changes to this bug.