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

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: 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.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: test case

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

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.  :-/

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

As far as I know, this has only been sighted on the Mac so

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Whoops, conflated two changes.  I just tested, and I see it on Windows too.

Comment 5

[Ben Bucksch (:BenB)]

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.

Comment 6

[Ben Bucksch (:BenB)]

> 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.

Comment 7

[Markus Stange [:mstange]]

What do I have to do to reproduce this bug? Apply the patch from bug 489609 and make my own Thunderbird build?

Comment 8

[Ben Bucksch (:BenB)]

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.

Comment 9

[Ben Bucksch (:BenB)]

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.

Comment 10

[Ben Bucksch (:BenB)]

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!

Comment 11

[Ben Bucksch (:BenB)]

I think that these lines should be "overflow: auto" anyways, even without this bug.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Attachment: An image showing the different overflows.

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.

Comment 13

[Ben Bucksch (:BenB)]

Attachment: Possible fix, v1: use "overflow: auto"

Comment 14

[Ben Bucksch (:BenB)]

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.

Comment 15

[Ben Bucksch (:BenB)]

Can't really do much more here without being able to reproduce it.

Comment 16

[Ben Bucksch (:BenB)]

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.

Comment 17

[Ben Bucksch (:BenB)]

Attachment: Reduced testcase 1 - no idea whether that triggers the bug

Comment 18

[Ben Bucksch (:BenB)]

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.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

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...

Comment 20

[Ben Bucksch (:BenB)]

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.

Comment 21

[Ben Bucksch (:BenB)]

> 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?

Comment 22

[Ben Bucksch (:BenB)]

Oh, FWIW: Neil++ !

Comment 23

[Ben Bucksch (:BenB)]

Attachment: Screenshot of bug, with TB 3.0 branch

Comment 24

[Ben Bucksch (:BenB)]

Attachment: Screenshot without bug, with TB 3.0 branch with patch from bug 458231 (same on TB trunk)

Comment 25

[Mark Banner (:standard8)]

(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.

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 27

[Ben Bucksch (:BenB)]

> 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).

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

Indeed, I meant the 3.0 branch.

Comment 29

[[e-mail]]

(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.

Comment 30

[Ben Bucksch (:BenB)]

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/>.

Comment 31

[CatThief]

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;
}

Comment 32

[Ben Bucksch (:BenB)]

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).

Comment 33

[Ben Bucksch (:BenB)]

Correction, it does help, if I put it in userChrome.css. I'll try to find out what went wrong at my end.

Comment 34

[Ben Bucksch (:BenB)]

Attachment: Fix, v1: wrap extremely long "words"

(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.

Comment 35

[Ben Bucksch (:BenB)]

Attachment: Screenshot with Fix v1, 3.0 branch

Comment 36

[Ben Bucksch (:BenB)]

Actually, URLs were fine before as well, because we are wrapping on / and . and - , but this is now also wrapping whenthereisaverylongwordwithoutspacesandwithoutanysuchspecialcharacterswhichwouldnotwrapbeforebutnowitisforcedtowrap.

Comment 37

[Ben Bucksch (:BenB)]

Comment on attachment 412857 [details] [diff] [review]
Fix, v1: wrap extremely long "words"

CatThief, thanks a lot for this tip! :-))

Comment 38

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 39

[Ben Bucksch (:BenB)]

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.

Comment 40

[[e-mail]]

(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.

Comment 41

[Ben Bucksch (:BenB)]

> 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 42

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 43

[CatThief]

What about narrowing things down by using this:

#expandedHeaderView[show_header_mode="all"] .headerValue[role="textbox"] {
  word-wrap: break-word;
}

Comment 44

[Ben Bucksch (:BenB)]

Attachment: Fix variant for branch: limit scope to "all headers" mode

OK, this is the same, just limiting it to "All headers" mode

Comment 45

[Blake Winton (:bwinton) (:☕️)]

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.  :)

Comment 46

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 412859 [details]
Screenshot with Fix v1, 3.0 branch

nice!

Comment 47

[Thomas Harold]

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 [e-mail] 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.)

Comment 48

[Mark Banner (:standard8)]

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.

Comment 49

[Ben Bucksch (:BenB)]

OK, I'll land on trunk.

Comment 50

[Ben Bucksch (:BenB)]

http://hg.mozilla.org/comm-central/rev/4edbf6991680

Comment 51

[Mark Banner (:standard8)]

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.

Comment 53

[Ben Bucksch (:BenB)]

Fix v1 would also fix bug 534095.

Comment 55

[Ben Bucksch (:BenB)]

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.

Comment 57

[Dan Mosedale (:dmosedale, :dmose)]

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?

Comment 58

[Mark Banner (:standard8)]

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?

Comment 59

[Mark Banner (:standard8)]

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.

Comment 60

[Ben Bucksch (:BenB)]

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).

Comment 61

[Ben Bucksch (:BenB)]

I think everything here is done. Marking FIXED.

Comment 62

[Mark Banner (:standard8)]

(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?

Comment 63

[Ben Bucksch (:BenB)]

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.