550487 - Show n lines of addresses in To/CC/BCC headers

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Ben Bucksch (:BenB)]

This is an alternative to bug 517611 (and bug 456596).

Make it so that we fill n screen rows with addresses, and then show "m more".

This should be dynamic insofar as we add exactly as many addresses as will fit in the available horizontal screen space, no more and no less, therefore both avoiding that "Other actions" overwrites addresses or "more", and not just showing 1-2 addresses when the user is running fullscreen with high resolution. 

Note that the width of addresses varies widely, depending on real name length, email address length, and whether we hide the email address for addresses already in the address book. We need to take that into account.

Limitations:
- It does not have to be dynamic insofar as we adapt to window size changes immediately. We should react to them when another mail is displayed.
- We will always show one address - even if that may be overwritten by "Other actions".
Of course, avoiding the limitations would be good, but may not be achievable at the moment.

Comment 1

[Ben Bucksch (:BenB)]

Attachment: Fix v1, not working

This is my first attempt. It does not work yet.

Problems:
1. I don't know how to get the size of the new nodes. Gecko seems to delay DOM changes and lay them out when it feels like it. This means the *Width properties and getClientRects() are all return 0. That's actually what the CSSOM View spec <http://www.w3.org/TR/cssom-view/#client-attributes> says: "If A does not have an associated CSS layout box return zero and stop this algorithm." I think that's a bad idea to start with, but worse, it never defines *when* a node will "have an associated CSS layout box", nor how I can ensure that it does. Therefore, the spec is pretty useless, because it just defines that the thing will return 0 and does not tell me how to get a proper value. Bad spec!

Does anybody know how to force Gecko to apply outstanding DOM changes and do the layout, now, sync, so that JS code gets the proper values?

2. With 20 addresses, it's slow (2-3s).
3. It it never shows "more", always shows all addresses.
At least Problem 3 is caused by Problem 1, and Problem 2 might be as well.

Comment 2

[Ben Bucksch (:BenB)]

I'm not assigning this to me, because I cannot proceed without an answer to the following question (maybe NeilAway does?):
Does anybody know how to force Gecko to apply outstanding DOM changes and do
the layout, now, sync, so that JS code gets the proper values?

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

getComputedStyle should do that.

Comment 4

[rsx11m]

Didn't work for me (bug 456596 comment #44), but maybe I applied it wrong.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

rsx11m: one possible interpretation of bug 456596 comment 44 is that getComputedStyle was doing the necessary flushing, and the fact that it didn't fix your problem meant that that problem was being caused by something not related to the DOM and being out of sync.  Right?

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

"to the DOM being out of sync", I meant...

Comment 7

[rsx11m]

I've seen the same issue as stated in comment #1 = all "undefined" properties returned a zero value, with or without getComputedStyle() applied to it before reading that value. Thus, it's hard to tell if anything was flushed at all...

Comment 8

[Ben Bucksch (:BenB)]

Tried
 getComputedStyle(newAddressNode, null);
 getComputedStyle(aAddressesNode, null);
but it doesn't make a difference.

I get proper values for some nodes, and 0 for others. I think the former are the cached nodes.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Perhaps gavin or philor has insight to offer here.  Adding them...

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

So I just spent some quality time with this patch, and while I did see some bugs, I'm not sure that I actually saw the lack of DOM flushing described here.  Watching all the dump() statements fly by, the only thing that seemed to ever be 0 or undefined was offsetWidth, which doesn't appear to be used in the code anywhere.  

Did you want to use offsetWidth in your calculations?  

Or were you seeing problems with other stuff (eg clientWidth) before?

If the latter, perhaps there was some Gecko regression that has disappeared since you tried this last...

Comment 11

[Ben Bucksch (:BenB)]

Attachment: Test message with many CC recipients

I'm seeing, on some messages (one of them attached):
i=1, address w 0/undefined/0 h 0, line 0, new line width 0, box w 0 h 0
Same on latest trunk.
The effect of the lack of width is that my algo thinks it all fits nicely, and therefore all addresses get added, which is too much for the header pane, so you only see header pane, no body.
If the widths were fine, I think it would work fine.

NeilAway said on IRC yesterday: BenB: all the element size properties appear to flush layout

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Our current theory is that this is the most likely thing we're going to do for 3.1.  Setting this as blocking to reflect that.  Ben, if that changes, or if it looks like this is going to more work than can actually happen between now and next Tuesday, please let me know.  Thanks!

Comment 13

[Simon Paquet [:sipaq]]

Comment on attachment 430645 [details] [diff] [review]
Fix v1, not working

>--- a/mail/locales/en-US/chrome/messenger/preferences/advanced.dtd
>+++ b/mail/locales/en-US/chrome/messenger/preferences/advanced.dtd
> 
>+<!ENTITY displayHeaders.caption         "Display of Headers">
>+<!-- LOCALIZATION NOTE (showNLines.before.label): This will concatenate to
>+     "Show only approximately [___] lines of addresses, at first",
>+     using (showNLines.before.label) (number) (showNLines.after.label).
>+-->
>+<!ENTITY showNLines.before.label        "Show only approximately">
>+<!ENTITY showNLines.before.accesskey    "i">
>+<!ENTITY showNLines.after.label         "lines of addresses, at first">

This is bad from an l10n standpoint. You're assuming a certain word order here, which may not be present in other locales. Please make this more flexible, so that localizers can work with this.

Comment 14

[Ben Bucksch (:BenB)]

>+<!ENTITY showNLines.before.label        "Show only approximately">
>+<!ENTITY showNLines.before.accesskey    "i">
>+<!ENTITY showNLines.after.label         "lines of addresses, at first">

> You're assuming a certain word order here

Inhowfar? "before" and "after" may be any string, including the empty string. This is, in effect, the same as a placeholder. (Here, the placeholder is the textfield.)
(Only problem may be the access key, if "before" is empty. If that is your concern, I'm sure we can find something.)

Comment 15

[rsx11m]

Well, you can always make it something like "Lines of addresses to be shown when opening a message:" rather than the two parts, but I have to agree with Ben that allowing a "before" and "after" label should provide sufficient flexibility for localization one way or the other.

Comment 16

[Vlado Valastiak [:wladow]]

Comment on attachment 430645 [details] [diff] [review]
Fix v1, not working

>+# Message Header Button Box (to show hidden email addresses)
>+# LOCALIZATION NOTE (headerMoreAddresses): %S is the number of hidden addresses
>+headerMoreAddresses=%S more
>+

A pluralForm here, please

Comment 17

[Ben Bucksch (:BenB)]

Attachment: Fix, v3 - "more" not working

dmose and dbaron and me were playing with this end of last week, and dbaron gave the crucial tip: "collapsed?". Indeed, some nodes were collapsed, and those get 0 size - some XUL "specialty". I hacked some code to uncollapse it, and now it's working much better.

Still not finished:
* Remove the unconditional uncollapsing, and instead find out why they are collapsed, and fix the code which does that. May be just a matter of uncollapsing earlier (before this fillAddressesNode() code instead of afterwards), or similar, but I haven't looked into it at all.
* "17 more" is properly displayed, but when I click on it, no additional addresses show up. The "more" is removed, the comma is there, but no new addresses. Again, I haven't looked into it yet.

I currently have no energy at all to look into this, so please somebody else pick this up? Thanks.

Comment 18

[Thomas D. (:thomas8)]

Ben, does the current patch for this provide a solution for Bug 511408 - Header pane should auto-resize and/or at least be resizable for large content (can't view many to-recipients when expanded, view all headers etc.)?
In other words, when I press more-button at the end of n lines that this bugs seeks to fill with addresses, will the height of the whole header pane automatically expand to show more if not all of the expanded addresses?
Bug 511408 comment 26 seems to suggest that, and I don't believe it.

Comment 19

[Ben Bucksch (:BenB)]

> will the height of the whole header pane automatically expand

no

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Thomas, sorry if I was insufficiently clear.  I wasn't suggesting that this bug would fix that problem, but rather that our current belief/expectation is that once this bug is fixed, the message header pane will be sufficiently better than in 3.0.x that we'd be comfortable shipping 3.1.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Fix, v4, with test; needs cleanup

Here's a version that basically works.  It doesn't do anything better on resizing (that might be easy, haven't tried yet), and I haven't investigated the forced collapsing that BenB mentioned, but it's distinctly better than the status quo.  This comes with a test that checks the basic functionality, but that's only partly working.  Need to do a bunch of cleanup before this is review-ready, but I'm optimistic...

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: fix, v5

Here's a much fixed and cleaned up version of the patch, it's not yet ready for review, but I think the interactions are Good Enough (TM), which is what I'm posting for feedback on.  I've just kicked off a tryserver build, and I'll set feedback? for clarkbw once they're done.  The specific feedback I'm looking for:

* the general feel of the message header

* the patch came with visible prefs changes for Advanced -> Reading & Display when I got it.  I've left most of it in for now.  My suspicion is that we want to drop that UI and just leave it as a hidden pref, as I think it's likely to be hard for many (most?) users to figure out what it means, and it's not obvious to me how one would make it clearer.

* the biggest problem that I'm aware of with this patch is that occasionally, if the combined length of the visible addresses is _just_ right, the (more) button ends up being partly or completely occluded by the (other actions) dropdown.  Initial attempts to fix this didn't work.  It might or might not be practical to fix before we ship.  It seems clear to me that we can live with this for the partial occlusion case.  The (significantly rarer) full occlusion case is fairly unhappy.  If there's more than one address on the line, it _can_ be worked around in the unlikely case that the user knows what's going on by resizing the window to be smaller, looking at another message, and coming back to this one.  I dislike the idea of shipping with this case live, but I think we should probably live with it.

* When (n more) is being shown: If the user resizes the window to be smaller, (n more) is then occluded. If the user resizes the window to be larger, the extra space is not taken advantage of.  It _might_ be possible to get this working, but so far it appears buggy for unclear reasons.  I think we could live without this.

Comment 23

[Ben Bucksch (:BenB)]

> the biggest problem that I'm aware of with this patch is that
> occasionally, if the combined length of the visible addresses is
> _just_ right, the (more) button ends up being partly or completely
> occluded by the (other actions) dropdown.

Ah, yes. That's because we fill up the line with addresses until it's too much, and then (remove the one which was too much and) stop. That doesn't consider the "more" node, so the fix would be to add it before the test for the width.

Only complication is that the "more" node includes the number of addresses not shown, and that's not known yet, so I'd suggest to first insert a fake node with "99 more", and then remove it and replace it with the one with the real number when you're done. That costs a tiny bit of execution time, but I think that's acceptable.
Have you tried that? The fact that you moved the "addCommaAndNMore" into a function suggests that. It didn't work?

Comment 24

[Ben Bucksch (:BenB)]

> If the user resizes the window

FWIW, it was clear that this wouldn't work, see initial description "Limitations".

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #23)
>
> Have you tried that? The fact that you moved the "addCommaAndNMore" into a
> function suggests that. It didn't work?

Good inference, that's indeed why I did that (along with playing with the resizing).  During my first attempt, fillAddresses grew too complex to be debuggable.  I do think it's possible for that strategy to work, but the complexity involved makes me think we should start off by not counting on that, and if we do manage to make it happen, all the better.

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 436735 [details] [diff] [review]
fix, v5

<http://s3.mozillamessaging.com/build/try-server/2010-04-02_12:[e-mail]-improved-header-more/[e-mail]> has the Mac build.  Other builds forthcoming at <http://tinderbox.mozilla.org/showbuilds.cgi?tree=ThunderbirdTry>.

Comment 27

[Wayne Mery (:wsmwk)]

looks good here based on limited test. using v3.2 http://s3.mozillamessaging.com/build/try-server/2010-04-02_12:[e-mail]-improved-header-more/[e-mail] 

note: for some reason it's not listed on http://s3.mozillamessaging.com/build/try-server/list.html

Comment 28

[Joe Sabash [:JoeS1]]

Alignment looks good here, but one observation.
The time (clock time) between clicking 17 more in the test eml was almost 5 seconds, without any indication of what was going on. I think this will lead to clicking again, or who knows what from the user.
Perhaps a progress bar, or something in the status bar, or some such notification
Maybe "...Expanding" in place of the "More" 
I'm using a P3 1.4Gig here purposely to test this sort of thing.

Comment 29

[Dan Mosedale (:dmosedale, :dmose)]

Thanks for the heads up, Joe.  I'll make a release build and profile a bit before I submit for review.

Comment 30

[Ben Bucksch (:BenB)]

> [clicking "more" takes several seconds]

I saw that, too, it took 2-3s in my case (but I thought it was gone, maybe I am confused). That's a bug, we do something wrong here which causes this. It has to do with the collapsed, I think.

From the IRC log from 2010-03-17, 19:37 CET:
<BenB> dbaron: interestingly enough, with collapsed = false, it's *much* faster, too.                                              
<BenB> dbaron: this is weird - it'd expect invisible stuff to be faster. I'm talking about 2-3s vs. <1s, for 18 email address nodes.
<dbaron> BenB, boy, that's a lot of work... I'd expect numbers in ms for most operations
<BenB> dbaron: so do I, and that's my normal experience, that's why I was so surprised.
<BenB> dbaron: just saying there may be something wrong for the collapsed = true case.

Comment 31

[Thomas D. (:thomas8)]

(In reply to comment #30)
> > [clicking "more" takes several seconds]
> I saw that, too, it took 2-3s in my case (but I thought it was gone, maybe I am
> confused). That's a bug, we do something wrong here which causes this. It has
> to do with the collapsed, I think.
> From the IRC log from 2010-03-17, 19:37 CET: [...]

Sounds familiar, I posted this in April 09:

Bug 487688  - Message Reader: Clicking on more-button to expand long lists of contacts takes too long

...and this in October 08:

Bug 460605  - Message Reader: header pane right-click context menu popup takes too long for contacts in long list with lots of addresses

Haven't checked on these lately, though.

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: fix, v6, with working tests + lots of cleanup; still WIP

Comment 33

[Vlado Valastiak [:wladow]]

Just a friendly reminder: don't forget to address the comment 16

Comment 34

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: fix, v7

Comment 35

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 438414 [details] [diff] [review]
fix, v7

OK, this version is ready for UI review, I think.  The pref UI is removed, the pref is now purely hidden.  I've integrated the (more) hoisting suggested by Bryan.  Resizing seems to work.  Clicking the (more) button causes the header pane to scroll down by the size of the default font on the expanded-header view in order to make it more obvious to the user how to reach the reach the rest of the addresses.

Specific UI feedback I'm looking for:

* does the font-size heuristic I'm using to scroll seem reasonable?
* note that when resizing to make the window larger, the number attached to the (more) is updated, but when resizing to make the window smaller, the number does not update.  Initial attempts to fix this bug have not panned out.  Given that we're trying to push 3.1 out the door, I'm inclined to say this is good enough, and split that off to a new bug for future fixing.  That said, I _could_ simply remove the number entirely instead.  What would you prefer?

Comment 36

[Dan Mosedale (:dmosedale, :dmose)]

Once this gets UI review, what remains to be done is:

a) assuming we keep the number in the (more), switch to PluralForms.js for l10n goodness
b) check if there are still performance issues and do more profiling.  I've played around with DTrace some today, and nothing super obvious jumped out from the data I've gotten so far.
c) extremely minor cleanups
d) upload an updated patch and request review

Comment 37

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 438414 [details] [diff] [review]
fix, v7

Bryan and I walked through the UI review using Skype's screen-sharing feature, which was very handy, and he gave ui-r+ for the current behavior.

Comment 38

[Philippe M. Chiasson (:gozer)]

Comment on attachment 438414 [details] [diff] [review]
fix, v7

try build fired off with id 'bug-550487-att438414'

Comment 39

[Joe Sabash [:JoeS1]]

Looks good here, perf is down from 5 sec to expand, to about 3 sec
Still, that's dead time with no indication of what's happening.
Maybe change xxMore to "...expanding
All-in-all this change is more of benefit than a "cost"

A little aesthetic glitch: if the body text is partially under the status bar, during the expansion process, that line is stretched vertically. Minor

* note that when resizing to make the window larger, the number attached to the
(more) is updated,

Is this ability from anther patch not included in the tryserver build.
I see no way to resize just the header pane window, and resizing the entire
window has no effect on the count.

Comment 40

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #39)
> Looks good here, perf is down from 5 sec to expand, to about 3 sec
> Still, that's dead time with no indication of what's happening.
> Maybe change xxMore to "...expanding
> All-in-all this change is more of benefit than a "cost"

Glad to hear it feels that way.  Agreed that the perf problem is badness.  However, I think we should be able to fix the performance to suck less.  If I don't do it as part of this bug for b2, I'll spin off an RC1 blocker, since fixing the performance doesn't require string changes.

> A little aesthetic glitch: if the body text is partially under the status bar,
> during the expansion process, that line is stretched vertically. Minor

I'm having a hard time understanding what this means.  Can you offer a screenshot?

> * note that when resizing to make the window larger, the number attached to the
> (more) is updated,
> 
> Is this ability from anther patch not included in the tryserver build.
> I see no way to resize just the header pane window, and resizing the entire
> window has no effect on the count.

I was referring to resizing the entire window, which should be included in the most recent try-server build.  It's entirely possible that the layout engine is isn't always firing when it should be, however.

Comment 41

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: fix, v8

Carrying forward ui-review, submitting for review.  This version uses PluralForm.js for l10n goodness and cleans up bits and pieces of cruft.

As mentioned before, there are two more things to tweak, which my current theory involves doing in separate patches in this bug.  Performance (which, if that doesn't make b2, I'll spin off into an rc1 blocker, since it doesn't need strings), and resizing down (which, if that doesn't make b2, I'll spin off into a non-blocking bug).

Comment 42

[Joe Sabash [:JoeS1]]

> A little aesthetic glitch: if the body text is partially under the status bar,
> during the expansion process, that line is stretched vertically. Minor

I'm having a hard time understanding what this means.  Can you offer a
screenshot?

Unfortunately screenshots alway "take" only after the expansion is finished.
I think what is actually happening is that the messagepane height is
oscillating during the expansion period.

Attaching a screenshot after expansion.

Comment 43

[Joe Sabash [:JoeS1]]

Attachment: screenshot after more expansion

Comment 44

[Ben Bucksch (:BenB)]

Yes, the time "during expansion" should be in the milliseconds-area. It happens synchronously on the UI thread and you may see glitches during that time while the engine renders and adjusts the other UI elements. The bug here is that it takes seconds instead of milliseconds. As said above, I could link that to the collapsed stuff (which is still open, in the current patch).

+            // XXX black magic to work around an indeterminate layout bug
+            for (let node = aAddressesNode; node; node = node.parentNode)
+              node.collapsed = false;

FWIW, this workaround is neither "black magic" :) nor is this a layout bug. Something simply makes these nodes collapsed. The task is to find out which code does that any why.

Comment 45

[Ben Bucksch (:BenB)]

(To be clear: I think it's some other mail header pane code which makes them collapsed, not the layout engine.)

Comment 46

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #42)
> 
> Unfortunately screenshots alway "take" only after the expansion is finished.
> I think what is actually happening is that the messagepane height is
> oscillating during the expansion period.

So the scrollbar moving at the top is actually intentional.  The idea is that it makes it obvious to the user how to reach the rest of the addresses.

Re the bottom of the screen, are you saying that before the screenshot was taken, the statusbar was actually vertically smaller than it was when the screenshot was taken?

(In reply to comment #44)
> As said above, I could link that to the
> collapsed stuff (which is still open, in the current patch).
> 
> +            // XXX black magic to work around an indeterminate layout bug
> +            for (let node = aAddressesNode; node; node = node.parentNode)
> +              node.collapsed = false;
> 
> FWIW, this workaround is neither "black magic" :) nor is this a layout bug.
> Something simply makes these nodes collapsed. The task is to find out which
> code does that any why.

My experience debugging suggests something weirder.  The dump() statements that we had there claimed that those nodes were already uncollapsed before we did the explicit uncollapsing, yet removing the uncollapsing still breaks the functionality.  So my speculation is that the DOM getters are returning something that doesn't correspond with reality.  Or maybe layout's reality itself is in some odd state.

Comment 47

[Ben Bucksch (:BenB)]

> The dump() statements that we had there claimed that those nodes were
> already uncollapsed before we did the explicit uncollapsing

All of them? Collapsing one high-level node means all subnodes are effectively collapsed, although they don't explicitly have the collapsed state themselves. That's what I saw, at least.

My suspicion was the collapsed ones are the CC/BCC/ReplyTo when before there was no CC line, or similar. That would make sense for our code to collapse them. Or something along those lines.
(The other suspicion was cached/non-cached nodes, but that wasn't the problem, I think.)

Comment 48

[Joe Sabash [:JoeS1]]

Re the bottom of the screen, are you saying that before the screenshot was
taken, the statusbar was actually vertically smaller than it was when the
screenshot was taken?

No, the scroll position jumps (can't catch it in a screenshot)
The lower scroll button appears up from the status bar by a few pixels,
and if there is text in this area it jitters by the same amount.
After expansion, all is normal. Not a big thing, just noticed it.

Comment 49

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #47)
> > The dump() statements that we had there claimed that those nodes were
> > already uncollapsed before we did the explicit uncollapsing
> 
> All of them? Collapsing one high-level node means all subnodes are effectively
> collapsed, although they don't explicitly have the collapsed state themselves.
> That's what I saw, at least.

I thought all of them were collapsed, though the very top node appeared to be different (empty?).  I'll add back the dump and poke around some more.

Yeah, I don't think caching the problem here either, but your suspicions may prove helpful.  Thanks.

Comment 50

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

Comment on attachment 439170 [details] [diff] [review]
fix, v8

>+++ b/mail/base/content/mailWidgets.xml
>@@ -347,92 +353,143 @@
>-            // before we try to create email address nodes, try to leverage any cached nodes...
>-            var remainder = this.fillCachedAddresses(aAddressesNode, aNumAddressesToShow);
>+            // try to leverage any cached nodes before creating new ones
>+            var cached = this.mAddresses.length -
>+              this.fillCachedAddresses(aAddressesNode, 1);

Should that just be 1, or should we try to guess at a better number of addresses to put there?

>+            // add addresses until we're done, or we overflow this line
>+            var i = 0;
>+            for (let curLine = 0, curLineWidth = 0;
>+                 i < this.mAddresses.length && (all || curLine < this.maxLinesBeforeMore);
>+                 i++)
>             {
>+              let newAddressNode;
> 
>+              if (cached-- > 0)
>+                newAddressNode = aAddressesNode.childNodes[i];
>+              else
>+              {
>+                newAddressNode = document.createElement("mail-emailaddress");

My understanding is that if the else requires {}s, then you should put them around the if, too.

We also seem to be doing the {-on-the-same-line thing in other places in this file, so we should probably stick to that with the new code, too.

>+      <!-- Append a comma after the last (email address, we hope!) node of
>+           this.emailAddresses -->
>+      <method name="appendComma">

I think you use this to append commas after pretty much all the email addresses.  (But I would accept a justification that when you're adding the commas to each of them, they're the last at that point in time.)

>@@ -455,41 +512,49 @@
>             // HACK 
>             // Workaround for , which prevents .

While you're here, could you fill in what it's for, and what it prevents?

>@@ -506,28 +571,31 @@
>- <!-- XXX This shouldn't really be its own binding, since it depends on the
>-      internal structure of mail-multi-emailHeaderField (thus the parentNode
>-      glop).  It really wants to be created from the JS inside that binding
>-      somewhere, but that implies a properties file change, and we're already
>-      past string freeze, so... -->
>-  <binding id="mail-multi-more-indicator">

Nice!  I like it when we can clean up old code.

>+    <handlers>
>+      <handler event="overflow" phase="capturing">
>+         <![CDATA[
>+           this.fillAddressesNode(this.emailAddresses,
>+                                  this.more && this.more.collapsed);
>+         ]]>
>+      </handler>
>+      <handler event="underflow" phase="capturing">
>+         <![CDATA[
>+           this.fillAddressesNode(this.emailAddresses,
>+                                  this.more && this.more.collapsed);
>+         ]]>
>+      </handler>

So, this got me thinking.  Is the problem that we're not getting the overflow/underflow handler being called, and that's why we don't show the correct number of addresses, or are we calculating the correct number of addresses incorrectly?

>@@ -551,16 +619,17 @@
>     </implementation>
>+
>   </binding>

Yeah?  Really?  And extra blank line there?  Sure, I guess.

>+++ b/mail/locales/en-US/chrome/messenger/messenger.dtd
>@@ -691,20 +691,16 @@ you can use these alternative items. Oth
>-<!ENTITY more.label "more">
>+++ b/mail/locales/en-US/chrome/messenger/messenger.properties
>@@ -492,16 +492,25 @@ processingJunkMessages=Processing Junk M
>+headerMoreAddrs=#1 more;#1 more

Nice.

>+++ b/mail/test/mozmill/message-header/test-message-header.js
>@@ -281,8 +281,72 @@ function test_that_msg_without_date_clea
>+/**
>+ * Test that we only display at most the max lines preference until we
>+ * display (n more), and, after the widget is clicked, we expand the header.
>+ */
>+function test_more_widget() {

I would be interested to see another test that tried out the "mailnews.headers.show_n_lines_before_more > 1" case of your code.  (Just in case there's a bug lurking there.)

>+  // get maxline pref
>+  let prefBranch = Cc["@mozilla.org/preferences-service;1"]
>+                     .getService(Ci.nsIPrefService).getBranch(null);
>+  let maxLines = prefBranch.getIntPref(
>+                "mailnews.headers.show_n_lines_before_more", false);

That's some odd indentation you've got there.  I hear it should be 2 spaces in from the LHS or RHS.

>+  // test that we've got a (more) node and that it's expanded
>+  let moreNode = mc.a('expandedtoBox', {class: 'moreIndicator'});
>+  if (!moreNode) {
>+    throw new Error("more node not found before activation");
>+  }
>+  if (moreNode.collapsed) {
>+    throw new Error("more node was collapsed when it should have been visible");
>+  }
>+
>+  // activate (n more)
>+  let moreElem = mc.aid('expandedtoBox', {class: 'moreIndicator'});

I think you can just wrap your moreNode in an elib.Elem, and avoid the extra lookup.  (And avoid the extra variable, too.)

Having said all that, those are really all nit-ish things, so you get the r+ from me, as long as you file followup bugs for the other stuff you wanted to spin off.

Later,
Blake.

Comment 51

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #50)
> (From update of attachment 439170 [details] [diff] [review])
> >+++ b/mail/base/content/mailWidgets.xml
> >@@ -347,92 +353,143 @@
> >-            // before we try to create email address nodes, try to leverage any cached nodes...
> >-            var remainder = this.fillCachedAddresses(aAddressesNode, aNumAddressesToShow);
> >+            // try to leverage any cached nodes before creating new ones
> >+            var cached = this.mAddresses.length -
> >+              this.fillCachedAddresses(aAddressesNode, 1);
> 
> Should that just be 1, or should we try to guess at a better number of
> addresses to put there?

I think it's possible that guessing at a number would help performance issues in some cases, but it's not clear to me off the top of my head how much of a win that would be.  I'm also fairly confident in this code as written; if we made this particular at this stage, I'd be concerned about inadvertently breaking some invariant that we're currently holding true.  I've added an XXX comment to play with this in the perf bug that I'll either fix this weekend or spinoff to RC1.

> My understanding is that if the else requires {}s, then you should put them
> around the if, too.
> 
> We also seem to be doing the {-on-the-same-line thing in other places in this
> file, so we should probably stick to that with the new code, too.

All of those things are entirely true.  The reason I've made some odd-seeming choices here is that, in my opinion, this method is now both somewhat complex and simultaneously too long to see all at once while editing, given a reasonable size editing window.  This makes me fairly unhappy, because, in my mind, that makes it harder to edit without inadvertently introducing bugs.  I've already factored out various things (eg appendComma) specifically to try and address this.  My current belief is that accepting this out-of-style-ness is a reasonable trade for keeping the code more easily visible and therefore (IMO) maintainable.  

This is, of course, an absurdly subjective judgment, and as module owner, it's really yours to make.  Given that you've already r+ed, unless you chime in again on this bug before I check this in, I'll leave it as is, and you're welcome to make me tweak it in a future checkin if you disagree.

> I think you use this to append commas after pretty much all the email
> addresses.  (But I would accept a justification that when you're adding the
> commas to each of them, they're the last at that point in time.)

Right, that was actually what I intended with that comment.  I've tweaked as follows for clarity:

      <!-- Append a comma after the (currently) final (email address, we hope!)
           node of this.emailAddresses -->

> >@@ -455,41 +512,49 @@
> >             // HACK 
> >             // Workaround for , which prevents .
> 
> While you're here, could you fill in what it's for, and what it prevents?

I've replaced that text with:

            // Workaround the fact that XUL line-wrapping and "overflow: auto"
            // don't interact properly (bug 492645), without which we
            // would be inadvertently occluding too much of the message header
            // text and forcing the user to scroll unnecessarily (bug 525225).

> >-  <binding id="mail-multi-more-indicator">
> 
> Nice!  I like it when we can clean up old code.

:-)  You can thank BenB for that part of the patch.

> So, this got me thinking.  Is the problem that we're not getting the
> overflow/underflow handler being called, and that's why we don't show the
> correct number of addresses, or are we calculating the correct number of
> addresses incorrectly?

The problem has to do in part with the fact that right now, overflow is only being called on the header value box.  Since we're currently always rendering one too many email addresses and allowing that to be clipped by the now-hoisted (more) element, that parent box is effectively always in a state of overflow, which means that occluding a second email address doesn't cause another overflow event to fire on it.

One of the things I started playing with is changing the overflow attribute on the each of the email address boxes themselves so that they too get overflow events firing on them.  This didn't quite work right in some way that I don't recall offhand, but it's probably the first thing to investigate further in the spinoff bug that I'll file.

> >@@ -551,16 +619,17 @@
> >     </implementation>
> >+
> >   </binding>
> 
> Yeah?  Really?  And extra blank line there?  Sure, I guess.

Whoops!  Removed.

> I would be interested to see another test that tried out the
> "mailnews.headers.show_n_lines_before_more > 1" case of your code.  (Just in
> case there's a bug lurking there.)

Done.

> >+  // get maxline pref
> >+  let prefBranch = Cc["@mozilla.org/preferences-service;1"]
> >+                     .getService(Ci.nsIPrefService).getBranch(null);
> >+  let maxLines = prefBranch.getIntPref(
> >+                "mailnews.headers.show_n_lines_before_more", false);
> 
> That's some odd indentation you've got there.  I hear it should be 2 spaces in
> from the LHS or RHS.

Fixed.  I also noticed that I was handing an unused extra argument to getIntPref, and I've fixed that as well.

> >+  // test that we've got a (more) node and that it's expanded
> >+  let moreNode = mc.a('expandedtoBox', {class: 'moreIndicator'});
> >+  if (!moreNode) {
> >+    throw new Error("more node not found before activation");
> >+  }
> >+  if (moreNode.collapsed) {
> >+    throw new Error("more node was collapsed when it should have been visible");
> >+  }
> >+
> >+  // activate (n more)
> >+  let moreElem = mc.aid('expandedtoBox', {class: 'moreIndicator'});
> 
> I think you can just wrap your moreNode in an elib.Elem, and avoid the extra
> lookup.  (And avoid the extra variable, too.)

Good catch; tweaked.

> Having said all that, those are really all nit-ish things, so you get the r+
> from me, as long as you file followup bugs for the other stuff you wanted to
> spin off.

I'll file on both the resize number problem and the performance problem before I land, or will fix and re-request review.

Thanks!

Comment 52

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: fix, v9

Review comments addressed.

Comment 53

[Ben Bucksch (:BenB)]

+      <handler event="overflow" phase="capturing">
+           this.fillAddressesNode(this.emailAddresses,
+                                  this.more && this.more.collapsed);
+      <handler event="underflow" phase="capturing">
+           this.fillAddressesNode(this.emailAddresses,
+                                  this.more && this.more.collapsed);

This is bothersome... Why do you need this? It looks like lots of wasted effort or even an infinite loop at worst, given that fillAddressesNodes fills elements and can again cause an over/underflow. If it works, I'd think it's more coincidence, unless you specifically guarded against that in the code.

If you can find another solution to whatever you tried to solve with these event handlers, that would probably make the code much less regression-prone.

Comment 54

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

(In reply to comment #51)
> > My understanding is that if the else requires {}s, then you should put them
> > around the if, too.
> > We also seem to be doing the {-on-the-same-line thing in other places in
> > this file, so we should probably stick to that with the new code, too.
> All of those things are entirely true.  The reason I've made some odd-seeming
> choices here is that, in my opinion, this method is now both somewhat complex
> and simultaneously too long to see all at once while editing, given a
> reasonable size editing window.  This makes me fairly unhappy, because, in my
> mind, that makes it harder to edit without inadvertently introducing bugs. 
> I've already factored out various things (eg appendComma) specifically to try
> and address this.  My current belief is that accepting this out-of-style-ness
> is a reasonable trade for keeping the code more easily visible and therefore
> (IMO) maintainable.  

It seems to me that putting the {s on the same line would increase the amount of code you can see on a screen.  :)

I guess we can leave it for now, but if you end up doing more refactoring to make it work better/easier to read, then you should probably update it to conform to the file's style.

> > >-  <binding id="mail-multi-more-indicator">
> > Nice!  I like it when we can clean up old code.
> :-)  You can thank BenB for that part of the patch.

Thanks, BenB!  :)

Later,
Blake.

Comment 55

[Dan Mosedale (:dmosedale, :dmose)]

Ben, when someone resizes the window to be larger than it was initially rendered, we end up with a bunch of addresses, then a block of blank space, and then the (more) element, which looks weird.  The underflow handler re-fills the addresses so that end up consuming that space, including one last address, which ends up being clipped by the (more) button, and the number in the (more) handler is updated.  The bug I mentioned previously appears to be in that fillAddresses call, so I'm still working on debugging that.

When the users resizes the window to be smaller than initially rendered, the overflow event handler is intended to also re-fill the addresses and correct the (more) count.  Unfortunately, it's not working quite right yet, which is what the spin-off bug mentioned even earlier would be for.

As far as infinite loops, are you referring to the possibility of alternating forever between underflow and overflow events?  I think I see how to guard against that.  Or were you referring to more than that?

Comment 56

[Ben Bucksch (:BenB)]

I mean you are in fillAddresses(), you add addresses, they overflow (intentionally, that's how the code is written), which calls the overflow handler, which calls fillAddresses() again. That is a loop. I think that's inherently dangerous, and at the very minimum must be specifically guarded against (even if it doesn't create a serious problem yet), and at best be structurally avoided.

If the overflow/underflows don't work properly yet anyways, I would recommend removing them entirely. The "resize window" case is not frequent in everyday use and trivially remedied: just click on another mail. It was specifically out of scope for this bug (see comment 0).

Comment 57

[Ben Bucksch (:BenB)]

(Note: I think it's not recursive, i.e. the overflow events fire only after firstAddressesNode() finishes, then fillAddressesNode() is called again afterwards, but I'm not sure. Even if not recursive, it's at least being called 3 times: original call, then overflow when we added the one which trips us over the max length, then underflow when we removed it again, and unintentionally so. Plus, if fillAddressesNode() assumes it has to rebuild, everything starts again, thus infinite loop. In fact, I'm surprised it's not looping and think that's more a coincidence. So, my recommendation: just remove the handlers for now.)

Comment 58

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #56)
> I mean you are in fillAddresses(), you add addresses, they overflow
> (intentionally, that's how the code is written), which calls the overflow
> handler, which calls fillAddresses() again. That is a loop. I think that's
> inherently dangerous, and at the very minimum must be specifically guarded
> against (even if it doesn't create a serious problem yet), and at best be
> structurally avoided.

Right; I've gotten a guard written for that locally, and it does seem to be working.

> If the overflow/underflows don't work properly yet anyways, I would recommend
> removing them entirely. The "resize window" case is not frequent in everyday
> use and trivially remedied: just click on another mail. It was specifically out
> of scope for this bug (see comment 0).

Indeed, my fallback strategy if I can't get the code into reasonable shape is to remove the handlers.  We'll see where things end up.
(In reply to comment #57)

> (Note: I think it's not recursive, i.e. the overflow events fire only after
> firstAddressesNode() finishes, then fillAddressesNode() is called again
  ^^^^^

I assume you mean fillAddressesNode?

> afterwards, but I'm not sure. Even if not recursive, it's at least being called
> 3 times: original call, then overflow when we added the one which trips us over
> the max length, then underflow when we removed it again, and unintentionally
> so. Plus, if fillAddressesNode() assumes it has to rebuild, everything starts
> again, thus infinite loop. In fact, I'm surprised it's not looping and think
> that's more a coincidence. 

Right, that's what the guards protect against.

Comment 59

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #57)
> Even if not recursive, it's at least being called
> 3 times: original call, then overflow when we added the one which trips us over
> the max length, then underflow when we removed it again, and unintentionally
> so.

Note also that we no longer do the removing.  Since the (more) has been hoisted up one level, we intentionally leave the overflowed node and allow (more) to clip it.

Comment 60

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: fix, v10

After playing around with the resize handlers and discussing with BenB, I've come to agree that they're fragile enough that including them as part of this patch adds an unnecessary amount of risk to 3.1.  Instead, I'll spin that off as a separate (non-blocking) bug.

My understanding from previous clarkbw comments is that he's cool with this, but re-requesting ui-r+ for explicit signoff.

Carrying forward r+ after discussion with bwinton.

I've also fixed the bracing stuff as requested by Blake, and I've replaced the "black magic" comment with a comment that describes what's actually going on.

Comment 61

[Dan Mosedale (:dmosedale, :dmose)]

As mentioned previously, I'll also be spinning off an blocker bug for the performance problem when clicking the (more) widget on a header with lots of extra addresses.

After more quality time with misleading-but-nonetheless-helpful DTrace output, it appears that adding & updating the nodes to the DOM is what's taking most (all?) of the extra time.  I _think_ much of this time is actually in layout.  My current speculation is that the reason this wasn't a problem in the old world but is in the new world is because we're now effectively force a reflow for each address that's appended.  With luck, I'll find some time tomorrow evening to test that theory and experiment with ways of addressing it.

Comment 62

[Dan Mosedale (:dmosedale, :dmose)]

Callek has graciously kicked off try-server builds for fix v10; they should be up on <http://tinderbox.mozilla.org/showbuilds.cgi?tree=ThunderbirdTry> after a while.

Comment 63

[Dan Mosedale (:dmosedale, :dmose)]

Links to the try server builds:

Linux: http://s3.mozillamessaging.com/build/try-server/2010-04-19_21:[e-mail]-more_v10/[e-mail]-more_v10-mail-try-linux.tar.bz2

Mac: http://s3.mozillamessaging.com/build/try-server/2010-04-19_21:[e-mail]-more_v10/[e-mail]-more_v10-mail-try-mac.dmg

Windows: http://s3.mozillamessaging.com/build/try-server/2010-04-19_21:[e-mail]-more_v10/[e-mail]-more_v10-mail-try-win32.installer.exe

Comment 64

[Dan Mosedale (:dmosedale, :dmose)]

Landed: <http://hg.mozilla.org/comm-central/rev/86fbaadd2cf3>.  I've spun off the performance issue with clicking (more) to bug 560695, which blocks rc1, and the lack of resizability to bug 560698.  Thanks to everyone who has been part of making this happen!

Comment 65

[Thomas D. (:thomas8)]

Hmmm. I know a lot of work has been spent on this...
But I am failing to understand the actual UI that came out. What I see in TB 3.1 is that the last address in row 1 of my to-recipients is cut off by the overlapping hardcoded UI element for "206 more". However, what's worse, more than 50% of the width of the last address are visible and yet it is not counted. How am I supposed to interpret the count when we show partial addresses? I consider the wrong count a bug (which is different from the resize window problem).

Do we really want to show partial addresses like that? Shouldn't "206 more" follow comma-separated and right after the last address that can be shown completely without overflowing the line (incl. "206 more")?
It's better than before (thank you!), but it still feels like header pain...

Comment 66

[Ben Bucksch (:BenB)]

Thomas D., in comparison to the severity of the bugs before (parts of the window unusable, "more" invisible and not clickable), I think it's totally unimportant whether the partially visible address counts as visible or not. Also, I see no problem in a address being partially shown, instead of not showing it at all. Please note that even the first address may not have enough space to show entirely (because it's simply too long for the window width), so I think the current behavior is fine.

In fact, I am impressed by status quo.

Comment 67

[Ben Bucksch (:BenB)]

> In fact, I am impressed by status quo.

(Thanks to work from rsx, Dan and bwinton.)

Comment 68

[Ben Bucksch (:BenB)]

Thomas D., bug 567062 is about the 'more count to small by one' sometimes.

Comment 69

[rsx11m]

(comment #67) Thanks back to Ben!  :-)

And yes, the patch I've posted there makes it more obvious whether or not the "sliding" address counts. It's a minor issue, the number in the "more" button is more of informational nature about the length of the address list.

Comment 70

[Thomas D. (:thomas8)]

(In reply to comment #68)
> Thomas D., bug 567062 is about the 'more count to small by one' sometimes.

Ben, thanks for pointing out bug 567062 which will address much of the problem of my comment 65. -> added to dependent bugs

(In reply to comment #65)
> Hmmm. I know a lot of work has been spent on this...
> It's better than before (thank you!)...

I think here I was trying to say something more like Ben's
> In fact, I am impressed by status quo.
...and thanks *a lot* to everyone involved :)

(In reply to comment #66)
> Thomas D., in comparison to the severity of the bugs before (parts of the
> window unusable, "more" invisible and not clickable), I think it's totally
> unimportant whether the partially visible address counts as visible or not.

Agree for part 1, but the conclusion is wrong: Users don't know how bad things were before we improved them. They'll judge what they see. *We* know it's a lot better than before, but from a neutral pov, we might be far from ideal. Besides, wrong counts should never be in a serious application, ever.
And ambiguity as to what counts or not will just lead to confusion, bad feelings of instability and avoidable bug reports.

> Also, I see no problem in a address being partially shown, instead of not
> showing it at all.

Maybe it just feels wrong that we cut things off in the middle of a letter without any indication. IMHO, better UI for this case would be either
a) don't show partial addresses OR
b) indicate omissions with an ellipsis (as we do in columns), like this:
Tom Sample <[e-mail]>, Tony Mill... 10 more
Is it very hard to code the ellipsis? Maybe code from columns' ellipsis can be reused?

> Please note that even the first address may not have enough
> space to show entirely (because it's simply too long for the window width), so
> I think the current behavior is fine.

Agreed, the first address should always be shown. Doesn't apply to subsequent ones, though. And arguably/alternatively we could wrap if the first address is too long, as we do with subjects. Just thoughts, I'm not recommending anything here.

> In fact, I am impressed by status quo.

YES!
<smallprint>
(...but, in spite of header pane's notorious history of code complexity and functional deficiencies - as when you click on the x-more button and it actually doesn't show much more, or even shows LESS, due to bug 511408 -
I can't help dreaming sometimes what header pane could be if it was different...)</smallprint>

Comment 71

[rsx11m]

Thomas, this bug is closed (and still has nothing to do with bug 511408).
Please open new bugs for any issues you think are wrong (as you know).

As confirmed by Bryan elsewhere, the semi-visible last address in the single-line mode was designed in this way on purpose.