Closed Bug 579738 Opened 14 years ago Closed 14 years ago

Make lightweight themes / personas work in mailnews windows

Categories

(SeaMonkey :: Themes, defect)

defect
Not set
normal

Tracking

(seamonkey2.1 wanted)

RESOLVED FIXED
seamonkey2.1b2
Tracking Status
seamonkey2.1 --- wanted

People

(Reporter: kairo, Assigned: InvisibleSmiley)

References

Details

Attachments

(2 files, 7 obsolete files)

We should also make lwthemes work in mailnews windows once they work in browser.
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.
Version: unspecified → Trunk
Attached patch basic lwthemes support (obsolete) — Splinter Review
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.]
Attachment #487074 - Flags: feedback?(mnyromyr)
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.
Attachment #487074 - Flags: feedback?(mnyromyr) → feedback+
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #487190 - Flags: superreview?
Attachment #487190 - Flags: review?(mnyromyr)
Attachment #487190 - Flags: superreview? → superreview?(neil)
Attachment #487074 - Attachment is obsolete: true
Assignee: nobody → jh
Status: NEW → ASSIGNED
(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 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...)
Attachment #487190 - Flags: superreview?(neil) → superreview-
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.
Attached patch patch v2a (obsolete) — Splinter Review
(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
Attachment #487190 - Attachment is obsolete: true
Attachment #487328 - Flags: superreview?(neil)
Attachment #487328 - Flags: review?(mnyromyr)
Attachment #487190 - Flags: review?(mnyromyr)
(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 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.
Attachment #487328 - Flags: superreview?(neil) → superreview-
Attachment #487328 - Flags: review?(mnyromyr)
(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.
Assignee: jh → nobody
Status: ASSIGNED → NEW
+#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.
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.
(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.)
Depends on: 609877
(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 ;-)
Attached patch patch v3 (obsolete) — Splinter Review
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.
Assignee: nobody → jh
Attachment #487328 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #488583 - Flags: superreview?(neil)
Attachment #488583 - Flags: review?(stefanh)
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.
Attachment #488583 - Flags: superreview?(neil) → superreview+
Attached patch patch v3a (obsolete) — Splinter Review
Attachment #488583 - Attachment is obsolete: true
Attachment #488983 - Flags: superreview+
Attachment #488983 - Flags: review?(stefanh)
Attachment #488583 - Flags: review?(stefanh)
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.
Attachment #488983 - Flags: review?(stefanh) → review-
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.
(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+.
Attachment #488983 - Attachment is obsolete: true
Attachment #489316 - Flags: superreview+
Attachment #489316 - Flags: review?(stefanh)
Attachment #489316 - Flags: review?(stefanh) → review+
Comment on attachment 489316 [details] [diff] [review]
patch v3b [Checkin: comment 22]

http://hg.mozilla.org/comm-central/rev/cc4bf8588a7e
Attachment #489316 - Attachment description: patch v3b → patch v3b [Checkin: comment 22]
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b2
(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.
The Compose window Subject box needs an override :-(
Attached patch fix compose subject opacity (obsolete) — Splinter Review
Attachment #490402 - Flags: superreview?(neil)
Attachment #490402 - Flags: review?(neil)
Bah, I failed to notice that the addressing widget is also affected :-(
Attached patch fix compose opacity (obsolete) — Splinter Review
Attachment #490402 - Attachment is obsolete: true
Attachment #490449 - Flags: superreview?(neil)
Attachment #490449 - Flags: review?(neil)
Attachment #490402 - Flags: superreview?(neil)
Attachment #490402 - Flags: review?(neil)
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?
Attachment #490449 - Attachment is obsolete: true
Attachment #490450 - Flags: superreview?(neil)
Attachment #490450 - Flags: review?(neil)
Attachment #490449 - Flags: superreview?(neil)
Attachment #490449 - Flags: review?(neil)
Attachment #490450 - Flags: superreview?(neil)
Attachment #490450 - Flags: superreview+
Attachment #490450 - Flags: review?(neil)
Attachment #490450 - Flags: review+
Comment on attachment 490450 [details] [diff] [review]
fix compose opacity v1a [Checkin: comment 30]

http://hg.mozilla.org/comm-central/rev/e43941867366
Attachment #490450 - Attachment description: fix compose opacity v1a → fix compose opacity v1a [Checkin: comment 30]
Depends on: 612144
You need to log in before you can comment on or make changes to this bug.