15.40 KB, image/gif
4.67 KB, image/gif
5.70 KB, image/gif
10.84 KB, image/png
3.67 KB, patch
|Details | Diff | Splinter Review|
67.67 KB, image/png
I kinda suspect this wouldn't block Tb3, but that's Bryan's call to make...
Created attachment 405431 [details] Screenshot of header buttons, junk button width too small Another nit that should be fixed: - in icon-only mode, all buttons are 26px wide and look square (good) - but junk button is only 22px wide, and looks squeezed (bad) I don't even know how this can happen, because they're all 16x16 icons, or not? -> please ensure iconic junk button looks square like all the other iconic buttons (26px wide)
Created attachment 405433 [details] Screenshot 3: with reply-to header: extra gap before to-header More wasted vspace: - mail with reply-to header -> before to-header, unused extra gap of +3px (please remove this)
Related bug 460647 - Focussing contacts causes respective header and label to move down some px Miraculously, the proposed fix for that bug appears to be vspace-neutral on the whole although it adds a transparent 1px border to contacts so that they stop jumping up and down when focused/unfocused.
Depends on: 460647
Summary: Nit: Improve header pane metrics (e.g. more actions button too big, right alignment, vertical space) → Nit: Improve header pane metrics (e.g. "other actions" button too big, fix right alignment, reduce vertical space)
this sounds like my area, so I'll see if I'm able to fix it.
Would block for an hour on this, but not for a day.
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Priority: -- → P3
Created attachment 406992 [details] [diff] [review] patch to fix the button size problem This patch fixes the button size, the alignment of the other button and the line height (it seems, but try it to be sure).
Comment on attachment 406992 [details] [diff] [review] patch to fix the button size problem forgot to ask for review!
The first two stanzas of the patch look fine. I'm a little concerned about the 1.9em thing in the third stanza: it's hard to tell whether this is really the right fix, as there's no comment explaining why it's there in the first place. It appears to have originally showed up with the "email bubbles" implemention in bug 456818. I have this vague recollection of davida, Standard8, and clarkbw wanting extra space for when the mouseover bubbles were drawn around email addresses. I've added them to the CC of this bugs in the hopes that one of them can confirm, deny, or, even better, clarify... Note that the differing line heights bug, at least, happens on the other platforms too, not just windows. Once it is clearer what the intended fix should be on this bug, I'd suggest checking whether all headerValueBoxes are guaranteed to have a singleline attribute, since, if not, this would likely only fix part of the problem. Finally, since I'm travelling for the next little while, I suggest bwinton as a reviewer for your next iteration.
I'm not seeing any effects of this patch on the 'bubbles' (which is good) I am seeing that I think we've regressed in that the keyboard focus ring shifts things down when we tab over the header values. So somewhere we lost a margin of 1px in the height computation, which I guess is bug 460647. I too don't understand the third chunk. What happens w/o it?
Created attachment 407752 [details] [diff] [review] new patch without the last part Here is a new patch without the last part. Still trying to figure out why the gap is so big.
(In reply to comment #9) > Once it is clearer what the intended fix should be on this bug, I'd suggest > checking whether all headerValueBoxes are guaranteed to have a singleline > attribute, since, if not, this would likely only fix part of the problem. It seems like the subject line does not have a headerValueBox and that might explain why the gap isn't at big between that and the surrounding line is smaller than others.
Created attachment 408018 [details] comparing with and without height removed Screenshot of height: 1.9em; in .headerValueBox[singleline="true"] removed
Created attachment 408845 [details] [diff] [review] patch for gnomestripe This makes all the entrys the same height (as far as I can see in DOM inspector). Qute and pinstripe patches not all done yet, as my Linux system is the only one with a dom inspector that works and it needs some more testing first.
Attachment #407752 - Attachment is obsolete: true
Created attachment 408850 [details] [diff] [review] qute + gnomestripe Only need to make look how things look in pinstripe
Attachment #408845 - Attachment is obsolete: true
Created attachment 408859 [details] [diff] [review] gnomestripe+qute+pinstripe Here is the patch for all three systems. Need to finish compiling the pinstripe part first to try it out in action first though.
Attachment #408850 - Attachment is obsolete: true
Comment on attachment 408859 [details] [diff] [review] gnomestripe+qute+pinstripe Looks good on OSX to it seems, marking for review!
Whiteboard: [no l10n impact] → [no l10n impact][needs review bwinton,clarkbw]
Target Milestone: --- → Thunderbird 3.0rc1
Comment on attachment 408859 [details] [diff] [review] gnomestripe+qute+pinstripe >+++ b/mail/themes/gnomestripe/mail/messageHeader.css >@@ -389,6 +389,9 @@ > -moz-margin-start: 3px !important; > border: none !important; > background-color: transparent; >+ padding-top: .1em; >+ padding-bottom: .1em; >+ margin-bottom: 3px !important; Maybe put these padding and margin values up a couple of lines to be next to the padding and margin values that are already there. (This applies to all three themes.) >+++ b/mail/themes/pinstripe/mail/messageHeader.css >@@ -414,12 +414,13 @@ > -moz-appearance: none !important; > > padding: 0px !important; > margin: 0px !important; >+ I don't know if we need this extra line, but if we do, we should really remove the two trailing spaces. ;) >+++ b/mail/themes/qute/mail/messageHeader.css >@@ -401,13 +400,16 @@ > padding: 0px; [snip…] >+ padding-top: .1em; >+ padding-bottom: .1em; Also I think there's an easier way to set these... From http://www.w3schools.com/css/css_padding.asp padding: .1em 0px; should do the same thing. r=me with these nits fixed. Thanks, Blake.
Attachment #408859 - Flags: review?(bwinton) → review+
Whiteboard: [no l10n impact][needs review bwinton,clarkbw] → [no l10n impact][needs ui-review clarkbw]
This seems like it got bitrotted for me :(
Whiteboard: [no l10n impact][needs ui-review clarkbw] → [no l10n impact][needs updated patch]
Created attachment 409195 [details] [diff] [review] updated patch this should work better
Whiteboard: [no l10n impact][needs updated patch] → [no l10n impact][needs review clarkbw]
Created attachment 409318 [details] [diff] [review] small update Seems that when trying to fix the bitrot issues, I reintroduced the issue with the junk button not being big enough. This should work better.
Created attachment 409319 [details] [diff] [review] sorry, last patch from wrong tree! I suck. :) This should work better.
Attachment #409318 - Attachment is obsolete: true
Created attachment 409321 [details] screenshots of header in gnomestripe, pinstripe and qute to ease the UI review.
Comment on attachment 409319 [details] [diff] [review] sorry, last patch from wrong tree! whups! forgot to ask for ui-r again!
Attachment #409319 - Flags: ui-review?(clarkbw)
Attachment #409319 - Flags: ui-review?(clarkbw) → ui-review+
Hardware: x86 → All
Whiteboard: [no l10n impact][needs review clarkbw] → [no l10n impact]
Version: 3.0 → Trunk
Checked in: http://hg.mozilla.org/comm-central/rev/32feaad3e42b http://hg.mozilla.org/releases/comm-1.9.1/rev/6f3e060b0f8d
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.