Colors in compose window don't match Firefox

RESOLVED FIXED in Thunderbird 5.0b1

Status

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: andreasn, Assigned: Paenglab)

Tracking

Trunk
Thunderbird 5.0b1
All
Windows Vista
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 10 obsolete attachments)

156.31 KB, image/png
Details
5.15 KB, image/png
Details
157.01 KB, image/png
Details
157.32 KB, image/png
Details
115.98 KB, image/png
Details
64.66 KB, image/png
Details
37.06 KB, image/png
Details
33.48 KB, image/png
Details
27.69 KB, image/png
Details
72.64 KB, image/png
Details
21.97 KB, patch
bwinton
: review+
andreasn
: ui-review+
Details | Diff | Splinter Review
Reporter

Description

8 years ago
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.
Reporter

Comment 1

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

Comment 2

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

Comment 3

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

Comment 4

8 years ago
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.
Reporter

Comment 5

8 years ago
Example of bookmarks dropdown button from Firefox.
Reporter

Comment 7

8 years ago
Even though it's outside the scope of this bug :)
Assignee

Comment 8

8 years ago
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.
Assignee

Comment 9

8 years ago
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.
Attachment #520436 - Flags: feedback?(nisses.mail)
Assignee

Comment 10

8 years ago
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?
Reporter

Comment 11

8 years ago
(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.
Reporter

Comment 12

8 years ago
(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.
Assignee

Comment 13

8 years ago
(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).
Assignee

Comment 14

8 years ago
(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.
Assignee

Comment 15

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

-  margin: 1px;
+  margin: 1px;
Attachment #520436 - Attachment is obsolete: true
Attachment #520436 - Flags: feedback?(nisses.mail)
Attachment #520736 - Flags: feedback?(nisses.mail)
Reporter

Comment 16

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

Comment 17

8 years ago
Posted patch some colors (obsolete) — Splinter Review
Still lacking hover and other niceties. Not sure if it applies due to oddness in applying previous patches.
Assignee

Comment 18

8 years ago
(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.
Assignee

Comment 19

8 years ago
Posted patch Patch for all platforms (obsolete) — Splinter Review
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.
Assignee: nobody → richard.marti
Attachment #520736 - Attachment is obsolete: true
Attachment #521180 - Attachment is obsolete: true
Attachment #520736 - Flags: feedback?(nisses.mail)
Attachment #522099 - Flags: ui-review?(nisses.mail)
Assignee

Comment 22

8 years ago
Assignee

Comment 23

8 years ago
Image shows the inactive menulists in FormatToolbar. Additionally I opened the AttachmentBox.
Assignee

Comment 24

8 years ago
Posted patch Updated patch (obsolete) — Splinter Review
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.
Attachment #522099 - Attachment is obsolete: true
Attachment #522099 - Flags: ui-review?(nisses.mail)
Attachment #522677 - Flags: ui-review?(nisses.mail)
Assignee

Comment 25

8 years ago
Found missing " in patch which made a xml error
Attachment #522677 - Attachment is obsolete: true
Attachment #522677 - Flags: ui-review?(nisses.mail)
Attachment #524423 - Flags: ui-review?(nisses.mail)
Reporter

Comment 26

8 years ago
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.
Assignee

Comment 27

8 years ago
Attachment #524423 - Attachment is obsolete: true
Attachment #524423 - Flags: ui-review?(nisses.mail)
Attachment #524451 - Flags: ui-review?(nisses.mail)
Assignee

Comment 28

8 years ago
Posted patch Unbitrotted patch (obsolete) — Splinter Review
Patch updated after landing of Bug 638964
Attachment #524451 - Attachment is obsolete: true
Attachment #524451 - Flags: ui-review?(nisses.mail)
Attachment #524842 - Flags: ui-review?(nisses.mail)
Reporter

Comment 29

8 years ago
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?
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.
Assignee

Comment 31

8 years ago
(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.
Assignee

Comment 32

8 years ago
I've used now diff instead of WinMerge. I hope this solves the problem with applying the patch.
Attachment #524842 - Attachment is obsolete: true
Attachment #524842 - Flags: ui-review?(nisses.mail)
Attachment #526475 - Flags: ui-review?(nisses.mail)
Reporter

Comment 33

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

Comment 34

8 years ago
Blake also says it's my fault from changeset 5997 :)
Reporter

Comment 35

8 years ago
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.
Reporter

Comment 36

8 years ago
Posted patch and a properSplinter Review
protz fixed me up with a new patch before I got around to do it myself!
Attachment #526475 - Attachment is obsolete: true
Attachment #526984 - Attachment is obsolete: true
Attachment #526475 - Flags: ui-review?(nisses.mail)
Attachment #526996 - Flags: ui-review?(nisses.mail)
Reporter

Updated

8 years ago
Attachment #526996 - Attachment is patch: true
Attachment #526996 - Attachment mime type: application/octet-stream → text/plain
Reporter

Comment 37

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

Looks great! ui-r+ on this one!
Attachment #526996 - Flags: ui-review?(nisses.mail) → ui-review+
Assignee

Updated

8 years ago
Attachment #526996 - Flags: review?(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.
Attachment #526996 - Flags: review?(bwinton) → review+
Assignee

Updated

8 years ago
Status: NEW → ASSIGNED
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/d93355b8d755
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a4
Depends on: 654426

Updated

8 years ago
Depends on: 654686

Updated

8 years ago
Depends on: 654754

Updated

8 years ago
Depends on: 654882

Updated

8 years ago
Depends on: 654916

Updated

8 years ago
Depends on: 668998
You need to log in before you can comment on or make changes to this bug.