Last Comment Bug 579738 - Make lightweight themes / personas work in mailnews windows
: Make lightweight themes / personas work in mailnews windows
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Themes (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b2
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on: SM-lwtheme 609877 612144
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-18 08:45 PDT by Robert Kaiser (not working on stability any more)
Modified: 2010-11-14 10:04 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wanted


Attachments
basic lwthemes support (3.11 KB, patch)
2010-10-29 16:06 PDT, Jens Hatlak (:InvisibleSmiley)
mnyromyr: feedback+
Details | Diff | Review
patch v2 (6.64 KB, patch)
2010-10-30 14:06 PDT, Jens Hatlak (:InvisibleSmiley)
neil: superreview-
Details | Diff | Review
patch v2a (6.53 KB, patch)
2010-11-01 07:01 PDT, Jens Hatlak (:InvisibleSmiley)
neil: superreview-
Details | Diff | Review
patch v3 (11.73 KB, patch)
2010-11-05 15:37 PDT, Jens Hatlak (:InvisibleSmiley)
neil: superreview+
Details | Diff | Review
patch v3a (13.63 KB, patch)
2010-11-08 14:34 PST, Jens Hatlak (:InvisibleSmiley)
stefanh: review-
jh: superreview+
Details | Diff | Review
patch v3b [Checkin: comment 22] (15.33 KB, patch)
2010-11-09 15:11 PST, Jens Hatlak (:InvisibleSmiley)
stefanh: review+
jh: superreview+
Details | Diff | Review
fix compose subject opacity (1.38 KB, patch)
2010-11-13 09:50 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Review
fix compose opacity (1.46 KB, patch)
2010-11-14 02:17 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Review
fix compose opacity v1a [Checkin: comment 30] (1.41 KB, patch)
2010-11-14 02:45 PST, Jens Hatlak (:InvisibleSmiley)
neil: review+
neil: superreview+
Details | Diff | Review

Description Robert Kaiser (not working on stability any more) 2010-07-18 08:45:35 PDT
We should also make lwthemes work in mailnews windows once they work in browser.
Comment 1 Jens Hatlak (:InvisibleSmiley) 2010-10-19 14:50:19 PDT
xref Bug 563101 / attachment 442999 [details] [diff] [review] / attachment 443425 [details] (personas.css; SEAMONKEY_ID case CSS part from personas.js). Requesting 2.1 status triage.
Comment 2 Jens Hatlak (:InvisibleSmiley) 2010-10-29 16:06:55 PDT
Created attachment 487074 [details] [diff] [review]
basic lwthemes support

First I tried using Stylish to check the effects of the CSS from the bug mentioned in my previous comment. Even changing [persona] to [lwtheme="true"] made no difference. Then I found the two "lightweightthemes" attributes on the <window> in navigator.xul and the matching rules in communicator.css. After I had applied those changes it just worked without any further CSS, just had to apply the changes to a few more XUL files. :-)

The result already looks quite promising, though the header pane in the standalone message window should probably not have a background, and the same is debatable for the compose window formatting toolbar.

What do you think?

[Not taking the bug since I'm not sure I'm the best person to actually determine where changes need to be done in the MailNews UI and how to do it in CSS.]
Comment 3 Karsten Düsterloh 2010-10-29 16:32:30 PDT
Comment on attachment 487074 [details] [diff] [review]
basic lwthemes support

(In reply to comment #2)
> Created attachment 487074 [details] [diff] [review]
> basic lwthemes support

> What do you think?

Cool. :)

> The result already looks quite promising, though the header pane in the
> standalone message window should probably not have a background,

True, especially since the header pane will usually "outheight" the top image. 
BTW, the standalone window status-bar seems to be broken with your patch?

> and the same is debatable for the compose window formatting toolbar.

Yes, that looks a bit strange; same for the address toolbar - too much height, again, the Persona images are usually too narrow.
Comment 4 Jens Hatlak (:InvisibleSmiley) 2010-10-30 14:06:41 PDT
Created attachment 487190 [details] [diff] [review]
patch v2
Comment 5 Jens Hatlak (:InvisibleSmiley) 2010-10-30 14:11:28 PDT
(In reply to comment #3)
> BTW, the standalone window status-bar seems to be broken with your patch?

Yeah, somehow the lightweightthemesfooter attribute breaks it. Removing it didn't seem to make any difference other than unbreaking it. :-/

BTW: TB (mail/) seems to add the attributes to messenger.xul.

You may want to cross-check the TB styles and those from bug 563101.
Comment 6 neil@parkwaycc.co.uk 2010-10-31 09:18:34 PDT
Comment on attachment 487190 [details] [diff] [review]
patch v2

>+        lightweightthemes="true"
>         windowtype="mail:messageWindow">
The standalone message window still has a status bar...

>+#msgHeaderView:-moz-lwtheme,
>+#messengerWindow[lwtheme="true"] treecols {
Everyone needs to be using :-moz-lwtheme now.
(sadly we have one [lwtheme="true"] hanging around...)
Comment 7 Robert Kaiser (not working on stability any more) 2010-10-31 14:05:51 PDT
Right, :-moz-lwtheme should work on all elements, AFAIK, so we should use that as it should be faster than walking the cascade up to the window.
Comment 8 Jens Hatlak (:InvisibleSmiley) 2010-11-01 07:01:17 PDT
Created attachment 487328 [details] [diff] [review]
patch v2a

(In reply to comment #6)
> >+        lightweightthemes="true"
> >         windowtype="mail:messageWindow">
> The standalone message window still has a status bar...

So? It has one with or without the patch. If you're referring to the lightweightthemesfooter attribute, see comment 5. I have no intention to check what's going wrong there; nothing suspicious in the JS console and without the attribute everything is right as far as I can see.

If anyone wants to check, the above attribute seems to only be checked by this code:
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/LightweightThemeConsumer.jsm#90
Comment 9 neil@parkwaycc.co.uk 2010-11-01 16:49:29 PDT
(In reply to comment #8)
> I have no intention to check what's going wrong there
Well fortunately for you I did check, and it's the mMsgNotificationBar which tries to retrieve the element at an unsafe time (i.e. while the document is loading). I'm not seeing the lightweight theme on the main 3 pane status bar though; it's not the same cause, so as yet I'm not sure what the issue is.
Comment 10 neil@parkwaycc.co.uk 2010-11-03 05:20:27 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > I have no intention to check what's going wrong there
> Well fortunately for you I did check, and it's the mMsgNotificationBar which
> tries to retrieve the element at an unsafe time (i.e. while the document is
> loading). I'm not seeing the lightweight theme on the main 3 pane status bar
> though; it's not the same cause, so as yet I'm not sure what the issue is.
In this case it's gMessengerBundle that's the culprit. Similarly in the compose window it's sComposeMsgsBundle and sBrandBundle.
Comment 11 Jens Hatlak (:InvisibleSmiley) 2010-11-03 06:52:35 PDT
(In reply to comment #10)
> In this case it's gMessengerBundle that's the culprit. Similarly in the compose
> window it's sComposeMsgsBundle and sBrandBundle.

Thanks for the analysis, but that's above my head. Feel free to take.
Comment 12 Stefan [:stefanh] 2010-11-03 14:46:34 PDT
+#searchInput:-moz-lwtheme,
+.tabmail-tab:-moz-lwtheme,
+toolbar menulist:-moz-lwtheme /* View and Folder menus */ {
+  opacity: .8;
+}

I think you might want to put the tab styling in mailWindow1.css since that's the only file where tab styles are. Also, this doesn't handle the mac part.
Comment 13 Jens Hatlak (:InvisibleSmiley) 2010-11-04 16:39:23 PDT
OK, so I basically already threw the towel, and the reason for that hasn't changed, but I can offer to add lwthemes support for the main window only. IIUC, comment 10 implies that the main window is not affected, and it's also the only window that TB is currently styling. It's not the full solution, but better than nothing. The rest could either be done in this bug or a follow-up.
Comment 14 neil@parkwaycc.co.uk 2010-11-05 06:05:37 PDT
(In reply to comment #13)
> IIUC, comment 10 implies that the main window is not affected
No, I see that all mail windows are affected. I'll file a separate bug on avoiding accessing DOM elements before the document loads.

Also, the Advanced button needs to be given some opacity. (The search and go buttons in the browser window have their own rules for this but it might be worthwhile creating a shared rule to cover this.)
Comment 15 neil@parkwaycc.co.uk 2010-11-05 06:22:21 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > I have no intention to check what's going wrong there
> Well fortunately for you I did check, and it's the mMsgNotificationBar
Speaking of which, it needs to have its text shadow and colours fixed ;-)
Comment 16 Jens Hatlak (:InvisibleSmiley) 2010-11-05 15:37:20 PDT
Created attachment 488583 [details] [diff] [review]
patch v3

Since Neil was kind enough to address the blocker, let's move on. Notes:
- patch is on top of the one from bug 579739, which I'll check in once the tree reopens
- .msgNotificationBar already sets colors -> no need for an override
- this is basically just another lwthemes bug, so I'll skip Karsten's review here and continue with the Stefan/Neil dream team
- of course this can only land once bug 609877 is ready.
Comment 17 neil@parkwaycc.co.uk 2010-11-08 05:31:14 PST
Comment on attachment 488583 [details] [diff] [review]
patch v3

>+toolbar menulist:-moz-lwtheme,
>+toolbar button:-moz-lwtheme {
>   opacity: 0.8;
Nit: can remove rules from navigator.css now.

>+.tabmail-tab:-moz-lwtheme {
>+  opacity: 0.8;
Except for the selected tab (see navigator.css).

>+  -moz-appearance: none;
I don't think we need this anywhere.

>+#searchInput:-moz-lwtheme {
>+  opacity: .8;
Except when it's focused. (More fuel for communicator.css? Also, should we remove opacity from focused menulists?)

sr=me with these fixed.
Comment 18 Jens Hatlak (:InvisibleSmiley) 2010-11-08 14:34:35 PST
Created attachment 488983 [details] [diff] [review]
patch v3a
Comment 19 Stefan [:stefanh] 2010-11-09 10:18:57 PST
Comment on attachment 488983 [details] [diff] [review]
patch v3a

--- a/suite/themes/classic/mac/communicator/communicator.css
+++ b/suite/themes/classic/mac/communicator/communicator.css
@@ -105,12 +105,13 @@ grippy {
 
 .sidebarTree {
   border: none;
   margin: 0px !important;
 }
 
 /* ::::: lightweight themes ::::: */
 
-toolbar menulist:-moz-lwtheme {
+toolbar button:-moz-lwtheme,
+toolbar menulist:-moz-lwtheme:not([open="true"]),
+toolbar textbox:-moz-lwtheme:not([focused="true"]) {
   opacity: 0.8;
 }

After some though, I think we only want the textbook styling here (buttons never had it on mac, and the menulist styling looks a bit weird (might re-visit that, but my current opinion is that the styling makes it look 50% disabled). Note that it should be 0.9 for the textbox (as it was for the urlbar!)

+#MsgHeadersToolbar menulist:-moz-lwtheme,
+#FormatToolbar menulist:-moz-lwtheme {
+  opacity: 1;
+}
The above means that you can get rid of this in the mac version of messengercompose.css ;-)

One question: Do we plan to make users be able to move them to the main toolbar and/or put other menulists in those toolbars? The reason I ask is that the above descendant selectors are the most expensive style rules you can have and you only want these if there are no other possibilities. Just asking. 

--- a/suite/themes/classic/mac/messenger/mailWindow1.css
+++ b/suite/themes/classic/mac/messenger/mailWindow1.css
@@ -236,8 +236,16 @@
 .tabmail-tab[type="message"] .tab-icon {
   margin-top: -2px;
 }
 
 .tabmail-tab[type="folder"][NewMessages="true"],
 .tabmail-tab[type="folder"][IsServer="true"] {
   font-weight: bold;
 }
+
+.tabmail-tab:-moz-lwtheme:not([selected="true"]) {
+  opacity: 0.8;
+}
+
+.tabmail-tab:-moz-lwtheme {
+  text-shadow: none;
+}

Don't need/want this. Most stuff is taken care of in tabbrowser.css and we also don't remove the shadow in tabbrowser, so I think we're fine having it in tabmail.

The current patch lacks some style rules in the mac version of mailWindow1.css to make it complete - for example, right now, the search bar and the tabmail-strip isn't lw-themed. IOTW:

1) You need to get rid of the background-image/color in #searchToolbar ("background-image: none", and "background-color: transparent" will do fine)
2) The .tabmail-strip needs a transparent backgrund
3) the #messagepane border is transparent and the #messagepanebox gets the lwtheme image as a background, so it shines through to the border. I'd suggest just setting a -moz-dialog background to the messagepanebox.

Minusing, mainly because I think it's too much to fix upon check-in.
Comment 20 Stefan [:stefanh] 2010-11-09 14:50:45 PST
Comment on attachment 488983 [details] [diff] [review]
patch v3a

+toolbar textbox:-moz-lwtheme:not([focused="true"]) 

forgot to mention that i guess this should make the addressbook searchbox styling obsolete.
Comment 21 Jens Hatlak (:InvisibleSmiley) 2010-11-09 15:11:53 PST
Created attachment 489316 [details] [diff] [review]
patch v3b [Checkin: comment 22]

(Needless to say that the Mac changes are totally untested by me. I depend on you telling me exactly what you need; I hope I got it about right.)

(In reply to comment #19)
> One question: Do we plan to make users be able to move them to the main toolbar
> and/or put other menulists in those toolbars? The reason I ask is that the
> above descendant selectors are the most expensive style rules you can have and
> you only want these if there are no other possibilities. Just asking. 

Bug 606683 will make the formatting toolbar customizable, so you'll then be able to drag the formatting drop-down to some other customizable toolbar.

(In reply to comment #20)
> forgot to mention that i guess this should make the addressbook searchbox
> styling obsolete.

Good catch. I'm assuming Neil is OK with that, so I'm carrying over his sr+.
Comment 22 Jens Hatlak (:InvisibleSmiley) 2010-11-10 11:27:07 PST
Comment on attachment 489316 [details] [diff] [review]
patch v3b [Checkin: comment 22]

http://hg.mozilla.org/comm-central/rev/cc4bf8588a7e
Comment 23 neil@parkwaycc.co.uk 2010-11-12 02:29:32 PST
(In reply to comment #17)
> Also, should we remove opacity from focused menulists?
I noticed you went for open menulists. An interesting choice, but I'm not 100% happy with it. I'll have to see whether I get used to it.
Comment 24 neil@parkwaycc.co.uk 2010-11-12 13:58:16 PST
The Compose window Subject box needs an override :-(
Comment 25 Jens Hatlak (:InvisibleSmiley) 2010-11-13 09:50:48 PST
Created attachment 490402 [details] [diff] [review]
fix compose subject opacity
Comment 26 neil@parkwaycc.co.uk 2010-11-13 14:54:06 PST
Bah, I failed to notice that the addressing widget is also affected :-(
Comment 27 Jens Hatlak (:InvisibleSmiley) 2010-11-14 02:17:25 PST
Created attachment 490449 [details] [diff] [review]
fix compose opacity
Comment 28 neil@parkwaycc.co.uk 2010-11-14 02:31:06 PST
Comment on attachment 490449 [details] [diff] [review]
fix compose opacity

>+#msgSubject:-moz-lwtheme,
>+#MsgHeadersToolbar textbox:-moz-lwtheme,
Doesn't the subject field live in the message header toolbar anyway?
Comment 29 Jens Hatlak (:InvisibleSmiley) 2010-11-14 02:45:48 PST
Created attachment 490450 [details] [diff] [review]
fix compose opacity v1a [Checkin: comment 30]
Comment 30 Jens Hatlak (:InvisibleSmiley) 2010-11-14 05:02:15 PST
Comment on attachment 490450 [details] [diff] [review]
fix compose opacity v1a [Checkin: comment 30]

http://hg.mozilla.org/comm-central/rev/e43941867366

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