545557 - better windows theme colors when not in a11y theme mode

Status: RESOLVED FIXED

(Thunderbird :: Theme, defect)

Description

[Bryan Clark (DevTools PM) [:clarkbw]]

Current CSS System colors are dull and dark, we should be using brighter and nicer colors that will make our users happy.

Using tricks like this we can use better, brighter, colors for certain areas of Thunderbird by detecting the theme being used.
http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/browser.css#61

Comment 1

[Andreas Nilsson (:andreasn)]

I think we can also get some inspiration from these mockups: https://wiki.mozilla.org/Firefox/4.0_Windows_Theme_Mockups and from Win7 apps like Paint, Notepad and File explorer.

Comment 2

[tymerkaev]

(In reply to comment #1)
> I think we can also get some inspiration from these mockups:
> https://wiki.mozilla.org/Firefox/4.0_Windows_Theme_Mockups and from Win7 apps
> like Paint, Notepad and File explorer.

This is unlikely. Of all the possible options for an interface with the App Button is selected the worst. It is unlikely that it will be final.

Comment 3

[Andreas Nilsson (:andreasn)]

I don't have any strong feelings about any kind of app button, but liked the colors and widget appearance.

Comment 4

[Andreas Nilsson (:andreasn)]

First mockup https://wiki.mozilla.org/Thunderbird:UX/Priorities/3.1/Theme-win7

Will add more stuff to that page.

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #4)
> First mockup https://wiki.mozilla.org/Thunderbird:UX/Priorities/3.1/Theme-win7

I like that, it's a lot cleaner and nicer with the brighter colors

Comment 6

[Heribert Wettels]

Shouldn't Thunderbird and Firefox, both being Mozilla apps, have a similar visual appearance?

I really love the work that Alex Faaborg does for Firefox. Even if I disagree with some details, the current designs for Firefox 4 Beta are great. Why is the design work not coordinated? Common icons for Firefox and Thunderbird - like in the old times - could help a lot.

Comment 7

[Andreas Nilsson (:andreasn)]

Attachment: Aero appearance patch

Here is a patch that I and Richard have been working on as part of bug 569400. This is most of the stuff without the actual glass on it (since it have some other implications and needs more testing and feedback), that the bug is about, so I'm posting the patch here instead.

Comment 8

[Andreas Nilsson (:andreasn)]

Attachment: patch in action (screenshot)

Comment 9

[Richard Marti (:Paenglab)]

(In reply to comment #8)
> Created attachment 466314 [details]
> patch in action (screenshot)

Why is .tabmail-tabs grey? This should also be light blue. In the patch I see no error about this. Please can you check with DOMi if line 49 of tabmail.css is used?

Comment 10

[Richard Marti (:Paenglab)]

Attachment: DOM of .tabmail-tabs on my computer

This is like the DOM for .tabmail-tabs looks on my computer.

Comment 11

[Richard Marti (:Paenglab)]

I've found the problem: With installed Personas our definition is over-steered by this Personas definition:

#messengerWindow[persona] #tabmail, .tabmail-tabs {
  background-color:transparent !important;
}

The comma after #tabmail is to much. After removing this, everything works as expected.

Should a bug be filed for Personas or will the lw-themes working without Personas in 3.2?

Comment 12

[Mark Banner (:standard8)]

(In reply to comment #11)
> Should a bug be filed for Personas or will the lw-themes working without
> Personas in 3.2?

Thunderbird 3.1 (and 3.2) already has lw-themes working without personas - the only reason for the personas extension is to provide an interface to drive the selection of a persona.

Comment 13

[Andreas Nilsson (:andreasn)]

Disabling personas did the trick. So yes, please file a bug.

Comment 14

[Richard Marti (:Paenglab)]

Attachment: Works now with multimessage view

Works now with multimessage view and unbitrotted because of Bug 581360.

Andreas, you haven't set ui-r? and r? so I leave it also.

Comment 15

[Andreas Nilsson (:andreasn)]

This works great on my system. Who should I set as code reviewer?

Comment 16

[Andreas Nilsson (:andreasn)]

Comment on attachment 466658 [details] [diff] [review]
Works now with multimessage view

Checking what Bryan thinks about this.

Comment 17

[Andreas Nilsson (:andreasn)]

Adding Blake to cc

Comment 18

[Richard Marti (:Paenglab)]

Attachment: Fixed gradient error in Main menu bar

Also updated because of potentially bitrot by Bug 588754 landing

Comment 19

[Andreas Nilsson (:andreasn)]

Bryan Clark and Dan Mosedale and myself discussed this patch a bit over the phone and we all agreed it was a great patch, but also quite big. In order to make it easier to review and to check in, a good approach would probably be to split it up into smaller pieces and get those reviewed separately. Easy targets would be:
* Only the tabs in one patch, only the toolbars in another.
* Split up compose window, address book etc.
* Icons in a separate patch.
* CSS that is just copied over from Firefox in one.

Comment 20

[Richard Marti (:Paenglab)]

Attachment: Part1 Buttons in Main window

Comment 21

[Richard Marti (:Paenglab)]

Attachment: Part2 Buttons in Address book

Comment 22

[Richard Marti (:Paenglab)]

Attachment: Part3 Buttons in Composer window

Comment 23

[Richard Marti (:Paenglab)]

Attachment: Part4 Toolbars in Main window

Comment 24

[Richard Marti (:Paenglab)]

Attachment: Part5 Toolbars in Address book

Comment 25

[Richard Marti (:Paenglab)]

Attachment: Part6 Toolbars in Composer window

Comment 26

[Richard Marti (:Paenglab)]

Attachment: Part7 Tabs

Comment 27

[Richard Marti (:Paenglab)]

The glyphs and the rest (splitter and search bars) comes tomorrow.

Andreas can you set the reviews to the correct persons?

Comment 28

[Richard Marti (:Paenglab)]

Attachment: Part8 Icons in Main window

primaryToolbar-aero.css is no more processed because we don't use the small icons anymore.

Comment 29

[Richard Marti (:Paenglab)]

Attachment: Part9 Icons in Address book

Comment 30

[Richard Marti (:Paenglab)]

Attachment: Part10 Icons in Composer window

Comment 31

[Richard Marti (:Paenglab)]

Attachment: Part 11 Search filter styling

Patch for Main window (GloDa and QuickFilterBar) and Address book together because separation makes no sense. The styling has to be the same.

Styling is a copy of Firefox.

Comment 32

[Richard Marti (:Paenglab)]

Attachment: Part12 Splitters in Main window

Splitters like in Firefox Places.

Comment 33

[Richard Marti (:Paenglab)]

Attachment: Part13 Splitter in Address book

Make the splitter styling the same as in Main window.

Comment 34

[Richard Marti (:Paenglab)]

This are now all patches. I hope I have nothing forgotten.

Comment 35

[Andreas Nilsson (:andreasn)]

Starting to mass-assign for review to the brave Blake & Bryan.

Comment 36

[Andreas Nilsson (:andreasn)]

If any of these makes sense to split out into into separate bugs, in order to ease review discussion, let's do that!

Comment 37

[Richard Marti (:Paenglab)]

I've changed the -moz-border-radius to border-radius (Bug 594991) on my local patches but I'm waiting for the reviews before I upload them here to not spam the bug.

Comment 38

[Bryan Clark (DevTools PM) [:clarkbw]]

ok, sorry for all the mail.  I've go over the code pieces (which look fine) but haven't reviewed this myself since I don't have a viable win7 machine to do a reasonable review.  So I've pushed the reviews over to Andreas in order to get this moving because it looks awesome and I'd like to see it land soon.

Comment 39

[Andreas Nilsson (:andreasn)]

Comment on attachment 470333 [details] [diff] [review]
Part2 Buttons in Address book

Looks good as far as I can see.
Only nitpick is the text color of the disabled buttons, and maybe that is addressed elsewhere in these patches. ui-r+ with that fixed.

Comment 40

[Richard Marti (:Paenglab)]

(In reply to comment #39)
> Comment on attachment 470333 [details] [diff] [review]
> Part2 Buttons in Address book
> Looks good as far as I can see.
> Only nitpick is the text color of the disabled buttons, and maybe that is
> addressed elsewhere in these patches. ui-r+ with that fixed.

What you don't like with the color? This is a direct copy from Firefox code and no change elsewhere in the patches.

Comment 41

[Andreas Nilsson (:andreasn)]

Comment on attachment 470334 [details] [diff] [review]
Part3 Buttons in Composer window

This one looks good too!

Comment 42

[Andreas Nilsson (:andreasn)]

Comment on attachment 470339 [details] [diff] [review]
Part7 Tabs

Looks great!

Comment 43

[Andreas Nilsson (:andreasn)]

Is there a particular order I need to apply these patches in?
It seems some of them fail to apply since they are looking for files that I suspect come with other patches.

Comment 44

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

(It might be nice if you could post one big roll-up patch, just for reference, Richard.  I won't review that one, but I'll probably apply it, and run the tests with it, if it exists.

Thanks,
Blake.)

Comment 45

[Richard Marti (:Paenglab)]

(In reply to comment #43)
> Is there a particular order I need to apply these patches in?
> It seems some of them fail to apply since they are looking for files that I
> suspect come with other patches.

When you follow the numbering (1 to 13) for applying it should work.

PS: you haven't replied to comment #40. Please can you do it so I can make the changes if needed?

Comment 46

[Richard Marti (:Paenglab)]

(In reply to comment #44)
> (It might be nice if you could post one big roll-up patch, just for reference,
> Richard.  I won't review that one, but I'll probably apply it, and run the
> tests with it, if it exists.

It doesn't exist, but I can do one. The only difference will be the -moz-border-radius to border-radius and -moz-box-shadow to box-shadow changes.

Comment 47

[Andreas Nilsson (:andreasn)]

(In reply to comment #40)
> (In reply to comment #39)
> > Comment on attachment 470333 [details] [diff] [review] [details]
> > Part2 Buttons in Address book
> > Looks good as far as I can see.
> > Only nitpick is the text color of the disabled buttons, and maybe that is
> > addressed elsewhere in these patches. ui-r+ with that fixed.
> 
> What you don't like with the color? This is a direct copy from Firefox code and
> no change elsewhere in the patches.

It didn't look disabled but had the same color of the text as the active ones. Will check what other apps do here, and if Firefox does it on purpose, we should as well.

Comment 48

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

If it doesn't exist, and the patches apply in the order you posted, then don't worry about it.

Thanks,
Blake.

Comment 49

[Andreas Nilsson (:andreasn)]

Comment on attachment 470332 [details] [diff] [review]
Part1 Buttons in Main window

UI wise this is good!
There seems to be a small extra space before the '*' on the line containing
'+ * skin/classic/aero/messenger/messageHeader.css                   (mail/messageHeader-aero.css)' and that makes the patch unbuildable (Jarbuilder freaks out).

Comment 50

[Andreas Nilsson (:andreasn)]

Comment on attachment 470335 [details] [diff] [review]
Part4 Toolbars in Main window

Nice and small change, ui-r+

Comment 51

[Andreas Nilsson (:andreasn)]

Rest of the reviews coming up shortly.

Comment 52

[Andreas Nilsson (:andreasn)]

due to the issue mentioned in comment 49, the patch in Part8 needs to be modified a bit before being able to apply due to the needed changes in part1.

Comment 53

[Andreas Nilsson (:andreasn)]

Comment on attachment 470536 [details] [diff] [review]
Part8 Icons in Main window

It seems this patch is lacking the actual icons for the main toolbar, and instead comes with addressbook-toolbar-aero.png

Comment 54

[Andreas Nilsson (:andreasn)]

Comment on attachment 470537 [details] [diff] [review]
Part9 Icons in Address book

I like it.
A small side note that this patch (to some extent) makes the [x] small icons checkbox in the toolbar editor window redundant.

Comment 55

[Andreas Nilsson (:andreasn)]

Comment on attachment 470538 [details] [diff] [review]
Part10 Icons in Composer window

We should also do new graphics for the composition toolbar, but that can be another bug.

Comment 56

[Andreas Nilsson (:andreasn)]

Comment on attachment 470544 [details] [diff] [review]
Part 11 Search filter styling

All good!

Comment 57

[Andreas Nilsson (:andreasn)]

Comment on attachment 470546 [details] [diff] [review]
Part12 Splitters in Main window

I'm not too happy about the 2px wide dividers. It would probably look better with a 1px line as we do in the address book.

Comment 58

[Andreas Nilsson (:andreasn)]

Attachment: address book vs. main window (screenshot)

Comment 59

[Andreas Nilsson (:andreasn)]

oh, you did that in Part13 as well I see.
Lets discuss where we want to go with that before I give a ui+ or a ui-

Comment 60

[Thomas Stache]

(In reply to comment #57)
> Comment on attachment 470546 [details] [diff] [review]
> Part12 Splitters in Main window
> 
> I'm not too happy about the 2px wide dividers. It would probably look better
> with a 1px line as we do in the address book.

How large would the resize hit-area of the splitter be? Does the patch employ something like Firefox' Places Library window? (The splitter there looks like 1px wide, but offers a 3px wide hit area)

Comment 61

[Richard Marti (:Paenglab)]

Attachment: Part8 Icons in Main window (now correct)

Patch now with correct icons. -moz-border-radius into border-radius changed

Comment 62

[Richard Marti (:Paenglab)]

(In reply to comment #57)
> Comment on attachment 470546 [details] [diff] [review]
> Part12 Splitters in Main window
> 
> I'm not too happy about the 2px wide dividers. It would probably look better
> with a 1px line as we do in the address book.

The splitters are like the splitter in AB 1px but the borders from the surrounding trees makes the splitter 1px wider.

I'm working on a new patch correcting this.

(In reply to comment #60)
> How large would the resize hit-area of the splitter be? Does the patch employ
> something like Firefox' Places Library window? (The splitter there looks like
> 1px wide, but offers a 3px wide hit area)

The hit-area is the same. This is a copy from the FF css.

Comment 63

[Richard Marti (:Paenglab)]

Attachment: Part12 Splitters in Main window (slimmer styling)

Patch addressing comments from ui-review

Comment 64

[Richard Marti (:Paenglab)]

Attachment: Part13 Splitter in Address book (1px wide)

Splitters again 1px but without space between vertical and horizontal splitter as in old addressbook-aero.css

Comment 65

[Andreas Nilsson (:andreasn)]

Comment on attachment 490146 [details] [diff] [review]
Part8 Icons in Main window (now correct)

Looks good. Had some negative feedback on the mono-colored icons from one person on IRC, but I feel this is the way forward and Firefox is doing this too.
(and it's starting to look really slick now)

Comment 66

[Andreas Nilsson (:andreasn)]

Comment on attachment 490184 [details] [diff] [review]
Part12 Splitters in Main window (slimmer styling)

Looks good. We might want to give some color to the sidebar in a similar way to the bookmarks/history sidepane in Firefox, but lets do that as another bug.

Comment 67

[Andreas Nilsson (:andreasn)]

Comment on attachment 490429 [details] [diff] [review]
Part13 Splitter in Address book (1px wide)

Looks good too!

Comment 68

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

Comment on attachment 470332 [details] [diff] [review]
Part1 Buttons in Main window

>+++ b/mail/themes/qute/mail/primaryToolbar-aero.css
>@@ -0,0 +1,114 @@
>+%include primaryToolbar.css
>+
>+.toolbarbutton-menubutton-button,
>+.toolbarbutton-menubutton-dropmarker,
>+.toolbarbutton-1 {
>+  -moz-appearance: none;
>+  padding: 1px 5px !important;
>+  background: rgba(151,152,153,.05)
>+              -moz-linear-gradient(rgba(251,252,253,.95), rgba(246,247,248,.47) 49%, 

Please get rid of the trailing space on this line.

[snip…]

>+.toolbarbutton-menubutton-button:not([disabled="true"]):not(:active):hover,
>+toolbarbutton[type="menu-button"]:not([open="true"]):not(:active):hover > .toolbarbutton-menubutton-dropmarker:not([disabled="true"]),

I would prefer to see this line split as:

toolbarbutton[type="menu-button"]:not([open="true"]):not(:active):hover >
  .toolbarbutton-menubutton-dropmarker:not([disabled="true"]),

So that it's less than 80 columns.

>+toolbarbutton[type="menu-button"]:hover:active > .toolbarbutton-menubutton-dropmarker:not([disabled="true"]),
>+toolbarbutton[type="menu-button"][open="true"] > .toolbarbutton-menubutton-dropmarker,

Ditto these two lines.

But those are all minor things, so fix them, and the stuff Andreas mentioned, and you've got an r=me.

Thanks,
Blake.

Comment 69

[Richard Marti (:Paenglab)]

Attachment: Part1 Buttons in Main window

Carrying over UI-r+ from previous patch.

I'm asking for review again because this line is 83 chars long and I don't know if it's okay like this or I should shorten this (but then how?): 

.toolbarbutton-1:not([disabled="true"]):not([checked="true"]):not(:active):hover {

Then I can shorten the button definitions for AB and composer the same way.

Comment 70

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

Comment on attachment 491932 [details] [diff] [review]
Part1 Buttons in Main window

(In reply to comment #69)
> I'm asking for review again because this line is 83 chars long and I don't know
> if it's okay like this or I should shorten this (but then how?): 

Yeah, there's no really good way to shorten that line, and it's only a little over 80 characters, so I'ld say leave it the way it is.  (It's a bit of a judgement call, I agree.)

Thanks,
Blake.

Comment 71

[Andreas Nilsson (:andreasn)]

Blake told me on IRC he wanted some screenshots on how things looked under Windows, but the tryserver I put up failed. So if anyone watching this bug has a TB build of XP running these patches, screenshots are welcome.

Comment 72

[Andreas Nilsson (:andreasn)]

and I mean a TB build _on_ XP of course, the other one would be fun. :)

Comment 73

[Richard Marti (:Paenglab)]

This patches shouldn't affect TB on XP because all patches applying only in aero space (Vista and Win7).

I'm no fan of the FF Vista/Win7 buttons under XP.

Comment 74

[tymerkaev]

(In reply to comment #73)
> This patches shouldn't affect TB on XP because all patches applying only in
> aero space (Vista and Win7).
> 
> I'm no fan of the FF Vista/Win7 buttons under XP.

These buttons also not supposed to be visible in Text/Full modes. But you doing that for them :)

Comment 75

[Richard Marti (:Paenglab)]

Attachment: Part2 Buttons in Address book

Patch addressing comments from review of part 1

Comment 76

[Richard Marti (:Paenglab)]

Attachment: Part3 Buttons in Composer window

Patch addressing comments from review of part 1.
Carrying over UI-r+ from previous patch.

Comment 77

[Richard Marti (:Paenglab)]

Attachment: Part4 Toolbars in Main window

Patch addressing comments from review of part 1.
Carrying over UI-r+ from previous patch.

Comment 78

[Richard Marti (:Paenglab)]

Attachment: Part5 Toolbars in Address book

Patch addressing comments from review of part 1.
Carrying over UI-r+ from previous patch.

Comment 79

[Richard Marti (:Paenglab)]

Attachment: Part6 Toolbars in Composer window

Patch addressing comments from review of part 1.
Carrying over UI-r+ from previous patch.

Comment 80

[Richard Marti (:Paenglab)]

I'm waiting with uploading updated patches 7 to 13 until previous patches are reviewed.

Comment 81

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

Comment on attachment 492157 [details] [diff] [review]
Part2 Buttons in Address book

>+++ b/mail/themes/qute/mail/addrbook/addressbook-aero.css
>@@ -36,6 +37,124 @@
>+.toolbarbutton-menubutton-button,
[snip…]
>+toolbarbutton[type="menu-button"] {

How does that differ from .toolbarbutton-menubutton-button?

>+  -moz-appearance: none !important;
>+  padding: 0 !important;
>+  background: none !important;
>+  border: none !important;
>+  box-shadow: none !important;

Do you really need the important here?  (It seems like it would make it harder for other people to override.)

Finally, the large Info icon isn't centered, as shown in http://dl.dropbox.com/u/2301433/Screenshots/TooLargeI.png

(But I trust that you can fix these without needing another review from me, so I'll say r+ with those things fixed.)

Thanks,
Blake.

Comment 82

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

(In reply to comment #81)
> (But I trust that you can fix these without needing another review from me, so
> I'll say r+ with those things fixed.)

Oh, and there's a trailing space at the end of:
-moz-linear-gradient(rgba(251, 252, 253, .95), 
that you should get rid of.  (Actually, just go through all the patches, and change all the tabs to spaces, and get rid of all the trailing spaces, and I won't mention it in the rest of my reviews.)

Thanks,
Blake.

Comment 83

[Richard Marti (:Paenglab)]

(In reply to comment #81)
> Comment on attachment 492157 [details] [diff] [review]
> Part2 Buttons in Address book
> 
> >+++ b/mail/themes/qute/mail/addrbook/addressbook-aero.css
> >@@ -36,6 +37,124 @@
> >+.toolbarbutton-menubutton-button,
> [snip…]
> >+toolbarbutton[type="menu-button"] {
> 
> How does that differ from .toolbarbutton-menubutton-button?

toolbarbutton[type="menu-button"] is the container containing .toolbarbutton-menubutton-button and .toolbarbutton-menubutton-dropmarker.

> >+  -moz-appearance: none !important;
> >+  padding: 0 !important;
> >+  background: none !important;
> >+  border: none !important;
> >+  box-shadow: none !important;
> 
> Do you really need the important here?  (It seems like it would make it harder
> for other people to override.)

Yes, if not, the :hover and :hover:active rules will also affect the container. This ends in doubled box-shadow and weird backgrounds.

> Finally, the large Info icon isn't centered, as shown in
> http://dl.dropbox.com/u/2301433/Screenshots/TooLargeI.png

I leave it like it's now. With the patches 8-10 the correct button icons will apply. See <http://mozilla.paenglab.ch/temp/AB-buttons.png>.

This came because splitted patches was desired.

Comment 84

[Richard Marti (:Paenglab)]

Attachment: Part1 Buttons in Main window

Carrying over r+ and UI-r+ from previous patch.
Fixed trailing spaces.

Comment 85

[Richard Marti (:Paenglab)]

Attachment: Part2 Buttons in Address book

Carrying over r+ and UI-r+ from previous patch.
Fixed trailing spaces.

Comment 86

[Richard Marti (:Paenglab)]

Attachment: Part3 Buttons in Composer window

Carrying over UI-r+ from previous patch.
Fixed trailing spaces.

Comment 87

[Richard Marti (:Paenglab)]

Attachment: Part5 Toolbars in Address book

Carrying over r+ and UI-r+ from previous patch.
Fixed trailing spaces.

Comment 88

[Richard Marti (:Paenglab)]

Attachment: Part6 Toolbars in Composer window

Carrying over UI-r+ from previous patch.
Fixed trailing spaces.

Comment 89

[Richard Marti (:Paenglab)]

(In reply to comment #87)
> Created attachment 492390 [details] [diff] [review]
> Part5 Toolbars in Address book
> 
> Carrying over r+ and UI-r+ from previous patch.
> Fixed trailing spaces.

Sorry, naturally carrying only UI-r+

Comment 90

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

> > Finally, the large Info icon isn't centered, as shown in
> > http://dl.dropbox.com/u/2301433/Screenshots/TooLargeI.png
> I leave it like it's now. With the patches 8-10 the correct button icons will
> apply. See <http://mozilla.paenglab.ch/temp/AB-buttons.png>.

Ooh, pretty!  :)

> This came because splitted patches was desired.

Yup, and I like all your answers here.

(I'm also going to assign this to you, since it looks like you're doing all the
work on it.)

Thanks,
Blake.

Comment 91

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

Comment on attachment 492388 [details] [diff] [review]
Part3 Buttons in Composer window

Okay, I like this patch.

Comment 92

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

Comment on attachment 492159 [details] [diff] [review]
Part4 Toolbars in Main window

This also looks good to me.

Later,
Blake.

Comment 93

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

Comment on attachment 492390 [details] [diff] [review]
Part5 Toolbars in Address book

Looks simple enough.

Also, thanks for the extra documentation comments!

Comment 94

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

Comment on attachment 492392 [details] [diff] [review]
Part6 Toolbars in Composer window

>+++ b/mail/themes/qute/mail/compose/messengercompose-aero.css
>@@ -47,6 +47,37 @@
>+#compose-toolbox > toolbar:last-of-type:not(:-moz-lwtheme) {

So, with the addressbook, you had:
toolbox > toolbar:last-of-type:not(:-moz-lwtheme)
but with the compose, you have:
#compose-toolbox > toolbar:last-of-type:not(:-moz-lwtheme) {

Why are those different?

(r=me with a decent explanation.  Also, if you think that using #compose-toolbox is better, we can change the addressbook XUL to include an appropriate id.)

Thanks,
Blake.

Comment 95

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

And now, I'm going to wait to review patches 7 to 13 until you upload the updated versions of them.  ;)

Thanks,
Blake.

Comment 96

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

Comment on attachment 492386 [details] [diff] [review]
Part2 Buttons in Address book

(I think you meant to carry forward ui-r+, not sr+…)

Comment 97

[Richard Marti (:Paenglab)]

(In reply to comment #94)
> Comment on attachment 492392 [details] [diff] [review]
> Part6 Toolbars in Composer window
> 
> >+++ b/mail/themes/qute/mail/compose/messengercompose-aero.css
> >@@ -47,6 +47,37 @@
> >+#compose-toolbox > toolbar:last-of-type:not(:-moz-lwtheme) {
> 
> So, with the addressbook, you had:
> toolbox > toolbar:last-of-type:not(:-moz-lwtheme)
> but with the compose, you have:
> #compose-toolbox > toolbar:last-of-type:not(:-moz-lwtheme) {
> 
> Why are those different?
> 
> (r=me with a decent explanation.  Also, if you think that using
> #compose-toolbox is better, we can change the addressbook XUL to include an
> appropriate id.)

The composer has two toolboxes, the standard toolbox on top and the toolbox containing the addresses-box, the attachment-box and the format toolbar. When the definition is to global also the second toolbox becomes this styling and this looks not good.

Comment 98

[Richard Marti (:Paenglab)]

Attachment: Part7 Tabs

Carrying UI-r+ from previous patch

Applied FF Bug 612812 to this patch.

Comment 99

[Richard Marti (:Paenglab)]

Attachment: Part8 Icons in Main window

Carrying UI-r+ from previous patch

Patch from Bug 594369 applied.

Comment 100

[Richard Marti (:Paenglab)]

Attachment: Part9 Icons in Address book

Carrying UI-r+ from previous patch

Comment 101

[Richard Marti (:Paenglab)]

Attachment: Part10 Icons in Composer window

Carrying UI-r+ from previous patch

Comment 102

[Richard Marti (:Paenglab)]

Attachment: Part 11 Search filter styling

Carrying UI-r+ from previous patch

Comment 103

[Richard Marti (:Paenglab)]

Attachment: Part12 Splitters in Main window

Carrying UI-r+ from previous patch

Comment 104

[Richard Marti (:Paenglab)]

Attachment: Part13 Splitter in Address book

Carrying UI-r+ from previous patch

Comment 105

[Richard Marti (:Paenglab)]

To clarify: when I have made a full -aero.css without %import from base css,
then it was because it was to complex to override or invert stylings from the
base css. And I think this could also affect the performance (IMHO).

Comment 106

[Andreas Nilsson (:andreasn)]

(In reply to comment #66)
> Comment on attachment 490184 [details] [diff] [review]
> Part12 Splitters in Main window (slimmer styling)
> 
> Looks good. We might want to give some color to the sidebar in a similar way to
> the bookmarks/history sidepane in Firefox, but lets do that as another bug.

Filed as Bug 614215

Comment 107

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

Comment on attachment 492407 [details] [diff] [review]
Part7 Tabs

>+++ b/mail/themes/qute/jar.mn
>@@ -420,31 +420,10 @@
>+  skin/classic/aero/messenger/icons/tab.png                      (mail/icons/tab.png)

I like what you've done here, with the single image, and the CSS to slice it up.

>+++ b/mail/themes/qute/mail/tabmail-aero.css
>@@ -0,0 +1,365 @@
>+* The Original Code is tabmail
>+* The Initial Developer of the Original Code is
>+*   Scott MacGregor <[e-mail]>.
>+* Portions created by the Initial Developer are Copyright (C) 2007
>+* the Initial Developer. All Rights Reserved.
>+*
>+* Contributor(s):
>+*   Richard Marti <[e-mail]>

So, it looks like you created this file, and if that's true, then you're the initial developer, and the copyright should be 2010.  (If you'ld like, you can make "The Mozilla Foundation" the initial developer, but I don't believe that's required.)

>+.tabmail-tab[selected="true"] > .tab-close-button {
>+  /* Make this button focusable so clicking on it will not focus the tab while
>+    it's getting closed */

One more space for this indent, please.

>+++ b/mail/themes/qute/mail/quickFilterBar-aero.css
>@@ -0,0 +1,183 @@
>+@import url("chrome://messenger/content/quickFilterBar.css");
>+

This file should also have the standard license boilerplate, which you can find at <http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c>

Other than that, I'm happy with it.  r=me.

Later,
Blake.

Comment 108

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

Comment on attachment 492409 [details] [diff] [review]
Part8 Icons in Main window

>+++ b/mail/themes/qute/mail/primaryToolbar-aero.css
>@@ -1,4 +1,51 @@
>-%include primaryToolbar.css
>+ * The Original Code is Mozilla Communicator client code, released
>+ * March 31, 1998.

That doesn't seem right…

>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 1998-1999

And I'm pretty sure neither of those are right.  :)
"The Mozilla Foundation" and "2010" are more likely the words you're looking for.

>+++ b/mail/themes/qute/mail/messageHeader-aero.css
>@@ -1,25 +1,239 @@
>+* The Original Code is Mozilla Communicator client code, released
>+* March 31, 1998.
>+* The Initial Developer of the Original Code is
>+* Netscape Communications Corporation.
>+* Portions created by the Initial Developer are Copyright (C) 1998-1999
>+* the Initial Developer. All Rights Reserved.

Ditto from above.

>+/* toolbar[mode="text"] is necessary so that we end up with more specificity
>+* than the !important rule in toolkit's toolbar.css.
>+*/

One extra space of indent for those two lines, please.

>@@ -30,8 +244,425 @@
>+/* For unclear reasons, toolkit's toolbarbutton.css forces a margin of 0,
>+* jamming the image up against the text, so we need this !important
>+* rule to override that.
>+*
>+* The second selector is a descendent selector rather than a child selector
>+* so that we effect both buttons and menubuttons.
>+*/

And those.

But that's small enough for me to give an r+, and trust you to fix them.

Later,
Blake.

Comment 109

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

Comment on attachment 492410 [details] [diff] [review]
Part9 Icons in Address book

>+++ b/mail/themes/qute/mail/addrbook/addressbook-aero.css
>@@ -36,8 +36,16 @@
>+/* ===== addressbook.css ================================================
>+  == Styles for the main Address Book window.
>+  ======================================================================= */

Those last two lines should probably be indented another space.

Other than that, looks good to me.

Thanks,
Blake.

Comment 110

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

Comment on attachment 492411 [details] [diff] [review]
Part10 Icons in Composer window

These changes look good to me.

Later,
Blake.

Comment 111

[Richard Marti (:Paenglab)]

(In reply to comment #107)
> Comment on attachment 492407 [details] [diff] [review]
> Part7 Tabs
> 
> >+++ b/mail/themes/qute/mail/tabmail-aero.css
> 
> So, it looks like you created this file, and if that's true, then you're the
> initial developer, and the copyright should be 2010.  (If you'ld like, you can
> make "The Mozilla Foundation" the initial developer, but I don't believe that's
> required.)
> 
> >+++ b/mail/themes/qute/mail/quickFilterBar-aero.css
> >@@ -0,0 +1,183 @@
> >+@import url("chrome://messenger/content/quickFilterBar.css");
> >+
> 
> This file should also have the standard license boilerplate, which you can find
> at <http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c>

I'm not sure if it's correct to take a new boilerplate because this are only copied from the original files and then maybe 10-20% changed by me. So I feel more like a contributor. But when you say, it's okay, I'm using the newest boilerplate.

Comment 112

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

Comment on attachment 492412 [details] [diff] [review]
Part 11 Search filter styling

Nice small patch.  So much easier to review!  :)

Later,
Blake.

Comment 113

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

Comment on attachment 492413 [details] [diff] [review]
Part12 Splitters in Main window

Makes sense to me.

Later,
Blake.

Comment 114

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

Comment on attachment 492414 [details] [diff] [review]
Part13 Splitter in Address book

Yup, I like it.  And with that, I think we're done.

Thank you for your work on these patches!

Comment 115

[Richard Marti (:Paenglab)]

Attachment: Part7 Tabs

Carrying r+ and UI-r+ from previous patch.

Fixed boilerplate and indent.

Comment 116

[Richard Marti (:Paenglab)]

Attachment: Part8 Icons in Main window

Carrying r+ and UI-r+ from previous patch.

Fixed boilerplate and indent.

Comment 117

[Richard Marti (:Paenglab)]

Attachment: Part9 Icons in Address book

Carrying r+ and UI-r+ from previous patch.

Fixed indent.

Comment 118

[Richard Marti (:Paenglab)]

Attachment: Part11 Search filter styling

Carrying r+ and UI-r+ from previous patch.

Updated because of preceding part update.

Comment 119

[Richard Marti (:Paenglab)]

For checkin: It's important to follow the numbering because some parts rely on previous parts.

Comment 120

[Ludovic Hirlimann [:Usul]]

Mass move to the new theme component.

Comment 121

[Mark Banner (:standard8)]

I now have the vast majority of the patches in my mq ready for landing. I'll be landing them tomorrow.

Comment 122

[Mark Banner (:standard8)]

Checked in:
http://hg.mozilla.org/comm-central/rev/70b77c6bb7bd
http://hg.mozilla.org/comm-central/rev/7f4fa309776d
http://hg.mozilla.org/comm-central/rev/57f7ffa85454
http://hg.mozilla.org/comm-central/rev/7795cf4483d9
http://hg.mozilla.org/comm-central/rev/e607872375ec
http://hg.mozilla.org/comm-central/rev/c9a7fbe44016
http://hg.mozilla.org/comm-central/rev/de0b65b3389b
http://hg.mozilla.org/comm-central/rev/e2c14ab70766
http://hg.mozilla.org/comm-central/rev/6adcd010e075
http://hg.mozilla.org/comm-central/rev/37ba7bbcb313
http://hg.mozilla.org/comm-central/rev/ac0dd508bbe3
http://hg.mozilla.org/comm-central/rev/31697b85904e
http://hg.mozilla.org/comm-central/rev/179391cb5a80

Please file follow-up issues in new bugs.

Comment 123

[rsx11m]

(In reply to comment #73)
> This patches shouldn't affect TB on XP because all patches applying only in
> aero space (Vista and Win7).

You can simulate the look on XP as described in bug 608792 comment #4. However, keep in mind that the aero space applies to *all* Vista and Windows 7 desktop themes, including Windows Classic and the High Contrast themes. Consequently, the redesigned gradient-only toolbar icons are also applied for a11y themes, making them virtually invisible on such themes attachment 495206 [details]).

For future theme modifications, please verify at least in Windows Classic and accessibility themes that no regressions occur. I've amended bug 608792 with respect to the icon issue, thus it hopefully will be covered there.

Comment 124

[Nikolay Shopik]

Is there way to use good old colored icons in w7, via hidden pref?

Comment 125

[rsx11m]

No, but see http://forums.mozillazine.org/viewtopic.php?p=10192649#p10192649 for a hack. That file is now "nonlocalized.manifest"in current trunk builds.