670639 - Compose, message and address book windows should support personas

Status: RESOLVED FIXED

(Thunderbird :: Theme, defect)

Description

[Geoff Lankow (:darktrojan) [PTO, back 9 Feb]]

This is pretty easy since the LWT code is already present. Just add the lightweightthemes="true" attribute to the window tag of each window and some minor adjustments to the CSS. I haven't got a comm-central tree to patch it myself.

Comment 1

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

Yeah, that is kinda annoying…

Protz, Squib, either of you want to grab this?

Thanks,
Blake.

Comment 2

[Richard Marti (:Paenglab)]

Blake, are tests needed? If not I can take it.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: Adressbook with personas

Should the sidebarheader in AB and composer become the personas background or be solid? For me it looks not so good with this small piece having personas in this area.

Comment 4

[Geoff Lankow (:darktrojan) [PTO, back 9 Feb]]

The sidebar in Firefox has the persona's background.

Comment 5

[Richard Marti (:Paenglab)]

I know, but for me it looks weird with a piece hanging down into the "content"

Comment 6

[Jim Porter (:squib)]

(In reply to comment #5)
> I know, but for me it looks weird with a piece hanging down into the
> "content"

Maybe we should just avoid the issue entirely and get rid of that header over in bug 667245?

Comment 7

[Richard Marti (:Paenglab)]

Attachment: Enable personas in AB, composer and mail window

This patch enables additionally also personas for the mail window. I haven't touched the AB sidebar header because it's planned to remove it.

The composer sidebar header shows no personas background because this was already default on MAC.

Blake: do you want a ui-r? Should I give it to Andreas?

Comment 8

[Geoff Lankow (:darktrojan) [PTO, back 9 Feb]]

The windows also need |background-position: right top| set to match other persona'd windows.

Comment 9

[Andreas Nilsson (:andreasn)]

I'm happy to take the ui-review on this one.

Comment 10

[Richard Marti (:Paenglab)]

Attachment: Enable personas in AB, Composer and MailWindow - V2

Now with background-position: right top for AB and Composer. MailWindow is using the definition from MainWindow.

Comment 12

[Andreas Nilsson (:andreasn)]

Did this bitrot this fast? I'm having issues applying the patch.

Comment 13

[Richard Marti (:Paenglab)]

Attachment: Enable personas in AB, composer and mail window v3

I've found a error in patch but also small changes in the aero files.

Now it should work.

Comment 14

[Andreas Nilsson (:andreasn)]

Attachment: Screenshot of issue

There seems to be a bit inconsistency between Aero and Basic in the address book.
With that fixed, ui-r from me.

(the only issue I ran across was that some themes had readability issues with text in the sidebar header in the contacts window, but that's going to be fixed with the patch in bug 667245 as mentioned earlier).

Comment 15

[Richard Marti (:Paenglab)]

Attachment: Enable personas in AB, composer and mail window v4

Sorry I haven't checked under Win7 Basic.

Comment 16

[Andreas Nilsson (:andreasn)]

Comment on attachment 547965 [details] [diff] [review]
Enable personas in AB, composer and mail window v4

It now looks the same across Aero and Basic, so ui-r+

Comment 17

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

Comment on attachment 547965 [details] [diff] [review]
Enable personas in AB, composer and mail window v4

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

My biggest problem is that the compose window doesn't look the same (on Mac) as the others, and when it's in the background it doesn't seem to have any persona applied to it at all!

Other than that, the code is good, but I think I'm going to have to ask for that to be fixed, so until then, it's an r-.

Sorry about that,
Blake.

Comment 18

[Richard Marti (:Paenglab)]

(In reply to comment #17)
> My biggest problem is that the compose window doesn't look the same (on Mac)
> as the others, and when it's in the background it doesn't seem to have any
> persona applied to it at all!

Please can you add a screenshot which shows the difference?
And the no persona when in background, what can this be? With Win7 and Linux I don't see such a behavior.

Comment 19

[Richard Marti (:Paenglab)]

Attachment: Enable personas in AB, composer and mail window v5

Luckily I had today the ability to access a Mac (without tools, but better than nothing). Hey, that's much easier than guessing from Win and Linux DOM to Mac ;).

The inactive window problem is solved.

The not painting in title bar is really strange. When the Composer window is open and I apply the personas, everything is okay. But after closing and reopening the composer the title bar isn't painted. It looks like it's falling back from unified title bar. The AB has no such problems.

I saw also the drawintitlebar=true isn't set in composer but in main window and AB. I tried with setting it in messengercompose.xul but with no luck.

On Win and Linux I see also problems with the composer, but with the status-bar. Is the composr open when applying personas, everything is okay. But after closing/opening the composer the status-bar has no persona. On Mac I saw always the personafied status-bar.

Could it be the composer is differently initiated? Could this bug be landed as is (naturally after review) and I'm opening a bug to track this problem?

Comment 20

[Jonathan Protzenko [:protz]]

Compose windows are recycled, i.e. we're reusing the same one, just resetting to its initial state. Could it be that's what you're hitting?

Comment 21

[Richard Marti (:Paenglab)]

Maybe, but this also happens after closing/opening of TB. So then it can't reuse a window.

Comment 22

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

Comment on attachment 550645 [details] [diff] [review]
Enable personas in AB, composer and mail window v5

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

> Could it be the composer is differently initiated? Could this bug be landed as is (naturally after review) and I'm opening a bug to track this problem?

I'm really uncomfortable doing that, since this is a definite regression, and people will yell at me a lot if we break things and don't manage to fix them before the next release ships.

Other than that, though, I like the patch.  I think that means an r-, but you're on the right track.

Thanks,
Blake.

Comment 23

[Richard Marti (:Paenglab)]

So who can I ask to check what happens with the composer window? I'm not able to debug this.

Comment 24

[Jonathan Protzenko [:protz]]

Looking into this. Looks like part of the styles for the compose window (namely, messengercompose.css in gnomestripe) have been checked-in already...

Comment 25

[Jonathan Protzenko [:protz]]

So the issue seems to be incredibly subtle and non-trivial. I think somehow a load event gets fired at the wrong time, and by the time the code reaches LighweightThemeConsumer, which is the JSM in charge of settings the right colors and background images, the status bar *is not even in the DOM yet*. Yay...

Comment 26

[Jonathan Protzenko [:protz]]

So the issue seems to be incredibly subtle and non-trivial. I think somehow a load event gets fired at the wrong time, and by the time the code reaches LighweightThemeConsumer, which is the JSM in charge of settings the right colors and background images, the status bar *is not even in the DOM yet*. Yay...

Comment 27

[Jonathan Protzenko [:protz]]

Attachment: Corrected patch

This should fix the issue.

- Richard, can you make sure this now works fine for you?
- Blake, I had to change some stuff in MsgComposeCommands.js, the gory details are in there. Could you just review that file? The changes are pretty trivial, except why that would make the thing file is not trivial at all.

Comment 28

[Jonathan Protzenko [:protz]]

Meant "make the thing fail", of course!

Comment 29

[Richard Marti (:Paenglab)]

(In reply to Jonathan Protzenko [:protz] from comment #24)
> Looking into this. Looks like part of the styles for the compose window
> (namely, messengercompose.css in gnomestripe) have been checked-in already...

This must be by error :(

(In reply to Jonathan Protzenko [:protz] from comment #27)
> Created attachment 557039 [details] [diff] [review]
> Corrected patch
> 
> This should fix the issue.
> 
> - Richard, can you make sure this now works fine for you?
> - Blake, I had to change some stuff in MsgComposeCommands.js, the gory
> details are in there. Could you just review that file? The changes are
> pretty trivial, except why that would make the thing file is not trivial at
> all.

I tried the patch and it didn't apply in MsgComposeCommands.js. This made the error:

     var modifiedAttachmentName = attachmentName.value;
     if (modifiedAttachmentName == "")
       return; // name was not filled, bail out
 
     var nameAndSize = modifiedAttachmentName;
     if (item.attachment.size != -1)
     {
       var size = gMessenger.formatFileSize(item.attachment.size);
-      item.label = gComposeBundle.getFormattedString(
+      item.label = getComposeBundle().getFormattedString(
           "attachmentNameAndSize", [modifiedAttachmentName, size]);
     }
     else
       item.label = modifiedAttachmentName;
 
     item.attachment.name = modifiedAttachmentName;
     gContentChanged = true;
   }

I have a patch which applies and has a change in messengercompose.css in gnomestripe (a already checked-in part which is a text-shadow: none missing).

With my patch the composer status bar is now always painted with the persona under Linux and Windows - Great work!!

I can't say if it works under Mac.

Comment 30

[Richard Marti (:Paenglab)]

Attachment: Another corrected patch

Patch which applies on latest trunk.

Blake, if you have problems with this patch, please try the one from protz

Comment 31

[Jonathan Protzenko [:protz]]

Attachment: Corrected corrected patch

Sorry, here's another updated patch that applies cleanly on trunk. I just made a small change in that I renamed the old gComposeBundle variable to _gComposeBundle to prevent people from using the old variable by accident.

Richard, are you positive the text-shadow change has been checked-in already on gnomestripe?

Comment 32

[Richard Marti (:Paenglab)]

(In reply to Jonathan Protzenko [:protz] from comment #31)
> Created attachment 557247 [details] [diff] [review]
> Corrected corrected patch
> 
> Richard, are you positive the text-shadow change has been checked-in already
> on gnomestripe?

The text-shadow change isn't checked-in. I looked in my local repository and also on <http://hg.mozilla.org/comm-central/file/e0953facfcfc/mail/themes/gnomestripe/mail/compose/messengercompose.css>

Comment 33

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

Comment on attachment 557247 [details] [diff] [review]
Corrected corrected patch

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

Looks good on OS X, and I checked the inter-diff and was happy with the changes, so r=me with the tiny nit below fixed!  :)

Also, good catch with the lazy initialization.  I can't imagine how you found that…

::: mail/themes/pinstripe/mail/compose/messengercompose.css
@@ -940,2 +959,2 @@
> >    border-bottom: 1px solid #A3A3A3;
> > -        background: url("chrome://global/skin/inset_gradient_1px.png") repeat !important;
> > +  background: url("chrome://global/skin/inset_gradient_1px.png") repeat !important;

I don't see this file anywhere in the source tree.  Are you sure we still need this rule?

Comment 34

[Richard Marti (:Paenglab)]

Attachment: Patch for check-in addressing comment

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #33)
> ::: mail/themes/pinstripe/mail/compose/messengercompose.css
> @@ -940,2 +959,2 @@
> > >    border-bottom: 1px solid #A3A3A3;
> > > -        background: url("chrome://global/skin/inset_gradient_1px.png") repeat !important;
> > > +  background: url("chrome://global/skin/inset_gradient_1px.png") repeat !important;
> 
> I don't see this file anywhere in the source tree.  Are you sure we still
> need this rule?

I haven't checked if this file exists. I saw only the wrong indentation and corrected it. You are right I can't also not find this file. I removed this unneeded line.

I also added the missing text-shadow: none to the Linux messengercompose.css.

Comment 35

[Jonathan Protzenko [:protz]]

http://hg.mozilla.org/comm-central/rev/96c168b9c5c9