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

RESOLVED FIXED in Thunderbird 3.1a1

Status

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dmose, Assigned: BenB)

Tracking

({regression})

Thunderbird 3.1a1
x86
All
regression
Dependency tree / graph
Bug Flags:
blocking-thunderbird3 -

Firefox Tracking Flags

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

Details

Attachments

(8 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
Created attachment 410671 [details]
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+
(Reporter)

Comment 1

9 years ago
Created attachment 410672 [details]
test case
(Assignee)

Updated

9 years ago
Assignee: mozilla → ben.bucksch
(Reporter)

Comment 2

9 years ago
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.  :-/
(Reporter)

Comment 3

9 years ago
As far as I know, this has only been sighted on the Mac so
OS: Mac OS X → All
(Reporter)

Comment 4

9 years ago
Whoops, conflated two changes.  I just tested, and I see it on Windows too.
(Assignee)

Comment 5

9 years ago
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.
(Assignee)

Comment 6

9 years ago
> 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?
(Assignee)

Comment 8

9 years ago
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.
(Assignee)

Comment 9

9 years ago
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.
(Assignee)

Comment 10

9 years ago
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!
(Assignee)

Comment 11

9 years ago
I think that these lines should be "overflow: auto" anyways, even without this bug.
Created attachment 410787 [details]
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.
(Assignee)

Comment 13

9 years ago
Created attachment 410790 [details] [diff] [review]
Possible fix, v1: use "overflow: auto"
(Assignee)

Updated

9 years ago
Attachment #410790 - Attachment is obsolete: true
(Assignee)

Comment 14

9 years ago
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.
(Assignee)

Comment 15

9 years ago
Can't really do much more here without being able to reproduce it.
(Assignee)

Comment 16

9 years ago
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.
(Assignee)

Comment 17

9 years ago
Created attachment 410804 [details]
Reduced testcase 1 - no idea whether that triggers the bug
(Assignee)

Comment 18

9 years ago
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.
(Reporter)

Comment 19

9 years ago
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
(Assignee)

Comment 20

9 years ago
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.
(Assignee)

Comment 21

9 years ago
> 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
(Assignee)

Comment 22

9 years ago
Oh, FWIW: Neil++ !
(Assignee)

Updated

9 years ago
Whiteboard: Fixed by bug 458231. Backport or workaround?
(Assignee)

Comment 23

9 years ago
Created attachment 410992 [details]
Screenshot of bug, with TB 3.0 branch
(Assignee)

Comment 24

9 years ago
Created attachment 410993 [details]
Screenshot without bug, with TB 3.0 branch with patch from bug 458231 (same on TB trunk)
Attachment #410804 - Attachment is obsolete: true

Updated

9 years ago
Whiteboard: Fixed by bug 458231. Backport or workaround? → [no l10n impact][Fixed by bug 458231. Backport or workaround?]
(Assignee)

Updated

9 years ago
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.
(Reporter)

Comment 26

9 years ago
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
(Assignee)

Comment 27

9 years ago
> 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).
(Reporter)

Comment 28

9 years ago
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.
(Assignee)

Comment 30

9 years ago
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

9 years ago
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;
}
(Assignee)

Comment 32

9 years ago
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).
(Assignee)

Comment 33

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

Comment 34

9 years ago
Created attachment 412857 [details] [diff] [review]
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.
Attachment #412857 - Flags: review?(bwinton)
(Assignee)

Comment 35

9 years ago
Created attachment 412859 [details]
Screenshot with Fix v1, 3.0 branch
(Assignee)

Comment 36

9 years ago
Actually, URLs were fine before as well, because we are wrapping on / and . and - , but this is now also wrapping whenthereisaverylongwordwithoutspacesandwithoutanysuchspecialcharacterswhichwouldnotwrapbeforebutnowitisforcedtowrap.
(Assignee)

Comment 37

9 years ago
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+
(Assignee)

Comment 39

9 years ago
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?
(Assignee)

Updated

9 years ago
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.
(Assignee)

Comment 41

9 years ago
> 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 :).
(Reporter)

Comment 42

9 years ago
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-

Comment 43

9 years ago
What about narrowing things down by using this:

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

Comment 44

9 years ago
Created attachment 412949 [details] [diff] [review]
Fix variant for branch: limit scope to "all headers" mode

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+
(Assignee)

Updated

9 years ago
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+

Comment 47

9 years ago
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.
(Assignee)

Comment 49

9 years ago
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
(Assignee)

Comment 53

9 years ago
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
(Assignee)

Updated

9 years ago
Duplicate of this bug: 534095
(Assignee)

Comment 55

9 years ago
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?

Updated

9 years ago
Duplicate of this bug: 536117
(Reporter)

Updated

9 years ago
Attachment #412949 - Flags: approval-thunderbird3.0.1? → approval-thunderbird3.0.1+
(Reporter)

Comment 57

9 years ago
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?
status-thunderbird3.0: --- → .1-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]
(Assignee)

Comment 60

9 years ago
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]
(Assignee)

Comment 61

9 years ago
I think everything here is done. Marking FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 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?
(Assignee)

Comment 63

9 years ago
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.
Keywords: verified-thunderbird3.0
(Assignee)

Updated

9 years ago
Duplicate of this bug: 538736
(Assignee)

Updated

9 years ago
Duplicate of this bug: 540664

Updated

9 years ago
Duplicate of this bug: 541084

Updated

9 years ago
Duplicate of this bug: 526424
You need to log in before you can comment on or make changes to this bug.