1652410 - Cc recipients needlessly not expanded

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Jorg K (CEST = GMT+2)]

In a received e-mail the Cc recipients aren't displayed in the header pane although there is enough space. See screenshots.

Alice, could you find the regression?

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 2020-07-13 11_02_37-Window.png

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 2020-07-13 11_01_36-Window.png

Comment 3

[Alice0775 White]

STEPS:

1. Reduce TB width so that CC field will be collapsed
2. Select an email which has multi CCs in thread pane
   --- observe, CC field is collapsed as expected
3. Select an email which has no CC in thread pane
4. Expand TB width
5. Select an email of step 2 again
   --- observe, CC field would not be expanded, still collapsed

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=3d544985b7296fd45fcd15123e607acf58bc1057&tochange=284f2743822fb496e79f927afcfa23dc4ee7c5cb
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=79b08a0a1c3fa2834e668b205fd69ff79c8630d1&tochange=c31591e0b66f277398bee74da03c49e8f8a0ede0

Comment 4

[Jorg K (CEST = GMT+2)]

Thanks, Alice, this is very helpful.

Comment 5

[Khushil Mistry [:khushil324]]

Do you see the similar problem with "To" field or it's just "CC" field?

Comment 6

[Jorg K (CEST = GMT+2)]

I didn't check, I just came across this in the morning for the Cc field.

Comment 8

[Jorg K (CEST = GMT+2)]

Note the analysis in bug 1646313 comment #0.

Comment 9

[Khushil Mistry [:khushil324]]

Attachment: Bug-1652410_mail-multi-emailheaderfield-not-expanded-0.patch

Comment 10

[Alessandro Castellani [:aleca]]

Comment on attachment 9163307 [details] [diff] [review]
Bug-1652410_mail-multi-emailheaderfield-not-expanded-0.patch

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

Granted that I don't have a full overview on this area, I don't think that's the proper solution to this problem.

First, manually going up 2 parent nodes is not great since we assume the HTML structure will always be the same, so this can break easily at any change.
The better location for this would be here: https://searchfox.org/comm-central/rev/923942d4a9ac2cccff4922c0b14d2d19c8c23876/mail/base/content/msgHdrView.js#1361, where we have that ID stored in a property we can easily access, therefore doing something like: `headerEntry.enclosingRow.removeAttribute("hidden");`, so at least we know we always reference the correct parent node.

But, as I said, I'm not sure that's the proper fix for this.
We already have the `updateExpandedView()` method, called here: https://searchfox.org/comm-central/rev/923942d4a9ac2cccff4922c0b14d2d19c8c23876/mail/base/content/msgHdrView.js#1019, which takes care of showing/hiding the nodes that come with addresses.

Could be that we're calling it too late?
Since we rely on a clientWidth calculation and that parent node needs to be not hidden in order for us to properly determine if we have enough space to show all the addresses.

These are just speculations, but I think there might be a problem here with the order in which we're triggering these methods.
Should be something like: SHOW header fields that have addresses > POPULATE those fields with addresses > TOGGLE the "more" in case we don't have enough space.

Am I wrong?

Comment 11

[Khushil Mistry [:khushil324]]

Check this out: https://searchfox.org/comm-central/source/mail/base/content/msgHdrView.js#963,966,1019,1028,1222-1223,1240
UpdateExpandedMessageHeaders function renders the header according to the message. We loop over all the header entries and set the headerEntry.valid and after the loop completion, we call updateExpandedView function to set the row visible which uses headerEntry.valid for the decision on showing the row. In the same loop, we populate fields with addresses. At this time, we need client width but rows are hidden.

Here we have two choices:

1. we don't populate those fields in the 1st loop, we just decide on headerEntry.valid part and run the second loop after calling updateExpandedView function to populate the addresses. It will add an extra loop and an extra array object.
2. If we want to avoid the 2nd loop and extra array object, we can simply call headerEntry.enclosingRow.toggleAttribute("hidden", false); on those fields before populating the addresses.

I am submitting the patch with the 2nd approach as I think we should not make it more complex.
If you think approach 1 is better then we can go with that also, it's not much work.

Comment 12

[Khushil Mistry [:khushil324]]

Attachment: Bug-1652410_mail-multi-emailheaderfield-not-expanded-1.patch

Comment 13

[Alessandro Castellani [:aleca]]

Comment on attachment 9163439 [details] [diff] [review]
Bug-1652410_mail-multi-emailheaderfield-not-expanded-1.patch

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

Yeah, I think this approach is correct since we're showing the row before populating the data.
At this point we can delete the `showHeaderView()` method since it was only used for this purpose.

::: mail/base/content/msgHdrView.js
@@ +1219,5 @@
>          // message isn't a newsgroup posting.
>          headerEntry.valid = false;
>        } else {
> +        // This ensures table row element is visible to find the available width
> +        // when it populates field with addresses.

Update the comment with:
"Set the row element visible before populating the field with addresses."

Comment 14

[Alessandro Castellani [:aleca]]

Attachment: Screen record from 2020-07-14 10.40.43.gif

I noticed another small issue.
It seems that the available space is not properly calculated when the message pane turns visible for the first time.

You can see that the space is correct if I switch between messages, but if I switch from account central to inbox, the last address is cut off.
This happens also with this patch applied.

Comment 15

[Khushil Mistry [:khushil324]]

Attachment: Bug-1652410_mail-multi-emailheaderfield-not-expanded-2.patch

Comment 16

[Alessandro Castellani [:aleca]]

It looks good and fixes the issues.
What about this comment I wrote before?

At this point we can delete the showHeaderView() method since it was only used for this purpose.

Did you explore the possibility to remove that method? It seems to not be necessary with the changes.
Cheers

Comment 17

[Khushil Mistry [:khushil324]]

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

At this point we can delete the showHeaderView() method since it was only used for this purpose.

Did you explore the possibility to remove that method? It seems to not be necessary with the changes.
Cheers

Ohh, Sorry. Missed this comment. Updating the patch ASAP.

Comment 18

[Khushil Mistry [:khushil324]]

showHeaderView() is necessary to hide other invalid/not presented headers.
Here, we are doing things to show the headers which are presented but at one point, we are showing all the headers and then hide them! Strange!

Comment 19

[Khushil Mistry [:khushil324]]

In the function UpdateExpandedMessageHeaders, we are looping through all the available message headers of the current message. So we need to hide rows which are shown with the previous message that's why we need showHeaderView(). Basically, now it's behaving as hiding the unwanted rows.

Comment 20

[Alessandro Castellani [:aleca]]

Comment on attachment 9163695 [details] [diff] [review]
Bug-1652410_mail-multi-emailheaderfield-not-expanded-2.patch

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

Sounds good, thanks for investigating and for the report.

Launch a try-run just to be sure all the tests pass.

Comment 21

[Alessandro Castellani [:aleca]]

Unfortunately, the build was busted due to a recent patch from m-c.
I fixed the build, so you need to pull the latest changesets and launch another try run, sorry :D

Also, for future reference, is always good to add the link to the try-run in a comment here in the bug report, so it's easier to find.
Cheers

Comment 22

[Khushil Mistry [:khushil324]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=050f76d2a67356536445f341ad73fcdf78ab58d6&selectedTaskRun=MILanVSPTt6g4U2R7UdUhw.0

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b6ca9f5986d7
Fix mail-multi-emailheaderfield element not expanding properly. r=aleca

Comment 24

[Jorg K (CEST = GMT+2)]

Please request uplift for this regression.

Comment 25

[Khushil Mistry [:khushil324]]

Comment on attachment 9163695 [details] [diff] [review]
Bug-1652410_mail-multi-emailheaderfield-not-expanded-2.patch

[Approval Request Comment]
Regression caused by (bug #): 1537729
User impact if declined: It will be annoying for users to see only one email address if there are multiple email-addresses presented overflowing the header row with. Users should be able to see email-addresses till the end of the row. 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky): Low

Comment 26

[Wayne Mery (:wsmwk)]

Comment on attachment 9163695 [details] [diff] [review]
Bug-1652410_mail-multi-emailheaderfield-not-expanded-2.patch

Approved for both beta and esr78

Comment 27

[Rob Lemley [:rjl]]

Thunderbird 79.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/236db0732add

Comment 28

[Rob Lemley [:rjl]]

Thunderbird 78.0.1:
https://hg.mozilla.org/releases/comm-esr78/rev/576beb2ed581