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)
Thunderbird
Toolbars and Tabs
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 10.0
People
(Reporter: squib, Assigned: squib)
References
Details
Attachments
(3 files, 3 obsolete files)
21.63 KB,
image/png
|
andreasn
:
ui-review+
|
Details |
48.52 KB,
image/png
|
Details | |
12.69 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #547580 -
Flags: ui-review?(nisses.mail)
Assignee | ||
Comment 2•13 years ago
|
||
Whoops, here's the patch.
Attachment #547581 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
Comment 3•13 years ago
|
||
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+
Assignee | ||
Comment 4•13 years ago
|
||
(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.
Assignee | ||
Comment 6•13 years ago
|
||
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)
Comment 7•13 years ago
|
||
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.
Assignee | ||
Comment 8•13 years ago
|
||
(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.
Comment 9•13 years ago
|
||
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?
Assignee | ||
Comment 10•13 years ago
|
||
The background color is bug 690241.
Updated•13 years ago
|
Attachment #562279 -
Flags: ui-review?(nisses.mail) → ui-review+
Comment 11•13 years ago
|
||
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+
Assignee | ||
Comment 12•13 years ago
|
||
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
Comment 13•13 years ago
|
||
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 → ---
Assignee | ||
Comment 14•13 years ago
|
||
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)
Assignee | ||
Updated•13 years ago
|
Attachment #562279 -
Attachment is obsolete: true
Comment 15•13 years ago
|
||
Comment on attachment 565873 [details] [diff] [review] Fix tests That looks better, thanks.
Attachment #565873 -
Flags: review?(mbanner) → review+
Assignee | ||
Comment 16•13 years ago
|
||
Checked in with tests fixed: http://hg.mozilla.org/comm-central/rev/47490a1cb67c
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•