Bug 489609 (subjectwrap)

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

RESOLVED FIXED in Thunderbird 3.0rc1

Status

RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: BenB, Assigned: BenB)

Tracking

(Depends on: 2 bugs, Blocks: 3 bugs)

Trunk
Thunderbird 3.0rc1
Dependency tree / graph
Bug Flags:
blocking-thunderbird3 +
wanted-thunderbird3 +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(3 attachments, 10 obsolete attachments)

76.69 KB, image/png
Details
8.56 KB, patch
Details | Diff | Splinter Review
5.25 KB, image/gif
Details
(Assignee)

Description

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

Comment 2

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

Comment 3

10 years ago
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)

Comment 4

10 years ago
Created attachment 375483 [details] [diff] [review]
Fix, v1. <html:div>, header pane height calculation wrong
Assignee: nobody → ben.bucksch
(Assignee)

Comment 5

10 years ago
Created attachment 375491 [details] [diff] [review]
Fix, v2. Uses <description> with -moz-user-select . No scrollbar, not auto either.

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

Updated

10 years ago
Depends on: 492645
(Assignee)

Comment 6

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

Comment 8

10 years ago
In bug 247833 comment 8 / 9, I encountered a similar problem.
(Assignee)

Updated

10 years ago
Duplicate of this bug: 501366
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+
Created attachment 401570 [details] [diff] [review]
patch, v3

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

Comment 13

10 years ago
You mean the problem in comment 3, second paragraph, or comment 8?
Created attachment 401583 [details] [diff] [review]
patch, v4

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
Created attachment 401882 [details] [diff] [review]
patch, v5

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

Comment 17

10 years ago
Do you also see the problem I described in comment 3 / comment 5, given that you use the same solution as my patch v3?
(Assignee)

Comment 18

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

Comment 21

10 years ago
Bug 513318 FIXED: Fantastic!

Comment 22

10 years ago
dupe of bug 325661?
(In reply to comment #22)
> dupe of bug 325661?

sounds like it. Ben ?

Comment 24

10 years ago
But this one has patches, so dupe bug 325661 to this one.
Duplicate of this bug: 325661
Assignee: ben.bucksch → david.ascher
(Assignee)

Updated

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

Comment 27

10 years ago
Yes, I planned to work on it tomorrow or Wednesday (european time).

Comment 28

10 years ago
So... what if I don't want the subject to wrap to a second line? Will this be something I can disable?
(Assignee)

Comment 29

10 years ago
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.

Comment 31

10 years ago
Created attachment 403579 [details]
My minimalist TB2 UI

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

Comment 32

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

Comment 34

10 years ago
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.

Comment 35

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

Comment 36

10 years ago
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.

Comment 37

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

Updated

10 years ago
Attachment #403579 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Alias: subjectwrap
(Assignee)

Comment 39

10 years ago
Created attachment 403807 [details] [diff] [review]
Patch, v6

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

Comment 40

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

Comment 42

10 years ago
Bug 513475 is OK for me in current builds, see bug 513475 comment 12. Removing dep.
No longer depends on: 513475
(Assignee)

Updated

10 years ago
Depends on: 520391
(Assignee)

Comment 43

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

Updated

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

Comment 45

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

Comment 48

10 years ago
Created attachment 406738 [details] [diff] [review]
Fix, v7

Updated
Attachment #403807 - Attachment is obsolete: true
Attachment #406738 - Flags: review?(dmose)
Attachment #403807 - Flags: review?(dmose)
(Assignee)

Updated

10 years ago
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.
Created attachment 407112 [details]
A screenshot showing a lot of extra space.

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

Comment 53

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

Comment 54

10 years ago
Created attachment 407128 [details] [diff] [review]
Fix, v8

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

Updated

10 years ago
Attachment #407128 - Flags: superreview?(neil)
(Assignee)

Updated

10 years ago
Attachment #407128 - Flags: superreview?(neil) → superreview?(dmose)
(Assignee)

Updated

10 years ago
Attachment #407128 - Flags: superreview?(dmose) → approval-thunderbird3?
(Assignee)

Comment 56

10 years ago
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.

Updated

10 years ago
Blocks: 360488
(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?
(Assignee)

Comment 59

10 years ago
Created attachment 407434 [details] [diff] [review]
Fix, v9

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

Updated

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

Comment 61

10 years ago
Created attachment 407439 [details] [diff] [review]
Fix, v10

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

Comment 63

9 years ago
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
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 64

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

Comment 67

9 years ago
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.
Created attachment 408767 [details]
Header after the above userchrome. After clicking more.

Comment 69

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

Comment 71

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

Comment 72

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

Comment 74

9 years ago
I think it's better to split up this problem, so I filed bug 525225 about comment 71/72.
(Assignee)

Comment 75

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

Comment 76

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

Comment 78

9 years ago
I like that idea. I'll play with it.

Comment 79

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

Comment 80

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

Comment 81

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

Updated

9 years ago
Depends on: 526292
(Assignee)

Comment 85

9 years ago
David A., if you need any help, let me know. I'm available.

Updated

9 years ago
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
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Duplicate of this bug: 528434
Flags: blocking-thunderbird3.1+
(Assignee)

Updated

7 years ago
No longer depends on: 77806
(Assignee)

Updated

7 years ago
No longer depends on: 77806
You need to log in before you can comment on or make changes to this bug.