Closed Bug 667384 Opened 13 years ago Closed 13 years ago

When "From" header is pushed down by message header buttons, too much space is wasted

Categories

(Thunderbird :: Message Reader UI, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 8.0

People

(Reporter: squib, Assigned: squib)

Details

Attachments

(5 files)

When the "From" header doesn't fit on the first row of the header beside the buttons, it gets pushed down; this is good. However, we waste 14px of space when doing so because of some padding set on the From field to make it line up with the buttons when it *doesn't* get pushed down; this is bad.

In addition, 14px is more than we need to line the "From" header up with the buttons. 10px is sufficient.

Attached is a patch to fix this; I also removed the HTML tag from the message header, since it's not actually necessary (setting "display: block" gets us all the niceties we need from HTML's rendering model).

In conclusion: this patch saves us between 6px and 14px vertically.Screenshots forthcoming.
Attachment #542089 - Flags: ui-review?(nisses.mail)
Attachment #542089 - Flags: review?(bwinton)
Attached image Normal view before
Attached image Wrapped view before
Attached image Normal view after
Attached image Wrapped view after
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
Comment on attachment 542089 [details] [diff] [review]
Reduce the space wasted by the from header.

Woohoo for saving pixels! Looks good to me. ui-r+
Attachment #542089 - Flags: ui-review?(nisses.mail) → ui-review+
In the "normal view after", the date looks like it's getting a little close to the buttons.  Is there any way to push that down a bit?  I'm hoping that we can get the same amount of space at the bottom of the "other actions" as at the top of the date.

More review coming soon.

Thanks,
Blake.
(In reply to comment #6)
> In the "normal view after", the date looks like it's getting a little close
> to the buttons.  Is there any way to push that down a bit?  I'm hoping that
> we can get the same amount of space at the bottom of the "other actions" as
> at the top of the date.

It's a balancing act, since if you look at the left, there's a lot of wasted space above "from", but if you look at the right, there's hardly any space above the date. As you say, it's worse because of the "other actions" button. I'd really like to move that button up alongside the main buttons (based on Asa's mockup, so would he), but that's a bug for another day.

One alternative would be to leave the normal view the way it is in the "before" state and then condense things when we get around to moving the other actions button up to the top.
Comment on attachment 542089 [details] [diff] [review]
Reduce the space wasted by the from header.

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

I'm concerned about https://bugzilla.mozilla.org/show_bug.cgi?id=530047#c63, but I don't think you're making it any worse.

I would also prefer if the date could line up with the From address when the from address gets pushed down below the buttons.

But I won't block the review on either of those, I don't think.

Other than the comment below, I'm pretty happy with the patch, so r=me, but I would really like it if someone would pick up bug 543954 so that we can get a real solution to this in the future.

Thanks,
Blake.

::: mail/themes/pinstripe/mail/messageHeader.css
@@ +87,3 @@
>  }
>  
>  #otherActionsButton {

Is there an #expandedHeaderRows rule we should be removing here, or was that only in gnomestripe and qute?  (And shame on me if it was!)
Attachment #542089 - Flags: review?(bwinton) → review+
(In reply to comment #8)
> I'm concerned about https://bugzilla.mozilla.org/show_bug.cgi?id=530047#c63,
> but I don't think you're making it any worse.

I'm not totally sure what that comment is about, unfortunately.

> I would also prefer if the date could line up with the From address when the
> from address gets pushed down below the buttons.

I think this would be doable if we moved the "other actions" button to the toolbar, but it'll be tough otherwise.

> Other than the comment below, I'm pretty happy with the patch, so r=me, but
> I would really like it if someone would pick up bug 543954 so that we can
> get a real solution to this in the future.

I don't think that bug will help us at all, actually (unless you think we should have libmime emit the header, but that has a whole host of its own problems). There's nothing about XHTML that's really "special" that would make resolving the issues mentioned in that bug any easier.

All the various text-wrapping issues can be solved with judicious use of display: block or display: inline, effectively giving us XHTML layouting without forcing us to rewrite the header and resort to unpleasant hacks to do the things the header already does (customizable toolbars, context/popup menus, etc). You may have noticed in my patch that I drop the <html:div> element and replace it with an <hbox>. That's for the same reason: HTML simply isn't necessary to get CSS float to work.

If you like, I can look at doing a bigger update of the message header, but most of the current issues won't be helped by switching to XHTML, except inasmuch as rewriting it from scratch (in XHTML or XUL) will give us a totally new set of issues to contend with.

As for the "Scroll header pane with message" bug referenced in bug 543954, the best way to solve that would be to annoy Firefox devs until they add seamless iframes for us (this would also be wonderful for inline attachments, which have a number of issues due to all being crammed into a single document). :)

Granted, the whole "in XUL, the HTML layout model is just a CSS declaration away" thing is totally non-obvious, and I only realized it from looking at this bug. However, I think it's the best of both worlds, and I've already managed to find another place to take advantage of that knowledge (bug 630795, which originally used some stupid hack with <description> elements to make the attachment list work right).

> ::: mail/themes/pinstripe/mail/messageHeader.css
> @@ +87,3 @@
> >  }
> >  
> >  #otherActionsButton {
> 
> Is there an #expandedHeaderRows rule we should be removing here, or was that
> only in gnomestripe and qute?  (And shame on me if it was!)

It appears that that was only in gnomestripe/qute.
(In reply to comment #9)
> (In reply to comment #8)
> > I'm concerned about https://bugzilla.mozilla.org/show_bug.cgi?id=530047#c63,
> > but I don't think you're making it any worse.
> I'm not totally sure what that comment is about, unfortunately.

http://dl.dropbox.com/u/2301433/SmallHeader.mov shows the problem, I hope.  ;)
But it's a different bug, I think.

> > Other than the comment below, I'm pretty happy with the patch, so r=me, but
> > I would really like it if someone would pick up bug 543954 so that we can
> > get a real solution to this in the future.
> I don't think that bug will help us at all, actually (unless you think we
> should have libmime emit the header, but that has a whole host of its own
> problems). There's nothing about XHTML that's really "special" that would
> make resolving the issues mentioned in that bug any easier.

Well, there are a lot more people trying to do layouts in html than in xul, and thus I would expect a lot more assistance existing on the web for this kind of thing.

> As for the "Scroll header pane with message" bug referenced in bug 543954,
> the best way to solve that would be to annoy Firefox devs until they add
> seamless iframes for us (this would also be wonderful for inline
> attachments, which have a number of issues due to all being crammed into a
> single document). :)

Or have a Thunderbird dev just implement it…

> Granted, the whole "in XUL, the HTML layout model is just a CSS declaration
> away" thing is totally non-obvious, and I only realized it from looking at
> this bug.

That's my biggest problem with the display:block solution you used.  I rejected it before for being too subtle.  (And now that I think about it, I'ld like you to add a comment in the CSS, explaining what you've done, and why.  ;)

The r+ stands, though.  :)

> > ::: mail/themes/pinstripe/mail/messageHeader.css
> > Is there an #expandedHeaderRows rule we should be removing here, or was that
> > only in gnomestripe and qute?  (And shame on me if it was!)
> 
> It appears that that was only in gnomestripe/qute.

Weird.  I try to keep those synchronized (with unused rules in comments), to facilitate diffs…  Just as an FYI.

Thanks,
Blake.
(In reply to comment #10)
> (In reply to comment #9)
> > I don't think that bug will help us at all, actually (unless you think we
> > should have libmime emit the header, but that has a whole host of its own
> > problems). There's nothing about XHTML that's really "special" that would
> > make resolving the issues mentioned in that bug any easier.
> 
> Well, there are a lot more people trying to do layouts in html than in xul,
> and thus I would expect a lot more assistance existing on the web for this
> kind of thing.

That's true, but any knowledge about HTML's layout model should apply in XUL just as well if you're not using the XUL box layout. Incidentally, I tried to do a similar thing in HTML for printing, and it's really, really hard to get right in HTML too.

My main worry with switching to XHTML is that it's much harder to make all the nice XUL things work in XHTML than vice-versa, so we'd basically be gaining simpler CSS at the cost of either 1) useful features, or 2) more complicated JS (to reimplement said features).

> > As for the "Scroll header pane with message" bug referenced in bug 543954,
> > the best way to solve that would be to annoy Firefox devs until they add
> > seamless iframes for us (this would also be wonderful for inline
> > attachments, which have a number of issues due to all being crammed into a
> > single document). :)
> 
> Or have a Thunderbird dev just implement it…

Not it! ;)

> > Granted, the whole "in XUL, the HTML layout model is just a CSS declaration
> > away" thing is totally non-obvious, and I only realized it from looking at
> > this bug.
> 
> That's my biggest problem with the display:block solution you used.  I
> rejected it before for being too subtle.  (And now that I think about it,
> I'ld like you to add a comment in the CSS, explaining what you've done, and
> why.  ;)

That's fair. Even without this change, some more documentation would probably be helpful for future developers.
Checked in with some bonus comments explaining how the layout works: http://hg.mozilla.org/comm-central/rev/6247f4ccff75
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 8.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: