425451 - message compose window defaults to four lines for recipients, wasting screen real estate/vertical space (add pref: mail.compose.addresswidget.numRowsShownDefault=3; allow manual resizing to minimum of 1 row)

Status: VERIFIED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Peter S.]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12
Build Identifier: version 2.0.0.12 (20080213)

The message compose window has four lines for recipients. This takes up a lot of screen real estate on small screens especially when you're trying to write an email while referring to a web page. There needs to be a way to reduce the number of default lines, and I should be able to drag to reduce or extend the box that contains those lines.

Reproducible: Always

Steps to Reproduce:
1. Open Thunderbird 
2. Compose New Message
3. There is no step three
Actual Results:  
The space for recipients is as big as the message body. I cannot change the size of the recipient list.

Expected Results:  
The mouse pointer changes to a vertical resize pointer when hovering between "Subject" and the recipient list. I drag the mouse up, hide the the unwanted lines, and the message body area grows.

Comment 1

[Peter S.]

It turns out you can change the size of the recipient list albeit in a non-intuitive place. Well, I'll be damned. However, it's still broken. It should shrink to at most one line.

Comment 2

[Magnus Melin [:mkmelin]]

So where exactly is that? I don't think most users would appreciate a one line input field.

Comment 3

[Peter S.]

(In reply to comment #2)
> So where exactly is that? I don't think most users would appreciate a one line
> input field.
> 

What do you mean where?

All I'm saying is that in the compose window the recipient list is "stuck" at four input fields. When people write messages the focus should be on content. There should be a way to expand the area for the message itself and not who the message is for.

Look at the way Thunderbird implements reading messages. You can collapse all the header information to one line. Do you really think people cannot remember who they are writing to?

Comment 4

[Magnus Melin [:mkmelin]]

(In reply to comment #3)
> What do you mean where?

That was in reply to "It turns out you can change the size of the recipient list albeit in a non-intuitive place."

Comment 5

[Rod Whiteley]

I can confirm that the default is indeed four lines. You can drag the splitter down to make more lines, but you cannot drag it up to make fewer.

In: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008032902 Thunderbird/3.0a1pre

The value 4 is hard coded:
http://lxr.mozilla.org/mailnews/source/mailnews/compose/resources/content/addressingWidgetOverlay.xul#49

FWIW, the Mail Tweak extension can fix this in Thunderbird 2: http://journal.mozdev.org/mailtweak.html

I am surprised that this has not been reported before, because it is quite often asked in forums, but I cannot find a duplicate.

Comment 6

[ovidiu]

Indeed, but on Shredder/3.0a2pre ID:2008072303 it seam 3 lines already. And mailtweak kinda works for 3 too, with a certain ver 1pre2.

This could be also OE UI compatib stuff as one may be used to have 1 line there. (But using comma separated ..) as much as contact sidebar is, and also stays in extension ..

Comment 7

[Thomas D. (:thomas8)]

For Seamonkey, it now seems to be
http://mxr.mozilla.org/comm-central/source/suite/mailnews/compose/addressingWidgetOverlay.xul#49

For Thunderbird, that's found in
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#667

However, changing the line to
<listbox id="addressingWidget" flex="1" seltype="multiple" rows="2">
does NOT change the problem.
The only difference I see is that it's 3 1/2 line instead of 4 then. But maybe someone else can try this, too?

So what else blocks the splitter from being dragged up to just show 1 or two address lines only?

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The problem starts here: http://mxr.mozilla.org/comm-1.9.1/source/mail/components/compose/content/addressingWidgetOverlay.js#239
As you see, the number of added rows is hardcoded and will be reset be the fitting function awFitDummyRows calls.

I would prefer if this is a preference and resize fitting independent of the recipient fields number (may only matching an integer value).

Comment 9

[u382332]

(In reply to comment #7)
> For Seamonkey, it now seems to be
> http://mxr.mozilla.org/comm-central/source/suite/mailnews/compose/addressingWidgetOverlay.xul#49
> 
> For Thunderbird, that's found in
> http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#667
> 
> However, changing the line to
> <listbox id="addressingWidget" flex="1" seltype="multiple" rows="2">
> does NOT change the problem.
> The only difference I see is that it's 3 1/2 line instead of 4 then. But maybe
> someone else can try this, too?
> 
> So what else blocks the splitter from being dragged up to just show 1 or two
> address lines only?

I've been changing it to two lines for a long time now. There are actually two lines that need to change to "2" in Thunderbird. I don't know about Seamonkey.

<listbox id="addressingWidget" flex="1" seltype="multiple" rows="4"
<listbox seltype="multiple" id="attachmentBucket" flex="1" rows="4"

Also needed to get exactly two lines is adding css.

#MsgHeadersToolbar {
min-height: 70px !important;
}

The reason id="attachmentBucket" also needs to be changed is because when adding an attachment the #MsgHeadersToolbar will flex downward slightly if not changed.

Comment 10

[[:Aureliano Buendía]]

Undupe this and dupe bug #529158 because this is the oldest and has more information than bug #529158

Comment 15

[u382332]

For anyone interested, see...

https://addons.mozilla.org/en-US/thunderbird/addon/address-widget-lines/

Comment 16

[Thomas D. (:thomas8)]

Also on GS, canonical report is here: http://gsfn.us/t/1dob7

Comment 17

[kenkamiya]

(In reply to Peter S. from comment #3)
> (In reply to comment #2)
> > So where exactly is that? I don't think most users would appreciate a one line
> > input field.
> > 
> 
> What do you mean where?
> 
> All I'm saying is that in the compose window the recipient list is "stuck"
> at four input fields. When people write messages the focus should be on
> content. There should be a way to expand the area for the message itself and
> not who the message is for.
> 
> Look at the way Thunderbird implements reading messages. You can collapse
> all the header information to one line. Do you really think people cannot
> remember who they are writing to?

I am also annoyed by this inflexible address field size.  One address field is enough initially.  As the user enters more, the field can grow to show 3 recipients.  If more than that, create a scroll bar, so the user can check to make sure that s/he entered desired recipient's address.

Flexibility is the key to satisfy a variety of users, in my view.

Comment 18

[Thomas D. (:thomas8)]

Attachment: Address Widget Lines Addon (tweaked to 3 default lines)

Blake, I'd like to fix this bug.
We really owe it to our users to return control over their screen real estate, and a static 4-lines addressing widget is a nuisance not only for small screens.

Can you pls try the attached addon and comment if this feels right:

- Address widget default number of lines: now 3 instead of 4 (save screen real estate for everyone, while preserving full scroll bar in case of more addresses)
- During composition, user can temporarily manually resize the address widget to a minimum of 1 address widget line (for new composition, or when re-opening existing composition, we restore default number of lines)
- Introduce internal pref to set default number of lines, e.g. mail.compose.addresswidget.defaultlines (integer)
- Optionally (and recommended) add options UI to set default number of lines:
E.g. Tools > Options > Composition > Addressing > (at the bottom)

Address widget number of lines: [ 3↑↓]

(with a restricted value range from min 1 to max 12 *default* lines; during composition, user can always manually resize the address widget in both directions)

I can provide reasons for all of these details on request.

Comment 20

[Dentist]

So what's the solution for this bug? it has been 2 years now.
Its 1 line in all mail clients, why is it 4 in Thunderbird?

Comment 21

[Dentist]

oh, I found the solution after I had read all the replies.
Its in comment 15. Thanks Bozz for mentioning it. Now Thunderbird is much better.

Comment 22

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

Comment on attachment 602586 [details]
Address Widget Lines Addon (tweaked to 3 default lines)

(In reply to Thomas D. from comment #18)
> Blake, I'd like to fix this bug.

Awesome, this seems like one of the papercuts we're interested in fixing.  :)

> - Address widget default number of lines: now 3 instead of 4 (save screen
> real estate for everyone, while preserving full scroll bar in case of more
> addresses)

I like three lines as the default.  I think you might even get away with two lines.  Or, dare I say it, one?

> - During composition, user can temporarily manually resize the address
> widget to a minimum of 1 address widget line (for new composition, or when
> re-opening existing composition, we restore default number of lines)

That seems reasonable, but…

I notice that I can expand the addressing to take up the entire window, which I don't think makes a lot of sense.  We should leave a certain amount of space for the content of the message.

> - Introduce internal pref to set default number of lines, e.g.
> mail.compose.addresswidget.defaultlines (integer)
> - Optionally (and recommended) add options UI to set default number of lines:
> E.g. Tools > Options > Composition > Addressing > (at the bottom)
> Address widget number of lines: [ 3↑↓]

And I'm -1 on this part.  Too many options is too many options.  ;)

It seems to me that a better idea would be to save the last height of the address box, and when the user opens a new compose window, make the address box that height again.

Another nice thing we probably want to do as part of this would be to auto-expand the addresses if the user types more than will fit into the current box, and hasn't resized the box during this session.

I think we also want to show all the addresses (or as many as possible) when someone is replying to a message, so that they get a chance to look them over before sending, to make sure the message is going to the correct people.  This height calculation should probably be separate from the new compose window case, since if I reply to 12 people, and then start a new message, it's unlikely that I'll want the address box to be 12 lines high.

Having said all that, the add-on is generally going the right direction, so f=me.

Thanks,
Blake.

Comment 23

[rsx11m]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #22)
> I like three lines as the default.  I think you might even get away with two
> lines.  Or, dare I say it, one?

There isn't a general rule on this. If you don't have any default cc/bcc or reply-to headings you may be fine with a single line, but that wouldn't be enough already to accommodate using even one of those settings. Given that it's a constant at least in the add-on implementation, something like n+1 by default with n being the number of lines needed for these options may be a bit hard to implement, thus I'd even recommend to stick with the n=4 default which would cover all three options and still leave a line for the first recipient, and then allow users to reduce it if they think they don't need it.

Comment 24

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

Gmail somehow gets away with two lines…

It will be nice to see some test-pilot results on how many people have default cc/bcc/reply-to headings…  :)

Also, why don't we make it not a constant, and actually fix the problem?  ;)

(And by "we", I of course mean Thomas D.  ;)

Comment 25

[Magnus Melin [:mkmelin]]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #24)
> Gmail somehow gets away with two lines…

Or one, depending on how you look at it. But we really needs multi address autocomplete to work properly for single lines (which we of course should do).

Comment 26

[rsx11m]

Autocomplete of multiple comma-separated addresses regression is bug 528503, if that's what you mean. Of course, the question relevant here is if the address widget should expand vertically to accommodate those (up to a certain limit) after address expansion, but that's likely a separate bug.

Comment 27

[u382332]

With one address line, the vertical scrollbar does not appear so there is no indicator when there is more than one address in the list.

Comment 28

[rsx11m]

... and after entering the first address and hitting the Enter key, it immediately disappears as the focus goes into the next line, and it's not obvious to the user that there actually *are* more than one line in this widget. Thus, n=2 should be the minimum unless the list expands on its own as necessary.

Comment 29

[u382332]

I do recommend that n=2 be the minimum, but I wouldn't want the list to expand. Let the user drag the widgett down when or if the user wanted to see more addresses without scrolling.

Comment 30

[kenkamiya]

Isn't the following possible?  Default is n=1.  If multiple addresses are entered, put '+' on the right edge, indicating there're more addresses.  Clicking on it will expand the window to 3 or 4 addresses (I prefer 3).  If there're more than that, provide a scroll bar so that the user can scroll down to see all.  Then, clicking on [Close] button will reduce the window back to one address AND '+' will be displayed on the right edge.

Of course, if there's only one address, the '+' is not necessary.

Comment 31

[rsx11m]

There is a related bug 734683 "Provide UI for adding more recipients than lines provided by default" pending already on adding a [+] button to explicitly expand the address widgets, which is slightly different than the comment #30 proposal.

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #24)
> It will be nice to see some test-pilot results on how many people have
> default cc/bcc/reply-to headings…  :)

Probably not too likely in a private environment, but it's rather common to have explicit reply or cc addresses in enterprise/organizational settings (e.g., for delegates or groups).

Comment 32

[Thomas D. (:thomas8)]

Ftr: While this bug is just first aid to alleviate some of the bad side effects of the status quo, the real cure is probably Bug 440377: Move away from the one-line-per-recipient paradigm and do like most others do, multiple recipients on a single line.

Comment 33

[kenkamiya]

Hello Folks,

I wonder if this effort is progressing.
I will sincerely appreciate if it's implemented asap.  Because my screen is only 12" aspect ratio, The extra blank lines of addresses spaces are blocking images and charts sent to me, so I cannot see the entire view.
Hope someone will be able to finish it.

Comment 34

[Thomas D. (:thomas8)]

(In reply to kenkamiya from comment #33)
> Hello Folks,
> I wonder if this effort is progressing.

I suppose we're almost there, but that means we're not there yet.

> ...so I cannot see the entire view.

Please use the addon referenced in comment 15 to solve the problem of this bug.

Comment 35

[Thomas D. (:thomas8)]

(In reply to Blake Winton (:bwinton - Thunderbird UX)[On vacation until Sept. 11th.] from comment #22)
> Comment on attachment 602586 [details]
> Address Widget Lines Addon (tweaked to 3 default lines)
> 
> (In reply to Thomas D. from comment #18)
> > Blake, I'd like to fix this bug.

I think I was trying to say: I want to get this bug into a state of ui-review+ so that somebody can fix it. I thought this would be straightforward, but after Blake's comment 22 and other comments, that seems no longer the case.

> > - Introduce internal pref to set default number of lines, e.g.
> > mail.compose.addresswidget.defaultlines (integer)
> > - Optionally (and recommended) add options UI to set default number of lines:
> > E.g. Tools > Options > Composition > Addressing > (at the bottom)
> > Address widget number of lines: [ 3↑↓]
> 
> And I'm -1 on this part.  Too many options is too many options.  ;)

I'm not sure if Blake's -1 also refers to adding the internal pref aka mail.compose.addresswidget.defaultlines (integer).
Without that pref, there's no point of doing this, because whichever number of default lines we chose, it will always be wrong for a lot of users.

And on second thoughts, imo having the pref but not having UI for it is equally bad. I suggested a minimalistic UI, but Blake doesn't like it :(

> It seems to me that a better idea would be to save the last height of the
> address box, and when the user opens a new compose window, make the address
> box that height again.

I doubt very much that "remember last number of lines" will be a useful behaviour. Just because user writes one msg to 10 recipients, hence increasing number of recipient lines to 10 for that particular msg, we cannot safely assume that he will need the same number of lines for the next msg, which might have 1 recipient only. "Remember" UIs may appear attractive at first sight, because we can skip default decisions and options for customization, but in the long run, we're just postponing the real problem of customizable defaults.
My impression is that for cases like this one, TB is moving away from "remember" UIs, and Jim has made convincing points to that end in Bug 647036 Comment 75:

> None of this really explains why someone would prefer the "remember state"
> pref over the other two. I think predictability is more useful in this case,
> since people who don't like the current default are complaining because they
> expect the attachment pane to start out expanded. Remembering the previous
> state only helps them if the user remembers to keep the attachment pane in
> the state they want. I suspect that what people want, based on this bug and
> comments I've seen about the add-on, that what they want is to set the
> initial state, and then sometimes *temporarily* expand (or collapse) the
> pane, either to get at the attachments, or to free up more space for the
> message body.

I think we have a very similar case here, so that comment applies exactly to the situation here with expanding/collapsing recipients, where "remember" does not seem to be all that helpful if you think about it.

> Another nice thing we probably want to do as part of this would be to
> auto-expand the addresses if the user types more than will fit into the
> current box, and hasn't resized the box during this session.

That adds a lot of complexity to the code, and so far we can't even handle that for the message reader header (try "x more..."), so I wouldn't try that route here.

> I think we also want to show all the addresses (or as many as possible) when
> someone is replying to a message, so that they get a chance to look them
> over before sending, to make sure the message is going to the correct
> people.  This height calculation should probably be separate from the new
> compose window case, since if I reply to 12 people, and then start a new
> message, it's unlikely that I'll want the address box to be 12 lines high.

Again, more complexity which needs more thought. Not sure if this really applies. When I reply (all), I know my msg will go to exactly the sender (or sender and all recipients) of the original msg, so we could equally assume that there's probably *no* need for re-checking recipients in this scenario. On the other hand, I'm sure everybody wants as much space as possible for composition of message body.

I'm not sure about all these auto-expanding ideas here. Imo the better assumption is to use as little space as possible for recipients and preserve valuable space for message composition. When I'm typing a long list of recipients, I can check on each one while or right after typing. If I want to check them all, I can temporarily expand the pane manually. That has no implications whatsoever how many lines I want *next* time. Otherwise, with the UI suggested in comment 18, I can adjust the default number of lines if I feel there should *always* be more or less lines.

> Having said all that, the add-on is generally going the right direction, so
> f=me.

There's a ray of hope.

Comment 36

[kenkamiya]

Hi folks,

Thanks for letting me use TB.

I have been receiving many updates, but this issue hasn't been resolved.  Are you guys working **** this, or hardly working any longer?  Hope you haven't dropped the ball.

I usually create a return window which has the sender's message, and write my response while reading it shown below in the same window.  So, it will be most convenient if the extra 3 address lines are not displayed.

I don't believe geniuses like you guys cannot fix this before breakfast.

Looking forward to having improvement to this.

A+
Kenji

Comment 37

[Thomas D. (:thomas8)]

Attachment: Screenshot 1: Proposed UI and behaviour (default to 3 lines, allow dragging splitter, allow customization of default number of lines)

(In reply to Blake Winton (:bwinton) from comment #24)
> Also, why don't we make it not a constant, and actually fix the problem?  ;)

+1! :)

(Translation: "We should make the default number of address widget lines a user-defined variable instead of a pre-defined one-size-for-all constant")

Blake, inspired by your above comment, here's a screenshot of the minimal fix for this bug in action, including a mockup of that tiny tweak to the addressing panel of the options dialog, a small input field allowing users to choose their favorite default (read *initial*) number of lines in the address widget.

Just reducing the default number of lines without offering customization UI would only lead to unhappy and clamoring users on both ends (those who initially want less lines, and those who want more...). And imho it would really be nonsensical to perpetuate the very problem this bug is trying to solve, namely that the initial number of lines we picked as a default is not right for many users (too many lines for many users; or not enough lines for other users, e.g. corporate users with many default cc's and bcc's and reply-to's), while there's no way of customizing it from the UI.

For the sake of moving this bug forward, I kindly request your thumbs-up for the minimal fix, we can still polish later if there's somebody who cares.

E.g., if users so wish, they *can* currently drag the address widget splitter all the way down which will make the body disappear (you have to drag really hard to do that...), which could be improved but otoh it doesn't hurt at all so the minimal fix proposed here just wouldn't touch that, notwithstanding later polishing of whatever your heart desires.

For now, I think most users just desire an improvement over the stiff and useless status quo which eats their vertical screen real estate.

Comment 38

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

Hmm, that's an interpretation of my comment, but you missed the other part:  ;)

>> - Introduce internal pref to set default number of lines, e.g.
>> mail.compose.addresswidget.defaultlines (integer)
>> - Optionally (and recommended) add options UI to set default number of lines:
>> E.g. Tools > Options > Composition > Addressing > (at the bottom)
>> Address widget number of lines: [ 3↑↓]
> And I'm -1 on this part.  Too many options is too many options.  ;)

(Now, it may be that we need an option for this, but I think a much better solution was presented in comment #17, and I'm not entirely sure why we wouldn't go with that instead…)

Thanks,
Blake.

Comment 39

[Thomas D. (:thomas8)]

(In reply to Blake Winton (:bwinton) from comment #38)
> Hmm, that's an interpretation of my comment,

I studied English & Theology so I've been conditioned to interpret ;)

> but you missed the other part: 
> ;)

Well, no, I've already tried my best to answer (and refute) that part in my detailed comment 35...

> >> - Introduce internal pref to set default number of lines, e.g.
> >> mail.compose.addresswidget.defaultlines (integer)
> >> - Optionally (and recommended) add options UI to set default number of lines:
> >> E.g. Tools > Options > Composition > Addressing > (at the bottom)
> >> Address widget number of lines: [ 3↑↓]
> > And I'm -1 on this part.  Too many options is too many options.  ;)
> 
> (Now, it may be that we need an option for this, but I think a much better
> solution was presented in comment #17, and I'm not entirely sure why we
> wouldn't go with that instead…)

Fair enough, so let's double-check on comment #17 (which unfortunately includes comment 3, so I'll answer that too)...

(In reply to kenkamiya from comment #17)
> (In reply to Peter S. from comment #3)
> > (In reply to comment #2)
> > > So where exactly is that? I don't think most users would appreciate a one line
> > > input field.
> > > 
> > 
> > What do you mean where?
> > 
> > All I'm saying is that in the compose window the recipient list is "stuck"
> > at four input fields. When people write messages the focus should be on
> > content. There should be a way to expand the area for the message itself and
> > not who the message is for.
> > 
> > Look at the way Thunderbird implements reading messages. You can collapse
> > all the header information to one line. Do you really think people cannot
> > remember who they are writing to?

Right, I understand that's a proposal to add UI for *collapsing* the address widget to one line (instead of dragging the current splitter).
In the current UI, when you install the addon of attachment 602586 [details] and then reduce the widget to one line, there will be no scrollbars, so there's no indication whatsoever if there is more than one recipient. So that's not safe to make it part of the default collapsing behaviour (while imo it's ok when users deliberately drag themselves into that state).

I'm not against the idea of collapsing, but it's a totally different approach which probably needs new UI elements, and hence requires more work. Pls correct me if I am wrong, including details of how collapsing UI should be easily realized.

> I am also annoyed by this inflexible address field size.  One address field
> is enough initially.

Unfortunately, NO. As mentioned several times in this bug (by me and others), we cannot make *one* address line the default unless we redesign the entire address widget, because when the current widget has just one line, scrollbar is missing, and users get stuck wondering how they can add more addresses.
(And to make it one line for *multiple* addresses, as in most other mailers, while desirable, is not this bug and needs a lot more work).

Sure, we could start adding [+] buttons or [add address] buttons or whatever other UI elements for that, but again you'll have to invest more effort to design and define that new UI and behaviour. As far as I'm concerned, I'm not very keen on having superfluous buttons added to my UI and I don't see the point of wasting time on that when much more simple and working solutions are available (as shown in addon of attachment 602586 [details], which has feedback+ from BWinton). I also suspect we'd wait much longer for someone to come up with that new UI and behaviour.

> As the user enters more, the field can grow to show 3
> recipients.

More design or coding work again, but that also depends on the single line default which currently is not feasible as explained above.

> If more than that, create a scroll bar, so the user can check
> to make sure that s/he entered desired recipient's address.

That's current behaviour already, and is not altered by my proposal.

> Flexibility is the key to satisfy a variety of users, in my view.

+1.

So to sum up, proposals of comment 3 / comment 17 will not work without further design work of arguable value, while most definitely delaying the minimal fix for this bug.

I think this bug is just trying to solve two simple problems:

1) users are not happy with the *default/initial* number of address widget lines (e.g. because they usually only write to one recipient, so they never use the remaining 3 lines).

2) users who may or may not be happy with the *default/initial* number of lines may still want to shrink (manually "collapse") the height of address widget *while composing*, to free up more space for the body.

Both of these problems are fully addressed by the proposed solution as realized in addon of attachment 602586 [details], which btw also includes the addon customization option for initial default number of lines (for core, we'd just move that little option into TB options dialogue as depicted in mockup screenshot attachment 700021 [details]).

I'm really getting tired of this, we're dragging our feet for more complex solutions while simple solutions are already available, ready to be implemented as-is. We shouldn't let the perfect be the enemy of the good. And if you look at the addon code, it's just a few lines, so it's not like we couldn't revert that later if somebody comes up with a better design or behaviour for the whole widget. From experience, more often than not, that somebody never comes, so might as well start waiting for another 5 years...

Comment 40

[Magnus Melin [:mkmelin]]

Comment on attachment 700021 [details]
Screenshot 1: Proposed UI and behaviour (default to 3 lines, allow dragging splitter, allow customization of default number of lines)

This really isn't a thing to have UI for, IMO. Realistically its not something many users would want to change, and even those who want would have to make it 2 to maybe 5, other values are just odd.

Comment 41

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

(In reply to Thomas D. from comment #39)
> (In reply to Blake Winton (:bwinton) from comment #38)
> Well, no, I've already tried my best to answer (and refute) that part in my
> detailed comment 35...

You certainly convinced me that we don't want to remember the number of lines, but I'm not convinced that having an expanding list of addresses that starts off showing one line is so complicated as to be un-doable.

> (In reply to kenkamiya from comment #17)
> > (In reply to Peter S. from comment #3)
> > > Look at the way Thunderbird implements reading messages. You can collapse
> > > all the header information to one line. Do you really think people cannot
> > > remember who they are writing to?
> Right, I understand that's a proposal to add UI for *collapsing* the address
> widget to one line (instead of dragging the current splitter).

I didn't read it that way.  That seemed to me to be the UI of the thing he was using as an example to get across the idea of collapsing, not the UI we would want for this feature.

> I'm not against the idea of collapsing, but it's a totally different
> approach which probably needs new UI elements, and hence requires more work.
> Pls correct me if I am wrong, including details of how collapsing UI should
> be easily realized.

So, here's my proposal:
The address area starts with one field, no scrollbar.
As people add addresses, the list expands so that there's always a blank field at the bottom.
Once we hit a maximum (say, 5 lines?) we stop auto-expanding the list, add a scrollbar, and let the user expand or contract the list as they wish.

That design didn't take that much time to come up with, and seems to cover all the concerns you raised.

> I think this bug is just trying to solve two simple problems:
> 
> 1) users are not happy with the *default/initial* number of address widget
> lines (e.g. because they usually only write to one recipient, so they never
> use the remaining 3 lines).
> 
> 2) users who may or may not be happy with the *default/initial* number of
> lines may still want to shrink (manually "collapse") the height of address
> widget *while composing*, to free up more space for the body.


> Both of these problems are fully addressed by the proposed solution as
> realized in addon of attachment 602586 [details],

At the cost of adding yet another preference, and piece of UI, to confuse people, most of whom don't care.  And it doesn't fix the problem for most of our users, who will still be annoyed by having too many lines showing by default, and are unlikely to change the preference.

> I'm really getting tired of this, we're dragging our feet for more complex
> solutions while simple solutions are already available, ready to be
> implemented as-is. We shouldn't let the perfect be the enemy of the good.

I guess the problem here is that I'm not convinced that adding another preference is actually good, ui-wise.  Like Magnus says, it's not really a thing to have UI for.

> And if you look at the addon code, it's just a few lines, so it's not like
> we couldn't revert that later if somebody comes up with a better design or
> behaviour for the whole widget. From experience, more often than not, that
> somebody never comes, so might as well start waiting for another 5 years...

This will certainly never happen, if we settle for a poor ui now, since then any impetus someone might have to actually fix it will be gone.  :(

I appreciate that this conversation is tiring for you.  In truth, I'm also finding it to be a depressing start to my day.  But user interface conversations are often like that, since they end up being full of trade-offs between various factors, often leaving none of the participants completely happy.  I believe that the UI I proposed above is the best way forward, and I will be happy to modify it to address any concerns you raise.

Perhaps we could ask Bozz nicely if he was interested in coding up this feature, in an attempt to move it forward?

Thanks,
Blake.

Comment 42

[rsx11m]

(In reply to Blake Winton (:bwinton) from comment #41)
> So, here's my proposal:
> The address area starts with one field, no scrollbar.

Remember to make that n+1 lines if other n fields ("Reply-To:" or "Bcc:", "Cc:") are to be populated by default, but that should be feasible if the logic is ok.

> As people add addresses, the list expands so that there's always a blank
> field at the bottom.
> Once we hit a maximum (say, 5 lines?) we stop auto-expanding the list, add a
> scrollbar, and let the user expand or contract the list as they wish.

Sounds good to me, where I would prefer the scroll bar over an expand/contract to avoid that the list floods too much into the message area (or have both).

Comment 43

[u382332]

Having the address lines expand from one line up to five lines defeats the purpose of being able to select a specific number of lines especially fewer than the current default of four lines.

I personally am satisfied with using two lines and if I want to see more added addresses I can scroll them.

Allowing the widgett to expand to five lines just gets us back to the widgett taking up too much vertical space and I would not want to see that happening.

Bozz

Comment 44

[rsx11m]

(In reply to Bozz from comment #43)
> I personally am satisfied with using two lines and if I want to see more
> added addresses I can scroll them.

Well, I wouldn't be happy with that and frequently need the four lines due to my additional default entries with usually quite a few recipients on the list. Thus, there is no "golden rule" for a good number of how many lines to show when opening the composition window.

> Allowing the widgett to expand to five lines just gets us back to the widgett
> taking up too much vertical space and I would not want to see that happening.

It was my understanding that this limit should be configurable similar to the proposed fixed size, thus you could still set that limit to 2 lines if you want?

If it's a constant number, I'm fine with that, especially given that code exists already to accomplish that (though the extension may not be taken verbatim for the actual code). But then, exposing rather than hiding the configuration option in the UI should be more important than in the more complex auto-expand version, given that it would be a ux-discovery issue to actually find that option (specifically if the default is changed from the now four lines to less).

Comment 45

[kenkamiya]

As a user, thank you all for working on this issue.
I'm too ignorant about how the program is designed, but when attaching an external file, a big space is allocated in the top-right corner, which is more than enough for displaying the file name.  Hope this space will also be optimized.

Comment 46

[rsx11m]

The height of the attachment pane should be determined by the height of the address widget and hence adjusted to match whatever number of address lines are displayed. Thus, I wouldn't expect this to be an issue.

Comment 47

[Thomas D. (:thomas8)]

As a side note, for tracking purposes:
Bug 407486

Comment 48

[Thomas D. (:thomas8)]

Thanks everybody for their cooperative responses. I especially appreciate the cooperative and tentative tone of Blake's response (Comment 41). I think if we view those last comments together and draw conclusions, we might be heading for a comprehensive solution that actually solves the problems at hand.

(In reply to Blake Winton (:bwinton) from comment #41)

> So, here's my proposal:
> The address area starts with one field, no scrollbar.
> As people add addresses, the list expands so that there's always a blank
> field at the bottom.
> Once we hit a maximum (say, 5 lines?) we stop auto-expanding the list, add a
> scrollbar, and let the user expand or contract the list as they wish.

Blake certainly convinced me that starting out from a single-line recipient area which auto-expands as required will go a long way to improve the ux for the common simple scenarios where users start out with a blank address widget. (1)
For those users, the great advantage is that we *default* to actually freeing up that unused space of 3 extra blank lines without any user intervention (which nicely solves an ux-discovery / ux-efficiency problem which I had overlooked in my current proposal).

Furthermore (but only *if* the maximum number of lines shown remains *4* like now), both Blake's and my proposal (or their combination) will improve the current inflexibel situation for everyone as we *allow _manual contraction_ of the address widget* to less than 4 lines whenever it has 4 or more recipients (with minimum number of lines shown = 1); manual _expansion_ of the widget is already possible at present.

> (Blake:) I believe that the UI I proposed above is the best way
> forward, and I will be happy to modify it to address any concerns you raise.

The concerns start here:

(In reply to Bozz from comment #43)
> Having the address lines expand from one line up to five lines defeats the
> purpose of being able to select a specific number of lines especially fewer
> than the current default of four lines.
> I personally am satisfied with using *two* lines...
> Allowing the widget to expand to five lines just gets us *back to the
> widget taking up too much vertical space...*

(In reply to rsx11m from comment #44)
> > (Bozz:) I personally am satisfied with using *two* lines...
> Well, I wouldn't be happy with that and frequently need the four lines due
> to my additional default entries with usually quite a few recipients on the
> list. Thus, there is no "golden rule" for a good number of how many lines to
> show...

My concerns (as evidenced by Bozz comment 43 and rsx11m's comment 44) are that this solution will still be unsatisfactory for a wide variety of users and scenarios, although we could easily address that while we are here. 

It's unsatisfactory because it only reduces the *initial* number of lines in the address widget, but offers no /permanent/ way of reducing (or increasing) the *default* number of lines shown after auto-expanding, *without user intervention*. Iow, with Blake's approach only, users will still not have a way of *permanently* (read "automatically") adjusting the vertical space needed for the address widget /when there are several recipients/. So e.g. on a small screen (netbook), users like Bozz will have to shrink the widget *manually* each time they compose a message with 3 or more recipients. Or, in a nutshell, Blake's approach will only shrink the *empty* widget, but never the *filled* widget.

(I assume that we must not treat /automatically/ added cc's and bcc's different from /manually/ added, so both types must expand the widget appropriately, because hiding automatically added cc/bcc's away when starting the composition would create ux issues including privacy error prevention problems).

This is Blake's approach with some more details spelled out:

a) when starting composition with 0 recipients, show 1 aw line (aw = address widget)

b) when starting composition with n automatically added cc/bcc recipients, start out with n+1 aw lines, but show only 4, add aw scrollbar for > 4 (we cannot make it 5 because that will make things worse than now for many users)

c) when manually adding x recipients, expand from 1 to 1+x aw lines, but show only 4, add aw scrollbar for > 4

d) at any time, user can manually resize the aw to show less or more than the current number of recipients shown (which in Blake's approach always defaults to 4 if there are 4 or more recipients)

Given that as shown above, there is no "golden rule" for the number of lines to show by default with > 1 recipients, I think the most versatile solution would be to combine Blake's approach with mine:

e) complement Blake's auto-expand solution with an option to set a custom *default* number of aw lines where we stop auto-expanding the widget and start showing the scroll bar

rsx11m's comment 44 requests the same:
> It was my understanding that this [auto-expand-max-number-of-lines] limit
> should be configurable similar to the proposed fixed size, thus you could
> still set that limit to 2 lines if you want [or 5 or more lines]

So that would require a backend preference at least, and (optionally) that tiny extra line in the options UI as suggested in attachment 700021 [details].

(In reply to Blake Winton (:bwinton) from comment #41)
> At the cost of adding yet another preference, and piece of UI, to confuse
> people, most of whom don't care.

Adding a backend preference for adjusting (limiting) the default size (vertical screen estate) of a crucial widget always present in a frequent and major scenario like composition imho is not asking too much.

For the same reason, I'd argue that the extra line in the options UI would be more than useful, but if we start out without that, well, I can live with that.

> Tools > Options > Composition > Addressing > (at the bottom) >
>   Address widget default number of lines shown: [ 4↑↓]

I don't see how a single input field, tucked away as deeply in nested options as shown above, could have a significant potential to "confuse people".

Without statistical evidence, I also doubt that "most users don't care" about the default size of the address widget. Discussion in this bug and numerous getsatisfaction reports seem to suggest otherwise. Even if we had adverse statistics on numbers, we'd still have to factor in *how severely* a minority of users is affected by this specific problem. Having to drag your address widget to the right size every time you compose a message can be more than highly annoying to affected users. (As I stated elsewhere, if you rip out all features of a software that are only used by a minority of users, you might be surprised to end up with useless bare bones or even no software at all. Intelligent quality design is more than just a matter of counting users of a feature.)

(1) As a footnote:
I like Blake's proposal of auto-expanding address widget, and I think we should try that line (if it's not too complex code-wise to further delay fixing this bug). As devil's advocate: there's anecdotal bug 734683 where user's are not aware how to add new recipients after they have filled the first line.
If we offer one line as the default for compositions with no pre-filled recipients, users might try what they know from elsewhere (other mailers, and webmail), and they might be surprised that *autocomplete will not trigger for any subsequent comma-separated addresses* (which is another bug). But well, comma-separated addresses are working otherwise, so we can try anyway...

Comment 49

[Thomas D. (:thomas8)]

To get this started whichever way, some of the code in question lives here:

http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#806

Comment 50

[Thomas D. (:thomas8)]

Attachment: composer.xul (overlay code used by the addon)

For ease of reference, here's the code used by the addon to overlay messengercompose.xul (1), including the code for reading a "address widget number of lines" preference

(1) http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul

Comment 51

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

(In reply to Thomas D. from comment #48)
> Thanks everybody for their cooperative responses. I especially appreciate
> the cooperative and tentative tone of Blake's response (Comment 41). I think
> if we view those last comments together and draw conclusions, we might be
> heading for a comprehensive solution that actually solves the problems at
> hand.

I agree!

> (In reply to Blake Winton (:bwinton) from comment #41)
> > (Blake:) I believe that the UI I proposed above is the best way
> > forward, and I will be happy to modify it to address any concerns you raise.
> It's unsatisfactory because it only reduces the *initial* number of lines in
> the address widget, but offers no /permanent/ way of reducing (or
> increasing) the *default* number of lines shown after auto-expanding,
> *without user intervention*. Iow, with Blake's approach only, users will
> still not have a way of *permanently* (read "automatically") adjusting the
> vertical space needed for the address widget /when there are several
> recipients/. So e.g. on a small screen (netbook), users like Bozz will have
> to shrink the widget *manually* each time they compose a message with 3 or
> more recipients. Or, in a nutshell, Blake's approach will only shrink the
> *empty* widget, but never the *filled* widget.

Yes, that is a problem with the approach I proposed.

> This is Blake's approach with some more details spelled out:
[snip…]
> e) complement Blake's auto-expand solution with an option to set a custom
> *default* number of aw lines where we stop auto-expanding the widget and
> start showing the scroll bar

Okay, I agree that there is a problem there, and I can't think of a better option which has any chance to be implemented.  (Putting all the addresses on one line would fix the problem, and be way more awesome, but I strongly doubt anyone will step up to implement that any time soon.  ;)  So ui-r=me for adding the preference.  But I still don't think that enough people will care about changing it to expose it in the config ui, so let's leave it as a hidden pref.

> Without statistical evidence, I also doubt that "most users don't care"
> about the default size of the address widget. Discussion in this bug and
> numerous getsatisfaction reports seem to suggest otherwise.

Thunderbird has millions of users.  Approximately 25 million, last I heard.  Discussion in this bug has been between at most 51 people (since there are 51 comments ;).  It would surprise me if there were more than 1000 getsatisfaction reports.  Even assuming a 1000:1 ratio of people who care to people who comment, that's still a long, long way from most users caring.  ;)

> Even if we had
> adverse statistics on numbers, we'd still have to factor in *how severely* a
> minority of users is affected by this specific problem. Having to drag your
> address widget to the right size every time you compose a message can be
> more than highly annoying to affected users.

If we were factoring that in, this still wouldn't be a particularly serious bug.  There's no data loss, there's no crashing, there's an annoying workaround (drag the widget every time), and I think there's even an add-on that implements the requested behaviour.  If Thunderbird were a paid-for, corporate project, I would have a hard time justifying to the CTO why we should spend time fixing this bug instead of other ones.  Fortunately, it's not, and I'll be happy to help any volunteer who wants to step up and take a stab at fixing this.  ;)

> (1) As a footnote:
> I like Blake's proposal of auto-expanding address widget, and I think we
> should try that line (if it's not too complex code-wise to further delay
> fixing this bug). As devil's advocate: there's anecdotal bug 734683 where
> user's are not aware how to add new recipients after they have filled the
> first line.
> If we offer one line as the default for compositions with no pre-filled
> recipients, users might try what they know from elsewhere (other mailers,
> and webmail), and they might be surprised that *autocomplete will not
> trigger for any subsequent comma-separated addresses* (which is another
> bug). But well, comma-separated addresses are working otherwise, so we can
> try anyway...

Perhaps someone will get annoyed and try to fix that bug next.  ;)

Thanks,
Blake.

Comment 52

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

Comment on attachment 700021 [details]
Screenshot 1: Proposed UI and behaviour (default to 3 lines, allow dragging splitter, allow customization of default number of lines)

So, I'm saying ui-r=me, but not for the added preference.
Also, I think we could default to 1 or two lines, and have it auto-expand.

Thanks,
Blake.

Comment 53

[Thomas D. (:thomas8)]

Thanks Blake for the ui-review+!

(In reply to Blake Winton (:bwinton) from comment #52)
> Comment on attachment 700021 [details]
> Screenshot 1: Proposed UI and behaviour (default to 3 lines, allow dragging
> splitter, allow customization of default number of lines)
> 
> So, I'm saying ui-r=me, but not for the added preference.

Or more precisely, per Blake's ui-review in Comment 51:

> So ui-r=me for adding the preference.  But I still don't think that enough
> people will care about changing it to expose it in the config ui, so let's
> leave it as a hidden pref.

I.e. green light for adding the hidden preference, but not for exposing it in options UI. Fair enough.

> Also, I think we could default to 1 or two lines, and have it auto-expand...

...up to the maximum default number of shown lines, as defined by the hidden preference.

Comment 54

[Thomas D. (:thomas8)]

clear needinfo

Comment 55

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #49)
> To get this started whichever way, some of the code in question lives here:
> 
> http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/
> messengercompose.xul#806

And the scripts for address widget behaviour live here:

http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/addressingWidgetOverlay.js

Comment 56

[Wayne Mery (:wsmwk)]

(In reply to Blake Winton (:bwinton) from comment #52)
> Comment on attachment 700021 [details]
> Screenshot 1: Proposed UI and behaviour (default to 3 lines, allow dragging
> splitter, allow customization of default number of lines)
> 
> So, I'm saying ui-r=me, but not for the added preference.
> Also, I think we could default to 1 or two lines, and have it auto-expand.

I don't normally weigh in on UI issues, but ++ to defaulting to just one or two lines.

Comment 57

[Thomas D. (:thomas8)]

Here's how to fix the biggest part of our problem, namely that the address widget height cannot be manually reduced to one line:

It's the min-height attribute on MsgHeaderToolbar which prevents downsizing:

> #MsgHeadersToolbar {
>    min-height: 132px;

So we need to change min-height to the equivalent of one line (including whatever else is there on that toolbar):

> #MsgHeadersToolbar {
>    min-height: 74px;

Search for style sheets affecting MsgHeaderToolbar:
http://mxr.mozilla.org/comm-central/search?string=MsgHeadersToolbar&find=css&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

In the following css files, change min-height from 132px to 74px:

http://mxr.mozilla.org/comm-central/source/mail/themes/qute/mail/compose/messengercompose-aero.css#56

http://mxr.mozilla.org/comm-central/source/mail/themes/qute/mail/compose/messengercompose.css#456

http://mxr.mozilla.org/comm-central/source/mail/themes/gnomestripe/mail/compose/messengercompose.css#348

On Mac, there's no min-height set for MsgHeaderToolbar currently, so I guess we better don't touch that, or do we?:
http://mxr.mozilla.org/comm-central/source/mail/themes/pinstripe/mail/compose/messengercompose.css#322

In seamonkey suite, min-height is mostly reduced to 0px already, so I don't think we want to change anything there.

Comment 58

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #57)
> Here's how to fix the biggest part of our problem, namely that the address
> widget height cannot be manually reduced to one line:
> It's the css min-height attribute on MsgHeaderToolbar which prevents downsizing

Furthermore, to allow downsizing, you also need to shrink the number of rows from 4 to 1 for the following xul control:

http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#806

> <listbox id="addressingWidget" flex="1" seltype="multiple" rows="4"

After changing that to rows="1", the default height of the address widget will actually collapse to 1 line. Which is what we want if we manage to get auto-expansion working. Otherwise, to change the default (current) height while preserving the flexibility, you have to calculate and set the *height* attribute of the MsgHeaderToolbar according to the number of lines you have specified in the hidden pref. Doing the calculations, setting height and reading the pref are all found in the addon code, see attachment 703422 [details] (or attachment 602586 [details]).

FTR (skipme): Comment 9 mentions another xul control...
> <listbox seltype="multiple" id="attachmentBucket" flex="1" rows="4"
...but that control has been morphed by Jim into:

http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#863

> <attachmentlist orient="vertical" id="attachmentBucket"
> 864                           disableonsend="true"
> 865                           seltype="multiple" flex="1" height="0"

Note that the rows="4" attribute has been removed, and instead flex="1" and height="0". The addon of attachment 602586 [details] works even if you remove the respective addon line to change number of rows for the attachment bucket, so that's no longer required.

Comment 59

[Thomas D. (:thomas8)]

Comment 57 and comment 58 largely cover the minimal band-aid fix for this bug.

For doing the more useful "default to 1 line for blank composition and auto-expand up to default number of lines per hidden pref = 4", this is the *major missing piece of the puzzle*:

Where (in the jungle of addressingWidgetOverlay.js ?) and how to hook up the code for pref-limited auto-expansion as described in comment Comment 48, *a)-e)*? Any suggestions?

What about http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/addressingWidgetOverlay.js#507 ?

> 507 function awAppendNewRow(setFocus)
> [snip]
> 523     top.MAX_RECIPIENTS++;

I understand that top.MAX_RECIPIENTS might be the current total number of enabled address widget lines, which is what we want to show, up to the default lines limit of the hidden pref.

I won't be able to code this, especially not the auto-expand part, therefore "helpwanted" :)

Comment 60

[Thomas D. (:thomas8)]

Per Blake's Comment 22 (nomination), and my voice (confirmation), this is a tb-papercut bug ("simple" fix, great effect), according to the current tentative papercut procedures.

Perhaps someone can add it to the wiki:
https://wiki.mozilla.org/Thunderbird/Papercuts

Comment 61

[rsx11m]

https://wiki.mozilla.org/index.php?title=Thunderbird%2FPapercuts&action=historysubmit&diff=502337&oldid=496069

Comment 62

[kenkamiya]

Modifying the program for optimizing the addressee space must be extremely difficult, as it's been a long time since this issue was addressed but no apparent update of the matter has been seen.

I wonder if the following suggestion might be a temporary easy fix until your full-blown version becomes ready.

When I place the cursor on the border between the subject field and formatting bar and, by keeping the left mouse button pressed down, drag it downward, the address field expands downward as much as it's dragged.  However, if I drag it upward, the window doesn't shrink, i.e., it's fixed to four addresses regardless of how many are there.

If you change this minimum number of addresses from 4 to 1 in your program, can't the user manually change it to his/her preference?  If yes, once it's changed, this preference is saved in the parameter table, so s/he doesn't have to change it back everytime Thunderbird is started.

Comment 63

[Thomas D. (:thomas8)]

(In reply to kenkamiya from comment #62)
> Modifying the program for optimizing the addressee space must be extremely
> difficult, as it's been a long time since this issue was addressed but no
> apparent update of the matter has been seen.

kenkymiya, thanks for commenting to move this forward.
Fixing the worst part of this is actually quite simple (as outlined by me in detail above), but it needs somebody who is willing and able (in terms of time and skills) to provide a patch. Would you be able to write up a patch?

> I wonder if the following suggestion might be a temporary easy fix until
> your full-blown version becomes ready.

You might be right that this could be a case of "the perfect being the enemy of the good". Auto-Expansion of recipient area as proposed by BWinton is certainly a more sophisticated solution (short of just doing what most other mailers do, have one line only for comma-separated addresses, Bug 440377), but it's also adding complexity to the fix which imo is one reason why this is still pending.

> When I place the cursor on the border between the subject field and
> formatting bar and, by keeping the left mouse button pressed down, drag it
> downward, the address field expands downward as much as it's dragged. 
> However, if I drag it upward, the window doesn't shrink, i.e., it's fixed to
> four addresses regardless of how many are there.

Yes, that's the main issue which we want to fix here.

> If you change this minimum number of addresses from 4 to 1 in your program,

yes, as agreed upon and ui-reviewed, this fixes the main problem

> can't the user manually change it to his/her preference?  If yes, once it's
> changed, this preference is saved in the parameter table, so s/he doesn't
> have to change it back everytime Thunderbird is started.

No, we are moving away from "Remember" UI because we think they'll bring as many problems as they solve. The fact that you are writing an odd message with 12 recipients for which you expand recipients area to 12 rows does not automatically imply that you'll want to start with 12 recipient lines next time. Same for shrinking. We think it's better to set a reasonable default number, and then offer temporary (manual/auto-)adjusting from there. The default number of recipient lines has been ui-approved to be a hidden pref (user-editable via inbuilt about:config editor), but so far I haven't been able to convince the UX lead (:bwinton) to also expose that pref in the advanced options UI.

To conclude, I think we could get away with doing the minimal fix as outlined in comment 57 and comment 58 which will be a major improvement over the status quo anyway, and doing the auto-expanding polishing stuff later.

Comment 64

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #57)
> Here's how to fix the biggest part of our problem, namely that the address
> widget height cannot be manually reduced to one line:
> It's the min-height attribute on MsgHeaderToolbar which prevents downsizing:
> In the following css files, change min-height from 132px to 74px:

Search for style sheets affecting MsgHeaderToolbar:
http://mxr.mozilla.org/comm-central/search?string=MsgHeadersToolbar&find=css&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

Updated links to css files:

http://mxr.mozilla.org/comm-central/source/mail/themes/linux/mail/compose/messengercompose.css#347

http://mxr.mozilla.org/comm-central/source/mail/themes/windows/mail/compose/messengercompose-aero.css#55

http://mxr.mozilla.org/comm-central/source/mail/themes/windows/mail/compose/messengercompose.css#455

> On Mac, there's no min-height set for MsgHeaderToolbar currently, so I guess
> we better don't touch that, or do we?:

http://mxr.mozilla.org/comm-central/source/mail/themes/osx/mail/compose/messengercompose.css#419

> In seamonkey suite, min-height is mostly reduced to 0px already, so I don't
> think we want to change anything there:

http://mxr.mozilla.org/comm-central/source/suite/themes/classic/mac/messenger/messengercompose/messengercompose.css#175 (etc.)

Comment 65

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #64)

More updated links to source code:

> Furthermore, to allow downsizing, you also need to shrink the number of rows
> from 4 to 1 for the following xul control:

http://mxr.mozilla.org/comm-central/search?string=id=%22addressingWidget%22

http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#811

> <listbox id="addressingWidget" flex="1" seltype="multiple" rows="4"
> 
> After changing that to rows="1", the default height of the address widget
> will actually collapse to 1 line. Which is what we want if we manage to get
> auto-expansion working. Otherwise, to change the default (current) height
> while preserving the flexibility, you have to calculate and set the *height*
> attribute of the MsgHeaderToolbar according to the number of lines you have
> specified in the hidden pref. Doing the calculations, setting height and
> reading the pref are all found in the addon code, see attachment 703422 [details]
> [details] (or attachment 602586 [details]).
> 
> FTR (skipme): Comment 9 mentions another xul control...
> > <listbox seltype="multiple" id="attachmentBucket" flex="1" rows="4"
> ...but that control has been morphed by Jim into: [snip; see Comment 58]

Comment 66

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #63)
> The default number of recipient lines shown has been ui-approved to be a
> hidden pref (user-editable via inbuilt about:config editor), but so far I
> haven't been able to convince the UX lead (:bwinton) to also expose that pref 
> in the advanced options UI.
> 
> To conclude, I think we could get away with doing the minimal fix as
> outlined in comment 57 and comment 58 which will be a major improvement over
> the status quo anyway, and doing the auto-expanding polishing stuff later.

OK, so I'll have mercy on this bug and give it a shot...

Comment 67

[Thomas D. (:thomas8)]

Attachment: Patch

fwd ui-review+ from comment 51, comment 52 (see comment 53)

This patch provides a great minimal fix to this bug, as seen in Screenshot 1 (but without options UI for the pref):

- default to 3 lines to regain some vertical space (instead of 4 now; less than 3 would be a problem because we don't have auto-expansion yet)
- allow dragging splitter both ways, minimum height of address widget = 1 row
- hidden pref to set the number of rows shown by default, as approved by :bwinton in comment 51

Comment 68

[Thomas D. (:thomas8)]

Comment on attachment 761976 [details] [diff] [review]
Patch

Fwiw, patch is semi-handmade and untested; sorry I can't do more bc I don't build yet, and I don't have time for tweaking my own install. Pls assist with addressing any problems potentially resulting from that.

FTR: Some code morphed from Bozz' Address Widget Lines Addon (1), thanks!

(1) see xul attachment 703422 [details], xpi attachment 602586 [details]
https://addons.mozilla.org/en-US/thunderbird/addon/address-widget-lines/

Comment 69

[Magnus Melin [:mkmelin]]

Comment on attachment 761976 [details] [diff] [review]
Patch

Review of attachment 761976 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, almost(?) all of the patch fails to apply

hg qpush
applying bug425451.patch
patching file mail/themes/windows/mail/compose/messengercompose-aero.css
Hunk #1 FAILED at 48
1 out of 1 hunks FAILED -- saving rejects to file mail/themes/windows/mail/compose/messengercompose-aero.css.rej
patching file mail/themes/windows/mail/compose/messengercompose.css
Hunk #1 FAILED at 448
1 out of 1 hunks FAILED -- saving rejects to file mail/themes/windows/mail/compose/messengercompose.css.rej
patching file mail/components/compose/content/messengercompose.xul
Hunk #1 FAILED at 803
1 out of 1 hunks FAILED -- saving rejects to file mail/components/compose/content/messengercompose.xul.rej
patching file mail/components/compose/content/addressingWidgetOverlay.js
Hunk #1 FAILED at 38
1 out of 1 hunks FAILED -- saving rejects to file mail/components/compose/content/addressingWidgetOverlay.js.rej
patching file mail/components/compose/content/MsgComposeCommands.js
Hunk #1 FAILED at 2526
1 out of 1 hunks FAILED -- saving rejects to file mail/components/compose/content/MsgComposeCommands.js.rej
patching file mailnews/mailnews.js
Hunk #1 FAILED at 146
1 out of 1 hunks FAILED -- saving rejects to file mailnews/mailnews.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bug425451.patch

Comment 70

[rsx11m]

Comment on attachment 761976 [details] [diff] [review]
Patch

I think I know what happened. Some lines in this patch end in LF, others in CRLF, so this can't work. After stripping those CR, the patch applies without offset against comm-central for me.

Comment 71

[Thomas D. (:thomas8)]

Attachment: Patch v2 (now with LFs only)

(In reply to rsx11m from comment #70)
> Comment on attachment 761976 [details] [diff] [review]
> Patch
> 
> I think I know what happened. Some lines in this patch end in LF, others in
> CRLF, so this can't work. After stripping those CR, the patch applies
> without offset against comm-central for me.

Thanks Ronald, that's a helpful feedback, and more appreciative/encouraging for a beginner in patches like me after my painstaking efforts to plan, code up and prepare the patch correctly.

Could you check out the new patch if it applies and does what it says (per Comment 67) before we request the next review from :mkmelin? Tia!

Comment 72

[rsx11m]

Comment on attachment 762250 [details] [diff] [review]
Patch v2 (now with LFs only)

Unfortunately this doesn't work for me, all I get is a single line to enter the "To:" address, not 3 as intended (or any other value for the pref).

>-          <listbox id="addressingWidget" flex="1" seltype="multiple" rows="4"
>+          <listbox id="addressingWidget" flex="1" seltype="multiple" rows="1"

That's apparently what it defaults to.

>+  // Initialize the default number of rows (height) for addresswidget from pref
>+  try {
>+    var awDefaultRows = getPref("mail.compose.addresswidget.defaultRows");
>+  }
>+  catch (ex) {
>+    // If getting the pref for awDefaultRows fails, fall back to 4 rows
>+    dump("failed to get mail.compose.addresswidget.defaultRows pref\n");
>+    var awDefaultRows = 4;
>+  }
>+  awSetNumberOfRowsShown(awDefaultRows);

The awDefaultRows variable should be declared before the try/catch block and not twice, the scoping looks rather flaky here (it won't work for me with that change applied locally either, though).

>+++ b/mailnews/mailnews.js
>+pref("mail.compose.addresswidget.defaultRows", 3); // number of recipient rows shown

As you are patching Thunderbird code only, this preference shouldn't be set in mailnews.js but rather all-thunderbird.js (i.e., not a MailNews Core setting).

I'd recommend that you get your own build running so that you can test and debug your code before posting a new patch, otherwise that's a tedious process.

Comment 73

[Thomas D. (:thomas8)]

(In reply to rsx11m from comment #72)
> Comment on attachment 762250 [details] [diff] [review]
> Patch v2 (now with LFs only)
> 
> Unfortunately this doesn't work for me, all I get is a single line to enter
> the "To:" address, not 3 as intended (or any other value for the pref).

Yep, I found 2 bugs in my code which cause that (like comparing apples with pears and then going into the wrong blocks), will file a new patch very soon.

Comment 74

[Thomas D. (:thomas8)]

Attachment: Patch v3 (revised & tested)

So here's a corrected and revised patch that should do the trick as advertised in Comment 67 (tweaks to omni.ja tested working against my local trunk install on WinXP, 24.0a1 (2013-06-14)).

Comment 75

[Thomas D. (:thomas8)]

Changelog for attachment 763103 [details] [diff] [review]

(In reply to rsx11m from comment #72)
> Comment on attachment 762250 [details] [diff] [review]
> Patch v2 (now with LFs only)
> 
> Unfortunately this doesn't work for me, all I get is a single line to enter
> the "To:" address, not 3 as intended (or any other value for the pref).

Done.

> >+  // Initialize the default number of rows (height) for addresswidget from pref
> >+  try {
> >+    var awDefaultRows = getPref("mail.compose.addresswidget.defaultRows");
> >+  }
> >+  catch (ex) {
> >+    // If getting the pref for awDefaultRows fails, fall back to 4 rows
> >+    dump("failed to get mail.compose.addresswidget.defaultRows pref\n");
> >+    var awDefaultRows = 4;
> >+  }
> >+  awSetNumberOfRowsShown(awDefaultRows);
> 
> The awDefaultRows variable should be declared before the try/catch block and
> not twice, the scoping looks rather flaky here (it won't work for me with
> that change applied locally either, though).

(Note: For attachment  I've renamed awDefault to become awNumRowsDefault.)

This wfm, and is intentional.
- Under normal circumstances, we just declare awNumRowsDefault once in the try statement.
- If (and only if) getPref throws an error [sic!], I think we can't be sure if awNumRowsDefault was declared or not, and if it has the right type. Hence we need to ensure that in this particular case the variable is still declared correctly in the catch block. Re-declaration is no problem in JavaScript according to internet sources, and I don't see problems of scope here. (And if getpref really throws an error, I think you'll have bigger problems than just awNumRowsDefault...)
- fyi, getPref() does NOT throw an error if the pref is not found, it will return null. I've tried that case and it falls back to showing 1 row per minHeight defined in the css (74px) which wins over height of 0 rows + extraheight (=55px).

But maybe there's a better way of ensuring that we initialize to a minimum of 1 row regardless of any problems with the pref.

> >+++ b/mailnews/mailnews.js
> >+pref("mail.compose.addresswidget.defaultRows", 3); // number of recipient rows shown
> 
> As you are patching Thunderbird code only, this preference shouldn't be set
> in mailnews.js but rather all-thunderbird.js (i.e., not a MailNews Core
> setting).

Done.

Comment 76

[rsx11m]

Attachment: Screenshot (v3 on Windows Classic)

Almost, definitely a good step forward. :-)

- With the Windows Classic desktop theme, there is a bit of the 4th line
  visible for n=3 lines which disappears when setting n>3 instead.

- I cannot shrink the pane below 3 lines, and trying with n<3 results in n=3.
  (I'm wondering if you can just get rid of the min-height: 74px; definitions?)

- Changing the pref may not take immediate effect; meaning, I open it first
  with n=3, then set n=5 in the Config Editor, clicking on Write /may/ open a
  fresh composition window still with n=3, trying next time then with n=5.

- I'll leave it up to Magnus to decide on the var awNumRowsDefault declaration,
  but this looks rather odd to me.

Comment 77

[rsx11m]

(In reply to rsx11m from comment #76)
>   (I'm wondering if you can just get rid of the min-height: 74px; definitions?)

On a second thought, maybe this should just reflect the size of the pane for n=1 to avoid that someone can collapse it by making it smaller than a single line?

Comment 78

[Thomas D. (:thomas8)]

(In reply to rsx11m from comment #76)
> Created attachment 763118 [details]
> Screenshot (v3 on Windows Classic)
> 
> Almost, definitely a good step forward. :-)
> 
> - With the Windows Classic desktop theme, there is a bit of the 4th line
>   visible for n=3 lines which disappears when setting n>3 instead.

I don't see that on WinXP with what claims to be "Default Theme".
Is Windows Classic desktop theme a theme which we still ship?
Which Version of Windows are you using?
And I don't understand how it can disappear for settings n>3.
The calculations are based on Bozz' Hack in the Addon, and I have no idea how this interacts with Themes - maybe Bozz knows more?

> - I cannot shrink the pane below 3 lines, and trying with n<3 results in n=3.
>   (I'm wondering if you can just get rid of the min-height: 74px;
> definitions?)
> On a second thought, maybe this should just reflect the size of the pane for
> n=1 to avoid that someone can collapse it by making it smaller than a single
> line?

74px IS the size of the pane (toolbar or whatever) for one row (n=1), so my min-height from the css is definitely not what's stopping you from shrinking to n<3. Perhaps you have some other css which overrides min-height. Try Safe-Mode?

Shrinking below 3 lines works for me with the changes as in the patch (that's the main purpose of the whole thing, come on!)
I have no idea why it wouldn't work for you.

Unless disabling the Addon is not sufficient on my side and the disabled addon still effects something which it shouldn't - unlikely.

> - Changing the pref may not take immediate effect; meaning, I open it first
>   with n=3, then set n=5 in the Config Editor, clicking on Write /may/ open a
>   fresh composition window still with n=3, trying next time then with n=5.

I suspect that's something about recycling compose windows, where perhaps the onload event doesn't work as expected, or gets otherwise confused.
If somebody knows how to force this into effect immediately every time without too much hassle, pls share with us. Otherwise, I think it's not a big deal because the new setting will definitely take effect after restarting TB, which I think sufficiently covers the usecase, so I will not spend more time on that. We don't expect users to change the default number of recipient rows shown in composition every hour or so, imo it's a more permanent setting.

I also see mail.compose.max_recycled_windows;1 is default, so onload should fire every time?

> - I'll leave it up to Magnus to decide on the var awNumRowsDefault
> declaration,
>   but this looks rather odd to me.

ok.

Comment 79

[rsx11m]

(In reply to Thomas D. from comment #78)
> I don't see that on WinXP with what claims to be "Default Theme".
> Is Windows Classic desktop theme a theme which we still ship?
> Which Version of Windows are you using?

Windows 7, but it has been around since Windows 2000. Right-click on your desktop and select "Properties" then you should be able to switch to it.

> And I don't understand how it can disappear for settings n>3.

My guess is that the gap is coming from the min-height of 74px, which may work for the Windows XP default theme, but other desktop themes may use different fonts and different spacing or border decoration, thus showing those differences.

> The calculations are based on Bozz' Hack in the Addon, and I have no idea
> how this interacts with Themes - maybe Bozz knows more?

Let's ask him.

> 74px IS the size of the pane (toolbar or whatever) for one row (n=1), so my
> min-height from the css is definitely not what's stopping you from shrinking
> to n<3. Perhaps you have some other css which overrides min-height.

Nope, that was a new profile, thus definitely no other settings active.

> I suspect that's something about recycling compose windows, where perhaps
> the onload event doesn't work as expected, or gets otherwise confused.

Someone else would need to help with that, I don't know.

Comment 80

[rsx11m]

Comment on attachment 763118 [details]
Screenshot (v3 on Windows Classic)

Sorry, my stupidity. The patch applied to messagecompose.css in the qute theme but I still had the aero version at the previous value, thus forcing it to 3+ lines. Conflict resolved and it works now as intended, no additional space.

I still see the delayed response to changes and I'm wondering if it depends on leaving the about:config dialog window open, or some other weird condition. Nothing conclusive at this time.

Comment 81

[Thomas D. (:thomas8)]

(In reply to rsx11m from comment #80)
> Comment on attachment 763118 [details]
> Screenshot (v3 on Windows Classic)
> 
> Sorry, my stupidity. The patch applied to messagecompose.css in the qute
> theme but I still had the aero version at the previous value, thus forcing
> it to 3+ lines. Conflict resolved and it works now as intended, no
> additional space.

Is that feedback+ for the patch then? ;)

I'd also like to see screenshots of how it looks on Windows classic theme, and screenshots how it looks on other OS (Linux/MAC) which I can't test. Anybody?

> I still see the delayed response to changes and I'm wondering if it depends
> on leaving the about:config dialog window open, or some other weird
> condition. Nothing conclusive at this time.

Again, for me, changing the pref in about:config seems to apply immediately, even with leaving about:config window open, or even with leaving options *and* about window open and starting a new mail from another TB window.

Comment 82

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #81)
> I'd also like to see screenshots of how it looks on Windows classic theme,
> and screenshots how it looks on other OS (Linux/MAC) which I can't test.
> Anybody?

Comment 83

[Thomas D. (:thomas8)]

More importantly: What's the deadline for regular patches to make it into TB24?

Comment 84

[rsx11m]

Attachment: Screenshot (v3 on Windows 7)

So, here we go...

 - Top row: Windows Classic theme on Windows 7, using GDI fonts
   (fits nicely for both n=3 and now also for n=1).

Still an issue with n=1 on Windows 7 default theme without DirectWrite fonts,

 - Bottom row, left image: Windows 7 Default Aero theme, GDI fonts
   (smaller than Windows Classic, leaving a slight gap again).

 - Bottom row, right image: The same with DirectWrite fonts on Direct2D
   (more height than GDI fonts, thus filling the gap shown before).

Unless that's acceptable, maybe reduce the min-height in the aero version by the difference seen, thus the bottom border is aligned when no DirectWrite fonts are available.

The spacing in the Linux version looked fine at 74px when I tried it Thursday.

Comment 85

[rsx11m]

Comment on attachment 763124 [details]
Screenshot (v3 on Windows 7)

(and no, this is not a patch - oops!)

Comment 86

[rsx11m]

Comment on attachment 763103 [details] [diff] [review]
Patch v3 (revised & tested)

(In reply to Thomas D. from comment #81)
> Is that feedback+ for the patch then? ;)

That's good enough for an f+, but it would be nice to resolve the remaining spacing issues and to figure out where the delay comes from. In general, it's fine after 20sec or so, and for sure when restarting, thus it looks like some timing issue which may be borderline enough that it shows up on Windows 7 but not on XP or so. Magnus can test on Linux, Blake could cover the Mac OSX part.

(In reply to Thomas D. from comment #83)
> More importantly: What's the deadline for regular patches to make it into TB24?

The merge is schedule for June 24th, likely some time in the morning PDT. Since this is technically a new feature, I think it would have to meet that deadline at least for the initial checkin, but the module owners/peers could give a more definite answer to that.

Comment 87

[Magnus Melin [:mkmelin]]

Comment on attachment 763103 [details] [diff] [review]
Patch v3 (revised & tested)

Review of attachment 763103 [details] [diff] [review]:
-----------------------------------------------------------------

You forgot min-height for osx.

Looks ok on linux.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2539,5 @@
> +  catch (ex) {
> +    // If getting the pref for awNumRowsDefault throws an error, fall back to 3 rows
> +    dump("failed to get pref: mail.compose.addresswidget.numRowsShownDefault\n");
> +    var awNumRowsDefault = 3;
> +  }

There's no need for all this try-catch. You've added the pref to defaults so the pref will exist.

(And i don't think you're really supposed to redeclare it like that, but it likely works.)

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +42,5 @@
>  
>    return gNumberOfCols;
>  }
>  
> +// function to adjust the number of visible recipient rows (height) for address widget

functions should be documented using javadoc style:
/**
 *
 */

@@ +51,5 @@
> +
> +  // height of one row
> +  var oneRowHeight = document.getElementById('addressCol1#1').parentNode.boxObject.height;
> +  // how much extra height we need to add to calculate MsgHeadersToolbar height from rows height;
> +  // include all other elements on MsgHeadersToolbar except addressingWidget client area 

trailing space.

@@ +61,5 @@
> +  // we never show less than one row due to min-height: 74px; in messengercompose.css, #MsgHeadersToolbar
> +  MsgHeadersToolbar.style.height = newHeight + 'px';
> +
> +  // update addressingWidget internals
> +  awCreateOrRemoveDummyRows();

Some of style stuff:

 - use let instead of var
 - variables always start with lowercase
 - don't mix " and ' as the code looks ugly. prefer "
 - comment sentences should be capitalized and end with period

Comment 88

[rsx11m]

Comment on attachment 763103 [details] [diff] [review]
Patch v3 (revised & tested)

I don't know what happened with the f+ I've set earlier, but it reverted back to a request. As I don't see anything for me to do here, just switching it back.

Comment 89

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #87)
> Comment on attachment 763103 [details] [diff] [review]
> Patch v3 (revised & tested)
> 
> Review of attachment 763103 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You forgot min-height for osx.

I left it out deliberately for MAC OSx as explained in Comment 64, because it was not there in the css before, and I'm afraid to break things if I add that. How does the current patch behave on OSx, for default n=3(default), n=1, and n=5?

> Looks ok on linux.
> 
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +2539,5 @@
> > +  catch (ex) {
> > +    // If getting the pref for awNumRowsDefault throws an error, fall back to 3 rows
> > +    dump("failed to get pref: mail.compose.addresswidget.numRowsShownDefault\n");
> > +    var awNumRowsDefault = 3;
> > +  }
> 
> There's no need for all this try-catch. You've added the pref to defaults so
> the pref will exist.

I just copied what was there for other prefs. Catch is only about whatever other errors might occur, so it will not even catch non-existing prefs as getPref() returns null in that case (which goes through correctly due to min-height, as explained above).

But yes, the pref must exist, so errors are unlikely and it's fine to simplify and just use the pref without try.

> ::: mail/components/compose/content/addressingWidgetOverlay.js
> > +// function to adjust the number of visible recipient rows (height) for address widget
> 
> functions should be documented using javadoc style:
> /**
>  *
>  */

Well, most of the comments in that file suggested otherwise, so I was just adapting to what was already there...

> trailing space.

Yes, these things tend to hide here and there.
Fortunately, they don't break anything... ;)

> @@ +61,5 @@
> > +  // we never show less than one row due to min-height: 74px; in messengercompose.css, #MsgHeadersToolbar
> > +  MsgHeadersToolbar.style.height = newHeight + 'px';
> > +
> > +  // update addressingWidget internals
> > +  awCreateOrRemoveDummyRows();
> 
> Some of style stuff:
> 
>  - use let instead of var

Whereever block-level scope is enough, sure.

>  - variables always start with lowercase
>  - don't mix " and ' as the code looks ugly. prefer "

Sure.

>  - comment sentences should be capitalized and end with period

And a bit more candy to the eye, as we are dreaming of perfection in Thunderbird code... That's a sweet dream for sure... ;)

Comment 90

[Thomas D. (:thomas8)]

@Magnus and rsx11m: Thanks for rapid feedback! :)

Comment 91

[Richard Marti (:Paenglab)]

(In reply to Thomas D. from comment #89)
> (In reply to Magnus Melin from comment #87)
> > Comment on attachment 763103 [details] [diff] [review]
> > Patch v3 (revised & tested)
> > 
> > Review of attachment 763103 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > You forgot min-height for osx.
> 
> I left it out deliberately for MAC OSx as explained in Comment 64, because
> it was not there in the css before, and I'm afraid to break things if I add
> that. How does the current patch behave on OSx, for default n=3(default),
> n=1, and n=5?

I would say the min-height could be removed. On my Win7 the real toolbar height is 77px with only one row and not the 74px from css. I tried also with removed min-height on Win7 and Linux and it still shows the full row when minimized. I think the correct calculation comes from here: http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2533 and in DOMi the toolbar has a attribute minheight="77"

Comment 92

[rsx11m]

If the size can be calculated (and it appears that this is what happens), that's certainly better than having fixed sizes in the CSS definitions which clearly vary among platforms, desktop themes, and fonts used.

BTW: Did either of you see the slightly delayed response in pref changes becoming effective which I've noticed?

Comment 93

[Richard Marti (:Paenglab)]

No delayed response on Win7 and Linux here.

Comment 94

[rsx11m]

Ok, so maybe I've just screwed up something else in that build...

Comment 95

[Thomas D. (:thomas8)]

(In reply to Richard Marti [:Paenglab] from comment #91)
 
> I would say the min-height could be removed.

Probably yes, if we change the handling in code accordingly.

> On my Win7 the real toolbar
> height is 77px with only one row and not the 74px from css. I tried also
> with removed min-height on Win7 and Linux and it still shows the full row
> when minimized.

Yes, because height is calculated quite correctly based on rows, per Bozz's formula (only I still don't understand where exactly the extra +2 px come from, and if they are right for all themes, but they seem required to avoid the vertical toolbar for empty rows). rsx11m, do I understand you correctly that regardless of OS and themes, you are now only seeing unwanted margins for n=1?

However, if we remove min-height from css, we need to re-include it in code. Because while the correct *default height* will be shown after changing the pref (even for one line), we need to prevent the user from shrinking the whole address widget to zero height which potentially creates usability problems.

Even shrinking it to one line can create usability problems, because no matter how many recipients you have, you will never see a vertical scrollbar (which I think is a bug in xul) so it looks as if there's only one recipient while there plenty, and you have to know there aree more, and it's not easy to flip through them with n=1. But allowing that manual shrink seems ok as long as we don't default to it unless we get auto-expansion working.

> I think the correct calculation comes from here:
> http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/
> MsgComposeCommands.js#2533 and in DOMi the toolbar has a attribute
> minheight="77"

Yes, but that's just setting the *min-height* to be the same as the current *total height* of MsgHeadersToolbar, so unfortunately we can't use that straight away for setting a min-height of one row.

I'm also wondering when we remove the min-height from css, shouldn't we just add a pref so that users/admins can set their preferred min-height in rows (mail.compose.addresswidget.awNumRowsMin)? That would make life so much simpler e.g. for company admins who want to avoid their users getting into trouble with multiple addresses after manually shrinking to awNumRows=1 and loosing their vertical toolbar and getting weird behaviour when entering addresses until they manually expand again (with n=1, try adding a recipient address, then press Enter to see the weirdness of addresses disappearing into nowhere)...

Comment 96

[Richard Marti (:Paenglab)]

(In reply to Thomas D. from comment #95)
> (In reply to Richard Marti [:Paenglab] from comment #91)
>  
> > I would say the min-height could be removed.
> 
> Probably yes, if we change the handling in code accordingly.

The code is calculating this already.

> However, if we remove min-height from css, we need to re-include it in code.
> Because while the correct *default height* will be shown after changing the
> pref (even for one line), we need to prevent the user from shrinking the
> whole address widget to zero height which potentially creates usability
> problems.

When you are checking the toolbar with DOMi, you will see the minheight is already set to (for me) 77 also with the initial 3 rows. This isn't a dynamic calculation of how many rows are shown.

> > I think the correct calculation comes from here:
> > http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/
> > MsgComposeCommands.js#2533 and in DOMi the toolbar has a attribute
> > minheight="77"
> 
> Yes, but that's just setting the *min-height* to be the same as the current
> *total height* of MsgHeadersToolbar, so unfortunately we can't use that
> straight away for setting a min-height of one row.

As I wrote above, minheight is already initially set to one row. Please test it with DOMi to be sure I'm not wrong what I'm seeing on my systems.

Comment 97

[Richard Marti (:Paenglab)]

Tried the patch on OS X and it looks good.

Comment 98

[Magnus Melin [:mkmelin]]

(In reply to Thomas D. from comment #95)
> I'm also wondering when we remove the min-height from css, shouldn't we just
> add a pref so that users/admins can set their preferred min-height in rows
> (mail.compose.addresswidget.awNumRowsMin)? That would make life so much
> simpler e.g. for company admins who want to avoid their users getting into
> trouble with multiple addresses after manually shrinking to awNumRows=1 and
> loosing their vertical toolbar and getting weird behaviour when entering
> addresses until they manually expand again (with n=1, try adding a recipient
> address, then press Enter to see the weirdness of addresses disappearing
> into nowhere)...

It can already be controlled using userChrome.css. But, it's indeed confusing with just one row, and maybe we're better off forcing a minimum of 2 rows. 

BTW, addressingWidget shouldn't have 1 row as default. I'd let it be 3 to match the default pref value.

Comment 99

[rsx11m]

I've removed the "min-height: 74px;" rule, and it works fine with both Windows 7 default Aero and the Windows Classic desktop themes. In particular, the issues I've seen with DirectWrite vs. GDI fonts in attachment 763124 [details]. Thus, it appears to be safe to just remove the min-height here completely and to let the code for calculating it from the measured row height take over.

I don't think this need another pref. If someone sets n=1 or shrinks the pane to that size, they know what they are doing (though the scroll bar disappears for me when forcing one row, still visible with n=2, which supports the argument to make two rows the lower limit).

Comment 100

[rsx11m]

(In reply to rsx11m from comment #86)
> That's good enough for an f+, but it would be nice to resolve the remaining
> spacing issues and to figure out where the delay comes from. In general,
> it's fine after 20sec or so, and for sure when restarting, ...

I think I've narrowed this glitch down. Motivated by bug 881588 comment #1, I've set mail.compose.max_recycled_windows=0, and it fixes the issue for me. Thus, it apparently comes from recycling the composition window in some cases. For example, if I change mail.compose.addresswidget.numRowsShownDefault it becomes effective for a new "Write" window immediately, but a "Reply" window still shows the old number of lines until that window is disposed of.

As said, not a big deal and sure could be deferred to a follow-up bug if someone else can reproduce this.

Comment 101

[rsx11m]

Thomas, are you going to post a new patch for review?

Comment 102

[Thomas D. (:thomas8)]

(In reply to rsx11m from comment #101)
> Thomas, are you going to post a new patch for review?

Yes, I'm working on the new patch.

Comment 103

[Thomas D. (:thomas8)]

Attachment: Patch v4 (completely rewritten, streamlined code)

I've completely rewritten the patch to address the review comments above, and have cleaner, leaner code.

Coded locally against 24.0a1 (2013-06-17), might have offsets.

Works perfectly as expected for me on WinXP / 24.0a1 (2013-06-17) with
mail.compose.max_recycled_windows=*0*, for all types of composition, so I believe the patch does the right thing.

Unfortunately, as I only noticed after the fact, it will still fail intermittently with mail.compose.max_recycled_windows=*1* (default), but only for Reply, and only intermittantly if you happen to catch the wrong one between those recycled windows for your reply, because of something like

Bug 671530 - New message, Reply or Forward leaves address and subject fields in disabled state

Tentative STR for that problem to test with my patch (see Bug 671530 Comment 34):

- Ensure mail.compose.max_recycled_windows=*1*
- Right after restart, without doing any other compositions before:
1 Reply to selected msg in 3pane
2 close reply window
3 Reply to same selected msg in 3pane

-> Send button (only sometimes!) and the whole address widget disabled, while
- address widget correctly filled
- address widget behaving correctly wrt the patch (default height/min-height)
- body correct and editable

Any chances to track that recycled-composition problem down?

Comment 104

[rsx11m]

Comment on attachment 765576 [details] [diff] [review]
Patch v4 (completely rewritten, streamlined code)

Works nicely for me, except that

>+  /* Set minimum number of rows shown for address widget. */
>+  addressingWidget.setAttribute("rows",numRowsShownMin.toString());
>+  msgHeadersToolbar.minHeight = msgHeadersToolbar.boxObject.height;

doesn't do anything for me on Linux; meaning, the minimum height remains a single row regardless of the pref's value. I'm wondering if there is some delay between modifying the "rows" attribute and grabbing the current height for setting the "minheight" attribute, or if it's possibly overridden elsewhere.

>-          <listbox id="addressingWidget" flex="1" seltype="multiple" rows="4"
>+          <listbox id="addressingWidget" flex="1" seltype="multiple" rows="1"

In contrast, setting rows="2" here already restricts the size I can minimize the address widget to. I can't get below two rows, thus still allowing the scrollbar to appear properly.

Hence the question if the second preference to provide a lower limit is really needed or if that value can be just hardwired to two rows and left to the listbox to take care of?

Comment 105

[rsx11m]

(In reply to Thomas D. from comment #103)
> Any chances to track that recycled-composition problem down?

I'd leave this for another bug, preferably after bug 671530 has sorted it out. I don't see how this can be easily resolved in the code that you're touching here, and the user impact isn't dramatic if the recycle-windows pref isn't changed from it's default (it's more of a surprise issue as far as the number of rows is concerned, thus no impact on usability).

Comment 106

[rsx11m]

(In reply to rsx11m from comment #104)
> >+  /* Set minimum number of rows shown for address widget. */
> >+  addressingWidget.setAttribute("rows",numRowsShownMin.toString());
> >+  msgHeadersToolbar.minHeight = msgHeadersToolbar.boxObject.height;

I see, setting rows="2" and commenting these lines out allows me to shrink even further than a single row, thus setting .minHeight is required for this to work even if it's just for getting the height of addressingWidget with the two rows initialized in the XUL definition. Thus, it indeed appears to be a matter of XUL-reflow timing, at least in my configuration.

Comment 107

[Thomas D. (:thomas8)]

(In reply to rsx11m from comment #104)
> Comment on attachment 765576 [details] [diff] [review]
> Patch v4 (completely rewritten, streamlined code)
> 
> Works nicely for me, except that
> 
> >+  /* Set minimum number of rows shown for address widget. */
> >+  addressingWidget.setAttribute("rows",numRowsShownMin.toString());
> >+  msgHeadersToolbar.minHeight = msgHeadersToolbar.boxObject.height;
> 
> doesn't do anything for me on Linux; meaning, the minimum height remains a
> single row regardless of the pref's value. I'm wondering if there is some
> delay between modifying the "rows" attribute and grabbing the current height
> for setting the "minheight" attribute, or if it's possibly overridden
> elsewhere.

Both might be possible, but it's still surprising, because that part works for me every time on Windows. Can you test again pls and ensure the following:

- verify that you don't have any #MsgHeadersToolbar min-hight stuff set up in userchrome.css or other such files on your profile
- from config editor, reset mail.compose.addresswidget.numRowsShownMin to it's default value, 2
- Exit Thunderbird via File > Exit (I don't trust the [x] buttons to do the right thing)
- Restart TB
- from config editor, verify if mail.compose.addresswidget.numRowsShownMin is still set to 2
- create a new message
- by default (per default pref) splitter should be at 3 rows
- drag it upwards
-> should stop at 2 rows - does it?

> 
> >-          <listbox id="addressingWidget" flex="1" seltype="multiple" rows="4"
> >+          <listbox id="addressingWidget" flex="1" seltype="multiple" rows="1"
> 
> In contrast, setting rows="2" here already restricts the size I can minimize
> the address widget to. I can't get below two rows, thus still allowing the
> scrollbar to appear properly.

Yes, that is why mail.compose.addresswidget.numRowsShownMin defaults to 2.
Although users could only experience those ux-oddities after manually shrinking to 1, and only for a single messsage, because for the next mail, we'll default to mail.compose.addresswidget.numRowsShownDefault of 3 rows again unless you have tweaked that to 1 row also. It also depends very much on use patterns if users would experience any problems for n=1 or not. E.g. for n=1 if you type a known address and just tab out of the recipient line, it will stay in sight as expected, for a happy user who has just reclaimed 3 rows of screen real estate unnecessarily taken by the unfortunate multi-row design of our address widget.

> Hence the question if the second preference to provide a lower limit is
> really needed or if that value can be just hardwired to two rows and left to
> the listbox to take care of?

The problem is that if you hardwire that value in the rows attribute, and then set the min-height from the code, there is no way whatsoever the user can ever override this - the code for setting min-height overrides userchrome.css in this case. So for those users who actually depend on reclaiming their vertical screen real estate (e.g. on netbooks) by using n=1 (via manual shrinking or as a default), we'd be stopping half-way down the road of really fixing this bug, and they would still be forced to use addon for shrinking addresswidget rows to 1 or 0. For such an important UI like composition, and given how much vertical space we are wasting there due to multi-line design, I would really feel embarrased if we hardwire n=2 again deep in the code, without remedy, and thus perpetuate the problems we are trying to fix with this bug. Even after setting numRowsShownMin=0 and then shrinking to n=0(!), the top part of composition window still consumes almost *half* of a regular netbook screen height which is really painful. I think the current patch attachment 765576 [details] [diff] [review] combines safe defaults with the minimum of customizability (per hidden pref) that can be expected from quality software designed with the user at heart, while keeping the code clean, lean, and consistent.

Comment 108

[Thomas D. (:thomas8)]

(In reply to rsx11m from comment #106)
> (In reply to rsx11m from comment #104)
> > >+  /* Set minimum number of rows shown for address widget. */
> > >+  addressingWidget.setAttribute("rows",numRowsShownMin.toString());
> > >+  msgHeadersToolbar.minHeight = msgHeadersToolbar.boxObject.height;
> 
> I see, setting rows="2" and commenting these lines out allows me to shrink
> even further than a single row, thus setting .minHeight is required for this
> to work even if it's just for getting the height of addressingWidget with
> the two rows initialized in the XUL definition. Thus, it indeed appears to
> be a matter of XUL-reflow timing, at least in my configuration.

It would be nice to know if others see the same problem?
If yes, any cure for that?

Well, in an effort to work around Bug 671530, I'll first try to remove
> addressingWidget.setAttribute("rows",numRowsShownMin.toString());
and replace that again with the more complex height calculation algorithm morphed from the addon code.

(In reply to rsx11m from comment #105)
> (In reply to Thomas D. from comment #103)
> > Any chances to track that recycled-composition problem down?
> 
> I'd leave this for another bug, preferably after bug 671530 has sorted it
> out. I don't see how this can be easily resolved in the code that you're
> touching here, and the user impact isn't dramatic if the recycle-windows
> pref isn't changed from it's default (it's more of a surprise issue as far
> as the number of rows is concerned, thus no impact on usability).

Oh, I think it's quite dramatic when for every second reply or so, your entire addressing widget often including the send button is *permanently disabled* so you can't use that window to send the reply. Note that the bug *does* occur with the *default* setting of mail.compose.max_recycled_windows=1!

I'm hoping that it might be triggered by setting the row attribute, so using the old algorithm for height calculation could work around that.

Does anybody see disabled addressing widget in reply window *without* the patch, following tentative STR of comment 103?

Comment 109

[Thomas D. (:thomas8)]

(In reply to rsx11m from comment #106)
> (In reply to rsx11m from comment #104)
> [snip]
> the two rows initialized in the XUL definition. Thus, it indeed appears to
> be a matter of XUL-reflow timing, at least in my configuration.

rsx11m, thanks for instant testing & feedback :)

Comment 110

[Thomas D. (:thomas8)]

re-setting needinfo for comment 108    (while cursing that tick which always sets itself to clear needinfos which I never wanted to clear)

Comment 111

[rsx11m]

(In reply to Thomas D. from comment #107)
> Both might be possible, but it's still surprising, because that part works
> for me every time on Windows. Can you test again pls and ensure the
> following:
> 
> - verify that you don't have any #MsgHeadersToolbar min-hight stuff set up
>   in userchrome.css or other such files on your profile
> - from config editor, reset mail.compose.addresswidget.numRowsShownMin to
>   it's default value, 2
> - Exit Thunderbird via File > Exit (I don't trust the [x] buttons to do the
>   right thing)
> - Restart TB
> - from config editor, verify if mail.compose.addresswidget.numRowsShownMin
>   is still set to 2
> - create a new message
> - by default (per default pref) splitter should be at 3 rows

Works fine up to this point.

> - drag it upwards
> -> should stop at 2 rows - does it?

Nope, it stops fairly accurately at 1 row for me.


(In reply to Thomas D. from comment #108)
> Well, in an effort to work around Bug 671530, I'll first try to remove
> > addressingWidget.setAttribute("rows",numRowsShownMin.toString());
> and replace that again with the more complex height calculation algorithm
> morphed from the addon code.

If that function is only called when the window is instantiated, I don't think that changing the height calculation would make much of a difference if the window is recycled afterwards. Meaning, upon reopening, that function may not be called again, thus whatever it would do to recalculate the minheight value won't be executed. But then, I don't know how the recycling process works, thus my assumptions may be wrong here.

Comment 112

[Thomas D. (:thomas8)]

(In reply to rsx11m from comment #111)
> (In reply to Thomas D. from comment #108)
> > Well, in an effort to work around Bug 671530, I'll first try to remove
> > > addressingWidget.setAttribute("rows",numRowsShownMin.toString());
> > and replace that again with the more complex height calculation algorithm
> > morphed from the addon code.

>+++ b/mail/components/compose/content/MsgComposeCommands.js
>@@ -2524,17 +2524,22 @@ ***function ComposeLoad()***
>-  // Prevent resizing the subject and format toolbar over the addressswidget.
>-  var headerToolbar = document.getElementById("MsgHeadersToolbar");
>-  headerToolbar.minHeight = headerToolbar.boxObject.height;
>+  /* Initialize the default and minimum number of rows shown for addresswidget.
>+   * To prevent resizing the subject and format toolbar over the addressswidget,
>+   * and to avoid UX problems for awNumRowsShownMin=1, we default to awNumRowsShownMin=2;
>+   * but we enable users to maximize available space for composition body by setting
>+   * hidden pref for awNumRowsShownMin to 1 or 0, especially on small screens. */
>+  let awNumRowsShownMin = getPref("mail.compose.addresswidget.numRowsShownMin");
>+  let awNumRowsShownDefault = getPref("mail.compose.addresswidget.numRowsShownDefault");
>+  awSetNumberOfRowsShown(awNumRowsShownDefault, awNumRowsShownMin);
> }

Up to here everything looks good I think and there's nothing I could change.

>--- a/mail/components/compose/content/addressingWidgetOverlay.js
>+++ b/mail/components/compose/content/addressingWidgetOverlay.js
>@@ -39,14 +39,33 @@
>     if (!gNumberOfCols)
>       gNumberOfCols = 1;  /* if no cols defined, that means we have only one! */
>   }
> 
>   return gNumberOfCols;
> }
> 
>+function awSetNumberOfRowsShown(numRowsShownDefault, numRowsShownMin)
>+/* Function to adjust the default and minimum number of visible recipient rows for addressingWidget
>+ * via setting .height and .min-height attributes of MsgHeadersToolbar accordingly. */
>+{
>+  let msgHeadersToolbar = document.getElementById("MsgHeadersToolbar");
>+  let addressingWidget = document.getElementById("addressingWidget");
>+
>+  /* Set minimum number of rows shown for address widget. */
>+  addressingWidget.setAttribute("rows",numRowsShownMin.toString());
>+  msgHeadersToolbar.minHeight = msgHeadersToolbar.boxObject.height;
>+
>+  /* Set default number of rows shown for address widget. */
>+  addressingWidget.setAttribute("rows",numRowsShownDefault.toString());
>+  msgHeadersToolbar.height = msgHeadersToolbar.boxObject.height;

I tried commenting out the lines that have .setAttribute("rows"...) because I thought they might be triggering the occurence of Bug 671530 under unknown circumstances for recycled reply windows, and for testing purposes, just set plain values for the heights, like this:

  /* Set minimum number of rows shown for address widget. */
  //addressingWidget.setAttribute("rows",numRowsShownMin.toString());
  msgHeadersToolbar.minHeight = "84px"//msgHeadersToolbar.boxObject.height;
  
  /* Set default number of rows shown for address widget. */
  //addressingWidget.setAttribute("rows",numRowsShownDefault.toString());
  msgHeadersToolbar.height = "134px" //msgHeadersToolbar.boxObject.height;


>+  /* Update addressingWidget internals. */
>+  awCreateOrRemoveDummyRows();
>+}

With that change (no more rows="xx" changes on the addresswidget), the patch was still working as it should, but Bug 671530 of sometimes disabled recipients for recycled reply window was still there.

I reverted to the original omni.ja of 2013-06-17, and to my surprise, the same bug was still there.

From what I see, my conclusion is that my patch has nothing to do with triggering Bug 671530, and that the build of 2013-06-17 build is broken for that case.

So from that analysis, I don't know how to continue, because if that's true and I'm not seeing some sort of artefact related to my setup, there's nothing more I can do for this patch.

Any advice?
-----------------

I'd like to confirm what others see:

Does anybody see the problem of disabled recipients for certain recycled replies, Bug 671530 (STR here in Bug 425451 Comment 103) with or without my patch applied?

Does anybody see the problem described by rsx11m that setting the min-height does not work for him?

Comment 113

[Thomas D. (:thomas8)]

Could there be some stale leftover from an old composition window which wrongly gets recycled all over again?

Comment 114

[rsx11m]

(In reply to Thomas D. from comment #112)
> From what I see, my conclusion is that my patch has nothing to do with
> triggering Bug 671530, and that the build of 2013-06-17 build is broken for
> that case.

Thomas, I didn't suggest that, on the contrary. If bug 671530 occurs it's independent from your specific patch, thus should be taken care of within the scope of that bug. It is my understanding that window recycling didn't work for some time, was reinstated, and then the respective glitch showed up.

> So from that analysis, I don't know how to continue, because if that's true
> and I'm not seeing some sort of artefact related to my setup, there's
> nothing more I can do for this patch.

In the interest of time, I'd suggest to get a patch in before the weekend wraps up so that the feature is available, as it's certainly desirable to have this in the 24.0 branch. Remaining issues can be worked out during the aurora phase.

Thus, the relevant design decisions are (from my point of understanding):

(1) Is a preference wanted for the minimum number of rows to enforce?

(2) If not, should the minimum be one row (which may be odd to handle and the
    scrollbar won't work, but if that's what the user wants...) or two rows
    (with users potentially complaining that they cannot reduce to one row)

Comment 115

[Thomas D. (:thomas8)]

(In reply to rsx11m from comment #114)
> (In reply to Thomas D. from comment #112)
> > From what I see, my conclusion is that my patch has nothing to do with
> > triggering Bug 671530, and that the build of 2013-06-17 build is broken for
> > that case.
> 
> Thomas, I didn't suggest that, on the contrary. If bug 671530 occurs it's
> independent from your specific patch, thus should be taken care of within
> the scope of that bug. It is my understanding that window recycling didn't
> work for some time, was reinstated, and then the respective glitch showed up.

Yep. Bug 881588! Scrap everything which I said here on this bug about my patch not working blabla, it works very well as expected, and I was just seeing Bug 881588 and wrongly assumed that it was somehow related to my patch.

Comment 116

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

1) No.  2) One row.  Or maybe one and a bit, to let the scrollbar work…

Comment 117

[rsx11m]

Comment on attachment 765576 [details] [diff] [review]
Patch v4 (completely rewritten, streamlined code)

Sounds good to me, thus f+ with the minimum-height preference removed and fixed to one row or a little more per Blake's comment #116.

Comment 118

[Richard Marti (:Paenglab)]

Comment on attachment 765576 [details] [diff] [review]
Patch v4 (completely rewritten, streamlined code)

Also for me f+ with the minheight pref removed.

Your patch didn't apply cleanly on all-thunderbird.js. it seems you used the file from XPI for the diff and not the one from repository.

I'll upload a correct version of your patch including the needed header for check-in.

Comment 119

[Richard Marti (:Paenglab)]

Attachment: Patch that applies and with header

The same patch like the one from Thomas. This has still the minheight pref in it.

Comment 120

[Thomas D. (:thomas8)]

Attachment: Patch v5 (awNumRowsShownMin = 1 now hardwired)

forwarding bwinton's ui-r+ from attachment 700021 [details], see Comment 22 and comment 52 ff., together with comment 116.

Per :bwinton's terminating comment 116, hardwire minimum number of rows awNumRowsShownMin = 1 in the code (before this patch, it was hardwired to 4) which sets min-height of MsgHeadersToolbar accordingly. (Scrollbars only show up for n=2, and setting partial rows as min-height would look really ugly and unprofessional especially for the recipient-type dropdowns which would be cut into pieces.)

After the removal of the mail.compose.addresswidget.numRowsShownMin pref, the minimum number of rows can no longer be customized from within TB and not by userchrome.css either, because we set the correct size in the code to avoid undesired visual offsets depending on (OS) themes, as seen in screenshot of attachment 763124 [details]. My reading is that the css min-heights for #MsgHeadersToolbar which this patch removed from messengercompose.css never did anything because the min-height was still explicitly set in the code, following rows="4" attribute of addressingWidget (see Comment 91).

Letting users shrink addressingWidget to awNumRowsShownMin = 1 is delicate UX when in single-row state (which will default to 3 again for your next composition), but has the benefit of letting users maximize the space available for editing composition body, which is especially useful for users with small screens where the awkward design of header toolbar and addressing widget can easily use up 50% of vertical screen space, even in shrinked status.

So then, I hope this is now ready for inclusion.

Comment 121

[Thomas D. (:thomas8)]

Richard, could you pls verify if the patch still applies correctly and if my name comes out correctly with an "ü" aka u-umlaut as it does on my text editor; if that's not possible, pls use "ue" instead. Thanks! Also for nitfixing the previous one.

Comment 122

[Thomas D. (:thomas8)]

Attachment: Patch v5.1ü (awNumRowsShownMin = 1 now hardwired)

Patch V5.1ü (same as V5, with utf8 nitfix for u-umlaut)

Patch as annotated in Comment 120

Comment 123

[Richard Marti (:Paenglab)]

Comment on attachment 766135 [details] [diff] [review]
Patch v5.1ü (awNumRowsShownMin = 1 now hardwired)

The patch applies cleanly and also the ü is shown.

Comment 124

[Magnus Melin [:mkmelin]]

Comment on attachment 766135 [details] [diff] [review]
Patch v5.1ü (awNumRowsShownMin = 1 now hardwired)

Review of attachment 766135 [details] [diff] [review]:
-----------------------------------------------------------------

(qimportbz complains about the ü and won't import)

All the comments inside the function should be // style. /* */ comments are annoying there

So what's the point of the minheight setting? (except that of course min 1 rows should be there)

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +42,5 @@
>  
>    return gNumberOfCols;
>  }
>  
> +function awSetNumberOfRowsShown(numRowsShownDefault, numRowsShownMin)

why are these parameters to begin with? seems pointless

@@ +44,5 @@
>  }
>  
> +function awSetNumberOfRowsShown(numRowsShownDefault, numRowsShownMin)
> +/* Function to adjust the default and minimum number of visible recipient rows for addressingWidget
> + * via setting .height and .min-height attributes of MsgHeadersToolbar accordingly. */

function level documentation should be like
/**
 * Adjusts the minimum level of recipients rows in the addressingWidget.
 */

@@ +50,5 @@
> +  let msgHeadersToolbar = document.getElementById("MsgHeadersToolbar");
> +  let addressingWidget = document.getElementById("addressingWidget");
> +
> +  /* Set minimum number of rows shown for address widget. */
> +  addressingWidget.setAttribute("rows",numRowsShownMin.toString());

space after comma, and no need for toString()

@@ +54,5 @@
> +  addressingWidget.setAttribute("rows",numRowsShownMin.toString());
> +  msgHeadersToolbar.minHeight = msgHeadersToolbar.boxObject.height;
> +
> +  /* Set default number of rows shown for address widget. */
> +  addressingWidget.setAttribute("rows",numRowsShownDefault.toString());

space after comma, and no need for toString()

::: mail/components/compose/content/messengercompose.xul
@@ +807,5 @@
>                        oncommand="LoadIdentity(false);" disableonsend="true">
>                <menupopup id="msgIdentityPopup"/>
>              </menulist>
>            </hbox>
> +          <listbox id="addressingWidget" flex="1" seltype="multiple" rows="1"

rows="3" to begin with

Comment 125

[rsx11m]

(In reply to Magnus Melin from comment #124)
> So what's the point of the minheight setting? (except that of course min 1
> rows should be there)

It's needed to avoid that you can collapse to less than a row of height.

> > +  /* Set minimum number of rows shown for address widget. */
> > +  addressingWidget.setAttribute("rows",numRowsShownMin.toString());

This is indeed redundant, the rows="1" takes care of the initial height needed to measure it for setting the minheight attribute correctly.

> > +  msgHeadersToolbar.minHeight = msgHeadersToolbar.boxObject.height;

This is still needed.

> > +          <listbox id="addressingWidget" flex="1" seltype="multiple" rows="1"
> rows="3" to begin with

No, it has to be rows="1" to establish the minheight value.

Comment 126

[rsx11m]

(The other option being to explicitly calculate a single row's minheight, but that didn't quite work out as it depends on desktop theme and font used.)

Comment 127

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #124)
> Comment on attachment 766135 [details] [diff] [review]
> Patch v5.1ü (awNumRowsShownMin = 1 now hardwired)
> 
> Review of attachment 766135 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the review.

> (qimportbz complains about the ü and won't import)

So names on patches are limited to ANSI? Well, suppose I'll just replace ü with ue then...

> All the comments inside the function should be // style. /* */ comments are
> annoying there

Misunderstood your previous comment and tried to make it according to your comment. Is there a style guide to these things?
Fortunately, types of comments don't affect functionality of the code...

> So what's the point of the minheight setting? (except that of course min 1
> rows should be there)

Setting rows attribute on the listbox by itself does nothing.
Only setting min-height on MsgHeaderToolbar effectively sets the minimum of rows.
In old code, rows=4, and in that state, min-height of MsgHeaderToolbar was set accordingly, but not transparently (Code was fixing the minheight, but it was entirely undocumented where that height came from).
In my code, rows=X is ignored, instead using explicit setting awNumRowsShownMin=1, and to apply that, setting rows=1 temporarily, then min-height accordingly. That tweak is actually quite simple compared to the calculations otherwise needed to set minheight.
 
> ::: mail/components/compose/content/addressingWidgetOverlay.js
> @@ +42,5 @@
> >  
> >    return gNumberOfCols;
> >  }
> >  
> > +function awSetNumberOfRowsShown(numRowsShownDefault, numRowsShownMin)
> 
> why are these parameters to begin with? seems pointless

1) Transparency. I thought it's good style to get the variables in msgcompose.js so that their origin is visible where they are set. But the details of setting them are just clutter which I outsourced to awoverlay.js.
2) Flexibility. Having parameters means we or others could reuse the function with different values. If we get the parameters inside the function, I think it's less versatile. I don't know for addons in terms of customizing this, is it easier for them when values are in- or outside the function?


> @@ +44,5 @@
> >  }
> >  
> > +function awSetNumberOfRowsShown(numRowsShownDefault, numRowsShownMin)
> > +/* Function to adjust the default and minimum number of visible recipient rows for addressingWidget
> > + * via setting .height and .min-height attributes of MsgHeadersToolbar accordingly. */
> 
> function level documentation should be like
> /**
>  * Adjusts the minimum level of recipients rows in the addressingWidget.
>  */

Oh. Again, I did not understand that from your previous comment. 
Should that comment be in- or outside the function?

> @@ +50,5 @@
> > +  let msgHeadersToolbar = document.getElementById("MsgHeadersToolbar");
> > +  let addressingWidget = document.getElementById("addressingWidget");
> > +
> > +  /* Set minimum number of rows shown for address widget. */
> > +  addressingWidget.setAttribute("rows",numRowsShownMin.toString());
> 
> space after comma, and no need for toString()

For perfect style, sure. 

> @@ +54,5 @@
> > +  addressingWidget.setAttribute("rows",numRowsShownMin.toString());
> > +  msgHeadersToolbar.minHeight = msgHeadersToolbar.boxObject.height;
> > +
> > +  /* Set default number of rows shown for address widget. */
> > +  addressingWidget.setAttribute("rows",numRowsShownDefault.toString());
> 
> space after comma, and no need for toString()
> 
> ::: mail/components/compose/content/messengercompose.xul
> @@ +807,5 @@
> >                        oncommand="LoadIdentity(false);" disableonsend="true">
> >                <menupopup id="msgIdentityPopup"/>
> >              </menulist>
> >            </hbox>
> > +          <listbox id="addressingWidget" flex="1" seltype="multiple" rows="1"
> 
> rows="3" to begin with

You can't have both - if you don't want a minrows variable, then we need to set rows to 1 and then fix the min-height value from there. If you want initial rows=3 there (although rows are not directly relevant for min-height nor height in the current code), then we need minrows=1 variable in the code unless we want the clumsy calculations again. I find it confusing to set rows=3 here because we'd wrongly suggest that's where the default height comes from...
Equally, I don't think it's good to take the min-height from initial rows=1, because it's not obvious why setting that attribute should affect min-height.
But perhaps it's better for addons to do it that way.

I'm getting tired of this, and literally tired because I've already spent lots of time on this. The patch works as expected, it's clear and transparent and well-documented, and I don't see anything grossly inappropriate about it. But we can also continue fixing comment styles and attributes that aren't used until the deadline for TB24 is over.

Comment 128

[Thomas D. (:thomas8)]

Alternatively, more than 5 years after this bug has been reported, perhaps we could land the well-working fix now as-is to ensure it actually lands for TB24, and then optimize whatever we need for perfection in style lateron.

Comment 129

[Magnus Melin [:mkmelin]]

(In reply to Thomas D. from comment #127)
> (In reply to Magnus Melin from comment #124)
> > Comment on attachment 766135 [details] [diff] [review]
> > Patch v5.1ü (awNumRowsShownMin = 1 now hardwired)
> > 
> > Review of attachment 766135 [details] [diff] [review]:
> > -----------------------------------------------------------------
> 
> Thanks for the review.
> 
> > (qimportbz complains about the ü and won't import)
> 
> So names on patches are limited to ANSI? Well, suppose I'll just replace ü
> with ue then...

I doubt it. Didn't have time to try manual download and apply yet - might be a bug in qimportbz.

> > > +function awSetNumberOfRowsShown(numRowsShownDefault, numRowsShownMin)
> > 
> > why are these parameters to begin with? seems pointless
> 
> 1) Transparency. I thought it's good style to get the variables in
> msgcompose.js so that their origin is visible where they are set. But the
> details of setting them are just clutter which I outsourced to awoverlay.js.

As it's just used locally, please move getting the prefs inside the function.

> 2) Flexibility. Having parameters means we or others could reuse the
> function with different values. If we get the parameters inside the
> function, I think it's less versatile. I don't know for addons in terms of
> customizing this, is it easier for them when values are in- or outside the
> function?

I don't see why anyone would change the parameters, and if they would they would just do it by changing the pref. Or overlay the whole function (which is certainly small enough).

> > function level documentation should be like
> > /**
> >  * Adjusts the minimum level of recipients rows in the addressingWidget.
> >  */
> 
> Oh. Again, I did not understand that from your previous comment. 
> Should that comment be in- or outside the function?

Outside the function. I don't know if there's a mozilla documentation style guide explicitly somewhere - but it should be doxygen (almost the same as javadoc) so google those.

> I'm getting tired of this, and literally tired because I've already spent
> lots of time on this. The patch works as expected, it's clear and
> transparent and well-documented, and I don't see anything grossly
> inappropriate about it. But we can also continue fixing comment styles and
> attributes that aren't used until the deadline for TB24 is over.

Understood, everybody has something they want to make sure gets into 24. I don't think its worth fixing later as the patch could equally well land once, and be approved to branch later.

Comment 130

[Thomas D. (:thomas8)]

Attachment: Patch V6

Address review comments of comment 124 and comment 125.

Removed parameters from awSetNumRowsShown() function, which thus becomes awInitializeNumRowsShown() function.

Removed explicit awNumRowsShownMin=1 setting from function which I understand was considered "pointless" by comment 124 and "redundant" by comment 125, and return to implicit setting of minimum number of rows via rows="1" setting of addressingWidget listbox (and then setting minHeight from that), same method as before the patch, but now documented to avoid further confusion about this in the future.

If there's anything else still, pls tell me exactly what I should change, or even change it yourself. This patch fixes a major annoyance of composition window, so we should really try to get this landed for TB24. I'll be away for the rest of today.

Richard, would you kindly pre-check if this still applies correctly and works as expected, setting address widget default number of rows to 3 and minimum number of rows to 1 when you shrink it manually.

Comment 131

[Thomas D. (:thomas8)]

Comment on attachment 766560 [details] [diff] [review]
Patch V6

fwd ui-r+

Comment 132

[Magnus Melin [:mkmelin]]

Comment on attachment 766560 [details] [diff] [review]
Patch V6

Review of attachment 766560 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2528,5 @@
>    var toolbarset = document.getElementById('customToolbars');
>    toolbox.toolbarset = toolbarset;
>  
> +  // Initialize the default and minimum number of rows shown for address widget, especially
> +  // to prevent resizing the subject and format toolbar over the address widget.

This comment should be moved to where minHeight is set

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +46,5 @@
> +function awInitializeNumberOfRowsShown()
> +/**
> + * Adjust the default and minimum number of visible recipient rows for addressingWidget
> + * via setting .height and .min-height attributes of MsgHeadersToolbar accordingly.
> + */

Put the documentation outside the function, please.

Also, you're not setting the attributes, you're setting the height and minHeight properties

@@ +54,5 @@
> +  let awNumRowsShownDefault = getPref("mail.compose.addresswidget.numRowsShownDefault");
> +
> +  // Set minimum number of rows shown for address widget, per hardwired rows="1" attribute of addressingWidget,
> +  // which lets users shrink the address widget to one row (with delicate UX) and maximize the space
> +  // available for composition body, especially on small screens. 

trailing space

::: mail/components/compose/content/messengercompose.xul
@@ +807,5 @@
>                        oncommand="LoadIdentity(false);" disableonsend="true">
>                <menupopup id="msgIdentityPopup"/>
>              </menulist>
>            </hbox>
> +          <!-- rows="1" of addressingWidget listbox is used for setting minHeight of MsgHeadersToolbar -->

don't think this is needed

Comment 133

[Magnus Melin [:mkmelin]]

Also getPref isn't in that file, so please put 
Components.utils.import("resource://gre/modules/Services.jsm"); 
on top of the file, then use Services.prefs.getIntPref instead

Comment 134

[:aceman]

Suyash, can you quickly fix the comments (and comment 133) so that Magnus can review it and it gets landed?

Comment 135

[Richard Marti (:Paenglab)]

Comment on attachment 766560 [details] [diff] [review]
Patch V6

The patch applies correctly. I can't check the functionality until this evening. To not hold the further process I give the f+

Comment 136

[Thomas D. (:thomas8)]

Attachment: Patch V6.1

(In reply to Magnus Melin from comment #132)
> Comment on attachment 766560 [details] [diff] [review]
> Patch V6
> Review of attachment 766560 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for rapid review, I appreciate that especially at this time where everybody has something they want to make sure gets into 24... :)

This patches addresses comment 129 and comment 132, otherwise same as patch V6 described in comment 130.

Comment 137

[:aceman]

What about comment 133?
In addressingWidgetOverlay.js Services.jsm and getPref are missing. If if still works for you, they may be inherited from messengercompose.xul. But they may not work if included into some other file.

Comment 138

[Richard Marti (:Paenglab)]

Comment on attachment 766575 [details] [diff] [review]
Patch V6.1

f+ only for applying flawlessly.

Comment 139

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #137)
> What about comment 133?
> In addressingWidgetOverlay.js Services.jsm and getPref are missing. If if
> still works for you, they may be inherited from messengercompose.xul. But
> they may not work if included into some other file.

I'm constantly having mid-air-collisions with :mkmelin's comments ;) I only got most of his last comments too late after filing the next patch based on earlier comments, and I'm not at my home computer where my Thunderbird is so I'm not seeing alerts for incoming bugmail either...

Comment 140

[Thomas D. (:thomas8)]

Attachment: Patch V7

fix problems of patch V6: comment 133, and a variable mismatch

I just modified per comment 133, but I can't test if the patch is working as-is.

I'm a bit irritated by the difference in the base path for these two lines:
+Components.utils.import("resource://gre/modules/Services.jsm");
 Components.utils.import("resource:///modules/mailServices.js");

Pls test and if there's still anything wrong, pls somebody fix it.

Comment 141

[Thomas D. (:thomas8)]

/away/

Comment 142

[:aceman]

Comment on attachment 766595 [details] [diff] [review]
Patch V7

Review of attachment 766595 [details] [diff] [review]:
-----------------------------------------------------------------

The difference in base path is correct, as Services is a Core module (e.g. for Firefox too), while mailServices is just mailnews specific. So they are stored in different places.

The change looks good. I also can't run this in the next hours so I asked Suyash to check this if he can.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +55,5 @@
> +
> +  // Set minimum number of rows shown for address widget, per hardwired rows="1" attribute of addressingWidget,
> +  // to prevent resizing the subject and format toolbar over the address widget.
> +  // This lets users shrink the address widget to one row (with delicate UX) and thus maximize the space
> +  // available for composition body, especially on small screens.

These lines should be wrapped at 80 chars.

Comment 143

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 766595 [details] [diff] [review]
Patch V7

Review of attachment 766595 [details] [diff] [review]:
-----------------------------------------------------------------

Thomas, nice work :)
This feedback is on behalf on aceman sir.

The patch doesn't apply properly on my system;
the error is: abort: decoding near 'Thomas D�llmann <b': 'utf8' codec can't decode byte 0xfc in position 8: invalid start byte!
but fixing the u in your surname works.

What I see is, in the composition window, the default is 3 lines for the recipients, shrink and expand works perfectly :)
But when the visible field is reduced to 1, the scroll doesn't work properly, but I don't think that should be a drastic issue preventing this from landing.

So, if my observations are what is desired out of the patch, f=Suyash :)
And, please wrap the lines as aceman sir mentioned.

Thanks.

Comment 144

[rsx11m]

(In reply to Suyash Agarwal (:sshagarwal) from comment #143)
> But when the visible field is reduced to 1, the scroll doesn't work properly,

That what I've observed earlier as well, the scrollbar disappears when the remaining height does no longer fit the up/down arrows, thus the actual height when this happens depends on the theme. However, if the user moves it manually he or she will see the effect, and if the preference is modified the solution to this issue (i.e., n=2) is obvious as well, thus shouldn't pose a real usability issue.

> So, if my observations are what is desired out of the patch, f=Suyash :)
> And, please wrap the lines as aceman sir mentioned.

It is my understanding that Thomas potentially won't be available before the merge happens to take care of this.

Comment 145

[Suyash Agarwal (:sshagarwal)]

(In reply to rsx11m from comment #144)
> It is my understanding that Thomas potentially won't be available before the
> merge happens to take care of this.
No problems, I am doing this on his behalf.

Comment 146

[:aceman]

(In reply to Suyash Agarwal (:sshagarwal) from comment #145)
> No problems, I am doing this on his behalf.
So could you rewrap the comments and attach new V7.1 with Thomas's headers preserved so that Magnus can review it and then check in quickly?

Comment 147

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v7.1

On behalf of Thomas.

Comment 148

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #146)
> So could you rewrap the comments and attach new V7.1 with Thomas's headers
> preserved so that Magnus can review it and then check in quickly?

Done sir.

Comment 149

[:aceman]

Comment on attachment 766635 [details] [diff] [review]
Patch v7.1

Review of attachment 766635 [details] [diff] [review]:
-----------------------------------------------------------------

Could you please change the "ü" to "ue" as it caused the problems. And please
change the patch description to:
"Bug 425451 - make the message compose addressing widget defaults to 3 lines or recipients and make it resizable."

Sorry for not noticing before.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +50,5 @@
> +function awInitializeNumberOfRowsShown()
> +{
> +  let msgHeadersToolbar = document.getElementById("MsgHeadersToolbar");
> +  let addressingWidget = document.getElementById("addressingWidget");
> +  let awNumRowsShownDefault = Services.prefs.getIntPref("mail.compose.addresswidget.numRowsShownDefault");

Please wrap this after the "=" sign and indent the wrapped line by 2 spaces.

@@ +52,5 @@
> +  let msgHeadersToolbar = document.getElementById("MsgHeadersToolbar");
> +  let addressingWidget = document.getElementById("addressingWidget");
> +  let awNumRowsShownDefault = Services.prefs.getIntPref("mail.compose.addresswidget.numRowsShownDefault");
> +
> +  // Set minimum number of rows shown for address widget, per hardwired 

Trailing space left.

@@ +56,5 @@
> +  // Set minimum number of rows shown for address widget, per hardwired 
> +  // rows="1" attribute of addressingWidget, to prevent resizing the
> +  // subject and format toolbar over the address widget.
> +  // This lets users shrink the address widget to one row (with delicate UX)
> +  // and thus maximize the space available for composition body, especially on small screens.

This line is still long.

Comment 150

[:aceman]

"or" = of ...

Comment 151

[Magnus Melin [:mkmelin]]

Comment on attachment 766635 [details] [diff] [review]
Patch v7.1

Review of attachment 766635 [details] [diff] [review]:
-----------------------------------------------------------------

Can't test this atm, but since others have. 
r=mkmelin (if someone have time fix the nits aceman had, otherwise lets roll)

Comment 152

[:aceman]

Attachment: patch v7.2 [to be checked in]

Carrying over r=mkmelin, ui-r=bwinton .

Comment 153

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/9311b3990406

Comment 154

[:aceman]

Verified as working on trunk.

Comment 155

[Thomas D. (:thomas8)]

Fwiw, bug 535484 is about the auto-expansion which didn't make it here.
But it's probably just another stop-gap until we normalize our recipients area per

Bug 440377: Move away from the one-line-per-recipient paradigm and do like most others do, multiple recipients on a single line.

Comment 156

[Thomas D. (:thomas8)]

I don't see this bug on the list of bugs fixed for TB24 (1)

Does that mean that it is not included in TB24, or the list is incomplete?
Do we have to request some flag to get this included?

(1) https://bugzilla.mozilla.org/buglist.cgi?bug_status=RESOLVED&chfield=cf_tracking_thunderbird18&chfield=cf_tracking_thunderbird19&chfield=cf_tracking_thunderbird20&chfield=cf_tracking_thunderbird21&chfield=cf_tracking_thunderbird22&chfield=cf_tracking_thunderbird23&chfield=cf_tracking_thunderbird24&chfieldto=Now&query_format=advanced&resolution=FIXED&target_milestone=Thunderbird%2018.0&target_milestone=Thunderbird%2019.0&target_milestone=Thunderbird%2020.0&target_milestone=Thunderbird%2021.0&target_milestone=Thunderbird%2022.0&target_milestone=Thunderbird%2023.0&target_milestone=Thunderbird%2024.0&order=bug_id%20DESC&limit=0&list_id=7520552

Comment 157

[Magnus Melin [:mkmelin]]

It's included, but the query is missing VERIFIED bugs.
(The tracking flags shouldn't also be included - likely you need only status-thunderbird24=fixed of the flags)

Comment 158

[rsx11m]

target_milestone=Thunderbird%2024.0 matches but bug_status=RESOLVED won't, thus you don't see it. If you want to double-check, just download a tinderbox build from thunderbird/tinderbox-builds/comm-beta-win32 and you should see what's included.

Comment 159

[Thomas D. (:thomas8)]

For future reference, add to summary what we actually did here, which also makes this easier to retrieve.

Comment 160

[Geoffroy Ménard]

Changing mail.compose.addresswidget.numRowsShownDefault does not affect number of lines displayed (which is always one) on my Thunderbird 31.4.0.
Neither does the AdressWidgetLines add-on.

Comment 161

[rsx11m]

Wrong bug, this one is about message *composition* not display (that was bug 550487 and friends).
You'll have to modify mailnews.headers.show_n_lines_before_more instead, which defaults to 1.

Also, please don't comment on bugs which have long been closed. Use the support forums first if you have a problem, https://support.mozilla.org/questions/thunderbird which is the Mozilla-run site, or for independent user-to-user support, http://forums.mozillazine.org/viewforum.php?f=39 is another option.