1717085 - Fix many no-op calls of checkPublicRecipientsLimit() for public bulk mail notification and follow Fluent best practice

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, task, P3)

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Bug #271405 +++

• It's quite amazing how often we're running checkPublicRecipientsLimit() without doing anything, which might affect performance with many recipients (think mass mails, the very use case of this feature!) and when using recipient autocomplete. Something wrong with the way we listen for changes here.
• Also our Fluent isn't quite fluent yet (see comment 3).

https://searchfox.org/comm-central/rev/e5e75651c5fb70526ae298312d99bc37ffd1ad32/mail/components/compose/content/MsgComposeCommands.js#3731-3739

  // Observe the changes of message recipient rows.
  gRecipientObserver = new MutationObserver(function(mutations) {
    if (mutations.some(m => m.type == "childList")) {
      checkPublicRecipientsLimit();
    }
  });
  gRecipientObserver.observe(document.getElementById("toAddrContainer"), {
    childList: true,
  });

We're constructing the observer correctly, but then observing only the To field...
Yet the notification also works for Cc, so there must be something else... here:

https://searchfox.org/comm-central/rev/e5e75651c5fb70526ae298312d99bc37ffd1ad32/mail/components/compose/content/MsgComposeCommands.js#5589-5597

function onRecipientsChanged(automatic) {
  if (!automatic) {
    gContentChanged = true;
  }
  if (gRecipientObserver) {
    checkPublicRecipientsLimit();
  }
  updateSendCommands(true);
}

Right, here's where we start going into overdrive:

• onRecipientsChanged() gets called here for every input event of every autocomplete address input field (including Bcc!), which in turn calls checkPublicRecipientsLimit().

Here's the net result:

• When you type [e-mail], it will call checkPublicRecipientsLimit() for each character entered: that's 20 calls.
• If your topmost autocomplete result changes with each character, that may add up to 20 calls again.
• So you may have 40 calls for a single address, however, as long as that address is still plain text (not pillified), we won't even count it for the notification... So that's a lot of no-op!

Plus, every pill which you add or remove manually in the To field will needlessly be double-checked: once by the direct gRecipientObserver, and then again by onRecipientsChanged().

• So adding 20 pills manually in To field will do 40 checks (in addition to the check-as-you-type calls) - more no-op!
• Btw, reply-all triggers the observer for each and every reply address, whereas pressing Enter after comma-separated addresses gets counted by the observer as a single childList change - should look into that difference, too! (in another bug)

So the total no-op calls of checkPublicRecipientsLimit when entering 20 To recipients manually may be like this:
20 recipients x 20 chars x2 for character autocompletion + 20 for final autocompletion + 20 from observer for adding pill.
That's a whopping 840 calls where 20 should suffice if we're only counting pills.

Btw dragging an address from one field to another field which has pills already creates as many as 6 onRecipientsChanged() calls...

Comment 1

[Alessandro Castellani [:aleca] (PTO until Apr 21st)]

Great analysis and great findings.
Indeed, the checkPublicRecipientsLimit() should only happen for these scenarios:

• Pill created
• Pill deleted

And we should create some smart conditions to avoid running that method if pills are moved from a field to another, as the number of currently available pills didn't change.

Definitely nothing should be hooked to the onRecipientsChanged() method as that method runs at every input to enable the send button, trigger the "dirty" status for draft saving, and other things I don't remember, but that method needs to run at every input.

Comment 2

[Thomas D. (:thomas8)]

(In reply to Thomas D. (:thomas8) from comment #0)

That's a whopping 120 840 calls where 20 should suffice if we're only counting pills.

I've corrected the total number of calls, it's up to 840 calls for 20 fully manually entered addresses with 20 characters each.

Comment 3

[Thomas D. (:thomas8)]

(In reply to Thomas D. (:thomas8) from comment #0)

• Also our Fluent isn't quite fluent yet (see comment 3).

https://searchfox.org/comm-central/rev/014b72575f40cf1d2e0cc1d997d8e4bebbd118f9/mail/components/compose/content/MsgComposeCommands.js#5147-5151

  msgText.setAttribute("data-l10n-id", "many-public-recipients-info");
  msgText.setAttribute(
    "data-l10n-args",
    JSON.stringify({ count: publicAddressPillsCount })
  );

When you need to use JSON.stringify, there's something wrong...
We're trying to localize the msgText element dynamically, based on a JS variable.
Here's the standard way of achieving the same result with less and cleaner code:

  document.l10n.setAttributes(
    msgText,
    "many-public-recipients-info",
    { count: publicAddressPillsCount }
  );

Notably, that will even work when msgText element has been created, but not yet appended to the document.

Comment 4

[Thomas D. (:thomas8)]

Attachment: 1717085_fixPublicBulkRecipientsNotificationObserverAndFluent.diff

It always looks so simple when it's done... getting there isn't!

• Ensure that gRecipientObserver which calls checkPublicRecipientsLimit()
  when pills get added or removed is observing both To and Cc field to check
  if we need to show public bulk mail notification.
• In turn, stop calling checkPublicRecipientsLimit() for every input event in
  onRecipientsChanged(), even twice if autocomplete changes the input text,
  which is a no-op because we're currently ignoring non-pillified addresses for
  the notification.
• In a nutshell, prevent that entering a single plain text recipient with 20
  characters may call checkPublicRecipientsLimit() up to 42 times where
  a single call for the new pill would suffice.
• Normalize dynamic Fluent attribute changes to the notification text.
• I've checked that this works correctly for:
  • Manually adding/removing recipients in the address row.
  • Reply all with >= 15 recipients.
  • Starting TB via CLI with -compose
  • Drag and drop of recipients between rows and from contacts sidebar.

Comment 5

[Thomas D. (:thomas8)]

@Aleca: Pls note that before and after this patch, we won't warn when there's >=15 CSV recipients as plain text in the input, and user hits Send immediately without going through pillification. Counting plain text CSV addresses is non-trivial (unless if we find an existing Mime helper for that), but if we'd want that, it would be for another bug. We'd then probably drop the Mutation observer and use only onRecipientsChanged(), which of course would also reverse most of the performance gains of this bug.

Comment 6

[Alessandro Castellani [:aleca] (PTO until Apr 21st)]

Comment on attachment 9227742 [details] [diff] [review]
1717085_fixPublicBulkRecipientsNotificationObserverAndFluent.diff

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

Great improvements, thanks for taking are of this.
I launched a try-run to be sure we're not breaking any tests: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=70ffc796536696db76395db283fe05197543873b

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5149,5 @@
>    msgText.classList.add("consider-bcc-notification-text");
> +  document.l10n.setAttributes(
> +    msgText,
> +    "many-public-recipients-info",
> +    { count: publicAddressPillsCount }

nit: the linter wants this formatted this way.
document.l10n.setAttributes(msgText, "many-public-recipients-info", {
  count: publicAddressPillsCount,
});

Comment 7

[Alessandro Castellani [:aleca] (PTO until Apr 21st)]

By looking at this I also noticed a wrong usage of the notification element.
We're adding extra elements with classes and divs which are redundant and also semantically inaccurate.
I'll create a follow up bug dependent on this to fix it.

Comment 8

[Thomas D. (:thomas8)]

Attachment: 1717085_fixPublicBulkRecipientsNotificationObserverAndFluent.diff

Appreciate the rapid review & positive feedback!

Linting nit fixed. r+ from Aleca's comment 6.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/345fa4270845
Fix frequent no-op calls of checkPublicRecipientsLimit() and related Fluent cleanup. r=aleca

Comment 10

[Alessandro Castellani [:aleca] (PTO until Apr 21st)]

Comment on attachment 9227854 [details] [diff] [review]
1717085_fixPublicBulkRecipientsNotificationObserverAndFluent.diff

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5147,5 @@
>  
>    let msgText = document.createElement("div");
>    msgText.classList.add("consider-bcc-notification-text");
> +  document.l10n.setAttributes(msgText, "many-public-recipients-info", {
> +    count: publicAddressPillsCount

Lint error. This requires a comma at the end.

You really gotta set up a linter in your system :P

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/28d910cf205d
Follow-up: Fix eslint error. r=eslint

Comment 12

[Alessandro Castellani [:aleca] (PTO until Apr 21st)]

This probably also broke the testMailingListMembersCounted test as it was relying on the l10nArgs dataset to get the count.
I'll fix it in bug 1717127.