1663526 - Cleanup buildRecipientRows() to disentangle row input creation for address autocomplete vs. other/raw header inputs [Bug 1658890 followup]

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, task)

Description

[Thomas D. (:thomas8)]

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

Let's cleanup buildRecipientRows() to disentangle row input creation for address autocomplete vs. other/raw header inputs [Bug 1658890 followup].

(In reply to Thomas D. (:thomas8) from Bug #1658890 comment #74)

Comment on attachment 9171835 [details] [diff] [review] ⧉[details] [diff] [review]
1658890-no-pill.patch

Review of attachment 9171835 [details] [diff] [review] ⧉[details] [diff] [review]:

Hmmm, why are we still attaching all sorts of autocomplete attributes and an
autocomplete-only function to an other header input (aka raw input) which
doesn't autocomplete?
This looks unfinished and needs cleanup. I have a patch.

::: mail/base/content/mailWidgets.js
@@ +2149,5 @@

 /**
  * Create a new recipient row container with the input autocomplete.
  *

• * @param {Object} recipient - An object for various element attributes.
  

• * @param {boolean} rawInput - A flag to disable pill and autocompletion.
  

pills

@@ +2240,5 @@

   input.setAttribute("completedefaultindex", true);
   input.setAttribute("forcecomplete", true);
   input.setAttribute("completeselectedindex", true);
   input.setAttribute("minresultsforpopup", 2);
   input.setAttribute("ignoreblurwhilesearching", true);

All of these attributes only apply for autocomplete inputs, why are we
setting them for rawHeader inputs?

@@ +2255,2 @@

   });
   input.onBeforeHandleKeyDown = event => {

onBeforeHandleKeyDown() is run from the autocomplete toolkit code, so it
won't run for raw header. Why are we still attaching this function to raw
header inputs although it will never run there?

@@ +2264,5 @@

   input.setAttribute("recipienttype", recipient.type);
   input.setAttribute("size", 1);

•  if (!rawInput) {
  

Having these small rawInput conditionals all over the place makes this code
very hard to understand. So combined with all of the above, this needs a
thorough cleanup where code applicable to raw input vs. autocomplete input
gets bundled each in their own condition.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3915,5 @@

       row: `addressRow${header}`,
       label: `${header}AddrLabel`,
       labelId: header,
       container: `${header}AddrContainer`,

•      class: "",
  

Good change. Let's hope this won't have side effects.

@@ +4738,5 @@

• let input =

•  container.querySelector(
  

•    `input[is="autocomplete-input"][recipienttype]`
  

•  ) || container.querySelector("input");
  

• input.focus();

querySelector can be simplified into ".address-input[recipienttype]", and
that's short enough then to remove the input variable which is only used
once.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +984,5 @@

let container = document.getElementById(rowID);

• let input =
• container.querySelector(input[is="autocomplete-input"]) ||
• container.querySelector("input");

Again, querySelector can be simplified to match both autocomplete and raw
input as shown above.

Comment 1

[Thomas D. (:thomas8)]

Attachment: 1663526_cleanupRawHeaderInput.diff

Let's clean up and put together what belongs together.
Lightly tested.

Comment 2

[Ping Chen [:rnons] (away)]

Sorry about the mess. In my opinion, buildRecipientRows is only used for other.header at the moment, not for any recipient fields. So one option is renaming buildRecipientRows to buildOtherHeaderRow and removing all auto-completion and pills related code from it.

Comment 3

[Thomas D. (:thomas8)]

(In reply to Ping Chen (:rnons) from comment #2)

Sorry about the mess.

No problem, it wasn't broken...

In my opinion, buildRecipientRows is only used for other.header at the moment, not for any recipient fields. So one option is renaming buildRecipientRows to buildOtherHeaderRow and removing all auto-completion and pills related code from it.

That isn't just an opinion, it's a fact! ;-) - only used for building other.header rows atm:
https://searchfox.org/comm-central/search?path=&q=buildRecipientRows
https://searchfox.org/comm-central/rev/f85c4e3ebe5c142fe2ed6f6e064a421720bdaa42/mail/components/compose/content/MsgComposeCommands.js#3899-3915

In spite of that, my gut feeling says that we could just keep the autocomplete flavor of this function anyway, maybe it'll be useful some time. Perhaps addons might want to create auto-completing fields... Otoh, we don't usually keep unused code. I'll let Alex decide.

If we go for the reduced function, we could consider moving a bit more stuff from the caller side into the buildOtherHeaderRow function. Thanks Ping for noticing that the function name in the current layout should be singular as we're only creating one row at a time.

Alex, wrt function names Sg. vs. Pl., pls take note ;-)

Comment 4

[Thomas D. (:thomas8)]

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

No problem, it wasn't broken...

Well, except for bug 1663583 ;-)

Comment 5

[Alessandro Castellani [:aleca]]

Comment on attachment 9174284 [details] [diff] [review]
1663526_cleanupRawHeaderInput.diff

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

I agree that we should keep this method also for autocomplete as with Magnus we discussed the possibility of dynamically generating each hidden addressing row only when the user clicks the labels to reveal them (Cc, Bcc, etc.).
Some lint fixes and small improvements for this.

Also, let's update the commit message to follow the format we use for follow ups:
Bug 1663526 follow-up - Cleanup buildRecipientRows() to create simple or autocomplete input addressing rows. r=aleca

::: mail/base/content/mailWidgets.js
@@ +2316,3 @@
>       * @return {Element} - The newly created recipient row.
>       */
>      buildRecipientRows(recipient, rawInput = false) {

Indeed, as pointed out, let's rename this as singular.

@@ +2417,5 @@
> +        // before going into autocompletion.
> +        input.onBeforeHandleKeyDown = event => {
> +          addressInputOnBeforeHandleKeyDown(event);
> +        };
> +        input.addEventListener("blur", () => {addressInputOnBlur(input);});

es-lint error, addressInputOnBlur(input); should be on its own line.

@@ +2422,5 @@
> +      } else {
> +        // Raw input from other header.
> +        input.addEventListener("blur", () => {
> +          input.closest(".address-container").removeAttribute("focused");
> +        });

Let's avoid the callback else only for the blur listener and update the addressInputOnBlur method to return if the input doesn't have the autocompletesearch attribute.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +667,5 @@
> +        // Prevent Shift+Tab from firing again where we move the focus to.
> +        event.preventDefault();
> +        input
> +          .closest("mail-recipients-area")
> +          .moveFocusToPreviousElement(input);

es-lint error, this all fit on the same line.

Comment 6

[Thomas D. (:thomas8)]

Attachment: 1663526_cleanupRawHeaderInput.diff

(In reply to Alessandro Castellani (:aleca) from comment #5)
Thanks for swift review! Nits addressed in this patch.

Also, let's update the commit message to follow the format we use for follow ups:
Bug 1663526 follow-up - Cleanup...

Yes, but note the difference between follow-up in the same bug vs. follow-up in another bug (applies here):
Bug 1663526 - Bug 1658890 follow-up: Cleanup...

A search for "follow" in commit messages reveals that everybody seems to have their own syntax, but for the same-bug case, I think this one looks best (most uniform in the list):
Bug 123456 - Follow-up: Description...

Let's avoid the callback else only for the blur listener and update the
addressInputOnBlur method to return if the input doesn't have the
autocompletesearch attribute.

Elegant!

Comment 7

[Thomas D. (:thomas8)]

Comment on attachment 9174597 [details] [diff] [review]
1663526_cleanupRawHeaderInput.diff

Ooops, this was the wrong patch, not yet updated.

Comment 8

[Thomas D. (:thomas8)]

Attachment: 1663526_cleanupRawHeaderInput.diff

• address nits of comment 5
• function addressInputOnBlur(element): replace element with input (we've done that elsewhere, too). Whilst I appreciate that generic element argument is convenient sometimes, I think it's not helpful for reading and understanding the code. With so many different elements around (containers, rows, pills, inputs), I think it's helpful to identify the specific element in the argument name and in the function body. Also easier for callers imo.

Comment 9

[Alessandro Castellani [:aleca]]

Comment on attachment 9174604 [details] [diff] [review]
1663526_cleanupRawHeaderInput.diff

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

This is good.
Just a little eslint issue to fix.

Are you able to push a try-run once the patch is updated?
We should run the tests before checking this in.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +765,5 @@
>  
> +  if (input.getAttribute("is") != "autocomplete-input") {
> +    // For other headers aka raw input, remove whitespace-only and we are done.
> +    if (!input.value.trim()) {
> +      input.value="";

eslint issue, there are no spaces around the equal sign.
Also here we could simplify it with `input.value = input.value.trim();` so we skip the condition and clear any whitespace leftover even if something was written.

Comment 10

[Thomas D. (:thomas8)]

Attachment: 1663526_cleanupRawHeaderInput.diff

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

Also here we could simplify it with input.value = input.value.trim(); so
we skip the condition and clear any whitespace leftover even if something
was written.

Yeah, had that and discarded it thinking that me must preserve data integrity of raw headers, but you're right, this is better, and TB 68 does the same. TB 78 also trims before adding the header to the internal object, another reason to also reflect that in the UI.

So this is ready for a try-run by someone who is able to push that.

Comment 11

[Alessandro Castellani [:aleca]]

Try run launched: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b5054f4d6a30734b848d31388323864539120834

Comment 12

[Alessandro Castellani [:aleca]]

bct5 failure is expected, and the X1 seems an intermittent failure that is already being tracked.
This seems good to land.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/068536f06cfa
Bug 1658890 follow-up: Cleanup buildRecipientRow(): creation of raw/other header vs. autocomplete input. r=aleca

Comment 14

[Thomas D. (:thomas8)]

Thank you, good people, Alex & Kai!

Comment 15

[Magnus Melin [:mkmelin]]

Not for 78, unless needed to make other uplifts easier.

Comment 16

[Thomas D. (:thomas8)]

Comment on attachment 9174749 [details] [diff] [review]
1663526_cleanupRawHeaderInput.diff

(In reply to Magnus Melin [:mkmelin] from comment #15)

Not for 78, unless needed to make other uplifts easier.

We need this to uplift dependent bug 1663583 to fix keyboard handling of other headers.

[Approval Request Comment]
Regression caused by (bug #): unfinished job of bug 1658890
User impact if declined: other headers no longer handling keyboard events for navigation, row deletion etc.
Testing completed (on c-c, etc.): yes, see comment 12, and my own testing.
Risk to taking this patch (and alternatives if risky): low

Comment 17

[Rob Lemley [:rjl]]

Comment on attachment 9174749 [details] [diff] [review]
1663526_cleanupRawHeaderInput.diff

[Triage Comment]
Required for bug 1663583.

Comment 18

[Rob Lemley [:rjl]]

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