642163 - Colors in compose window don't match Firefox

Status: RESOLVED FIXED

(Thunderbird :: Theme, defect)

Description

[Andreas Nilsson (:andreasn)]

Attachment: mockup of how it would look

With all the amazing changes we're making to the main window and the address book, it's only fair that the compose window get some additional love as well.
I think it would be neat to have the background of the address, subject and toolbar area pick up the Firefox toolbar look&feel.

Comment 1

[Andreas Nilsson (:andreasn)]

Putting Richard in cc to see if he have any good feedback on this before I start doing any patches.

Comment 2

[Andreas Nilsson (:andreasn)]

A good question is how we should style the dropdowns. Like the bookmark dropdown in Firefox, or keep it the same as now.

Comment 3

[Andreas Nilsson (:andreasn)]

Watching this window also reminds me how much I want to fix the alignment of things, but that is for another bug :)

Comment 4

[Richard Marti (:Paenglab)]

Actually I have no idea and I don't know what you mean with bookmark dropdown. In plain text mode without the bottom toolbar I see only the possibility of giving radii on the address field.

Comment 5

[Andreas Nilsson (:andreasn)]

Attachment: Firefox bookmarks dropdown screenshots

Example of bookmarks dropdown button from Firefox.

Comment 6

[Andreas Nilsson (:andreasn)]

Attachment: how it would look in action with those buttons

Comment 7

[Andreas Nilsson (:andreasn)]

Attachment: and with the alignment

Even though it's outside the scope of this bug :)

Comment 8

[Richard Marti (:Paenglab)]

Ah okay, now I know what you mean. The dropdown images should be easy doable. I think the aligning only with CSS is only in a hackish way doable (different label length in other languages). This weekend I'll try to put these headers in a grid. But this will also affect the other platforms, which I think is also appreciated.

Comment 9

[Richard Marti (:Paenglab)]

Attachment: Patch only for Win7 and Glass patch

I tried first with grid but failed because of the addressingWidget listbox. I gave now the first columns a fixed width, which is localized to match different word length. I've added now only the styles for Vista/Win7.

Comment 10

[Richard Marti (:Paenglab)]

Attachment: MessengerCompose with patch in action

Unlike your mockup I have on bottom a visible splitter. Should I do it invisible and maybe make a glow effect on hover like the folderpane splitter when it is collapsed?

Comment 11

[Andreas Nilsson (:andreasn)]

(In reply to comment #10)
> Created attachment 520437 [details]
> MessengerCompose with patch in action
> 
> Unlike your mockup I have on bottom a visible splitter. Should I do it
> invisible and maybe make a glow effect on hover like the folderpane splitter
> when it is collapsed?

If you drag it to the bottom you mean? It looks like it don't get collapsed in exactly the same way as the sidebar, and they cover somewhat different use cases it seems (dragging the header all the way down seems stupid). It might still make sense to do something similar though, just in case.

Comment 12

[Andreas Nilsson (:andreasn)]

(In reply to comment #8)
> Ah okay, now I know what you mean. The dropdown images should be easy doable. I
> think the aligning only with CSS is only in a hackish way doable (different
> label length in other languages). This weekend I'll try to put these headers in
> a grid. But this will also affect the other platforms, which I think is also
> appreciated.

Do I need another patch applied before applying this? I seem to get some rejects on the css file.

Comment 13

[Richard Marti (:Paenglab)]

Attachment: Comparison between Mockup and Patch

(In reply to comment #11)
> (In reply to comment #10)
> > Created attachment 520437 [details]
> > MessengerCompose with patch in action
> > 
> > Unlike your mockup I have on bottom a visible splitter. Should I do it
> > invisible and maybe make a glow effect on hover like the folderpane splitter
> > when it is collapsed?
> 
> If you drag it to the bottom you mean? It looks like it don't get collapsed in
> exactly the same way as the sidebar, and they cover somewhat different use
> cases it seems (dragging the header all the way down seems stupid). It might
> still make sense to do something similar though, just in case.

In the image you can see the difference. I marked the splitter. I meant, make the splitter invisible as in your mockup. When the pointer is over the splitter, it will glow to say it exists a splitter (additional to the pointer change with up/down arrow).

Comment 14

[Richard Marti (:Paenglab)]

(In reply to comment #12)
> (In reply to comment #8)
> > Ah okay, now I know what you mean. The dropdown images should be easy doable. I
> > think the aligning only with CSS is only in a hackish way doable (different
> > label length in other languages). This weekend I'll try to put these headers in
> > a grid. But this will also affect the other platforms, which I think is also
> > appreciated.
> 
> Do I need another patch applied before applying this? I seem to get some
> rejects on the css file.

Maybe I played to much. I'll try to make a new with only Aero Glass applied.

Comment 15

[Richard Marti (:Paenglab)]

Attachment: Patch needing Aero Glass patch applied

Patch with new messengercompose-aero.css from hg and Aero Glass patch applied. But still with a weird diff result:

-  margin: 1px;
+  margin: 1px;

Comment 16

[Andreas Nilsson (:andreasn)]

Odd indeed. Might be better to wait to land this until we land the patch in bug 569400

Comment 17

[Andreas Nilsson (:andreasn)]

Attachment: some colors

Still lacking hover and other niceties. Not sure if it applies due to oddness in applying previous patches.

Comment 18

[Richard Marti (:Paenglab)]

(In reply to comment #17)
> Created attachment 521180 [details] [diff] [review]
> some colors
> 
> Still lacking hover and other niceties. Not sure if it applies due to oddness
> in applying previous patches.

No problem for me, I'm applying manual ;)

> <hbox class="top-gradient-thing">

Are you planning something with this class?

The menulists becomes your new button styles only in Aero Glass. Is this intended? Wouldn't it be cleaner if this applies also for every theme in Vista/Win7 like the buttons? Then we have only to add menulist to the button rules and make only special rules for the differences.

Comment 19

[Richard Marti (:Paenglab)]

Attachment: Patch for all platforms

Patch with changes for all platforms.

Included is also Andreas's additional patch. I applied the Win7 menulist styles together with the toolbarbuttons. The only difference was the text-shadow.

Andreas if you still want the different text-shadow be applied, I can add it.

Comment 20

[Richard Marti (:Paenglab)]

Attachment: Compose window under OSX

Comment 21

[Richard Marti (:Paenglab)]

Attachment: Compose window under Linux

Comment 22

[Richard Marti (:Paenglab)]

Attachment: Compose window under XP

Comment 23

[Richard Marti (:Paenglab)]

Attachment: Compose window under Win7

Image shows the inactive menulists in FormatToolbar. Additionally I opened the AttachmentBox.

Comment 24

[Richard Marti (:Paenglab)]

Attachment: Updated patch

Today I had again access to an XP machine and found a border around the addressingWidget and a to little margin between addressingWidget and msgSubject. The screenshot under XP shows this little margin.

Comment 25

[Richard Marti (:Paenglab)]

Attachment: Fixed missing " in messengercompose.xul

Found missing " in patch which made a xml error

Comment 26

[Andreas Nilsson (:andreasn)]

In general, looks really good on Linux and OS X. Great job!
My Windows build is in too much of a "funny" state right now in order for me to test it there, but I'll give it another try tonight and give it a review based on that.

Two small issues:
* As mentioned on IRC, I get an error when applying the patch for the file  mail/themes/qute/mail/compose/messengercompose-aero.css on line 434. Probably a bitrot.
* The compose window on Linux gets too slim and the rightmost widgets in the compose toolbar falls of the screen. I haven't observed this on OS X, not sure why this happens yet.

Comment 27

[Richard Marti (:Paenglab)]

Attachment: Hopefully working now under Aero

Comment 28

[Richard Marti (:Paenglab)]

Attachment: Unbitrotted patch

Patch updated after landing of Bug 638964

Comment 29

[Andreas Nilsson (:andreasn)]

This is really odd, I still get issues when trying to apply the patch.
Here are the lines it seems to have issues with http://pastebin.mozilla.org/1202362

It also seems I'm not getting a gradient in the background. Is this an older patch?

Comment 30

[Jim Porter (:squib)]

For the Vista/7 theme, you could probably make the attachment area splitter invisible (or however it works for the splitters in the three-pane). That one looks worse than than the splitter at the bottom anyway, since it just stops all of a sudden.

Comment 31

[Richard Marti (:Paenglab)]

(In reply to comment #30)
> For the Vista/7 theme, you could probably make the attachment area splitter
> invisible (or however it works for the splitters in the three-pane). That one
> looks worse than than the splitter at the bottom anyway, since it just stops
> all of a sudden.

I know. I had planned to do this in Bug 643262. First I have to solve the problem mentioned in Comment 29.

Comment 32

[Richard Marti (:Paenglab)]

Attachment: Patch made with diff instead WinMerge

I've used now diff instead of WinMerge. I hope this solves the problem with applying the patch.

Comment 33

[Andreas Nilsson (:andreasn)]

Attachment: patch with rejected lines fixed

No, still get the fail, and Blake did too. Fixed the lines in this patch.

Comment 34

[Andreas Nilsson (:andreasn)]

Blake also says it's my fault from changeset 5997 :)

Comment 35

[Andreas Nilsson (:andreasn)]

So it seems the patch I just posted have some incorrect line endings (and that's the reason the patches break). I got a better editor now and will fix those lines up. New patch coming.

Comment 36

[Andreas Nilsson (:andreasn)]

Attachment: and a proper

protz fixed me up with a new patch before I got around to do it myself!

Comment 37

[Andreas Nilsson (:andreasn)]

Comment on attachment 526996 [details] [diff] [review]
and a proper

Looks great! ui-r+ on this one!

Comment 38

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

Comment on attachment 526996 [details] [diff] [review]
and a proper

Review of attachment 526996 [details] [diff] [review]:

So, this patch failed to apply for me, due to some line ending issues, but once I fixed those, it seemed pretty good.

r=me.

Comment 39

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/d93355b8d755