565209 - Preference to show n lines before "more" is shown doesn't display multiple lines if needed

Status: VERIFIED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[rsx11m]

The mailnews.headers.show_n_lines_before_more preference was introduced in
bug 550487 to allow a message to open with n lines of To: or Cc: addresses. Testing with 3.1 beta 2, changing the pref from its default "1" only has the effect of changing the number for the "more" and its hidden status:

For mailnews.headers.show_n_lines_before_more = 1:
- one line is shown
- the last address may be truncated
- the "more" button appears

For mailnews.headers.show_n_lines_before_more > 1:
- only one line is shown as well
- the last address may be truncated
- the "more" button only appears if the selected number of lines is exceeded,
  then it represents the number of additional addresses that would be shown.

My guess is that the "singleline" attribute isn't removed to allow wrapping of the headers when more than one line is to be shown on opening by default.

Comment 1

[rsx11m]

The following userChrome.css code indeed allows the wrapping:

.headerValueBox > .headerValue {
  white-space: normal !important;
}

.headerValueBox {
  overflow: visible !important;
}

Changing mailnews.headers.show_n_lines_before_more doesn't seem to take effect until restart, and the number there doesn't correspond to the actual number of lines shown. I get 3 lines for n=2, 5 lines for n=3, and 6 lines for n=4 before the "more" button appears.

That's Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.5pre) Gecko/20100430 Thunderbird/3.1b2.

Comment 2

[Ben Bucksch (:BenB)]

> My guess is that the "singleline" attribute isn't removed

Yup. My patch didn't had that singleline removed. dmose re-added it in this patch revisions, with the rationale "we can fix that later".

Comment 3

[Ben Bucksch (:BenB)]

In other words, you are very welcome to play with this further and remove the singleline again. IIRC that caused some bugs (don't remember what), you'd have to fix those. But I agree the "singleline" attribute = overflow hack should die.

Comment 4

[rsx11m]

For n=1, a slight overflow of the last address would cause it to wrap though, maybe that was the underlying thought. Maybe retain the "singleline" for n=1,
but remove it for n>1 is a viable compromise (see early bug 456596 patches).

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

My recollection was that the breakage was significantly nastier than that and was related to the (more) hoisting, but it's been a little while...

Comment 6

[rsx11m]

While I was playing with it yesterday, it looked fairly stable after disabling the "singleline" attribute for the headers. Let's summarize where we are:

- The main issue of showing a full line of addresses before the "more" pops
  up was solved in bug 550487; this satisfies a lot of complaints, good!

- The mailnews.headers.show_n_lines_before_more preference is there but at the
  moment doesn't serve any purpose (even worse, changing it causes the "more"
  behavior to change as internally it thinks n lines are shown and not just 1
  while the single-line display is retained, suggesting nothing has changed).

- Users who find mailnews.headers.show_n_lines_before_more in the Config Editor
  and would like to use that hidden feature would likely accept that it isn't
  fully working as expected. If I pick n=2, but get n=3, I'm happy anyway.

Bottom line: Making the n>1 feature available for "use at your own risk" should be possible following comment #4. The functionality of the n=1 default case would not be impaired by such an approach, so it should be safe despite any glitches still present when opting for the multi-line display.

What do you think?

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Just from reading your text, that sounds sane, but I'd need to look at the code in more detail in order to have an opinion that I'd be willing to stand behind.  Because we're trying to code-freeze today, chances that I'll find that time today are slim-to-none, unfortunately.

As far as accepting a patch for this this late in the game, I'm somewhat leery, given the demonstrated fragility and under-testedness of the message-header code.

On the upside, I suspect it would be trivial to put together an add-on to work around this until we get it fixed in the core.

Comment 8

[rsx11m]

I won't have time for any quick patches today either, unfortunately. On the other hand, I agree that this doesn't necessarily have to be in the initial release. I'd think of a very simple fix for the "singleline" attribute, with respective adjustments to the tests (i.e., keeping the 1-line test but disabling the n-line test for the time being until it can be done right).

My simple rationale is to make that preference somewhat functional for those people who want to use it, without in any way jeopardizing the 1-line scenario that certainly *must* work. I can propose such a hotfix patch some time next week, maybe suitable for RC2 (if there is one) or a 3.1.x minor release.

Comment 9

[rsx11m]

Attachment: Minimal patch

Ok, I gave the code I've used in bug 456596 attachment 421270 [details] [diff] [review] a shot,
and it actually does the job. This implements comment #6, switching off the "singleline" for n>1 to allow the header line to expand.

Ben & Dan: If you have a minute to look at this, please try and comment as needed. Note that the "singleline" remains intact for fillAddressNode() and only afterward that removes it, thus providing for some safety in cases where your code may rely on the overflow condition based on that attribute.

Bryan: I didn't request ui-review as this has more of a backend character. Anyway, feel free to flag or comment as needed.

During the 10 minutes I could spare to test this, it worked without any impact for the default n=1 case. After modifying the pref, the lines nicely expanded and wrapped. In all cases the "more" button behaved as expected. This doesn't take care though of the wrong line count or the need for restarting Thunderbird after changing the pref for it to take effect.

Note that this patch disables the test_more_widget_with_maxlines_of_3 test as it would obviously throw and error. I left a comment referring to this bug. This will need to be reinstated once a follow-up bug took care of the n count.

Comment 10

[Ben Bucksch (:BenB)]

r=BenB for the mailWidgets.xml part (dunno about the test), given that it only affects people setting the pref manually in <about:config>, and gives that pref any change of working at all.

Comment 11

[rsx11m]

Attachment: Minimal patch (unbitrotted)

This caught some bitrot from the bug 563612 check-in, only offsets in mailWidgets.xml, but needed updated test.

Carrying forward r=BenB from previous comment, r? bwinton to also cover
disabling the test for the time being.

Comment 12

[Ben Bucksch (:BenB)]

Comment on attachment 445485 [details] [diff] [review]
Minimal patch (unbitrotted)

Given that the disabled test only tests the n>1 case which was broken, I think it's safe to disable it rather than letting it keep testing broken behavior.

Comment 13

[rsx11m]

Thanks Ben, the deadline for the RC1 code freeze has apparently passed already, thus I'm looking forward to some suggestions how this can still be made work.

After various checkins last night, mainly bug 530047 and bug 560695 affecting mailWidgets.xml, I've retested the patch with the updated code and it still works as intended = no errors in the console or differences in behavior.

Comment 14

[Ben Bucksch (:BenB)]

+               this.longEmailAddresses.removeAttribute("singleline");
> the deadline for the RC1 code freeze has apparently passed already,
> thus I'm looking forward to some suggestions how this can still be made work.

You mean fix the code without changing code?
Actually, it's possible, you (and the user) should be able to do it via userChrome.css in your profile, without code changes.

Comment 15

[rsx11m]

I know (see comment #1), the question was if - by some magic - the patch can still make it into RC1 or at least then as a ride-along for an RC2. We sure
can recommend the userChrome.css workaround to anybody asking, but this change should be trivial enough to eventually become part of the regular code
[also, that part of my comment was more directed at dmose or other drivers, sorry for any ambiguity here...].

Comment 16

[Mark Banner (:standard8)]

rsx11m: request approval-thunderbird3.1 on the patch with an explanation of the risk/benefit. We'll either look at getting it into one of the RCs or consider it for 3.1.1.

Comment 17

[rsx11m]

Blake, I've just noticed that the RC1 code freeze has been moved to midnight today - any chance you can give this an up or down vote this afternoon?

Comment 18

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

Sorry, no.

I was up sick all last night, and so today I'm taking a sick day, and mostly napping.  (With the occasional checking of my email.)  You could try asking someone else for a review, but I would guess that it's unlikely that you'll get an r+ for a patch that disables a test.

Later,
Blake.

Comment 19

[rsx11m]

Comment on attachment 445485 [details] [diff] [review]
Minimal patch (unbitrotted)

Sorry to hear that, get well!

I'm moving the review to dmose for the time being (note that BenB has already r+'ed it, but I don't know if this is sufficient as he isn't listed as a peer).

I think that disabling part of the test (again, n=1 isn't touched) has been well motivated. That test cannot be working anyway as intended until the n>1 case is fully fixed.

Comment 20

[rsx11m]

Attachment: Alternative patch

This is an alternative patch which still runs test_more_widget() for n=3,
but does not check on numLines > maxLines in subtest_more_widget_display().
All other subtests should be executed for both n=1 and n=3.

Maybe that's an acceptable compromise... I leave both patches up for review.

Comment 21

[rsx11m]

Comment on attachment 445485 [details] [diff] [review]
Minimal patch (unbitrotted)

I'm moving the requests back to bwinton after some E-mail discussion with him.

As Blake pointed out, there may be some time now until 3.1 RC2 (or 3.1.1) to identify what is causing this overshooting of addresses beyond the given line limit to start with. Thus, the following proposal on how to proceed:

1. Review one of the patches and - as possible - check the hotfix in on trunk
   already to get it some exposure for figuring out any bugs beyond counting
   the number of addresses wrong. With this baking time it would also be more
   feasible to get it in quickly for an RC2 (next week?) or a dot release if
   the full fix doesn't work out in time, so that the preference is usable.

2. Over the next few days, I'll have a look into fillAddressesNode() how the
   available space and the calculation for the addresses that can be fit is
   handled. I may not be able to come up with a fix but maybe find something
   that was missed. Unless the code is re-re-rewritten again to get rid of the
   "singleline" attribute altogether, a fix here would likely need the #1 part
   as well since only the counting algorithm would be corrected.

3. The subtest_more_widget_display() function should be modified in a way that
   it test "approximately equal" rather than "less or equal" for the number of
   lines, thus catching any undercounting of lines as well.

Comment 22

[rsx11m]

I think I've got a step closer to a solution - here an analysis of the current mailWidgets.xml code. There are actually a couple of omissions and issues that seem to sum up to the large error in lines generated in the end.

> 319       <method name="fillAddressesNode">

The first problem appears to be right at the beginning of this method:

> 335             var availableWidth = aAddressesNode.clientWidth;

While it is ensured that none of the address nodes is collapsed, the "more" node itself most likely is. For the calculation of this width, that node's space requirement has to be taken into account, otherwise the available space will be estimated larger than it would be once the "more" is shown. It's a bit of a chicken-and-egg problem though: The address list without the "more" may fit into n lines after wrapping, adding the node may sufficiently reduce that space to force the list into n+1 lines.

> 345               // First, add a comma as long as this isn't the first address.
> 346               if (i > 0) {
> 347                 if (cached-- > 0)
> 348                   aAddressesNode.childNodes[i*2 - 1].hidden = false;
> 349                 else
> 350                   this.appendComma();
> 351               }

The above code doesn't calculate and add the width of the comma node to curLineWidth for each additional address shown, thus this small amount of
space may lead to a premature wrapping for borderline cases.

> 371               if (!all) {
> 376                 curLineWidth += newAddressNode.clientWidth;
> 377                 if (curLineWidth > availableWidth && i > 0) {
> 378                   curLine++;
> 379                   curLineWidth = 0;
> 380                 }
> 381               }

This likely has the biggest impact: The maximum width of the line is allowed to be exceeded, then the actual wrapping ignored. Let's assume that 2.5 addresses fit into the line. The if() statement will be triggered for 3 addresses. While in reality the 3rd address would be wrapped into the next line, it is assumed here that it stays in the line just filled, and the next line starts with a width of zero despite the last address in fact having been wrapped into it.

If I get something satisfactorily running, I'll post a patch for those issues by Saturday. Please comment if anything else catches your eye.

Comment 23

[rsx11m]

Attachment: Proposed patch (v3)

This patch addresses the tasks listed in comment #22 and incorporates the "singleline" changes of attachment 445485 [details] [diff] [review].

> While it is ensured that none of the address nodes is collapsed, the "more"
> node itself most likely is. For the calculation of this width, that node's
> space requirement has to be taken into account, otherwise the available space
> will be estimated larger than it would be once the "more" is shown.

This is a bit of a hack but works. I'm filling the "more" node with the number of addresses and briefly uncollapse it to get the correct clientWidth, then collapse it again so that it remains invisible (mailWidgets.xml, hunk #3). In the worst case, a 2-digit number will be replaced by a 1-digit number in the end, so this is on the safe side and rather underestimates the available width than assuming too much of it.

> The above code doesn't calculate and add the width of the comma node to
> curLineWidth for each additional address shown, thus this small amount of
> space may lead to a premature wrapping for borderline cases.

For the sake of performance, the clientWidth of the comma node is calculated only once, then reapplied for any following commas (hunks #2 and #4). It may seem weird to define commaNodeWidth as a <field> but actually provides a significant performance advantage, assumes though that fonts and themes won't change until restart.

> This likely has the biggest impact: The maximum width of the line is allowed to
> be exceeded, then the actual wrapping ignored. [2-address example].  While
> in reality the 3rd address would be wrapped into the next line, it is assumed
> here that it stays in the line just filled, and the next line starts with a
> width of zero despite the last address in fact having been wrapped into it.

This has been resolved by initializing curLineWidth with the clientWidth of
the last address rather than zero, thus correctly considering the wrapping now (hunk #5). This leaves the problem though that the last address has already been added to the list when the curLine = n condition is met (i.e., we are in line n+1 now). Consequently, the last address is hidden if n>1 is selected and n lines were filled. Note that this doesn't apply for the default n=1 option. The behavior hasn't changed here, the last address runs underneath the "more" button and thus utilizes the remaining space on the line to provide partial information on that address before the list is cut off. The "singleline" attribute remains active in this case for that purpose (hunk #6).

> Changing mailnews.headers.show_n_lines_before_more doesn't seem to take effect
> until restart ... (comment #1)

This has been fixed (hunks #1 and #6) by moving Application.prefs.getValue() from the constructor (only run once during the lifetime) to buildViews (run once for each header), similar to my patches in bug 456596. The additional reads don't seem to produce any performance penalty, but this allows immediate feedback to the user and a more cleaner starting condition for the Mozmill test (not having to rely on the widget being recreated). 

> 3. The subtest_more_widget_display() function should be modified in a way that
>    it test "approximately equal" rather than "less or equal" for the number of
>    lines, thus catching any undercounting of lines as well. (comment #21)

The test has been revised to allow a range of total-height/line-height ratio. It is allowed to deviate by 15%, thus for the first pass with n=1 the ratio can be 0.85-1.15, whereas the second pass with n=3 accepts a ratio of 2.55-3.45, accounting for any possible padding artifacts involved. It's not clear to me how much space each generated address occupies, but to ensure that enough addresses are in the list to actually fill three lines in maximum window width, the number was increased from 20 to 35 (10 addresses per line + 5 to expand into a 4th line for "more").

Estimated impact on performance for the n=1 default case:
 - The pref is read for each header (insignificant based on experience made
   in bug 456596);
 - uncollapsing and recollapsing of the "more" node (should be minor as reflow
   has to be done anyway);
 - reflow caused by the first comma node when determining its width (applies
   only once when opening the first message).

I don't see any noticeable change (patched vs. unpatched version) for the n=1 case; for n=3 minor split-second delay occurs for 1-line headers, which increases with the number of addresses to be shown for a specific message, but overall in the range of up to a second or two. Tested on Windows XP and Linux platforms with various scenarios.

Comment 24

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

Comment on attachment 446893 [details] [diff] [review]
Proposed patch (v3)

>+++ b/mail/base/content/mailWidgets.xml	Sat May 22 10:53:15 2010 -0500
>@@ -249,14 +249,10 @@
>     <implementation>
>       <constructor>
[snip…]
>-          this.maxLinesBeforeMore = Application.prefs.getValue(
>-            "mailnews.headers.show_n_lines_before_more",
>-            this.maxLinesBeforeMore);

So, I see you've moved this down to the buildViews method, but could I ask why?  (I don't think it's wrong, I'm just curious.)

>@@ -330,13 +331,16 @@
>+            // this ensures that the worst-case "n more" width is considered
>+            this.addNMore(this.mAddresses.length);
>             var availableWidth = aAddressesNode.clientWidth;
>+            this.more.collapsed = true;

Wouldn't that give us too little space for all the lines before the last one?  (No, it won't, because we show the (n more) node beside the first line, not at the end of the last line.  Which seems like a bug to me, so I'm going to ask Bryan for a ui-r for this, to see what he thinks.)

>@@ -371,14 +379,21 @@
>                 if (curLineWidth > availableWidth && i > 0) {
>                   curLine++;
>-                  curLineWidth = 0;
>+                  curLineWidth = newLineWidth;

Good catch!  Although, it got me thinking…  We don't really handle the case where it's the comma that forces the line break, do we?  Perhaps we should, by adding the width of the comma, if there are more addresses, and adding the comma before calculating the width…

(I'm not entirely sure what the changes to the algorithm would be to handle this case, but I think we should document it somewhere, so that the next time I review the patch, I can figure out if it's doing what we think it should.  ;)

>+                

Trailing spaces here.

>+                // hide the last node spanning into the additional line (n>1)

Shouldn't we do this in the n=1 case as well?  Or is that handled by the singleline attribute?

>+++ b/mail/test/mozmill/message-header/test-message-header.js	Sat May 22 10:53:15 2010 -0500
>@@ -364,12 +364,13 @@ function subtest_more_widget_display(toD
>-  if (numLines > maxLines) {
>-    throw new Error("expected <= " + maxLines + "lines; found " + numLines);
>+  // allow for a 15% tolerance for any padding that may be applied
>+  if (numLines < 0.85*maxLines || numLines > 1.15*maxLines) {
>+    throw new Error("expected == " + maxLines + "lines; found " + numLines);

I'ld prefer it if we could figure out how much padding we had, and took that into account, but I would accept this if you didn't feel like it.

Thanks,
Blake.

Comment 25

[rsx11m]

Attachment: Proposed patch (v4)

Blake, thanks for the review, and here an updated patch (modifications relative to v3 are indicated as such) along with answers to your remarks:

>>+++ b/mail/base/content/mailWidgets.xml        Sat May 22 10:53:15 2010 -0500
>>@@ -249,14 +249,10 @@
> So, I see you've moved this down to the buildViews method, but could I ask why?

That's for two reasons: (1) to ensure that the user sees the effect of changing that pref immediately; (2) to ensure that the Mozmill test recognizes the n=3 case properly.

>>@@ -330,13 +331,16 @@
> Wouldn't that give us too little space for all the lines before the last one?
> (No, it won't, because we show the (n more) node beside the first line, not at
> the end of the last line.  Which seems like a bug to me, so I'm going to ask
> Bryan for a ui-r for this, to see what he thinks.)

These are two neighboring boxes: One for the header itself (longEmailAddresses, xul:hbox), the other one for the button (more, xul:label), thus your assumption is correct here. And yes, if it is considered wrong, this would be another bug which I wouldn't intend to address here (rearranging the boxes is definitely beyond the scope of this specific bug).

>>@@ -371,14 +379,21 @@
> Good catch!  Although, it got me thinking…  We don't really handle the case
> where it's the comma that forces the line break, do we?  Perhaps we should, by
> adding the width of the comma, if there are more addresses, and adding the
> comma before calculating the width...

Modification #1: Correct, yes - it's a tiny value (about 4px) but may make a difference in borderline cases. I've changed this in the new version of the patch. Now it is explicitly adding the commaNodeWidth of the (i+1)th node if
we are not determining the width of the last address in the list.

>>+
> Trailing spaces here.

Modification #2: Oops, spaces removed.

>>+                // hide the last node spanning into the additional line (n>1)
> Shouldn't we do this in the n=1 case as well?  Or is that handled by the
> singleline attribute?

The singleline="true" will ensure that the header line is not wrapped for n=1, correct. I kept the behavior as implemented in bug 550487, assuming that it was done on purpose. This allows the last half-visible address to slide underneath the "more" button, thus giving the user a hint what is coming. If Bryan decides that's not desired, it would be easy to extend the removal of the cut-off address for n=1 as well, thus reverting to the TB3.0 behavior (with a space though between the comma and the "more"). 

>>+++ b/mail/test/mozmill/message-header/test-message-header.js  Sat May 22 10:53:15 2010 -0500
>>@@ -364,12 +364,13 @@ function subtest_more_widget_display(toD
> I'ld prefer it if we could figure out how much padding we had, and took that
> into account, but I would accept this if you didn't feel like it.

The actual padding depends on various factors like the theme used and the nesting of the boxes, thus may well be different across platforms and settings. I'd rather tend to allow a fuzzy half-line max. deviation from the expected value to account for this than to turn orange just because we are a single pixel off somewhere.

Comment 26

[rsx11m]

> (comment #25) I kept the behavior as implemented in bug 550487, assuming that it was
> done on purpose. This allows the last half-visible address to slide underneath
> the "more" button, thus giving the user a hint what is coming. If Bryan decides
> that's not desired, it would be easy to extend the removal of the cut-off
> address for n=1 as well, thus reverting to the TB3.0 behavior

If you want to try that version, here the interdiff relative to the v4 patch:

>                  // hide the last node spanning into the additional line (n>1)
> -                if (curLine >= this.maxLinesBeforeMore && this.maxLinesBeforeMore > 1) {
> +                if (curLine >= this.maxLinesBeforeMore && i > 1) {
>                    aAddressesNode.lastChild.hidden = true;
>                    i--;
>                  }

Comment 27

[rsx11m]

> (comment #24) because we show the (n more) node beside the first line, not at
> the end of the last line.  Which seems like a bug to me, so I'm going to ask
> Bryan for a ui-r for this, to see what he thinks.

Well, I've tried -moz-box-align: end; for mail-multi-emailHeaderField to flush the "more" button to the bottom of the addresses shown, but this looks ugly in both the n=1 and n>1 cases (it's too low, and the "to" more button seems to reach into the "cc" area). Thus, if you want to further investigate the "more" styling, please let's spin this off into a separate bug.

Comment 28

[rsx11m]

Comment on attachment 447859 [details] [diff] [review]
Proposed patch (v4)

Bryan, any opinion on comment #26?

Comment 29

[rsx11m]

Attachment: Alternative patch (v4 + comment #26)

For easier comparison, please flag the version you'd like better.

Comment 30

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

Comment on attachment 447859 [details] [diff] [review]
Proposed patch (v4)

I like both the new changes, and the answers to my questions.

r=me, for either of the patches.

Later,
Blake.

Comment 31

[rsx11m]

Thanks, I've pinged Bryan for the ui-review already. Since this seems to have
a realistic chance to make the RC2 deadline tonight, I'm flagging this bug as
a potential ride-along to keep it on the radar.

Risk assessment per comment #16: There is no apparent risk involved by this patch for the default use case. The patch corrects a miscounting of pixels used for this case as well (bug 567062). If attachment 449288 [details] [diff] [review] is chosen,
a slight change in appearance (avoiding display of cut-off addresses) will be introduced. To trigger any further visible effect, the user will have to know about and to manually modify a hidden preference (comment #10). Based on the chosen number of lines to display and the actual number of addresses present
in the respective message, a slight increase in time needed for building the header pane was noticed (see last part of comment #23 and bug 560695 for background). Since this is a voluntary choice by the user and can be easily reset to default, any risk posed by this patch should be minimal.

Comment 32

[Mark Banner (:standard8)]

(In reply to comment #31)
> Thanks, I've pinged Bryan for the ui-review already. Since this seems to have
> a realistic chance to make the RC2 deadline tonight, I'm flagging this bug as
> a potential ride-along to keep it on the radar.

Although the patch seems reasonably low risk, it does still seem to touch some of the core math. Added to that, this is a preference which is off by default, and hidden, so the majority of users won't notice this issue. Therefore we're not going to hold rc2 or land this in rc2, but we'll let it bake on trunk for a little bit and consider for 3.1.1.

For getting the patch into 3.1.1, please follow the procedure here: https://wiki.mozilla.org/Thunderbird/Thunderbird3_Security_And_Stability_Releases (which I still need to get updated for 3.1, but for now where it says 3.0, just read 3.1).

Comment 33

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

Comment on attachment 447859 [details] [diff] [review]
Proposed patch (v4)

This looks the best option to me.

(comment #25)
> >>+                // hide the last node spanning into the additional line (n>1)
> > Shouldn't we do this in the n=1 case as well?  Or is that handled by the
> > singleline attribute?
> 
> The singleline="true" will ensure that the header line is not wrapped for n=1,
> correct. I kept the behavior as implemented in bug 550487, assuming that it was
> done on purpose. This allows the last half-visible address to slide underneath
> the "more" button, thus giving the user a hint what is coming. If Bryan decides
> that's not desired, it would be easy to extend the removal of the cut-off
> address for n=1 as well, thus reverting to the TB3.0 behavior (with a space
> though between the comma and the "more"). 

Correct, we want this behavior.  I had meant to try sneaking in some CSS to give a slight fade effect to the last node so it was even more obvious that it had slid under the more button.

(comment #27)
> > (comment #24) because we show the (n more) node beside the first line, not at
> > the end of the last line.  Which seems like a bug to me, so I'm going to ask
> > Bryan for a ui-r for this, to see what he thinks.
> 
> Well, I've tried -moz-box-align: end; for mail-multi-emailHeaderField to flush
> the "more" button to the bottom of the addresses shown, but this looks ugly in
> both the n=1 and n>1 cases (it's too low, and the "to" more button seems to
> reach into the "cc" area). Thus, if you want to further investigate the "more"
> styling, please let's spin this off into a separate bug.

It does feel a bit odd but I'm not worried about it as we could try for better in another bug.  Also I'm not sure how well we will be able to fix it up because the multi-line presents a different visual situation that might need to be addressed differently.

Comment 34

[rsx11m]

Thanks for the review, this is ready to land then. Per comment #32, requesting push of attachment 447859 [details] [diff] [review] on comm-central only for baking on trunk.

Comment 35

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/b6094a3ed2ae

Comment 36

[rsx11m]

Tinderboxes show TEST-PASS for both test_more_widget_with_maxlines_of_3 and test_more_widget; build 1276071456 (WINNT 5.2 comm-central) works as intended.

Comment 37

[rsx11m]

Comment on attachment 447859 [details] [diff] [review]
Proposed patch (v4)

I've applied the patch to 3.1 RC2 and used it for the last couple of days on "real" mail and in various modes without any problems. No bug reports came in regarding headers or button on trunk builds. Thus, requesting branch approval.

Comment 38

[rsx11m]

@standard8: I've noticed that THUNDERBIRD_3_1_1_BUILD1 was tagged and build already. Does this mean that the patch here (along with 5 other patches in the approval queue) won't go into 3.1.1? If yes, by which reasoning?

Comment 39

[Mark Banner (:standard8)]

(In reply to comment #38)
> @standard8: I've noticed that THUNDERBIRD_3_1_1_BUILD1 was tagged and build
> already. Does this mean that the patch here (along with 5 other patches in the
> approval queue) won't go into 3.1.1? If yes, by which reasoning?

That is undetermined at the current time, if it does happen, then the reasons will be made clear (and flags updated appropriately).

Comment 40

[Mark Banner (:standard8)]

Comment on attachment 447859 [details] [diff] [review]
Proposed patch (v4)

I've had a think about this. Given where we are with 3.1.1, the limited time before release, and the fact we need it to be stable as possible, we're not going to take it for 3.1.1. However, we will take it for 3.1.2 and we'll get it into the 3.1.x nightlies as soon as we've got build 2 of 3.1.1 sorted.

Comment 41

[rsx11m]

Since TB 3.1.1 has now been released, could you check in the patch here and the one for bug 567062 on comm-1.9.2 so that interested people can test/use them in the 3.1.2pre nightly builds? Thanks.

Comment 42

[Mark Banner (:standard8)]

Comment on attachment 447859 [details] [diff] [review]
Proposed patch (v4)

Not for the quick 3.1.2, but we'll take it for 3.1.3

Comment 43

[Mark Banner (:standard8)]

(In reply to comment #42)
> Not for the quick 3.1.2, but we'll take it for 3.1.3

Oh and this can land on 1.9.2 now, but default branch only.

Comment 44

[Mark Banner (:standard8)]

Checked in to 1.9.2: http://hg.mozilla.org/releases/comm-1.9.2/rev/7e0a7b343343

Comment 45

[rsx11m]

Great, works fine in today's nightly build, the tests are happy. I have posted instructions on how to use the nightly for those interested, also to give it some exposure - http://forums.mozillazine.org/viewtopic.php?p=9696749#p9696749

Comment 46

[Ludovic Hirlimann [:Usul]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.9) Gecko/20100825 Thunderbird/3.1.3

Comment 47

[Thomas D. (:thomas8)]

Has this preference been exposed somewhere in the UI?

Comment 48

[rsx11m]

No, only the preference setting exists for an interested user but no UI has been implemented.