1666463 - If many recipients, composition's addressing area vertical size increases without limit, shrinking body height to near-zero.

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect, P2)

Description

[[:AlanK]]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:80.0) Gecko/20100101 Firefox/80.0

Steps to reproduce:

When adding recipients to the TO: field, the more you add, the more the TO: window grows and shrinks the composition window.
Also if doing a reply all to an email that has a large number of recipients, it too products a very large recipients window and almost if not no compose window.

Actual results:

See above. No room or little room to compose.

Expected results:

The number of recipient lines should be limited to a few and have a scroll bar.
It works okay in version 68.
Changing mail.compose.addresswidget.numRowsShownDefault in config editor does not adjust the # of rows as it should.

Comment 1

[Thomas D. (:thomas8)]

Attachment: Screenshot 1: Addressing area knows no height limit

Confirming. I added 5 addresses each as CSV text, then pressed Enter to convert them to pills, repeat about 10 times. This is the result. Addressing area grows all the way down. Ironically, when you add another 5 now, it will suddenly remember to shrink to a healthy height. Definitely something wrong with our height algorithm here. Note that we do want to allow the user to drag to any height temporarily, to see more addresses and/or more attachments. However, I don't see how that would stop us from applying a sane default height beyond which we would start to show scroll bars by default without blocking the user from manually changing the height.

Comment 2

[Thomas D. (:thomas8)]

Alex, I think the original mechanism which was put in place to control the height of the addressing area is broken. It collapses to a healthy height only if you add even more addresses after it has already shrinked the msg body space away. That's too late. Whilst this can be cured by dragging the body big again, it does not make us look good.

Comment 3

[Alessandro Castellani [:aleca]]

This was working before, something must have regressed it.

Comment 4

[Thomas D. (:thomas8)]

Alice, the addressing area is supposed to stop expanding when many addresses are added at around 40% to 50% of window height, but now it doesn't. Regression window would be great. I guess you can just have all addresses (or a third of them) in clipboard as CSV, paste, press Enter. Or to save a draft msg in the test profile with the full large number of recipients and then edit that, which should also limit the default size of the addressing area and does not according to reporter (or else reply to such message).

Comment 5

[Onno Ekker [:nONoNonO UTC+1]]

It looks like this never worked quite right... In latest Daily when I keep adding addresses the addressing area expands until it about covers all of the window and then after adding one more address jumps back to about 40%.

In Thunderbird 73.0a1, where the addressing pills were first introduced, the behavior was kind of the same, but the area jumped back when about 75% of the window was used.

Comment 6

[Thomas D. (:thomas8)]

(In reply to Onno Ekker [:nONoNonO UTC+1] from comment #5)

In Thunderbird 73.0a1, where the addressing pills were first introduced, the behavior was kind of the same, but the area jumped back when about 75% of the window was used.

I'm failing to see the design point of "jumping back" which must appear random to user, and it's not very effective because if it's one pill less than what triggers the jump, the oversized header will continue to hang around. We should agree on a height that we don't exceed by default (and which should probably be customizable as it was before), but allow the user to override that with dragging the splitter either direction.

We could experiment with jumping when the area loses focus, but it can cause other problems, like when you start a selection in body with mouse right away and whilst you are still selecting, the body suddenly jumps away, which doesn't sound good.

Comment 7

[Alice0775 White]

https://hg.mozilla.org/comm-central/pushloghtml?fromchange=c10cbc3c87fb2004d7ca3ae2525cf4be48bf377c&tochange=019eaf0f9cf72621a436ce06b2607bc5a3532fae
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d6d7d85a1b56cd92f74447d28381423d6980cd92&tochange=192e0e33eb597e8d923eb89f6d49bf42654e9d11

Before landing Bug 440377,
Default number of recipients address fields row is 3.
And Scroll bar appears if the recipients address fields is overflowed.

After landing Bug 440377,
The height of the recipients address fields will automatically increase until the message body field reaches height of 100px and eroding the status-bar of the compose window.
And then the recipients address fields is reduced to 9 rows with scrollbar (i.e. jumping back).

Comment 8

[Onno Ekker [:nONoNonO UTC+1]]

Attachment: Addressing widget expanded below message area.png

Hmz, in Thunderbird 78.2.2 it's even worse for me. The area didn't jump back when reaching 90%, but instead I could keep adding (invisible) addresses, that were added behind the composition area and eventually became visible again under the composition area.

I think it makes sense to add a max-height to the addresses-box of, let's say, 6 lines... One for the sender, one for the subject and four for the recipients (and/or reply-to address) and add a scrollbar if more ilnes are needed.

Comment 9

[[:AlanK]]

What about the about:config setting that is supposed to let the user adjust what the max height is???

mail.compose.addresswidget.numRowsShownDefault defaults to 2

Comment 10

[Alessandro Castellani [:aleca]]

mail.compose.addresswidget.numRowsShownDefault defaults to 2

That pref is not used anymore, not sure if it's worth keeping it as the current implementation doesn't really use rows.

I think it makes sense to add a max-height to the addresses-box of, let's say, 6 lines

With the pills, the concept of lines is very fluid as the amount of pills that can fit in one line variates based on each user screen size and how big they make the compose window.
That's why the code I implemented keeps in mind the current height of the compose window, and converts the area into a scrollable container when the content grows bigger that a specific %, and this inconsistent behaviour is unexpected.

Anyway, I'm sure all of this is a symptom of my crappy code, so, hold on while I fix it and apologies for the inconvenience.

Comment 11

[Alessandro Castellani [:aleca]]

Attachment: 1666463-recipients-area.diff

This fixes everything, it prevents visual jumps, and adds the scrollbar at around 20% of the full height of the window composer.
You'll need to load the patch in bug 1667173 if you're planning to use the drag and drop from the Address Book sidebar.

Comment 12

[Alessandro Castellani [:aleca]]

Hey Thomas, do you have time for this review?

Comment 13

[Thomas D. (:thomas8)]

Comment on attachment 9177734 [details] [diff] [review]
1666463-recipients-area.diff

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

Thank you for looking into this! This is an improvement, but we also need to fix those cases where a composition gets opened with many recipients (Edit draft, reply all, edit as new etc.).
Also there's a variant of bug 1667173 which isn't covered yet: Pasting string of CSV which is longer than the rest of the addressing row will expand the row horizontally beyond the screen. Guess they could share the same resizing code in a function. Or maybe we could just listen for input event on the input and attach the resizing there, which hopefully also covers the drop.

Comment 14

[Alessandro Castellani [:aleca]]

Thank you for looking into this! This is an improvement, but we also need to fix those cases where a composition gets opened with many recipients (Edit draft, reply all, edit as new etc.).

I'll take care of that.

Also there's a variant of bug 1667173 which isn't covered yet

Would you be able to tackle that in a dedicated bug?

Comment 15

[Thomas D. (:thomas8)]

(In reply to Alessandro Castellani (:aleca) from comment #14)

Also there's a variant of bug 1667173 which isn't covered yet

Filed bug 1667602 for that.

Comment 16

[Alessandro Castellani [:aleca]]

Attachment: 1666463-recipients-area.diff

All right, this should take care of every possible scenario.

Comment 17

[Thomas D. (:thomas8)]

Comment on attachment 9178592 [details] [diff] [review]
1666463-recipients-area.diff

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

Thank you Alex, this works very well!

But I'm worried about performance because we are re-calculating the addressing area height for every single pill which gets added, which doesn't scale well for hundreds of recipients.
I tried 500, and without the patch it feels less than a second, with the patch, between 1 and 2 secs.
Calculating *after* adding *all* pills is fine, we just need to tweak the max height thing a bit to work on the entire header.
To ease your workload and test my proposal, I have an updated version of your patch.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +749,5 @@
>      recipientArea.createRecipientPill(element, address);
> +    // Calculate the header height for every created pill in order to properly
> +    // add the scrollable overflow when multiple pills are created at once, like
> +    // when opening drafts or reply and reply-all actions.
> +    calculateHeaderHeight();

Sorry, but this is way to expensive. We cannot re-calculate header height for every single pill, what if there are 200, 500, or even 1000 pills? It's also not needed to get the desired effect. So this line must remain where it came from.

@@ -771,5 @@
>        .classList.add("addressing-field-edited");
>      onRecipientsChanged();
>    }
>  
> -  calculateHeaderHeight();

This must remain here.

@@ +1211,5 @@
>    }
>  }
>  
>  /**
> + * Calculate the height of the composer header area when a pill is created or

We won't do that for every pill, so: ...when pills are created or

@@ +1212,5 @@
>  }
>  
>  /**
> + * Calculate the height of the composer header area when a pill is created or
> + * deleted in order to autoamtically add or remove the scrollable overflow.

automatically

@@ +1237,5 @@
>      header.removeAttribute("height");
>      return;
>    }
>  
> +  let maxHeight = window.outerHeight * 0.2;

I think what we really want is to define a maximum height for the entire header so that the body doesn't get squeezed. Then it must be a bit more like before, 0.3, which is now headerMaxHeight. That simplifies the entire logic a lot, rather than splitting the logic between two different containers.

@@ +1240,5 @@
>  
> +  let maxHeight = window.outerHeight * 0.2;
> +  // Add overflow in case the recipients container height grows above 20% of the
> +  // window height.
> +  if (container.clientHeight > maxHeight) {

if (header.clientHeight > maxHeaderHeight) {

@@ +1242,5 @@
> +  // Add overflow in case the recipients container height grows above 20% of the
> +  // window height.
> +  if (container.clientHeight > maxHeight) {
> +    if (!header.hasAttribute("height")) {
> +      header.setAttribute("height", header.clientHeight);

We can no longer use the header.clientHeight here as we're not calculating for every pill, so this must become headerMaxHeight instead.

header.setAttribute("height", headerMaxHeight);

@@ -1236,4 @@
>      container.classList.add("overflow");
> -
> -    if (!header.hasAttribute("height")) {
> -      header.setAttribute("height", 300);

Yeah, absolute heights were not a good idea.

Comment 18

[Thomas D. (:thomas8)]

Attachment: 1666463-recipients-area.diff

Modified version of Alex' patch per my review of comment 17.
Please check it out!

Comment 19

[Thomas D. (:thomas8)]

Note to self: Realize more performance improvements in a new bug.
--> Bug 1668848 - Cleanup, rectify and improve readibility and performance of recipientAddPills()

Comment 20

[Alessandro Castellani [:aleca]]

Comment on attachment 9179160 [details] [diff] [review]
1666463-recipients-area.diff

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

Looks good, thanks for the thorough review and for updating the patch.
You can give it an r+ if you want.

Comment 21

[Thomas D. (:thomas8)]

Comment on attachment 9179160 [details] [diff] [review]
1666463-recipients-area.diff

Alex' patch with the improvements from my review in comment 17, double-checked/ui-r+ by Alex per comment 20.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/551da1084bf9
Fix max height of recipients area in the compose window. r=thomas8

Comment 23

[Magnus Melin [:mkmelin]]

Comment on attachment 9179160 [details] [diff] [review]
1666463-recipients-area.diff

[Approval Request Comment]
Pills polish, should be safe.

Comment 24

[Wayne Mery (:wsmwk)]

Comment on attachment 9179160 [details] [diff] [review]
1666463-recipients-area.diff

[Triage Comment]
Approved for beta

Comment 25

[Rob Lemley [:rjl]]

Thunderbird 82.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/40e1a122a79e

Comment 26

[Rob Lemley [:rjl]]

Thunderbird 82.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/40e1a122a79e

Comment 27

[WaltS48 [:walts48]]

Looks good to me testing the 82.0b3 release candidate on Ubuntu 18.04 LTS.

Comment 28

[Wayne Mery (:wsmwk)]

Comment on attachment 9179160 [details] [diff] [review]
1666463-recipients-area.diff

[Triage Comment]
Approved for esr78

Comment 29

[Rob Lemley [:rjl]]

Thunderbird 78.4.0:
https://hg.mozilla.org/releases/comm-esr78/rev/c9e78086a0db