Closed Bug 526918 Opened 12 years ago Closed 11 years ago

[3.0.x only] extraneous space in header pane above the headers in "All Headers" mode

Categories

(Thunderbird :: Message Reader UI, defect)

x86
All
defect
Not set
normal

Tracking

(blocking-thunderbird3.0 .1+, thunderbird3.0 .1-fixed)

RESOLVED FIXED
Thunderbird 3.1a1
Tracking Status
blocking-thunderbird3.0 --- .1+
thunderbird3.0 --- .1-fixed

People

(Reporter: dmose, Assigned: BenB)

References

Details

(Keywords: regression)

Attachments

(8 files, 2 obsolete files)

Attached image screenshot
This seems to be at least partly content-dependent, as it doesn't happen on all messages, and perhaps also dependent on some setting, as not everyone seems to be able to reproduce it.  This is a regression caused by bug 489609.

Different messages have different amounts of blank space, but in my Inbox, at least, a significant number of messages have so much blank space that the header box appears completely blank with a scrollbar until the user manually scrolls down.  Interestingly, the amount of space displayed for a particular message is different if the message is displayed in its own tab as opposed to in the preview pane.

For people who see it frequently, this effectively regresses "All Headers" in such a way as to be so unpleasant to use that it's effectively unusable.
Flags: blocking-thunderbird3+
Attached file test case
Assignee: mozilla → ben.bucksch
Note that in order for this to have a chance of landing on the branch, we're going to tests for this fix as well.  :-/
As far as I know, this has only been sighted on the Mac so
OS: Mac OS X → All
Whoops, conflated two changes.  I just tested, and I see it on Windows too.
I can't reproduce it on Linux with your test msgs (went directly to the news server). They are all fine for me, even in "All headers" mode.
This might be related to the theme?

I suspect it's caused by a XUL bug, too. There's no reason why that extra space would be added, based on the changed made in bug 489609.

I'll keep poking, but any hints which lead to or help with the arrest of the bug would be appreciated.
> This might be related to the theme?

Did the brutal thing and, in source, did
- cd mail/themes/
- mv gnomestripe gnomestripe.real
- cp -a pinstripe gnomestrip (and later the same for qute)
and (in build dir) "make -s". I also made a change to the theme in gnomestripe, to check that the change really is applied, and it is.

I still don't see the bug, though. Which suggests to me that the theme (alone) is *not* the cause.
What do I have to do to reproduce this bug? Apply the patch from bug 489609 and make my own Thunderbird build?
Markus, Fix v10 in bug 489609 is long checked in. Just get a nightly build. If you can't see it in a recent nightly, you are not affected by the bug.
Given that the bug appears only in All headers mode, it's most likely to be
related to one of these rules:
#expandedHeaderView[show_header_mode="all"] {
  overflow-x: hidden;
  overflow-y: visible;
  max-height: 14em;
}

Actually, that reminds me that I did debug this bug half a year ago. See bug
489609 comment 3 and 5.

David Ascher said in comment 14 and 19 that this was fixed, but it seems it
wasn't.

So, I need to look at that CSS overflow again.
dmose, when you wake up, can you please try the following:
In your theme (is pinstripe used on Mac?), open messageHeader.css and change the overflow-* lines in the last comment to:
  overflow: scroll;
and then to:
  overflow: auto;
and test in each case whether it fixes the bug. Thanks!
I think that these lines should be "overflow: auto" anyways, even without this bug.
I still see an extra line at the top with both of those overflows, if I click the "more" button, and I'm in the all headers mode.
Attachment #410790 - Attachment is obsolete: true
blake made another test, to remove overflow entirely, because that fixed the extra space for me half a year ago when I still saw it (see bug 489609 comment 3 as mentioned above).

So, this pretty much confirms that we're running into a XUL bug around overflow.

dmose, please still try comment 9. I want to see whether your extra space stays so big or is reduced to that one line that blake sees.
Can't really do much more here without being able to reproduce it.
So, status is:
- dmose sees a huge extra space, blake sees one line extra space.
  I used to see extra space at the bottom half a year ago, but I no longer do.
- The factor is not the theme - even with pinstripe and qute, I don't see it.
- It *is* triggered by the following:
#expandedHeaderView[show_header_mode="all"] {
  overflow-x: hidden;
  overflow-y: auto;
  max-height: 14em;
}
  If you change that to both auto or both scroll, the bug remains.
  If you *remove* the overflow rules entirely, the extra spaces goes away.
  (However, that's not shippable, because you won't have scrollbars and
  I thin the pane will expand beyond the 14em.)

I'll make a reduced testcase with only the overflow, but I suspect that it won't trigger the bug, that some unknown factors are involved.
This testcase has "overflow: auto; max-height: 14em;" (which triggered the bug for blake, but you might want to change it to overflow-x: hidden; overflow-y: auto; as well), and a grid and wrapping header content. I have no idea whether that's enough to trigger the bug.
The other approach would to take Mail UI, and start deleting content from the header pane, like deleting all the buttons etc., and see at which point the bug disappears, to find out what else triggers it. But obivously you need to see the bug to start with, and I don't.
Interestingly, this seems to be a problem on the 1.9.1 branch, but not on the trunk.  Ben is working on figuring out more details...
Version: unspecified → 3.0
Neil, do you have any idea which fix on mozilla-central, which is not in 1.9.1, might have fixed this (see last comment)? While wildly searching bugzilla, I came across a patch for yours in bug 458231 as best candidate.

My only other hope to find the cause is to make a hg bisect (given that frankenbirds with current Mail XUL combined with various Geckos, esp. from the nightlies, don't work), which is extremely time-consuming, as each build takes 30 minutes.
> While wildly searching bugzilla, I came across a patch for yours in
> bug 458231 as best candidate.

hihahahiiihahhaaa *laughing crazy*
Indeed, it was this one. I applied the patch to my 3.0 branch build (with Moz 1.9.1) - I had to manually apply it, and attached the backported patch there - and indeed, the bug here was gone. Both the extra space is gone, and the Reply etc. buttons are visible again despite scrollbar, like on trunk.

So, now the question is:
- Can we get this bugfix in Mozilla 1.9.1? Probably not, not a security fix.
- Shall we get this bugfix into our version of Mozilla that we use?
- Is there a workaround? Neil, any idea?
Depends on: 458231
Oh, FWIW: Neil++ !
Whiteboard: Fixed by bug 458231. Backport or workaround?
Whiteboard: Fixed by bug 458231. Backport or workaround? → [no l10n impact][Fixed by bug 458231. Backport or workaround?]
Whiteboard: [no l10n impact][Fixed by bug 458231. Backport or workaround?] → [Fixed by bug 458231. Backport or workaround?][no l10n impact]
(In reply to comment #21)
> So, now the question is:
> - Can we get this bugfix in Mozilla 1.9.1? Probably not, not a security fix.

There's no tests either afaict which may make it harder.

> - Shall we get this bugfix into our version of Mozilla that we use?

That's mozilla-1.9.1 the same as Firefox. We don't want to be branching it for each version we have. As it stands even getting it into 1.9.1 for TB 3 release will be difficult as we're currently planning on picking up 1.9.1.5 and I suspect 1.9.1.6 won't be out for a while yet.
Because only some people see this, and because there's an easy workaround (scroll down or use view source), we've decided that this doesn't need to block 3.0.  That said, we'd certainly consider a workaround, and it would also be worthwhile to get bug 458231 onto the 3.1 branch ASAP.
Flags: blocking-thunderbird3+ → blocking-thunderbird3-
Keywords: relnote
> it would also be worthwhile to get bug 458231 onto the 3.1 branch ASAP.

I think TB 3.1 will be based on Moz 1.9.2 or newer, right? According to the bug, it's in the Moz 1.9.2 branch (and trunk).
Indeed, I meant the 3.0 branch.
(In reply to comment #22)
> Oh, FWIW: Neil++ !
Seriously, I only fixed the bug because text-shadow (needed for XP Classic native theme) wasn't working on disabled checkboxes or radio buttons. In that case though I managed to come up with a workaround using negative margins. Without a test case (no, I really don't want to build Thunderbird just for you) I can only speculate on why that bug might have any effect, suffice to say that the previous calculation was... interesting.
Neil, in case you want to try it out, you wouldn't have to build, you could get a Thunderbird 3.0 nightly <http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/>.
An effective workaround for this issue is to add this code to the theme's messageHeader.css:

#expandedHeaderView[show_header_mode="all"] .headerValue {
  word-wrap: break-word;
}
CatThief, thanks for the tip, but it doesn't help in my tests (the "New Activities Modules" thread on mozilla.governance newsgroup that dmose pointed me to).
Correction, it does help, if I put it in userChrome.css. I'll try to find out what went wrong at my end.
(Of course it had to be something stupid. I was editing trunk source and building and running 3.0 branch.)

This word-wrap works very nicely. It also shows why the problem was appearing only for some messages: The usenet posts have very long header values without spaces, e.g. the Path: and X-Trace: headers. Without this patch, they are much longer than the visual pane, and there's no horizontal scrollbar either (because we suppress it in CSS). With this path, they wrap nicely, which is more natural. It occupies more space, but that's what the user asked for when choosing all headers :).

It shouldn't cause much problems either, because it only affects "words" which are longer than one line, which is typically no natural language word, but only URLs (and other computer values). They can still be copied properly and without whitespace, so I think this is good.

In fact, it's so nice that I also propose this for trunk.
Attachment #412857 - Flags: review?(bwinton)
Actually, URLs were fine before as well, because we are wrapping on / and . and - , but this is now also wrapping whenthereisaverylongwordwithoutspacesandwithoutanysuchspecialcharacterswhichwouldnotwrapbeforebutnowitisforcedtowrap.
Comment on attachment 412857 [details] [diff] [review]
Fix, v1: wrap extremely long "words"

CatThief, thanks a lot for this tip! :-))
Attachment #412857 - Flags: ui-review?(clarkbw)
Comment on attachment 412857 [details] [diff] [review]
Fix, v1: wrap extremely long "words"

So, this doesn't fix all the problems, but it is an improvement in some cases.

If there's another patch that fixes the issue after I click the "more" button
on the "To:" line, that would be cool.  :)

Thanks,
Blake.
Attachment #412857 - Flags: review?(bwinton) → review+
Comment on attachment 412857 [details] [diff] [review]
Fix, v1: wrap extremely long "words"

> If there's another patch that fixes the issue after I click the "more" button
> on the "To:" line, that would be cool.  :)

I tried to make the email addresses force-wrap, too, but I can't, because these labels are <label value="...">, so they won't wrap at all. Changing this is too risky for 3.0. So, I guess we'll have to live with this. I still this this patch improves the situation, so asking for 3.0 approval.
Attachment #412857 - Flags: ui-review?(clarkbw) → approval-thunderbird3?
Attachment #412859 - Flags: ui-review?(clarkbw)
(In reply to comment #39)
> I tried to make the email addresses force-wrap, too, but I can't, because these
> labels are <label value="...">, so they won't wrap at all.
You have to be really careful there; you need the entire line to wrap, not just the email address on its own. I do have a patch to do that somewhere, but it was very hacky and probably needs rewriting for Gecko 1.9.2+ anyway.
> I do have a patch to do that somewhere, but it
> was very hacky and probably needs rewriting for Gecko 1.9.2+ anyway.

dito, in bug 247833. But the more I look at the header pane, the more I think it's faster to just rewrite it in XHTML.

> You have to be really careful there [with email addresses]

I know, that's why I avoiding touching that here in this bug :).
Comment on attachment 412857 [details] [diff] [review]
Fix, v1: wrap extremely long "words"

This patch changes the behavior of many headers in both the default and all-headers mode in order to fix all-headers mode.  Given how demonstrably fragile this code is and how little test coverage there is, this seems like the wrong tradeoff for the branch.  We could consider taking it 
on branch if the selectors were tweaked so that the word-wrap changes only applied in all-headers mode.

As I've mentioned before, I think it's no longer reasonable to accept changes to the message header code without tests.  So I don't believe this should land without an automated test as well.
Attachment #412857 - Flags: approval-thunderbird3? → approval-thunderbird3-
What about narrowing things down by using this:

#expandedHeaderView[show_header_mode="all"] .headerValue[role="textbox"] {
  word-wrap: break-word;
}
OK, this is the same, just limiting it to "All headers" mode
Attachment #412949 - Flags: review?(bwinton)
Attachment #412949 - Flags: approval-thunderbird3?
Comment on attachment 412949 [details] [diff] [review]
Fix variant for branch: limit scope to "all headers" mode

Yup, I like it.  Just like the previous one, only smaller.  :)
Attachment #412949 - Flags: review?(bwinton) → review+
Attachment #412949 - Attachment description: Fix variant for branch: limit scropt to "all headers" mode → Fix variant for branch: limit scope to "all headers" mode
Comment on attachment 412859 [details]
Screenshot with Fix v1, 3.0 branch

nice!
Attachment #412859 - Flags: ui-review?(clarkbw) → ui-review+
I'm not sure what you'd want for a test case, but what I see in 3.0RC1 build 2 for Win32 is that when in "All Headers" mode, messages with DKIM signatures also cause the problem.  Huge amount of whitespace above the first displayed header and the buttons get shoved off to the right side.

(This particular message appeared in the support-firefox@lists.mozilla.org mailing list, with the subject line of "Telechargement" on Nov 19th.)

DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.fr; s=s1024;	t=1258654039; bh=mJu/QT5aLVrr354rOWtmNdHavHfyaO+wfObwmHX137I=;	h=Message-ID:X-YMail-OSG:Received:X-Mailer:Date:From:Subject:To:MIME-Version:Content-Type:Content-Transfer-Encoding;	b=d2Ow9m+4T0DilYz0NL6VuNtUltT2k+rR+VLG2iYOonEWF4skqCbXeIbSSnIuRZa6agNXRS4bS5rl/zEXz+u8X3sHwCGe/T2OalzEdBq99Ck3o+T6TA+Skrq+vzmrujLDAdQFA

DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=s1024; d=yahoo.fr;	h=Message-ID:X-YMail-OSG:Received:X-Mailer:Date:From:Subject:To:MIME-Version:Content-Type:Content-Transfer-Encoding;	b=D5gPrbK6mUJZ0Z4jN7/zFHxIw7ps6HJKbQp24rqXiVcHTqFpRdIGYRLQcKd6Wm920sfHYctoXefsr/4eC6kmk7loinZ+SBoaM/8/uBRZieeFU0zHCP8M4IllJmixg42nMDC0KMwnI7Kl5R4QlzTcUt+RMUz6LoV881GgR8EHs5I=;

X-YMail-OSG: 0M1y75YVM1myEcFL6hfC7XjnbBgWx_0895KHU_Uy7NzRaki6pfn0pCsY833.Bc0zU_4FB8VRQ3lVZj7PP4DE.Kd8htJpyvGOQgiA4q7GU0dLfqACrYLZ.FXnDHoR26uUtXs89Iip8Ikb_.mz1t0U5_CX6k9Y2Gv3wuPo0OVlbyaVHL3fYpfMXz5VJcPAeYEKcPo3IMrwYVlL9c5fVbjrHionGQAQlLs1aCgayPJcbkTWBKI2r5_pzXN1hxWnwwGIPrgSLb7rcy4-

(Mostly adding this comment because searching for "DKIM" didn't show this bug.)
From what I can tell from the comments this patch will land on trunk as well as potentially landing on branch? If so, please land it on trunk so that we can get at least some testing before we consider it for approval.
OK, I'll land on trunk.
Comment on attachment 412949 [details] [diff] [review]
Fix variant for branch: limit scope to "all headers" mode

Patch didn't make 3.0, moving to consider for 3.0.1.
Attachment #412949 - Flags: approval-thunderbird3?
Attachment #412949 - Flags: approval-thunderbird3.0.1?
Attachment #412949 - Flags: approval-thunderbird3-
Duplicate of this bug: 532657
Fix v1 would also fix bug 534095.
Summary: extraneous space in header pane above the headers in "All Headers" mode → [3.0.x only] extraneous space in header pane above the headers in "All Headers" mode
Duplicate of this bug: 534095
Comment on attachment 412857 [details] [diff] [review]
Fix, v1: wrap extremely long "words"

Requesting approval for 3.0.1

This is in trunk since a few weeks.
Attachment #412857 - Flags: approval-thunderbird3.0.1?
Duplicate of this bug: 536117
Attachment #412949 - Flags: approval-thunderbird3.0.1? → approval-thunderbird3.0.1+
Since this patch is constrained to all-headers mode, I've decided that we can probably live without a test time around.

Since the underlying bug has been fixed on trunk and 1.9.2, can this patch be backed out from the trunk at the same time it lands on 1.9.1?
blocking-thunderbird3.0: --- → needed
blocking-thunderbird3.0: needed → .1+
Target Milestone: Thunderbird 3.0rc1 → Thunderbird 3.1a1
Checked in on branch: http://hg.mozilla.org/releases/comm-1.9.1/rev/b67c17fbcc55

(In reply to comment #57)
> Since the underlying bug has been fixed on trunk and 1.9.2, can this patch be
> backed out from the trunk at the same time it lands on 1.9.1?

Ben, can you do this when you get back and resolve this bug as fixed?
Keywords: relnote
Whiteboard: [Fixed by bug 458231. Backport or workaround?][no l10n impact]
Comment on attachment 412857 [details] [diff] [review]
Fix, v1: wrap extremely long "words"

I believe that as we're taking the other patch, we don't need this one.
Attachment #412857 - Flags: approval-thunderbird3.0.1?
Whiteboard: [waiting backout from trunk before closing]
Why did you write "[waiting backout from trunk before closing]" in the status whiteboard? The fix on trunk in comment 50 is correct and should stay, as it makes sense anyways and fixes e.g. bug 534095 (which is marked dup of this).
Status: NEW → ASSIGNED
Whiteboard: [waiting backout from trunk before closing]
I think everything here is done. Marking FIXED.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(In reply to comment #60)
> Why did you write "[waiting backout from trunk before closing]" in the status
> whiteboard? The fix on trunk in comment 50 is correct and should stay, as it
> makes sense anyways and fixes e.g. bug 534095 (which is marked dup of this).

Due to Dan's comment 57:

(In reply to comment #57)
> Since the underlying bug has been fixed on trunk and 1.9.2, can this patch be
> backed out from the trunk at the same time it lands on 1.9.1?
Yup, but the patch fixed other problems that the 1.9.2 Gecko fix doesn't fix entirely. So, I think everything's fine now as-is.
Duplicate of this bug: 538736
Duplicate of this bug: 540664
Duplicate of this bug: 541084
Duplicate of this bug: 526424
You need to log in before you can comment on or make changes to this bug.