Closed Bug 545557 Opened 11 years ago Closed 10 years ago

better windows theme colors when not in a11y theme mode

Categories

(Thunderbird :: Theme, defect)

All
Windows Vista
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.3a2

People

(Reporter: clarkbw, Assigned: Paenglab)

References

Details

(Whiteboard: [UXprio])

Attachments

(16 files, 28 obsolete files)

224.90 KB, image/png
Details
46.15 KB, image/png
Details
100.93 KB, image/png
Details
1.58 KB, patch
bwinton
: review+
Details | Diff | Splinter Review
7.82 KB, patch
Paenglab
: review+
Paenglab
: ui-review+
Details | Diff | Splinter Review
4.59 KB, patch
Paenglab
: review+
bwinton
: ui-review+
Details | Diff | Splinter Review
4.81 KB, patch
bwinton
: review+
Paenglab
: ui-review+
Details | Diff | Splinter Review
1.18 KB, patch
bwinton
: review+
Paenglab
: ui-review+
Details | Diff | Splinter Review
1.21 KB, patch
bwinton
: review+
Paenglab
: ui-review+
Details | Diff | Splinter Review
17.92 KB, patch
bwinton
: review+
Paenglab
: ui-review+
Details | Diff | Splinter Review
1.01 KB, patch
bwinton
: review+
Paenglab
: ui-review+
Details | Diff | Splinter Review
1.33 KB, patch
bwinton
: review+
Paenglab
: ui-review+
Details | Diff | Splinter Review
24.55 KB, patch
Paenglab
: review+
Paenglab
: ui-review+
Details | Diff | Splinter Review
41.37 KB, patch
Paenglab
: review+
Paenglab
: ui-review+
Details | Diff | Splinter Review
13.36 KB, patch
Paenglab
: review+
Paenglab
: ui-review+
Details | Diff | Splinter Review
2.15 KB, patch
Paenglab
: review+
Paenglab
: ui-review+
Details | Diff | Splinter Review
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
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.
(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.
I don't have any strong feelings about any kind of app button, but liked the colors and widget appearance.
Whiteboard: [UXprio]
(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
Assignee: nobody → nisses.mail
Depends on: 568187, 568185
Depends on: 568193, 568194
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.
Depends on: 569400
Attached patch Aero appearance patch (obsolete) — Splinter Review
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.
(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?
This is like the DOM for .tabmail-tabs looks on my computer.
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?
(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.
Disabling personas did the trick. So yes, please file a bug.
Depends on: 587980
Depends on: 588007
Attached patch Works now with multimessage view (obsolete) — Splinter Review
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.
Attachment #466312 - Attachment is obsolete: true
This works great on my system. Who should I set as code reviewer?
Comment on attachment 466658 [details] [diff] [review]
Works now with multimessage view

Checking what Bryan thinks about this.
Attachment #466658 - Flags: ui-review?(clarkbw)
Adding Blake to cc
Also updated because of potentially bitrot by Bug 588754 landing
Attachment #466658 - Attachment is obsolete: true
Attachment #468778 - Flags: ui-review?(clarkbw)
Attachment #466658 - Flags: ui-review?(clarkbw)
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.
Attached patch Part1 Buttons in Main window (obsolete) — Splinter Review
Attachment #468778 - Attachment is obsolete: true
Attachment #468778 - Flags: ui-review?(clarkbw)
Attached patch Part2 Buttons in Address book (obsolete) — Splinter Review
Attached patch Part3 Buttons in Composer window (obsolete) — Splinter Review
Attached patch Part4 Toolbars in Main window (obsolete) — Splinter Review
Attached patch Part5 Toolbars in Address book (obsolete) — Splinter Review
Attached patch Part7 Tabs (obsolete) — Splinter Review
The glyphs and the rest (splitter and search bars) comes tomorrow.

Andreas can you set the reviews to the correct persons?
Attached patch Part8 Icons in Main window (obsolete) — Splinter Review
primaryToolbar-aero.css is no more processed because we don't use the small icons anymore.
Attached patch Part9 Icons in Address book (obsolete) — Splinter Review
Attached patch Part10 Icons in Composer window (obsolete) — Splinter Review
Attached patch Part 11 Search filter styling (obsolete) — Splinter Review
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.
Attached patch Part12 Splitters in Main window (obsolete) — Splinter Review
Splitters like in Firefox Places.
Attached patch Part13 Splitter in Address book (obsolete) — Splinter Review
Make the splitter styling the same as in Main window.
This are now all patches. I hope I have nothing forgotten.
Starting to mass-assign for review to the brave Blake & Bryan.
Attachment #470332 - Flags: ui-review?(clarkbw)
Attachment #470332 - Flags: review?(bwinton)
Attachment #470333 - Flags: ui-review?(clarkbw)
Attachment #470333 - Flags: review?(bwinton)
Attachment #470334 - Flags: ui-review?(clarkbw)
Attachment #470334 - Flags: review?(bwinton)
Attachment #470335 - Flags: ui-review?(clarkbw)
Attachment #470335 - Flags: review?(bwinton)
Attachment #470336 - Flags: ui-review?(clarkbw)
Attachment #470336 - Flags: review?(bwinton)
Attachment #470337 - Flags: ui-review?(clarkbw)
Attachment #470337 - Flags: review?(bwinton)
Attachment #470339 - Flags: ui-review?(clarkbw)
Attachment #470339 - Flags: review?(bwinton)
Attachment #470536 - Flags: ui-review?(clarkbw)
Attachment #470536 - Flags: review?(bwinton)
Attachment #470537 - Flags: ui-review?(clarkbw)
Attachment #470537 - Flags: review?(bwinton)
Attachment #470538 - Flags: ui-review?(clarkbw)
Attachment #470538 - Flags: review?(bwinton)
Attachment #470544 - Flags: ui-review?(clarkbw)
Attachment #470544 - Flags: review?(bwinton)
Attachment #470546 - Flags: ui-review?(clarkbw)
Attachment #470546 - Flags: review?(bwinton)
Attachment #470549 - Flags: ui-review?(clarkbw)
Attachment #470549 - Flags: review?(bwinton)
If any of these makes sense to split out into into separate bugs, in order to ease review discussion, let's do that!
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.
Attachment #470332 - Flags: ui-review?(clarkbw) → ui-review?(nisses.mail)
Attachment #470333 - Flags: ui-review?(clarkbw) → ui-review?(nisses.mail)
Attachment #470334 - Flags: ui-review?(clarkbw) → ui-review?(nisses.mail)
Attachment #470335 - Flags: ui-review?(clarkbw) → ui-review?(nisses.mail)
Attachment #470336 - Flags: ui-review?(clarkbw) → ui-review?(nisses.mail)
Attachment #470337 - Flags: ui-review?(clarkbw) → ui-review?(nisses.mail)
Attachment #470339 - Flags: ui-review?(clarkbw) → ui-review?(nisses.mail)
Attachment #470536 - Flags: ui-review?(clarkbw) → ui-review?(nisses.mail)
Attachment #470537 - Flags: ui-review?(clarkbw) → ui-review?(nisses.mail)
Attachment #470538 - Flags: ui-review?(clarkbw) → ui-review?(nisses.mail)
Attachment #470544 - Flags: ui-review?(clarkbw) → ui-review?(nisses.mail)
Attachment #470546 - Flags: ui-review?(clarkbw) → ui-review?(nisses.mail)
Attachment #470549 - Flags: ui-review?(clarkbw) → ui-review?(nisses.mail)
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 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.
Attachment #470333 - Flags: ui-review?(nisses.mail) → ui-review+
(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 on attachment 470334 [details] [diff] [review]
Part3 Buttons in Composer window

This one looks good too!
Attachment #470334 - Flags: ui-review?(nisses.mail) → ui-review+
Comment on attachment 470339 [details] [diff] [review]
Part7 Tabs

Looks great!
Attachment #470339 - Flags: ui-review?(nisses.mail) → ui-review+
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.
(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.)
(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?
(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.
(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.
If it doesn't exist, and the patches apply in the order you posted, then don't worry about it.

Thanks,
Blake.
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).
Attachment #470332 - Flags: ui-review?(nisses.mail) → ui-review+
Comment on attachment 470335 [details] [diff] [review]
Part4 Toolbars in Main window

Nice and small change, ui-r+
Attachment #470335 - Flags: ui-review?(nisses.mail) → ui-review+
Rest of the reviews coming up shortly.
Attachment #470337 - Flags: ui-review?(nisses.mail) → ui-review+
Attachment #470336 - Flags: ui-review?(nisses.mail) → ui-review+
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 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
Attachment #470536 - Flags: ui-review?(nisses.mail) → ui-review-
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.
Attachment #470537 - Flags: ui-review?(nisses.mail) → ui-review+
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.
Attachment #470538 - Flags: ui-review?(nisses.mail) → ui-review+
Comment on attachment 470544 [details] [diff] [review]
Part 11 Search filter styling

All good!
Attachment #470544 - Flags: ui-review?(nisses.mail) → ui-review+
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.
Attachment #470546 - Flags: ui-review?(nisses.mail) → ui-review-
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-
(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)
Patch now with correct icons. -moz-border-radius into border-radius changed
Attachment #470536 - Attachment is obsolete: true
Attachment #490146 - Flags: ui-review?(nisses.mail)
Attachment #490146 - Flags: review?
Attachment #470536 - Flags: review?(bwinton)
(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.
Patch addressing comments from ui-review
Attachment #470546 - Attachment is obsolete: true
Attachment #490184 - Flags: ui-review?(nisses.mail)
Attachment #490184 - Flags: review?
Attachment #470546 - Flags: review?(bwinton)
Splitters again 1px but without space between vertical and horizontal splitter as in old addressbook-aero.css
Attachment #470549 - Attachment is obsolete: true
Attachment #490429 - Flags: ui-review?(nisses.mail)
Attachment #490429 - Flags: review?
Attachment #470549 - Flags: ui-review?(nisses.mail)
Attachment #470549 - Flags: review?(bwinton)
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)
Attachment #490146 - Flags: ui-review?(nisses.mail) → ui-review+
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.
Attachment #490184 - Flags: ui-review?(nisses.mail) → ui-review+
Comment on attachment 490429 [details] [diff] [review]
Part13 Splitter in Address book (1px wide)

Looks good too!
Attachment #490429 - Flags: ui-review?(nisses.mail) → ui-review+
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.
Attachment #470332 - Flags: review?(bwinton) → review+
Attached patch Part1 Buttons in Main window (obsolete) — Splinter Review
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.
Attachment #470332 - Attachment is obsolete: true
Attachment #491932 - Flags: ui-review+
Attachment #491932 - Flags: review?
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.
Attachment #491932 - Flags: review? → review+
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.
and I mean a TB build _on_ XP of course, the other one would be fun. :)
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.
(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 :)
Attached patch Part2 Buttons in Address book (obsolete) — Splinter Review
Patch addressing comments from review of part 1
Attachment #470333 - Attachment is obsolete: true
Attachment #492157 - Flags: ui-review+
Attachment #492157 - Flags: review?
Attachment #470333 - Flags: review?(bwinton)
Attached patch Part3 Buttons in Composer window (obsolete) — Splinter Review
Patch addressing comments from review of part 1.
Carrying over UI-r+ from previous patch.
Attachment #470334 - Attachment is obsolete: true
Attachment #492158 - Flags: ui-review+
Attachment #492158 - Flags: review?
Attachment #470334 - Flags: review?(bwinton)
Patch addressing comments from review of part 1.
Carrying over UI-r+ from previous patch.
Attachment #470335 - Attachment is obsolete: true
Attachment #492159 - Flags: review?
Attachment #470335 - Flags: review?(bwinton)
Attached patch Part5 Toolbars in Address book (obsolete) — Splinter Review
Patch addressing comments from review of part 1.
Carrying over UI-r+ from previous patch.
Attachment #470336 - Attachment is obsolete: true
Attachment #492160 - Flags: ui-review+
Attachment #492160 - Flags: review?
Attachment #470336 - Flags: review?(bwinton)
Patch addressing comments from review of part 1.
Carrying over UI-r+ from previous patch.
Attachment #470337 - Attachment is obsolete: true
Attachment #492161 - Flags: ui-review+
Attachment #492161 - Flags: review?
Attachment #470337 - Flags: review?(bwinton)
I'm waiting with uploading updated patches 7 to 13 until previous patches are reviewed.
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.
Attachment #492157 - Flags: review? → review+
(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.
(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.
Carrying over r+ and UI-r+ from previous patch.
Fixed trailing spaces.
Attachment #491932 - Attachment is obsolete: true
Attachment #492384 - Flags: ui-review+
Attachment #492384 - Flags: review+
Carrying over r+ and UI-r+ from previous patch.
Fixed trailing spaces.
Attachment #492157 - Attachment is obsolete: true
Attachment #492386 - Flags: superreview+
Attachment #492386 - Flags: review+
Carrying over UI-r+ from previous patch.
Fixed trailing spaces.
Attachment #492158 - Attachment is obsolete: true
Attachment #492388 - Flags: ui-review+
Attachment #492388 - Flags: review?
Attachment #492158 - Flags: review?
Carrying over r+ and UI-r+ from previous patch.
Fixed trailing spaces.
Attachment #492160 - Attachment is obsolete: true
Attachment #492390 - Flags: ui-review+
Attachment #492390 - Flags: review?
Attachment #492160 - Flags: review?
Carrying over UI-r+ from previous patch.
Fixed trailing spaces.
Attachment #492161 - Attachment is obsolete: true
Attachment #492392 - Flags: ui-review+
Attachment #492392 - Flags: review?
Attachment #492161 - Flags: review?
(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+
> > 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.
Assignee: nisses.mail → richard.marti
Status: NEW → ASSIGNED
Comment on attachment 492388 [details] [diff] [review]
Part3 Buttons in Composer window

Okay, I like this patch.
Attachment #492388 - Flags: review? → review+
Comment on attachment 492159 [details] [diff] [review]
Part4 Toolbars in Main window

This also looks good to me.

Later,
Blake.
Attachment #492159 - Flags: review? → review+
Comment on attachment 492390 [details] [diff] [review]
Part5 Toolbars in Address book

Looks simple enough.

Also, thanks for the extra documentation comments!
Attachment #492390 - Flags: review? → review+
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.
Attachment #492392 - Flags: review? → review+
And now, I'm going to wait to review patches 7 to 13 until you upload the updated versions of them.  ;)

Thanks,
Blake.
Comment on attachment 492386 [details] [diff] [review]
Part2 Buttons in Address book

(I think you meant to carry forward ui-r+, not sr+…)
Attachment #492386 - Flags: superreview+ → ui-review+
(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.
Attached patch Part7 Tabs (obsolete) — Splinter Review
Carrying UI-r+ from previous patch

Applied FF Bug 612812 to this patch.
Attachment #470339 - Attachment is obsolete: true
Attachment #492407 - Flags: ui-review+
Attachment #492407 - Flags: review?
Attachment #470339 - Flags: review?(bwinton)
Attached patch Part8 Icons in Main window (obsolete) — Splinter Review
Carrying UI-r+ from previous patch

Patch from Bug 594369 applied.
Attachment #490146 - Attachment is obsolete: true
Attachment #492409 - Flags: ui-review+
Attachment #492409 - Flags: review?
Attachment #490146 - Flags: review?
Attached patch Part9 Icons in Address book (obsolete) — Splinter Review
Carrying UI-r+ from previous patch
Attachment #470537 - Attachment is obsolete: true
Attachment #492410 - Flags: ui-review+
Attachment #492410 - Flags: review?
Attachment #470537 - Flags: review?(bwinton)
Carrying UI-r+ from previous patch
Attachment #470538 - Attachment is obsolete: true
Attachment #492411 - Flags: ui-review+
Attachment #492411 - Flags: review?
Attachment #470538 - Flags: review?(bwinton)
Attached patch Part 11 Search filter styling (obsolete) — Splinter Review
Carrying UI-r+ from previous patch
Attachment #470544 - Attachment is obsolete: true
Attachment #492412 - Flags: ui-review+
Attachment #492412 - Flags: review?
Attachment #470544 - Flags: review?(bwinton)
Carrying UI-r+ from previous patch
Attachment #490184 - Attachment is obsolete: true
Attachment #492413 - Flags: ui-review+
Attachment #492413 - Flags: review?
Attachment #490184 - Flags: review?
Carrying UI-r+ from previous patch
Attachment #490429 - Attachment is obsolete: true
Attachment #492414 - Flags: ui-review+
Attachment #492414 - Flags: review?
Attachment #490429 - Flags: review?
Attachment #492410 - Attachment is patch: true
Attachment #492410 - Attachment mime type: application/octet-stream → text/plain
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).
(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 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 <mscott@mozilla.org>.
>+* Portions created by the Initial Developer are Copyright (C) 2007
>+* the Initial Developer. All Rights Reserved.
>+*
>+* Contributor(s):
>+*   Richard Marti <mozilla@paenglab.ch>

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.
Attachment #492407 - Flags: review? → review+
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.
Attachment #492409 - Flags: review? → review+
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.
Attachment #492410 - Flags: review? → review+
Comment on attachment 492411 [details] [diff] [review]
Part10 Icons in Composer window

These changes look good to me.

Later,
Blake.
Attachment #492411 - Flags: review? → review+
(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 on attachment 492412 [details] [diff] [review]
Part 11 Search filter styling

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

Later,
Blake.
Attachment #492412 - Flags: review? → review+
Comment on attachment 492413 [details] [diff] [review]
Part12 Splitters in Main window

Makes sense to me.

Later,
Blake.
Attachment #492413 - Flags: review? → review+
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!
Attachment #492414 - Flags: review? → review+
Attached patch Part7 TabsSplinter Review
Carrying r+ and UI-r+ from previous patch.

Fixed boilerplate and indent.
Attachment #492407 - Attachment is obsolete: true
Attachment #493098 - Flags: ui-review+
Attachment #493098 - Flags: review+
Carrying r+ and UI-r+ from previous patch.

Fixed boilerplate and indent.
Attachment #492409 - Attachment is obsolete: true
Attachment #493101 - Flags: ui-review+
Attachment #493101 - Flags: review+
Carrying r+ and UI-r+ from previous patch.

Fixed indent.
Attachment #492410 - Attachment is obsolete: true
Attachment #493102 - Flags: ui-review+
Attachment #493102 - Flags: review+
Carrying r+ and UI-r+ from previous patch.

Updated because of preceding part update.
Attachment #492412 - Attachment is obsolete: true
Attachment #493105 - Flags: ui-review+
Attachment #493105 - Flags: review+
Attachment #493102 - Attachment is patch: true
Attachment #493102 - Attachment mime type: application/octet-stream → text/plain
For checkin: It's important to follow the numbering because some parts rely on previous parts.
Keywords: checkin-needed
Assignee: richard.marti → nobody
Component: Message Reader UI → Theme
QA Contact: message-reader → theme
Mass move to the new theme component.
Assignee: nobody → richard.marti
I now have the vast majority of the patches in my mq ready for landing. I'll be landing them tomorrow.
Whiteboard: [UXprio] → [UXprio] [standard8 will land tomorrow]
Depends on: 608792
(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.
Is there way to use good old colored icons in w7, via hidden pref?
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.
Depends on: 616932
Depends on: 618467
Depends on: 614215
Blocks: 644988
No longer blocks: 644988
Depends on: 668336
Depends on: 661745
You need to log in before you can comment on or make changes to this bug.