Closed Bug 1647050 Opened 5 years ago Closed 5 years ago

Recipients2CompFields() not working as expected after bug 440377, losing Cc/Bcc recipients, onRecipientsChanged() fires too early.

Categories

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

Product:
Thunderbird
Component:
Message Compose Window
Type:
defect

Tracking

(thunderbird_esr78 fixed, thunderbird79 fixed, thunderbird80 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 80.0
Tracking Flags:
Tracking Status
thunderbird_esr78 --- fixed
thunderbird79 --- fixed
thunderbird80 --- fixed

People

(Reporter: jorgk-bmo, Assigned: aleca)

References

(Depends on 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(1 file, 11 obsolete files)

1647050-compose-regression.diff
10.24 KB, patch
aleca
: review+
wsmwk
: approval-comm-beta+
wsmwk
: approval-comm-esr78+
Details | Diff | Splinter Review

This is a tricky one. Have an add-on that listens to NotifyComposeFieldsReady and calls Recipients2CompFields(). Reply to an e-mail that has To: and Cc: recipients. When NotifyComposeFieldsReady fires, msgCompFields.to and msgCompFields.cc are already populated. If you now call Recipients2CompFields() you lose the Cc: recipients.

I debugged it a bit, this loop:
https://searchfox.org/comm-central/rev/ea1b0a2bd38a192b4520cec322e290aae0809d3b/mail/components/compose/content/addressingWidgetOverlay.js#43
for (let pill of document.getElementsByTagName("mail-address-pill")) {
somehow only returns the To: recipients. So there might be some sort of timing issue. I guess NotifyComposeFieldsReady fires too early when the pills aren't in the DOM yet, maybe because the Cc: field hasn't put into the DOM yet.

This bug will potentially affect all these 20 add-ons:
https://github.com/search?q=Recipients2CompFields++path:/xall+repo%3Acleidigh%2FThunderKdB+repo%3Athundernest%2Ftb-web-ext-experiments+path:/xall/x68+repo%3Acleidigh%2FThunderKdB&type=Code

I fixed the issue with a workaround in my add-on:
https://pep-security.lu/dev/repos/pEp_for_Thunderbird/rev/b4378065e67b

(I edited the previous comment and then reverted it back to the previous version.)

This is even more tricky. When calling Recipients2CompFields() in NotifyComposeFieldsReady, you lose all recipients, even in TB 68, so that hasn't changed.

I'm not calling it in NotifyComposeFieldsReady, but instead I patch over onRecipientsChanged. And when that first fires, in TB 68 it's save to call Recipients2CompFields(), but not in TB 78, where only the To: pills are available and Cc:/Bcc: content is lost.

So there is still a timing issue, either NotifyComposeFieldsReady or onRecipientsChanged is called too early.

Summary: Recipients2CompFields() not working as expected after bug 440377, losing Cc/Bcc recipients → Recipients2CompFields() not working as expected after bug 440377, losing Cc/Bcc recipients, NotifyComposeFieldsReady fires too early.

Called here: https://searchfox.org/comm-central/rev/ea1b0a2bd38a192b4520cec322e290aae0809d3b/mailnews/compose/src/nsMsgCompose.cpp#1590
That's when the backend thinks that we're ready, but that doesn't take any later processing in JS into account. Maybe we should delay the onRecipientsChanged() call until all pills are ready, but then, that's not an official function with guaranteed behaviour :-(

EDIT: Note that add-ons use that, too:
https://github.com/search?q=onRecipientsChanged++path:/xall+repo%3Acleidigh%2FThunderKdB+repo%3Athundernest%2Ftb-web-ext-experiments+path:/xall/x68+repo%3Acleidigh%2FThunderKdB&type=Code

Thank you very Jörg much for unearthing and addressing these enterprise-relevant problems.

Also, I noticed that there is zero comments on things like NotifyComposeFieldsReady() and even the stateListener itself, like what they do and under which circumstances they are used. I hope we don't have duplicated functionality in there which differs from the regular patterns of compose startup, which in itself is a can of spaghetti code worms.

Comment 3 EDIT: Even stateListener itself should have a comment to explain its purpose

This is somewhat off-topic:
The basic problem is, that add-ons want to watch as recipients are added or removed from a message.
Then NotifyComposeFieldsReady() is first called, msgCompFields.to/.cc/.bcc etc. are populated as they should be. NotifyComposeFieldsReady() and msgCompFields are semi-official interfaces used by a ton of add-ons. Try https://www.google.com.au/search?q=NotifyComposeFieldsReady for references.

When the user adds/removes recipients, there is no event that would get triggered. So people went looking and found the internal function onRecipientsChanged() which does get called, but really doesn't do anything in TB:
https://searchfox.org/comm-central/rev/ea1b0a2bd38a192b4520cec322e290aae0809d3b/mail/components/compose/content/MsgComposeCommands.js#5174
In TB 68 this actually gets called on every keystroke, when it should get called one an address is complete. Fortunately, this is what happens in TB 78, it gets called when a pill is complete. However, when onRecipientsChanged() is called, the msgCompFields are not populated accordingly, so one has to revert to using Recipients2CompFields(), and that has changed behaviour and isn't safe to call any more, in fact it was never safe to call.

So in an ideal world there would be a documented and stable interface that would notify when the overall recipients have changed, and at the same time, msgCompFields should contain those recipients so no calling of the undocumented Recipients2CompFields() would be necessary.

Maybe this API already exists in the MailExtension world, but I doubt it. So we could change the bug to: Provide API to notify when recipients have changed and make sure that msgCompFields or some other data structure holds all the recipients.

Summary: Recipients2CompFields() not working as expected after bug 440377, losing Cc/Bcc recipients, NotifyComposeFieldsReady fires too early. → Recipients2CompFields() not working as expected after bug 440377, losing Cc/Bcc recipients, onRecipientsChanged() fires too early.

Thank you so much Jorg for the detailed analysis on this issue.
I don't have much capacity for the upcoming two weeks.
Thomas, would you be able to take care of this bug?

We probably should have an API for onRecipientsChanged, similar to https://thunderbird-webextensions.readthedocs.io/en/latest/compose.html#onbeforesend-tab-details

I've just noticed that onRecipientsChanged() is called when the From: changes, before the compose-from-changed notification :-(

I'll grab this and try to have a patch for next week, apologies for the regression caused by my patch.

Assignee: nobody → alessandro

(In reply to Jorg K (CEST = GMT+2) from comment #8)

I've just noticed that onRecipientsChanged() is called when the From: changes, before the compose-from-changed notification :-(

On a related note, From: actually changes even if you just navigate the identity dropdown with cursor keys (without confirming any option with Enter), which probably makes this problem worse.

Depends on: 1635124

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

I'll grab this and try to have a patch for next week, apologies for the regression caused by my patch.

Thanks, Alex. In guess in absence of a real event that fires when recipient pills are changed/moved, the ideal behaviour would be:

Fire onRecipientsChanged() when a pill is completely entered, moved between To/Cc/Bcc or deleted. That's when in the true sense of the word the recipients have changed. Anyone interested in evaluating something based on the recipients should be happy with that.

When onRecipientsChanged() fires, it should be safe to call Recipients2CompFields(), or even better, at that stage, make the msgCompFields already contain the recipients in .To, .Cc and .Bcc so Recipients2CompFields() wouldn't be necessary. (Sorry, that's a repeat of the 2nd last paragraph of comment #5).

And also, don't call onRecipientsChanged() when the From: is changed since we have an "official" event for that.

Thank you so much for these pointers, this helps a lot.

Attached patch 1647050-compose-regression.diff (obsolete) — — Splinter Review

Indeed, we were firing the onRecipientsChanged() method twice from the identity field, even when it wasn't changed.

I did some quick test and if I reply to a message with the fields To, Cc, and Bcc filled with addresses, the onRecipientsChanged() method is called after the compose window is properly populated, and upon printing the content of the gMsgCompose.compFields, I can see all the addresses in there.

Also, onRecipientsChanged() gets fired whenever a pill is entered, altered, moved, or removed, which is correct.

Jorg, do you have the chance to load this patch and see if by simply removing that extra call the issue was fixed?
Asking for feedback to Thomas to see if I missed something.

Attachment #9163676 - Flags: feedback?(bugzilla2007)
Comment on attachment 9163676 [details] [diff] [review] 1647050-compose-regression.diff This patch doesn't appear to fix anything: 1. When you change the From:, onRecipientsChanged() still fires, not from LoadIdentity() but earlier. 2. When you press reply on a message with To and Cc, it fires at a time where Recipients2CompFields() doesn't work, that is the original complaint here. The desired behaviour is to fire, but only when To and Cc fields are ready. Try adding the code below to Recipients2CompFields() to see what I mean. Replying to a message with Cc field doesn't deliver that Cc field, most likely because it's not ready yet. ``` // Each pill knows from which recipient they were generated // (addr_to, addrs_bcc, etc.). let recipientType = pill.getAttribute("recipienttype"); console.log("Recipients2CompFields ", recipientType, recipient); ```
Attachment #9163676 - Flags: feedback?(bugzilla2007) → feedback-

I see.
Here's a stack trace of the call happening when replying to a message:

console.trace: 
chrome://messenger/content/messengercompose/MsgComposeCommands.js 5173 onRecipientsChanged
chrome://messenger/content/messengercompose/addressingWidgetOverlay.js 692 recipientAddPill
chrome://messenger/content/messengercompose/addressingWidgetOverlay.js 139 CompFields2Recipients
chrome://messenger/content/messengercompose/MsgComposeCommands.js 2678 ComposeFieldsReady
chrome://messenger/content/messengercompose/MsgComposeCommands.js 506 NotifyComposeFieldsReady

console.trace: 
chrome://messenger/content/messengercompose/MsgComposeCommands.js 5173 onRecipientsChanged
chrome://messenger/content/messengercompose/addressingWidgetOverlay.js 692 recipientAddPill
chrome://messenger/content/messengercompose/addressingWidgetOverlay.js 154 CompFields2Recipients
chrome://messenger/content/messengercompose/MsgComposeCommands.js 2678 ComposeFieldsReady
chrome://messenger/content/messengercompose/MsgComposeCommands.js 506 NotifyComposeFieldsReady
console.log: "Recipients2CompFields " "addr_to" "..."
console.log: "Recipients2CompFields " "addr_to" "..."
console.log: "Recipients2CompFields " "addr_cc" "..."

As you pointed out the onRecipientsChanged gets fired too early, resulting in an empty pills list, and called once again after the pills have been added (I replaced the addresses with ... just for privacy).

I'm getting to the bottom of this.

Why would commenting out a call to Recipients2CompFields(compFields); make a difference when onRecipientsChanged() fires at the wrong time, see comment #14?

Here's what's happening for me and why I proposed to remove that:

  • I click Reply All to a message with To and multiple Cc addresses.
  • The onRecipientsChanged() method gets called and this is the stack trace:
chrome://messenger/content/messengercompose/MsgComposeCommands.js 5173 onRecipientsChanged
chrome://messenger/content/messengercompose/addressingWidgetOverlay.js 692 recipientAddPill
chrome://messenger/content/messengercompose/addressingWidgetOverlay.js 154 CompFields2Recipients
chrome://messenger/content/messengercompose/MsgComposeCommands.js 2678 ComposeFieldsReady
chrome://messenger/content/messengercompose/MsgComposeCommands.js 506 NotifyComposeFieldsReady
  • Inside the onRecipientsChanged() method, I'm printing in the log all the currently available pills, and all the fields in the gMsgCompose.compFields
  • Both logs show all the pills and all the addresses in those fields.

So, this is my issue. For me, when onRecipientsChanged(), all my pills and addresses are properly in place.

I noticed 2 things issues:

  • onRecipientsChanged() is called twice when the identity changes. Removing it like applied in my patch solves this issue and the method fires only one after a new identity is loaded.
  • Recipients2CompFields(compFields) is called multiple times by the enigmail code. Is called on focus/blur on any field (to, cc, subject, etc), therefore constantly updating the gMsgCompose.compFields at the wrong times. I noticed this is causing a delay in the final update of those fields.
  • When trying to echo those fields like you suggested with console.log("Recipients2CompFields ", recipientType, recipient);, if I don't remove that method, that console log happens 2 or 3 seconds after the compose window is ready.
Status: NEW → ASSIGNED

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

  • onRecipientsChanged() is called twice when the identity changes. Removing it like applied in my patch solves this issue and the method fires only one after a new identity is loaded.

OK, you reduce the number of calls from 2 to 1, OK, but it should be 0. Recipients don't change, that's what you have the "compose-from-changed" notification for.

I'll take a look at the other issue separately.

Attached patch 1647050-demonstration.patch (obsolete) — — Splinter Review

Hmm, looks like we're still talking past each other :-(

Please apply the patch (disclaimer: might not apply cleanly due to my old build), then "Reply All" to an e-mail with To and Cc recipients. Result: The Cc recipient is lost.

(In reply to Jorg K (CEST = GMT+2) from comment #19)

OK, you reduce the number of calls from 2 to 1, OK, but it should be 0. Recipients don't change, that's what you have the "compose-from-changed" notification for.

Are you sure it should be 0?
If a user loads an identity with pre filled CC or BCC, shouldn't we trigger that?
Anyway, to remove the last call when the identity changes we can put this into a condition:
https://searchfox.org/comm-central/rev/337b77d55861522d05198bf971b042e8ed129a04/mail/components/compose/content/addressingWidgetOverlay.js#1045

This is called when a recipient row is deleted. We can change it by getting called only if pills were actually deleted.

Attached patch 1647050-compose-regression.diff (obsolete) — — Splinter Review

With this patch, this is what I get when replying to a message with 3 CC

onRecipientsChanged
Recipients2CompFields addr_cc castellani.ale@gmail.com
Recipients2CompFields addr_cc me@alecaddd.com
Recipients2CompFields addr_cc asd@akiraux.org
Attachment #9163676 - Attachment is obsolete: true

Sounds promising, I'll try it when my build is done.

Without my patch, I get this:

onRecipientsChanged
Recipients2CompFields addr_cc castellani.ale@gmail.com
Recipients2CompFields addr_cc me@alecaddd.com
Recipients2CompFields addr_cc asd@akiraux.org
Recipients2CompFields addr_cc castellani.ale@gmail.com
Recipients2CompFields addr_cc me@alecaddd.com
Recipients2CompFields addr_cc asd@akiraux.org

The second group of addresses is dumped 2 seconds after the first.

So, in my case, even without my patch, that method is called once when replying, and only Recipients2CompFields() is called twice, the second time from the enigmail code.

The only difference I can think of is that I'm on trunk, but all the patches that touch those sections are synced between the 2 branches.
I'll compile 78 and try this.

(In reply to Jorg K (CEST = GMT+2) from comment #23)

Sounds promising, I'll try it when my build is done.

The build is busted due to https://hg.mozilla.org/mozilla-central/rev/e34785ffe0ed472731d2c3235f2578f513664ae4 (tell Geoff when you see him), so I'm calling it a day. My old build was from May 2020, so during the 78 cycle. And the problem is observed in TB 78. Surely things have changed since then. So are you saying that with attachment 9163868 [details] [diff] [review] a "Reply All" works when there are To and Cc recipients?

I'm building 78 now, it'll take a while but I'll test.

So are you saying that with attachment 9163868 [details] [diff] [review] [diff] [review] a "Reply All" works when there are To and Cc recipients?

Yes, tested on daily and the result is on comment 42.

I just finished compiling 78 and tested attachment 9163868 [details] [diff] [review].

This is the dump:

onRecipientsChanged
Recipients2CompFields addr_cc castellani.ale@gmail.com
Recipients2CompFields addr_cc me@alecaddd.com
Recipients2CompFields addr_cc asd@akiraux.org

I can't seem to stumble upon the problem, and I don't even see the double call coming from enigmail.

Hmm, well, apart from seeing this issue when developing my add-on, I've now disabled the add-on, unpacked TB 78 ESR's omni.ja file and patched it accordingly. I'll try it on trunk as soon as someone fixes the build.

When doing a "Reply All" to a message with one To and one Cc recipient, the Cc recipient is lost and the debug shows:
onRecipientsChanged MsgComposeCommands.js:5186:11
Recipients2CompFields addr_to Jxxxxx <jx@pep.security> addressingWidgetOverlay.js:56:13

I also tried with three Cc recipients and they are all lost. So what am I (or are you) missing? Of course you need to call Recipients2CompFields(0 during onRecipientsChanged() to get the damage done by the latter:

function onRecipientsChanged(aAutomatic) {
  console.log("onRecipientsChanged");
  Recipients2CompFields(gMsgCompose.compFields);
  if (!aAutomatic) {
    gContentChanged = true;
  }
  updateSendCommands(true);
}

Hmm, are you testing with only Cc recipients? That's what your debug suggests. You need To and Cc recipients to trigger the issue. Also in comment #24 (not #42) I don't see To recipients.

I don't have the To because I sent the email to myself.
What's your recipient situation?

You received an email where the To is yourself? Or are you listed as Cc?

Ah! I finally stumbled upon the issue.
The Cc addresses are missing, not only from the gMsgCompose.compFields but also from the UI!

This is inconsistent as it seems that only one addressing field is populate.
If the To if populated, the Cc is not, and viceversa.

How is this possible? This whole section is covered by tests and they're all passing.
Am I losing my mind?

Severity: S2 → S1

Correction, is your demonstration patch that it's causing CC fields to not appear.

It seems that onRecipientsChanged gets called once per addressing row. So if you have To, Cc, etc. that method is called after each one of that row is populated with addresses, independently from the number of addresses is only called once per row widget.

So, your add-on calling Recipients2CompFields(gMsgCompose.compFields) after the first onRecipientsChanged, actively blocks the other run.

Both good and bad news.
Good, because it means that TB is not totally broken and we're not leaking or losing addresses when replying.
Bad, because it means any add-on using that method will break.

Okay, now I think I have a better and clearer idea of what's going on.
Onto writing a proper fix.

Hey Kai, can you shed some light on this method?
https://searchfox.org/comm-central/rev/337b77d55861522d05198bf971b042e8ed129a04/mail/extensions/openpgp/content/ui/enigmailMsgComposeOverlay.js#1414

This seems to be triggered at every focus/blur on any input fields of the compose window, even the subject line.
Is this necessary?

Flags: needinfo?(kaie)
Attached patch 1647050-compose-regression.diff (obsolete) — — Splinter Review

FIXED!!!

Try run launched to be sure I haven't busted anything: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ea27e9ba285cca481305a484bd43b0baff5cbd97

Attachment #9163875 - Attachment is obsolete: true
Attachment #9163917 - Flags: review?(geoff)

Indeed, I was expecting those bct4 failures.
I'll fix them tomorrow.

Function determineSendFlags was preexisting from Enigmail. I guess it shouldn't be executed that often. Did you fix this already?

Flags: needinfo?(kaie)

(In reply to Kai Engert (:KaiE:) from comment #37)

Function determineSendFlags was preexisting from Enigmail. I guess it shouldn't be executed that often. Did you fix this already?

No, I didn't touch that section. Do you want to take a look at in a dedicated bug?

onRecipientsChanged() needs to be fired after a new identity is picked as we need to enable/disable the send button if the identity comes with prepopulated addressing fields.
Having it there doesn't interfere with the expected order of methods and it doesn't cause an early trigger.

The early trigger was due to the change in the UI when showing/hiding extra addressing rows.
We shouldn't trigger that if the rows didn't contain any pill.

Attached patch 1647050-compose-regression.diff (obsolete) — — Splinter Review

Richard, I'm pinging you on this review.
Be sure the Send button updates properly (the tests are all green so it should be good), and add those few lines from the demostration patch to be sure the onRecipientsChanged is called only once when the message is ready and all recipient are available when you click Reply All.

Attachment #9163917 - Attachment is obsolete: true
Attachment #9163917 - Flags: review?(geoff)
Attachment #9164083 - Flags: review?(richard.marti)
Comment on attachment 9164083 [details] [diff] [review] 1647050-compose-regression.diff Review of attachment 9164083 [details] [diff] [review]: ----------------------------------------------------------------- Have you addressed the concern form comment #10, that is, moving through the drop-down for the identities already triggers the function? Maybe a compose peer should review this. Somehow BMO is broken since it removes the reviewer :-( ::: mail/components/compose/content/addressingWidgetOverlay.js @@ +207,5 @@ > addRecipientsToIgnoreList(gCurrentIdentity.fullAddress); > } > + > + // Trigger this method only after all the pills have been created. > + onRecipientsChanged(true); This is an additional call whereas the others just moved?
Attachment #9164083 - Flags: review?(richard.marti)

I can look at it once the two questions above are answered.

Have you addressed the concern form comment #10, that is, moving through the drop-down for the identities already triggers the function?

Yes, I have.
As I wrote in comment 39 , the onRecipientsChanged method is necessary to handle the send button status if the user has any prefilled Cc or Bcc fields per identity.
I moved it in order to be triggered only if the identity is actually changed and doesn't match the current one.

This is an additional call whereas the others just moved?

That's the only place where that call should be triggered after the compose window completed everything that needs to do (load identity, fill addresses, body, etc.).
Since I moved the onRecipientsChanged methods in order to trigger only if an actual change from the user happens (remove/add pill, actually change identity, etc.), that method is not randomly triggered anymore when we generate a new compose window, or a compose window is opened during a reply.

Calling that method there ensures that it only runs once at the end of everything.
We didn't have it there before because, as I said, we were calling it all over the place.

Jörg, are the questions answered and can you review the patch? I'm not feeling comfortable to do it.

Comment on attachment 9164083 [details] [diff] [review] 1647050-compose-regression.diff Thanks for the explanation, Alex, ... and taking care of the issue, of course.
Attachment #9164083 - Flags: review?(jorgk-bmo)

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

Have you addressed the concern form comment #10, that is, moving through the drop-down for the identities already triggers the function?

Yes, I have.
As I wrote in comment 39 , the onRecipientsChanged method is necessary to handle the send button status if the user has any prefilled Cc or Bcc fields per identity.
I moved it in order to be triggered only if the identity is actually changed and doesn't match the current one.

That sounds like an improvement, but it doesn't sound enough to fix the concern of comment 10, described in bug 1635124.
The problem is that navigating up and down the list with cursor up/down will instantly apply the navigated identity although the user has not yet confirmed it with ENTER (premature change of identity). So if I cursor-navigate from identity1 to identity5, i understand that onRecipientsChanged() will still needlessly fire 4 times because the dropdown widget applies navigated options prematurely. As a side effect, customize-from also fires prematurely on navigation, so keyboard-only users will find it very hard if not impossible to customize the from address of any identity other than the last.

That's an issue we can tackle on bug 1635124.
With my patch, the early trigger of onRecipientsChanged() is fixed. What you're describing is a keyboard navigation issue.

A few things here:

  1. I share the concern of comment #46 and I'd like to reiterate that changing the EDIT:From: recipient does NOT trigger onRecipientsChanged() at all in TB 68. Once again, we have a custom notification for that. I understand that switching identities can trigger addition/removal of recipients, so if this is the case, then the function should trigger since recipients have changed, not in other situations where they haven't. I don't actually see the need for another bug, but be that as it may.
  2. Moving a pill from To to Cc or vice versa triggers onRecipientsChanged() twice, which is unnecessary. onRecipientsChanged() should be the final notification after a "transaction" that modified the recipients.
  3. This is from my wish list: If you create a new recipient manually, then onRecipientsChanged() should be called and gMsgCompose.compFields.Xx should already be populated to eliminate the need for calling Recipients2CompFields() in the first place.

I can live with 3. not being addressed, but I think you should look into 1. and 2. some more. That said, the patch fixes the original issue.

I'd prefer to let someone else tackle the other issues, maybe Thomas could jump in.
I'm pretty overwhelmed by many other bugs and I wanted to address this regression first.

Well, 1. should be doable by removing the call of onRecipientsChanged() in the identity load code. We'd have to see what happens when the switch really modifies the recipients. Sadly you know the pill code best, so it would be good it you could point us to the spot where the overall "transaction" is finished, like for a move of the pill. That's where the call should go.

Attached patch 1647050-compose-regression.diff (obsolete) — — Splinter Review

To fix 1, we can check if the recipients have changed from the previous identity.
Not sure if it's a dirty solution.

This check needs to happen here because the onRecipientsChanged call when a pill is created, doesn't happen when the identity changes since the creation wasn't triggered by the user but by TB when populating those fields.

Attachment #9164083 - Attachment is obsolete: true
Attachment #9164083 - Flags: review?(jorgk-bmo)
Attachment #9164147 - Flags: review?(jorgk-bmo)

Not sure if it's a dirty solution.

Seems OK to me, I'll try it tomorrow. That will still trigger bug 1635124, that is, it will happen on drop-down traversal, not acceptance via the enter key. But I'll let Thomas worry about that one. So what about point 2? Is there a transaction bracket so the call could be moved to outside that bracket instead of calling it on each pill change?

Attached patch 1647050-compose-regression.diff (obsolete) — — Splinter Review

Fixed 2 by passing a boolean tot he remove pills method. Since in order to move pills first we copied them, then remove them, and then add the array to the new container, we can prevent the onRecipientsChange to be called if the removal wasn't done by the user.

Regarding point 3, if I write console.log(gMsgCompose.compFields); in the onRecipientsChange method, I get all fields without the necessity of calling Recipients2CompFields.
Does it work for you with this patch?

Attachment #9164147 - Attachment is obsolete: true
Attachment #9164147 - Flags: review?(jorgk-bmo)
Attachment #9164175 - Flags: review?(jorgk-bmo)
Attached patch 1647050-compose-regression.diff (obsolete) — — Splinter Review

What's wrong with lint/prettier here? Isn't this the right indentation?

I don't see any linting problems, what is that patch? Why did you upload an identical one?
Did you review my patch?

13KB and 10KB can't be identical. Please click "Splinter Review" on both to see the difference. Does linting pass for you for my version. Looks good, but I haven't tested it yet. Will do so in a few hours.

Argh, I see.
My patch pushed the hinting of the removeSelectedPills up by 4 spaces, darn.
Yours is correct. Let me fix mine, sorry about that.

Attached patch 1647050-compose-regression.diff (obsolete) — — Splinter Review

My linter broke after yesterday's pull from m-c and I had to reinstall it, so weird.

Attachment #9164175 - Attachment is obsolete: true
Attachment #9164338 - Attachment is obsolete: true
Attachment #9164175 - Flags: review?(jorgk-bmo)
Attachment #9164407 - Flags: review?(jorgk-bmo)
Attached patch 1647050-demonstration.patch (obsolete) — — Splinter Review

This is my testing patch. With this patch you can see that ...

Regarding point 3, if I write console.log(gMsgCompose.compFields); in the onRecipientsChange method, I get all fields without the necessity of calling Recipients2CompFields.

... is not true.

Does it work for you with this patch?

No.

With my patch + your demonstration patch it works for me.

I have an email with these fields:

From: castellani***.com
To: sabrina****.com
Cc: miso***.com, me***.com

And this is the console output once I hit Reply All:

onRecipientsChanged
Alessandro Castellani <castellani***.com>, Sabrina <sabrina****.com>-to
Miso <miso***.com>, me***.com-cc
Comment on attachment 9164407 [details] [diff] [review] 1647050-compose-regression.diff Review of attachment 9164407 [details] [diff] [review]: ----------------------------------------------------------------- Looks OK seems to do what we want. I'm not 100% sure about the two hunks below, but you're the expert here. Thanks for going the extra mile ;-) ::: mail/components/compose/content/addressingWidgetOverlay.js @@ +688,5 @@ > if (!automatic) { > element > .closest(".address-container") > .classList.add("addressing-field-edited"); > + onRecipientsChanged(); You're sure you only want to call it in this case? @@ +1045,5 @@ > document.getElementById(labelID).removeAttribute("collapsed"); > > // Update the sender button only if the content was previously changed. > + if (isEdited) { > + onRecipientsChanged(true); Same question here.
Attachment #9164407 - Flags: review?(jorgk-bmo) → review+

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

And this is the console output once I hit Reply All:

Oh, yes, but not when you move a pill or add one :-( - EDIT: It would be ideal if the comp fields were always right when onRecipientsChange() is called.

(In reply to Jorg K (CEST = GMT+2) from comment #62)

Oh, yes, but not when you move a pill or add one :-( - EDIT: It would be ideal if the comp fields were always right when onRecipientsChange() is called.

We can deal with that in another bug since is not a defect/regression affecting in 78.
Speaking of which, without bug 1646857 landing on 78 the patch doesn't apply.
I'll upload a variation for 78 if the uplift I requested gets rejected.

Regarding your questions, yes I'm sure those should be called inside those conditions.
If we don't do that, every time a pill is created or an addressing field is hidden by an automated trigger (eg. reply all, change identity from IMAP to NNTP account, etc.) that method will be constantly triggered even if not necessary since we don't need to update the Send Button unless something actually changes.
All those scenarios are covered by tests, which are all passing with the edits done in the patch.

Reviewer updated.

Attachment #9163868 - Attachment is obsolete: true
Attachment #9164407 - Attachment is obsolete: true
Attachment #9164417 - Attachment is obsolete: true
Attachment #9164421 - Flags: review+

This is the 78 variation in case bug 1646857 doesn't get uplifted.

Attachment #9164423 - Flags: review+
status-thunderbird78: --- → affected
status-thunderbird79: --- → affected
status-thunderbird80: --- → affected
Keywords: checkin-needed-tb
Target Milestone: --- → Thunderbird 80.0
Comment on attachment 9164421 [details] [diff] [review] 1647050-compose-regression.diff Review of attachment 9164421 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Alex for taking on this intricate and important bug! From reading the code, I am seeing some problems. I might be wrong, blame it on the compose spaghetti code. ::: mail/base/content/mailWidgets.js @@ +2670,5 @@ > + > + // Don't call onRecipientsChanged() if the pills were removed > + // automatically during the move to another addressing widget. > + if (!moved) { > + onRecipientsChanged(); Sorry, but this looks wrong in several ways. - First, when recipients are moved from CC to BCC, that's clearly a change of recipients, so *not* firing onRecipientsChanged violates the inherent notion of onRecipientsChanged and will cause even more confusion in the future for anyone consuming and relying on this as a pseudo-Event. - Second (and worse), as you're skipping onRecipientsChanged, I understand that you're also skipping "gContentChanged = true;" which is contained in onRecipientsChanged. So the net result is - pls correct me if I'm wrong - that when I move 100 recipients from To to BCC (and that's the only change in this draft-editing session), Thunderbird will now discard that change without warning when closing composition window. If that's what happens, I find that not acceptable. ::: mail/components/compose/content/MsgComposeCommands.js @@ -7190,5 @@ > [identityElement.selectedItem.value] > ); > } > SetMsgToRecipientElementFocus(); > - onRecipientsChanged(true); So here you're skipping `updateSendCommands(true);` which is inside `onRecipientsChanged(true);` for the case where identity is changed (not at compose startup). That looks wrong to me because changing an identity can remove CC/BCC/Reply-To and if these were the only recipients, we must make sure the send button is disabled when they are removed by identity change. ::: mail/components/compose/content/addressingWidgetOverlay.js @@ +422,5 @@ > addRecipientsToIgnoreList(aAddressArray.join(", ")); > calculateHeaderHeight(); > udpateAddressingInputAriaLabel(element.closest(".address-row")); > + > + if (element.id != "replyAddrInput") { I think this conditional should have a comment. @@ +1041,5 @@ > let input = addressRow.querySelector(`.address-input[recipienttype]`); > input.value = ""; > addressRow.classList.add("hidden"); > document.getElementById(labelID).removeAttribute("collapsed"); > // Update the sender button only if the content was previously changed. Spelling: Send button @@ +1043,5 @@ > addressRow.classList.add("hidden"); > document.getElementById(labelID).removeAttribute("collapsed"); > // Update the sender button only if the content was previously changed. > + if (isEdited) { > + onRecipientsChanged(true); Hmmm. Did we really change recipients here? If not, why are we firing the entire `onRecipientsChanged()` function? Or maybe we should just `updateSendCommands(true);` if that's what we want as the above comment suggests?
Attachment #9164421 - Flags: review-
Comment on attachment 9164421 [details] [diff] [review] 1647050-compose-regression.diff Review of attachment 9164421 [details] [diff] [review]: ----------------------------------------------------------------- It's too late in the day so there was at least one error in my previous review. However, I think other similar problems still apply. ::: mail/components/compose/content/MsgComposeCommands.js @@ +7182,5 @@ > window.dispatchEvent(new CustomEvent("compose-from-changed")); > gComposeNotificationBar.clearIdentityWarning(); > + > + // Trigger this method only if the Cc or Bcc recipients changed from the > + // previous identity. I think this is problematic. I think we need more changes to ultimately trigger `gContentChanged = true`. You're not running onRecipientsChanged() for changes of reply-to and several other things that can change with identity like vCard etc. So gContentChanged = true; inside onRecipientsChanged() does no longer occur for such changes, but should. Yes, some have their own dedicated global variables to check for changes (e.g. gAttachVCardOptionChanged), but those are cunning, because I found that the variables of change are *not* set true when the change occurs via identity change, so as to preserve them for *direct manual* changes only, which are part of the conditions used here. It's all very twisted and undocumented. @@ -7190,5 @@ > [identityElement.selectedItem.value] > ); > } > SetMsgToRecipientElementFocus(); > - onRecipientsChanged(true); > So here you're skipping `updateSendCommands(true);` which is inside ... This review comment of mine is not correct because you're actually covering the cases where CC/BCC changes. However, other variants of similar problems still remain, as explained in my other comment of this review.
Comment on attachment 9164421 [details] [diff] [review] 1647050-compose-regression.diff Let's go with this now, maybe tweak some comments. > First, when recipients are moved from CC to BCC, that's clearly a change of recipients. Right. You need to read the context. A move is an add and a remove, so now we're not firing on the remove any more if it was part of a move. I tested it. > So here you're skipping `updateSendCommands(true);` which is inside `onRecipientsChanged(true);` Again, please read the context. Alex is calling onRecipientsChanged() further up if an identity change in fact changed the recipients. He therefore removed the wholesale call which is correct. Anything else can go into a now bug, especially anything related to "content changed". Here we're just correcting bad firing of onRecipientsChanged().
Attachment #9164421 - Flags: review-
Keywords: checkin-needed-tb

Thomas, thanks for looking at the patch but please, don't change the flags based on assumptions.
Reviews and tests have been done and, as you can read from the previous comments, all the "issues" you reported have been addressed and explained.

First, when recipients are moved from CC to BCC, that's clearly a change of recipients, so not firing onRecipientsChanged violates the inherent notion of onRecipientsChanged and will cause even more confusion in the future for anyone consuming and relying on this as a pseudo-Event.

I'm preventing the firing of that method only if the pills where not removed by a user.
This might happen when changing identity, which might remove pills if changing from an identity that has pre-filled Cc/Bcc fields, to an identity that has nothing.
Also, I'm preventing the firing of the onRecipientsChanged twice if the delete is done during a move. As you know, moving a pill means: copy > remove > add, and since we trigger that method when adding a pill, we don't need to trigger it twice during the move process.
If you remove the pill, that's not a "move", so the method gets called.

Second (and worse), as you're skipping onRecipientsChanged, I understand that you're also skipping "gContentChanged = true;" which is contained in onRecipientsChanged. So the net result is - pls correct me if I'm wrong - that when I move 100 recipients from To to BCC (and that's the only change in this draft-editing session), Thunderbird will now discard that change without warning when closing composition window. If that's what happens, I find that not acceptable.

Did you load the patch and tried it?
Once again, that method is called if is the user that moves the pills.

So here you're skipping updateSendCommands(true); which is inside onRecipientsChanged(true); for the case where identity is changed (not at compose startup). That looks wrong to me because changing an identity can remove CC/BCC/Reply-To and if these were the only recipients, we must make sure the send button is disabled when they are removed by identity change.

I'm not skipping updateSendCommands(true);. The tests cover that area and they all pass, meaning the original behaviour is maintained.
I'm calling the onRecipientsChanged(true); only if the fields have been changed when switching identity.
There's no need to call that method if loading identities doesn't fill/remove any pill.

Also, we've been always ignoring the Reply-To field when updating the sensitivity of the Send button, I didn't change that behaviour.

Hmmm. Did we really change recipients here? If not, why are we firing the entire onRecipientsChanged() function? Or maybe we should just updateSendCommands(true); if that's what we want as the above comment suggests?

Why? If no pill was deleted in the process of hiding an addressing row, what's the point of updating the send command? No recipients changed.

I think this is problematic. I think we need more changes to ultimately trigger gContentChanged = true. You're not running onRecipientsChanged() for changes of reply-to and several other things that can change with identity like vCard etc. So gContentChanged = true; inside onRecipientsChanged() does no longer occur for such changes, but should. Yes, some have their own dedicated global variables to check for changes (e.g. gAttachVCardOptionChanged), but those are cunning, because I found that the variables of change are not set true when the change occurs via identity change, so as to preserve them for direct manual changes only, which are part of the conditions used here. It's all very twisted and undocumented.

This is outside the scope of this bug, which has the purpose of not triggering the onRecipientsChanged when not necessary.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/994ac4bfe245
Fix onRecipientsChanged() firing too early and when not necessary. r=jorgk DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED

I fixed the spelling issue here: https://hg.mozilla.org/comm-central/rev/994ac4bfe245#l3.88

Comment on attachment 9164421 [details] [diff] [review] 1647050-compose-regression.diff [Approval Request Comment] Regression caused by (bug #): 440377 User impact if declined: This impacts add-ons which want to be notified when the recipients change. Testing completed (on c-c, etc.): Yes. Risk to taking this patch (and alternatives if risky): Should be OK, this area is also covered by tests.
Attachment #9164421 - Flags: approval-comm-esr78?
Attachment #9164421 - Flags: approval-comm-beta?
Comment on attachment 9164421 [details] [diff] [review] 1647050-compose-regression.diff Approved for esr78 and beta - please land
Attachment #9164421 - Flags: approval-comm-esr78?
Attachment #9164421 - Flags: approval-comm-esr78+
Attachment #9164421 - Flags: approval-comm-beta?
Attachment #9164421 - Flags: approval-comm-beta+

the -78 version of the patch doesn't apply to MsgComposeCommands on 78.
I saw that Rob landed whitespace changes yesterday, but there seem to be other context changes that I don't understand without looking more closely.

I'd like to ask the developers of the patch to look and provide an updated patch that applies cleanly to 78 and 79

Comment on attachment 9164423 [details] [diff] [review] 1647050-compose-regression-78.diff This is obsolete since bug 1646857 was uplifted to 78. The variation was necessary without that patch.
Attachment #9164423 - Attachment is obsolete: true

(In reply to Kai Engert (:KaiE:) from comment #75)

the -78 version of the patch doesn't apply to MsgComposeCommands on 78.

Sorry Kai, we should have provided a clearer patch description.
The so-called -78 version was a just-in-case alternative version of the patch in the event that bug 1646857 didn't get uplifted to 78. However the other bug did get uplifted (with the context changes correctly observed by you), hence obsoleting the -78 Version.

I'd like to ask the developers of the patch to look and provide an updated patch that applies cleanly to 78 and 79

It has been available ;-) Please land the remaining patch (attachment 9164421 [details] [diff] [review]) which has wsmwk: approval-comm-beta+ / wsmwk: approval-comm-esr78+. That should apply cleanly. Tia.

Flags: needinfo?(kaie)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: