Closed Bug 489609 (subjectwrap) Opened 15 years ago Closed 15 years ago

Show whole subject - Wrap subject in message pane (long subjects not wrapped)

Categories

(Thunderbird :: Message Reader UI, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0rc1

People

(Reporter: BenB, Assigned: BenB)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Whiteboard: [no l10n impact][needs patch for bug 526918 BenB, more driver discussion, possible backout])

Attachments

(3 files, 10 obsolete files)

+++ This bug was initially created as a clone of Bug #166892 +++

The subject in the message header pane (message pane/viewer) is currently one line only. If it's more than that (which is fairly frequent, e.g. with bugzilla and in many other cases), the rest of the subject isn't visible.

Not anywhere else either:
* subject column in thread pane (folder contents) is also very limited
* It's not repeated in the scrolling body.
Only workarounds, none of them obvious:
* View Source
* Mark and select subject with the mouse or keyboard, which scrolls the text (without scrollbar)
None of that works for non-techies, so normal users have *no way to get at the full subject*. Given the importance of the subject, I think that's unacceptable.

Reason:
The subject is a readonly XUL <textbox>, to allow selection and copy&paste. (Normal XUL <label>s are not selectable and thus can't be copied either.) Also, I think accessibility of a textbox is better than labels.

Fix:
However, there are only two types of <textbox>: single line (which we use) and multiline. multiline has a fixed height though, it does not automatically adapt to the content, but shows a scrollbar where necessary. It's not really applicable here (although it would be an improvement).
So, I don't know which widget to use. label, single line textbox and multiline textbox all seem to fail important criteria.

What we need is a selectable <label>. This would be important for XUL in general, as it would be used wherever a non-modifiable data is displayed (this is exactly our case, and it's very common in apps). I think readonly textbox is just a workaround, which doesn't too well, as we can see (styling is also different etc.).
Would adding flex to the multiline textbox help?
No, because flex distributes the available size of the *container* to all children evenly (based on flex value). What we need is the text element to grow based on its *content*. flex doesn't do that, but gives the element always the same size (for the same container/window size), no matter the content.
The guy with the unpronounceable name (Mnyromyr :) ) suggested I use html. I am trying to use <html:div>. That works and fulfills all criteria (seletion/copy&paste, wraps, readonly, label style, grows height as needed).

But the header pane then seems to calculate the height wrong, I get vertical scrollbars (although almost all of the content is visible, and the header pane is resized to content), and sometimes unneeded empty space at the bottom (scrolled away). I haven't found out why that is, I'll keep poking and attach the broken patch in case somebody has an idea.
Assignee: nobody → ben.bucksch
This patch uses <description> for the subject (and other header fields like Organisation), instead of <textbox>, which wraps the text. It's also selectable thanks to the -moz-user-select, which is very useful and new in chrome.

The mysterious superfluous space and scrollbar goes away when I remove
#expandedHeaderView {
  overflow-y: auto;
  overflow-x: hidden;
}

However, that also has a max-height: 14em; If we were to ever run into that (I didn't), we wouldn't get a scrollbar anymore, because I removed the overflow-y: auto; . Not so good. But if I re-add that, the height / scrollbar bug is back. We still don't know what exactly causes that.

Only workaround I have so far is to remove the max-height as well, allowing the header pane to get very big for long subjects or many recipients which the user explicit and manually expands. I think that's acceptable.
Attachment #375483 - Attachment is obsolete: true
Depends on: 492645
See bug 492645, which is one reason for the scrollbar problems.
Not being able to see the end of the Subject if you have a sufficiently small screen seems like a really bad failure mode to me.  Marking as blocking.
Flags: blocking-thunderbird3+
Target Milestone: --- → Thunderbird 3.0rc1
In bug 247833 comment 8 / 9, I encountered a similar problem.
The Thunderbird drivers wish to release Thunderbird 3 as soon as possible. As a result, we feel that this bug shouldn't stand in the way of all the other good work getting into the hands of users sooner rather than later. Therefore we are retargeting it for 3.1. See http://ccgi.standard8.plus.com/blog/archives/242 for more details. The 3.1 release is expected to be a quick release soon after Thunderbird 3.
Flags: blocking-thunderbird3+ → blocking-thunderbird3-
Target Milestone: Thunderbird 3.0rc1 → ---
Flags: blocking-thunderbird3.1+
Flags: wanted-thunderbird3+
Attached patch patch, v3 (obsolete) — Splinter Review
Forward-ported to fix bit-rot.  This still needs to have the CSS added to other themes.  As written, it still seems to trigger bug 513475.  Also as written, it currently applies to all mail-headerfields, not just the subject.  I suspect that's ok, but haven't given a lot of thought to it yet.
Attachment #375491 - Attachment is obsolete: true
It appears to me that now that this header is inside a grid, the bug in comment 8 is likely no longer a problem.  I haven't investigated this in detail either.
You mean the problem in comment 3, second paragraph, or comment 8?
Attached patch patch, v4 (obsolete) — Splinter Review
Sweet!  

I found that i was getting scrollbars on wrap rather than resizing, which were due to the overflow-y, even though we weren't close to max-width.

Making the overflow-y:auto happen only in the all-headers mode makes this work really nicely for me on the mac.  I'm curious to know if the weird thing w/ the buttons still happens on windows/linux w/o the overflow-y in the "normal headers" mode.
Attachment #401570 - Attachment is obsolete: true
Attached patch patch, v5 (obsolete) — Splinter Review
I thought I'd merged the patches, but I suspect this needs layering on top of the previous patch
Note: AFAICT w/ at least some of these patches copying the subject using Cmd-C on the mac is broken. I don't know exactly what's going on there.
Attachment #401583 - Attachment is obsolete: true
Attachment #401882 - Attachment description: i think this probably needs layering on the previous patch → patch, v5
Do you also see the problem I described in comment 3 / comment 5, given that you use the same solution as my patch v3?
eh, I mean patch v2 of course.
Just wondering whether something changed, I was hunting ghosts, or what's up.
Ben: those sound like the bug that was fixed in 513318.
From what I can tell, the remaining things to be done are:

a) sort out bug 513475, which I believe this depends on, as per comment 11

b) make the edit context-menu come back, as per comment 16

c) figure out what the bad consequences of removing "overflow-y: auto" are, if any, and then request ui-r on that
Depends on: 513475
Bug 513318 FIXED: Fantastic!
dupe of bug 325661?
(In reply to comment #22)
> dupe of bug 325661?

sounds like it. Ben ?
But this one has patches, so dupe bug 325661 to this one.
Assignee: ben.bucksch → david.ascher
Assignee: david.ascher → ben.bucksch
Ben, we're on the verge of pulling this bug back in to block Thunderbird 3.  Is this something you can turn around quickly?
Yes, I planned to work on it tomorrow or Wednesday (european time).
So... what if I don't want the subject to wrap to a second line? Will this be something I can disable?
No, because you wouldn't be able to see the whole subject anywhere. Plus, it's nothing that we can easily make a pref for, code-wise.
It should be possible to do an effective customization with some userStyle.css, though.
Attached image My minimalist TB2 UI (obsolete) —
Sure I can - I can just mouse-over the subject in the message list if I want to see it (it then pops up in a tooltip), just like I do now if the subject is too long for either the header or the subject column in the message list. I just confirmed this still works in TB3 b4.

Why can't you just do that? Just make the whole thing show up as a tooltip if its too long...

Besides, I've even seen rumblings about some of the header stuff being redundant (not that I necessarily agree), because its already there in the message list...

I have a netbook, and as anyone with a netbook knows, every millimeter of screen real estate is precious.

Another apparently unilateral UI decision, the sum of which are making TBird less and less attractive with each passing day...

<sigh> this sucks... I've been using TBird since about version 0.7, and this is the first time I've been forced to start wondering if KMail on Windows might be usable, or maybe Evolution... or maybe Claws...

No, its not a threat, its a simple statement of fact. I don't *want* to change to something else. I *like* TBird as it is now (2.0.0.23), warts and all (e.g., the horrible IMAP behavior (downloading messages over and over again) that is now mostly being addressed (or already been addressed) in v3...

I was so ecstatic hearing about the IMAP improvements... then I ran the brick wall that are some of the horrible UI 'improvements' (tabs, loss of calendar folderpane icons and other folderpane header mods, message header crammed full of useless (to me) widgets, etc etc ad nauseum.

Sorry, I'm really not intending to **** on your hard work, its just really painful to see something so beautiful (I have my TB2 UI heavily modified to maximize usable screen real estate, and *exactly* the way I like it - see attached - and as far as I can see, I won't be able to even come CLOSE to this with TB3)...
David... ok... I guess I just need to take a few thousand deep breaths, and wait and see... lets get v3 shipped, then if extensions haven't taken care of at least the biggest of my pet peeves, I guess then it will be time to come begging for some relief...

Like I said - I'm a very long time TB user - our whole office is (I'm the IT guy), all 60+ users use TB exclusively, and most really like it (with the exception of the Outlook freaks)...

Thanks for letting me vent... and again, I really do appreciate all the hard work you guys put in... I just wish I could help by coding (no skills)...
Re comment 16 (Cmd-C does not work on Mac):
It works for me on Linux. I created a minimal testcase <http://www.bucksch.org/xfer/select.xul>, and David A. reports that works. He wants to test the patch again, to verify that it really doesn't work in TB.
Note that Cmd-C is independent from the context menu. I'll investigate the latter tomorrow.
Re: comment #33 - yes, I'm u sing it now... but my understanding was this subject line wrapping would still cause the header to take up two lines, even if this extension is being used... I'd be relieved to know I was wrong... :)
Charles, if that is the case, it would be trivial for the extension to change it back to single line, just some CSS, as DavidA said.
Ok... thanks for the confirmation... one down...

In case anyone wondered, the main source of my frustrated outburst was due to the fact that apparently there will be no way to totally kill tabs... I still am not sure whether or not this will be a deal-breaker for me...
Not that it has anything to do with this bug, but mail.tabs.autoHide?
Attachment #403579 - Attachment is obsolete: true
Alias: subjectwrap
Attached patch Patch, v6 (obsolete) — Splinter Review
- Add context menu for Copy, as requested.
  - I intentionally didn't let it open on left click,
    so that normal select (for parts of the subject, e.g. bug number)
    doesn't break.
  - I reused the string from utilityOverlay, with tooltip = label,
    to save string changes.
  - I intentionally left the oncommand code in XUL, to avoid scattering
    all over the place.
  - Needs icon
  - There should be a <description selectable="true" /> in toolkit
    which does all that (CSS -select: text, context menu for copy).
- AdjustHeaderView()
  - Changed AdjustHeaderView() to take the mode as parameter,
    because all callers either get or set the pref anyways.
  - Changed XUL attribute to be a text enum instead of an int enum,
    for readablility.
  - Changed hardcoded 1/2 values to IDL constant.
- Added -moz-user-select: text; etc. to the other themes.
  That may also be the reason why copy didn't work for David A. on Mac.
Attachment #401882 - Attachment is obsolete: true
DavidA reports that Cmd-C still doesn't work on Mac. I don't know why, given that <http://www.bucksch.org/xfer/select.xul> works.
random possibly useful info: 

mail/base/content/utilityOverlay.xul has a <key id="key_copy" key="&copyCmd.key;" modifiers="accel"/> which doesn't bind it to cmd_copy, while the one at http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/editMenuOverlay.xul#38 does. 

hoping philor has some ideas.
Bug 513475 is OK for me in current builds, see bug 513475 comment 12. Removing dep.
No longer depends on: 513475
Depends on: 520391
I filed bug 520391 about Cmd-C on Mac, because it is a general issue of XUL or Thunderbird code. Is not caused by this patch apart from we happen to run into it.
Making this bug depend on it, though. This bug is BLOCKED on that problem.

It would make sense for the Copy context menuitem to copy the selection only, if there is one, and otherwise the whole subject. (Currently, it always copies the whole subject.) However, I couldn't find a way to get the selected text from JS. <xul:textbox> just uses <html:input>, which has some special C++ methods to get the selection - we don't have that here, so I see no way to find the current selection. Given that the other fields have "Copy Email Address", "Copy newsgroup" and "Copy Link" content menuitems, which also copy the whole contents, and you can easily cut down on the paste target, I think it's OK to just copy the whole subject from the Copy context menu item. That's what the last patch does.

I think these are the remaining problems. I think the patch is ready, apart from bug 520391.
No longer depends on: 520391
Depends on: 520391
(In reply to comment #43)
> I filed bug 520391 about Cmd-C on Mac, because it is a general issue of XUL or
> Thunderbird code. Is not caused by this patch apart from we happen to run into
> it.

I've just tried the v6 patch on my Thunderbird 1.9.1 dev builds. Cmd-C (and any form of copy) of the subject seems to work fine regardless of if the subject is wrapped or not.
Comment on attachment 403807 [details] [diff] [review]
Patch, v6

Thanks for testing it.
Seems like I was hunting non-existing bugs for a whole day. :-(

That closes all remaining issues apart from the context menu copying the whole subject. I think that's fine, so requesting review.
Attachment #403807 - Flags: ui-review?(clarkbw)
Attachment #403807 - Flags: review?(dmose)
Comment on attachment 403807 [details] [diff] [review]
Patch, v6

this just failed to apply for me because of the following bit rot: 

--- mailWindowOverlay.js
+++ mailWindowOverlay.js
@@ -2006,23 +2015,27 @@ function MsgApplyFiltersToSelection()
 
 function ChangeMailLayout(newLayout)
 {
   gPrefBranch.setIntPref("mail.pane_config.dynamic", newLayout);
 }
 
 function MsgViewAllHeaders()
 {
-  gPrefBranch.setIntPref("mail.show_headers", 2);
+  gPrefBranch.setIntPref("mail.show_headers",
+        Components.interfaces.nsMimeHeaderDisplayTypes.AllHeaders); // 2
+  AdjustHeaderView("all");
   ReloadMessage();
 }
 
 function MsgViewNormalHeaders()
 {
-  gPrefBranch.setIntPref("mail.show_headers", 1);
+  gPrefBranch.setIntPref("mail.show_headers",
+        Components.interfaces.nsMimeHeaderDisplayTypes.NormalHeaders); // 1
+  AdjustHeaderView("normal");
   ReloadMessage();
 }
 
 function MsgBodyAllowHTML()
 {
   gPrefBranch.setBoolPref("mailnews.display.prefer_plaintext", false);
   gPrefBranch.setIntPref("mailnews.display.html_as", 0);
   gPrefBranch.setIntPref("mailnews.display.disallow_mime_handlers", 0);
Attachment #403807 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 403807 [details] [diff] [review]
Patch, v6

despite that bit rot; copy and paste seem to work fine for me (on mac) and the wrapping looks good.
Summary: Show whole subject - Wrap subject in message pane → Show whole subject - Wrap subject in message pane (long subjects not wrapped)
Attached patch Fix, v7 (obsolete) — Splinter Review
Updated
Attachment #403807 - Attachment is obsolete: true
Attachment #406738 - Flags: review?(dmose)
Attachment #403807 - Flags: review?(dmose)
Attachment #406738 - Flags: superreview?(dmose)
Attachment #406738 - Flags: superreview?(dmose)
Attachment #406738 - Flags: superreview-
Attachment #406738 - Flags: review?(dmose)
Attachment #406738 - Flags: review?(bwinton)
Comment on attachment 406738 [details] [diff] [review]
Fix, v7

blake has graciously agreed to pick up some of my reviews; delegating to him.
Comment on attachment 406738 [details] [diff] [review]
Fix, v7

>+++ b/mail/base/content/mailWidgets.xml
>@@ -174,21 +174,22 @@
>-      <xul:textbox class="headerValue" anonid="headerValue" flex="1" readonly="true"/>
>+      <xul:description class="headerValue" anonid="headerValue" flex="1" readonly="true"
>+                        context="copyPopup" />

If we're going to split this onto two lines, we might as well make the first line <80 characters.  (And there's an extra space before the "/>", while you're there.)

>+++ b/mail/base/content/mailWindowOverlay.js
>@@ -409,22 +409,31 @@ function initMoveToFolderAgainMenu(aMenu
>+// headermode = "all" or "normal"
>+function AdjustHeaderView(headermode)
>+{
>+  document.getElementById("expandedHeaderView")
>+        .setAttribute("show_header_mode", headermode);
>+}

You didn't want to make that into a Doxygen comment, eh?
(None of the other functions near it have them, so I wouldn't mind if you didn't, but it seems like it might be nice.)

>@@ -2020,23 +2029,27 @@ function ChangeMailLayout(newLayout)
> function MsgViewAllHeaders()
> {
>-  gPrefBranch.setIntPref("mail.show_headers", 2);
>+  gPrefBranch.setIntPref("mail.show_headers",
>+      Components.interfaces.nsMimeHeaderDisplayTypes.AllHeaders); // 2
>+  AdjustHeaderView("all");
>   ReloadMessage();
> }
> 
> function MsgViewNormalHeaders()
> {
>-  gPrefBranch.setIntPref("mail.show_headers", 1);
>+  gPrefBranch.setIntPref("mail.show_headers",
>+      Components.interfaces.nsMimeHeaderDisplayTypes.NormalHeaders); // 1
>+  AdjustHeaderView("normal");
>   ReloadMessage();
> }

So, these are the second and third places you've done the conversion from an nsMimeHeaderDisplayTypes to a show_header_mode attribute.  Maybe we should just have AdjustHeaderView take the nsMimeHeaderDisplayTypes, and do the conversion there?

>+++ b/mail/base/content/messenger.xul
>@@ -188,16 +188,26 @@
>+<popup id="copyPopup">
>+     <menuitem id="copyMenuitem"

4-space indents?  Weird.  (The same as the rest of the file, but still kinda weird.)

>+              label="&copyCmd.label;"
>+              accesskey="&copyCmd.accesskey;"
>+              tooltiptext="&copyCmd.label;"
>+              oncommand="Components.classes['@mozilla.org/widget/clipboardhelper;1']

I think these need an extra space of indent to align with id.

>+++ b/mail/themes/gnomestripe/mail/messageHeader.css
>+++ b/mail/themes/pinstripe/mail/messageHeader.css
>+++ b/mail/themes/qute/mail/messageHeader.css
>@@ -46,17 +46,17 @@
>-#expandedHeaderView {
>+#expandedHeaderView[show_header_mode="all"] {
>   overflow-y: auto;
>   overflow-x: hidden;
>   max-height: 14em;
> }

So, why are these only for the "all" mode?  (See the followup attached image for a problem it might be causing.)

Thanks,
Blake.
Do you see this sort of behaviour on Linux/Windows, too?

Thanks,
Blake.
Comment on attachment 406738 [details] [diff] [review]
Fix, v7

I'm marking this as r-, mainly because of the bug shown in the image I attached.  I'll happily change it to r+ if you can either convince me that that's not your fault, or if you just fix it.  ;)
Attachment #406738 - Flags: review?(bwinton) → review-
Blake, the problem you see is the same I described in comment 3 and 5, and David A. referred to in comment 19.
I have no idea why these are, but I suspect a XUL bug with wrapping.
I don't see that either. When in "all headers" mode (or normal mode), I don't see any extra space.
You are correct that removing the overflow-x/-y would solve it. But then the header would get too big in "all headers" mode, which is probably why David A. specifically excluded that mode in patch v5 and introduced AdjustHeaderMode().
Attached patch Fix, v8 (obsolete) — Splinter Review
Fixed all nits above.
Attachment #406738 - Attachment is obsolete: true
Attachment #407128 - Flags: review?(bwinton)
Attachment #407128 - Flags: review?(bwinton) → review+
Comment on attachment 407128 [details] [diff] [review]
Fix, v8

Sounds good to me.
Attachment #407128 - Flags: superreview?(neil)
Attachment #407128 - Flags: superreview?(neil) → superreview?(dmose)
Attachment #407128 - Flags: superreview?(dmose) → approval-thunderbird3?
Remaining problems:
- Bug 520391 - Cmd-C doesn't work on Mac.
  David A and Blake Winton saw it, Standard8 (on Mac) did not see it.
  I don't have a Mac, so I'm left in the dark with regard to that.
  I spent a whole day trying to investigate and try on a remote server,
  but could neither find a reason nor a reproduction.
- "Copy" context menu copies the whole subject, even if there's a selection.
  Not a problem on Linux, as you typically just select and
  middle-click-paste there. On Mac, it may be the only way to copy.
  On Windows, it's an annoyance, if you select e.g. a bug number and want
  to copy it with the mouse only, but keyboard (Ctrl-C) works and copies
  only the selection. Similarly, of course you can edit the subject
  after paste.
  I tried to fix that, but XUL provides no API to get the current selection,
  as far as I could see. The <xul:textbox> is implemented simply as
  <html:input>, and that has the selection getter hardcoded in C++.
- Extra space in "All headers" mode.
  See comment 51 and comment 53.
  Blake Winton sees it. I don't see it in any of my test mails, incl.
  those with extra long headers. I guess David A. didn't see it either,
  because he provided that part of the patch.
Blocks: TB2SM
(In reply to comment #56)
> Remaining problems:
> - "Copy" context menu copies the whole subject, even if there's a selection.

(From update of attachment 407128 [details] [diff] [review])

> .copyString(document.popupNode.textContent);" />

This is where we copy the whole textContent (instead of just selection) of the xul:description that contains the subject text. I'm not sure, but I think what we actually want is window.getSelection() to get just the selected text. This should work because when we show the copyPopup, we'll always have at least a cursor-selection on the subject so there can't be any other selection at that moment. However, the selection might be empty (cursor), in which case we should copy the whole subject as a courtesy. So perhaps this would work:

> +<popup id="copyPopup">
> +     <menuitem id="copyMenuitem"
> +               label="&copyCmd.label;"
> +               accesskey="&copyCmd.accesskey;"
> +               tooltiptext="&copyCmd.label;"
+               oncommand="Components.classes['@mozilla.org/widget/clipboardhelper;1']
+                          .getService(Components.interfaces.nsIClipboardHelper)
+                          .copyString((window.getSelection().isCollapsed) ? document.popupNode.textContent : window.getSelection().toString());" />
 </popup>

I didn't test this, so I wouldn't know if it works and even it works, I wouldn't know if it belongs here or in some .js file. Just an idea how to get at the selection.
Comment on attachment 407128 [details] [diff] [review]
Fix, v8

Please try Thomas' suggestion and re-request review/approval as appropriate.
Attachment #407128 - Flags: approval-thunderbird3?
Attached patch Fix, v9 (obsolete) — Splinter Review
Thomas,
thanks a lot! I didn't know this function existed.
Yes, it works beautifully, exactly as you suggested. Thanks!
That's the only change I made between v8 and v9.
Attachment #407128 - Attachment is obsolete: true
Attachment #407434 - Flags: superreview?(dmose)
Attachment #407434 - Flags: approval-thunderbird3?
Attachment #407434 - Flags: superreview?(dmose) → review?(bwinton)
Comment on attachment 407434 [details] [diff] [review]
Fix, v9

I really like the changes.  We might want to note somewhere that you can use the right-click-copy menu if you're on DavidA's (or my) Mac.

Anyways, aside from nits reported over IRC, I'm happy with the change.
Attachment #407434 - Flags: review?(bwinton) → review+
Attached patch Fix, v10Splinter Review
Whitespace nits from Blake Winton fixed. Thanks for the quick review!
Attachment #407434 - Attachment is obsolete: true
Attachment #407439 - Flags: approval-thunderbird3?
Attachment #407434 - Flags: approval-thunderbird3?
Comment on attachment 407439 [details] [diff] [review]
Fix, v10

This seems like a useful patch. I don't see either of the remaining problems in comment 56 (cmd-c not working or extra space in all-headers mode).

Therefore, please land this on trunk and branch asap so that we can get some testing and we'll monitor for regressions, if necessary we may back out, but hopefully that won't happen.

a=Standard8
Attachment #407439 - Flags: approval-thunderbird3? → approval-thunderbird3+
http://hg.mozilla.org/comm-central/rev/7b62063ef778
http://hg.mozilla.org/releases/comm-1.9.1/rev/517403b5b4da
http://hg.mozilla.org/releases/comm-1.9.1/rev/b9bf6840e551

FIXED. Thanks all!

The Cmd-C Mac problem is tracked in bug 520391.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
A problem seen with this patch (using today's Win32 nightly) is that you may actually see a much longer header pane in "Normal" compared with "All" header view. If you have a message with many recipients and click on "more", the header pane now expand beyond the former 14em limit (well, it didn't expand at all before, which was a different issue caused by the overlay-* and discussed in bug 511408). If you switch then form "Normal" to "All", surprisingly the header pane shrinks to 14em height and the scroll bar appears.
If it's true that the patch for this bug killed the scrollbar (entirely), as is claimed by rsx11m in comment #64, and bug 511408, comment #17, then we have a big problem here:

After expanding a header with many addresses, or probably already with an overlong subject, we're back to exactly the same problem that was solved by the scrollbar after countless years of trouble: Header expands beyond bottom screen border, message preview is pushed out of reach, so user can see neither full header nor anything of the body. This definitely needs some action.

My proposal to solve all these issues is outlined in bug 511408, and I maintain that it's not half as difficult to implement as it sounds.

I suggest either this bug should be reopenend or some other bug (e.g. bug 511408) should be made blocking.
Even when normal header view is selected, this userchrome seems to fix it:
  #expandedHeaderView {
   max-height: 14em !important;
   overflow: auto !important;
 }

So I guess clicking "more" shifts to the expanded header view somehow.
Actually, it's always expandedHeaderView, but the CSS rules are now limited by

>-#expandedHeaderView {
>+#expandedHeaderView[show_header_mode="all"] {

to apply only for View > Headers > All, thus not covering the "more" case. Without being able to come up with some code myself, a max-height: 14em would have to be maintained in general without overflow to start with, then if a vertical overflow is detected (by a long subject line or the "more" button), the overflow-x/y properties would have to be triggered by a similar attribute to enable the vertical scroll bar.
Yes, that's reestablishing what bug 223132 introduced, nothing more.
Expanding the header pane height when opening the list is bug 511408.
w00t! This sure is useful for bugmail.
Status: RESOLVED → VERIFIED
Target Milestone: --- → Thunderbird 3.0rc1
Depends on: 524800
(In reply to comment #67)
> Actually, it's always expandedHeaderView, but the CSS rules are now limited by
> - #expandedHeaderView {
> + #expandedHeaderView[show_header_mode="all"] {
> to apply only for View > Headers > All

Indeed. So, all it takes for the normal view to be limited in height and have scrollbars (when necessary) again is to change this line back, to remove the [show_header_mode="all"]. It's mainly a matter of what we want instead of code.

The only problem may be the extra space bug (comment 56), which I personally haven't seen anymore.

Assuming that "extra space" bug doesn't exist, I would also rather opt for re-adding the scrollbars. Maybe with a bit more maximum space (20em?) before the scrollbars appear - it'll still use only as much as needed.
I spoke too soon. If do the above described change, I run into what I filed as bug 492645 back then.

In practice, this means: The height of the headers pane is calculated *before* the line-wrapping, so if you have a long subject, you get e.g. a header pane with 6 lines visible and 7 lines in the scroll pane, and a scrollbar, although we specificed the message pane may increase up to 10 lines, but it only takes the 6 lines it has *without* wrapping, so you have to scroll to see the last line in the header pane.

Apparently, that's also what David A. referred to in comment 14, and why he introduced the distinction between normal and all headers mode you see here.
So, to fix that, we'd need to have bug 492645 fixed :-( .
So comment 71 and comment 72 make me think that if we want to resolve the problem with the header pane expanding over its limit for Thunderbird 3, then we probably need to back this out. We'd then have to push for bug 492645 fixing in whatever version of gecko we take next so that we could have this sensibly working.

Thoughts?
I think it's better to split up this problem, so I filed bug 525225 about comment 71/72.
Mark, whether
1) the growing header pane after clicking "more" with lots of recipients or
2) a cut-off subject
is worse is a judgment call. I don't think either are inherently worse than the other. I like neither of them.
(But if it was my call, I think I'd live 1 rather than 2.)
Before just regular comments.  I just did a quick test with the patch inline at the bottom of this comment.  By using the (more) link to fake out the all headers mode it seems to me that we can force a scroll bar when someone clicks on the more link and then on cleanup we reset the mode back to what it was.  Note that in my patch here I didn't save the original mode and set it back which would likely be the more correct thing to do.

In my testing I looked at a message with a lot of addresses that would right now fill up the entire message area when more was clicked.  After clicking more I selected another message with a long subject and it renders correctly as the mode gets reset.  I did this in both All and Normal header modes selected.

The only issue I see is that whatever the size of the header at the time you click (more) is what it will render with, it doesn't expand the headers to a larger height and give you the scroll bars but I find it acceptable.

Is something like this a possible option for TB3?

diff --git a/mail/base/content/mailWidgets.xml b/mail/base/content/mailWidgets.xml
--- a/mail/base/content/mailWidgets.xml
+++ b/mail/base/content/mailWidgets.xml
@@ -425,16 +425,19 @@
 
       <!-- Called when the (more) indicator has been clicked on; re-renders
            the widget with all the addresses. -->
       <method name="toggleWrap">
         <body>
           <![CDATA[
             this.more.collapsed = true;
 
+            // Fake the all headers mode so we get a scroll bar
+            document.getElementById("expandedHeaderView").setAttribute("show_header_mode","all");
+
             // Causes different CSS selectors to be used, which allows all
             // of the addresses to be properly displayed and wrapped.
             this.longEmailAddresses.removeAttribute("singleline");
 
             this.clearChildNodes(this.emailAddresses);
 
             // Re-render the node, this time with all the addresses.
             this.fillAddressesNode(this.emailAddresses, -1);
@@ -462,16 +465,22 @@
       </method>
 
       <method name="clearHeaderValues">
         <body>
           <![CDATA[
             // clear out our local state
             this.mAddresses = new Array;
             this.longEmailAddresses.setAttribute("singleline", "true");
+
+            // Reset the mode to normal now that we've rendered the way we intend
+            // 2 == All Headers, 1 == Normal
+            if (Application.prefs.getValue("mail.show_headers", 1) != 2)
+              document.getElementById("expandedHeaderView").removeAttribute("show_header_mode");
+
             // remove anything inside of each of our labels....
             this.clearChildNodes(this.emailAddresses);
           ]]>
         </body>
       </method>
     </implementation>
   </binding>
I like that idea. I'll play with it.
What happens if you remove show_header_mode before this.clearChildNodes() and set it after this.fillAddressesNode() instead? The idea being that you allow the header pane to flex in height while refilling the headers after "more" has been clicked, and once this is done, allow it to be cropped to max-height and the scrollbar to appear if necessary? This may be a delicate timing though.
If there's a truely long header (I made an artificial test with a post to 30 newsgroups), the header pane in normal mode takes the majority of the message pane, without scrollbar, without ever having clicked "more" (the Newsgroups header has no "more" button). The same would be with a subject with 500+ characters, although you could argue that the subject then is the message ;-).
I filed bug 525302 to track and discuss Bryan's idea and the implementation.
Depends on: 525302
Reopening, due to fallout, after discussion with clarkbw and Standard8.  Because of the risk involved in touching this code, we've decided that the rather than accepting more risk by taking the fix to bug 525302, we'd like this patch to be backed out.

This may also require the changes from bug 524800 to be backed out as well, unfortunately.

My takeaway from this is that I was far too optimistic in thinking that it was possible to get away with not requiring automated tests for the msgHdrOverlay code.
Status: VERIFIED → REOPENED
Flags: blocking-thunderbird3- → blocking-thunderbird3+
Resolution: FIXED → ---
Whiteboard: [needs backout]
OMG, I don't believe this. We're a mail reader, and we're unable to show the full subject in the cursed header pane. That's painful, really.
Whiteboard: [needs backout] → [no l10n impact][needs backout]
Please wait on backout for a while. I have an idea I'm working on.
Hardware: x86 → All
Depends on: 526292
David A., if you need any help, let me know. I'm available.
Blocks: 520846
After discussion with davida yesterday, we had almost convinced ourselves that it would be reasonable to go forward with the status quo and not back out.  

Unfortunately, we now know about two other regressions caused by this bug which are now listed in the "depends on" field:

Bug 526292, which appears to be also fixed by the patch in bug 525302, as well as bug 526918, which does not yet have a patch, but which BenB is going to look at tomorrow.

Normally, this would have convinced me to go forward with the back out today.  However, after more discussion with clarkbw about the value of the wrapping subject, we're going to hold off on our decision until we see what the patch for bug 526918 looks like and have some more discussion about the tradeoffs and risk involved.
Whiteboard: [no l10n impact][needs backout] → [no l10n impact][needs patch for bug 526918 BenB, more driver discussion, possible backout]
After driver discussion about the various bugs involved, we're accepting some fixes, and we're not going to back this out.  Re-resolving as fixed.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Flags: blocking-thunderbird3.1+
Depends on: 77806
No longer depends on: 77806
Depends on: 77806
No longer depends on: 77806
Depends on: 77806
See Also: → 548062
You need to log in before you can comment on or make changes to this bug.