Closed Bug 673322 Opened 13 years ago Closed 13 years ago

Multi-message summary buttons don't match message header buttons

Categories

(Thunderbird :: Toolbars and Tabs, defect)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 10.0

People

(Reporter: squib, Assigned: squib)

References

Details

Attachments

(3 files, 3 obsolete files)

Attached image Before and after
On Windows and Linux, the multi-message summary button don't match their message header counterparts. This is especially bad on Linux, since the multi-message buttons don't have icons (see attached screenshot).

Patch for Linux forthcoming. I could use some help with Windows and Mac, since I'd just be doing guesswork to fix them.

As a note, I intentionally made the buttons line up so that the delete button is in exactly the same spot in both the multi-message and the single-message (assuming the multi-message doesn't have a scrollbar). For anyone who'd like to help out on other platforms, it would be nice if we did that there as well.
Attached patch The patch, for Linux (obsolete) — Splinter Review
Attachment #547580 - Flags: ui-review?(nisses.mail)
Attached patch The patch, for Linux (obsolete) — Splinter Review
Whoops, here's the patch.
Attachment #547581 - Attachment is obsolete: true
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
Comment on attachment 547580 [details]
Before and after

Looks good. Do you want me to give a ui-r+ to the actual patch as well?
Attachment #547580 - Flags: ui-review?(nisses.mail) → ui-review+
(In reply to comment #3)
> Comment on attachment 547580 [details]
> Before and after
> 
> Looks good. Do you want me to give a ui-r+ to the actual patch as well?

We probably need to see how this works for other themes too, since it might cause problems with them.
Attached patch Fix this on all platforms (obsolete) — Splinter Review
I forgot I had this in my queue; this should work on all platforms. I've tested this myself on Linux and Windows (XP and 7) and everything lines up right; not sure about Mac, but I made an educated guess there.

In particular, the buttons should be exactly the same size, so if you 1) hide the Junk button in the single message view, and 2) go to a multi-message summary with no scroll bar, the Archive and Delete buttons shouldn't move when you alternate between the summary and the single message view.

The trick here is that I just replicate the XUL structure of the message header toolbar and link to the appropriate CSS files, so all the styles work same.
Attachment #547583 - Attachment is obsolete: true
Attachment #562279 - Flags: ui-review?(nisses.mail)
Attachment #562279 - Flags: review?(bwinton)
Going to try and get a mac build running before I ok this, but in general seems to work well (only tested on Win7 so far).
One thing that I think we should consider (but maybe in a separate bug) is to change the background of the multi message header to match the single message one a bit better. I think Richard brought this up some time ago on IRC, but we never really followed up on it.
(In reply to Andreas Nilsson (:andreasn) from comment #7)
> One thing that I think we should consider (but maybe in a separate bug) is
> to change the background of the multi message header to match the single
> message one a bit better. I think Richard brought this up some time ago on
> IRC, but we never really followed up on it.

I (sort of) have a patch for this. It changes the multi-message summary to use the same color scheme as my Mail Summaries add-on. This is useful because all the colors are derived from the platform, so it fits in and doesn't hurt accessibility. I'll post a patch this evening.
Finally got my Lion build running!
Looks good on the Mac too. So ui-r+ for all 3 platforms from me regarding that.
Squib: Do you want to add the background color patch to this bug as well, or did you post it to another bug already?
The background color is bug 690241.
Attachment #562279 - Flags: ui-review?(nisses.mail) → ui-review+
Comment on attachment 562279 [details] [diff] [review]
Fix this on all platforms

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

Other than the small nit below, I like it.  r=me.

::: mail/base/content/multimessageview.xhtml
@@ +84,4 @@
>          </script>
>      </head>
>  <body onload="hookResize()">
> +  <div id="headingwrappertable">

We seem to be using four space indentation in the head and two space in the body.  Can we make it the same in both?
Attachment #562279 - Flags: review?(bwinton) → review+
Checked in with nits fixed: http://hg.mozilla.org/comm-central/rev/adf6153b25db
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 10.0
Backed out as this broke unit tests. Please remember to keep an eye on the tree after landing patches:

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

Example log:

http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTrunk/1318109149.1318109646.27243.gz#err0

TEST-START | /buildbot/comm-central-linux-opt-unittest-mozmill/build/mozmill/folder-display/test-message-commands.js | test_disabled_archive
Step Pass: {"function": "Controller.keypress()"}
Step Pass: {"function": "Controller.keypress()"}
Test Failure: archiveBtn is null
TEST-UNEXPECTED-FAIL | /buildbot/comm-central-linux-opt-unittest-mozmill/build/mozmill/folder-display/test-message-commands.js | test_disabled_archive
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Fix testsSplinter Review
Whoops, I always forget that Mozilla buildbots don't send out an email when a build fails. Here's a fix for the test and an accompanying try build (though I'm not sure the try server is fully working yet): <http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=96a34e326625>
Attachment #565873 - Flags: review?(mbanner)
Attachment #562279 - Attachment is obsolete: true
Comment on attachment 565873 [details] [diff] [review]
Fix tests

That looks better, thanks.
Attachment #565873 - Flags: review?(mbanner) → review+
Checked in with tests fixed: http://hg.mozilla.org/comm-central/rev/47490a1cb67c
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.