1601740 - Wrap the recipients area of the compose window into a Custom Element

Status: RESOLVED FIXED

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

Description

[Alessandro Castellani (:aleca)]

Bug 440377 introduced the new mail-address-pill Custom Element which uses methods like getSiblingPills(), getAllPills(), and getAllSelectedPills() to collect and interact with other pills in the recipients area.

This is not correct and a CE shouldn't use methods bleeding into the DOM and outside its self contained nature.
We should convert the entire recipients area into a Custom Element in order to properly handle these scenarios.

Comment 1

[Alessandro Castellani (:aleca)]

Attachment: 1601740-recipients-area.patch

This is nowhere near complete or final, it's just a proof of concept on how to handle the entire recipient area.

I created a Custom Element and I'm generating all the various recipient fields in JS.

The various methods I copied are not used so far, so please ignore those for now.
The idea is to hook the various mouse and keypress events to this container CE and, based on the target, trigger the various effects on the pills.

What do you think about this approach?
Do you have some directions or suggestions for a more sane and logical approach?

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9116609 [details] [diff] [review]
1601740-recipients-area.patch

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

Probably reasonable. Can't really say since not enough is set up yet.

::: mail/base/content/mailWidgets.js
@@ +2221,5 @@
> +      if (this.delayConnectedCallback()) {
> +        return;
> +      }
> +
> +      this.setAttribute("id", "recipientsContainer");

shouldn't set id, that would be in the markup

@@ +2260,5 @@
> +          class: "pop-imap-input",
> +          type: "addr_reply",
> +        },
> +        newsgroups: {
> +          id: "newsgroupsAddrInput",

We shouldn't really be using ids here.

@@ +2284,5 @@
> +    }
> +
> +    buildRecipientRows(recipient) {
> +      let row = document.createXULElement("hbox");
> +      row.setAttribute("id", recipient.row);

... and to set them.
There could (in theory) be any recipient areas in one window, and then you'd be hosed.

Comment 3

[Alessandro Castellani (:aleca)]

Attachment: 1601740-recipients-area.patch

The entire area has been converted to a CE.

All the event listeners are now generated from within the mail-recipients-area, so all the query selections are containerized.
Also, since all the recipient inputs are now generated inside the CE, the setupAutocompleteInput() method doesn't need to be called when the compose window is loaded.

We shouldn't really be using ids here.

I need those IDs for the input fields. What kind of problem we could stumble upon by using IDs? Do you have a better approach to suggest?

There could (in theory) be any recipient areas in one window, and then you'd be hosed.

I'm not sure I understood this, and I can't imagine a scenario where the compose window gets generated without any recipient.

Other questions

• Does this feel weird? Having the pill event listeners handled in the mail-recipients-area instead of the mail-address-pill own CE?
• Should we maybe consider moving the mail-recipients-area and the mail-address-pill into a dedicated file inside the components/compose/ folder? Maybe a MsgComposeWidgets.js file?

Here's a try-run to see how many things got broken by these changes: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ffe52a16b599a9705a251a6a104bd7a290faf1c5

Comment 4

[Magnus Melin [:mkmelin]]

Looks pretty ok I think. The pill specific things in startEditing should probably be moved inside the pill class instead.

So, regarding ids, you should not use those internally in a custom element. An element should be self contained, and it should be possible to have many of the same elements in the same document without problems. If you use id's things won't work if that ever is the case. Sure, we don't do it in the UI at least atm, but it's still good practice to keep away from the ids since it means your component would be polluting the global document with potential difficult to track bugs in the future. What you can do instead is to querySelector on a suitable class (or attribute, but that's slower). Maybe create a class recipienttype-to, recipienttype-cc etc...

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9117171 [details] [diff] [review]
1601740-recipients-area.patch

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

::: mail/base/content/mailWidgets.js
@@ +2019,5 @@
> +  class MailRecipientsArea extends MozXULElement {
> +    connectedCallback() {
> +      if (this.delayConnectedCallback()) {
> +        return;
> +      }

should also protect against running more than once

Comment 6

[Alessandro Castellani (:aleca)]

I moved the startEditing parts of the pill back into the pill class, and added this.hasChildNodes() in the connectedCallback method.

Regarding the IDs, I see what you're saying, thanks for explaining.

This is kinda of a unique case since I don't see this CE being used anywhere else, being very specific of the messenger compose, so setting IDs here doesn't seem too bad.
I need those IDs for quick focus selection and for the label control="" attribute for accessibility and focus, so, if you think this is super odd and you feel more comfortable in not setting IDs inside a CE (which I can totally relate), I'd like to propose to revert back those input fields as statically written in the composer window, instead of being generated inside the CE.
In this way we won't deal with any ID, and the entire CE can be reused in the future for the address book or the calendar attendees section, if we want to.
What do you think?

Comment 7

[Alessandro Castellani (:aleca)]

Attachment: 1601740-recipients-area.patch

I updated the patch in order to keep all those input fields with IDs statically written in the XHTML file.

Comment 8

[Alessandro Castellani (:aleca)]

Attachment: 1601740-recipients-area.patch

A little bit of clean up to remove unnecessary strings and to initialize the autocomplete of pre-existing inputs.
Here's a try-run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bbc49f206b3e88d77592a67be7570a89944874b2

Comment 9

[Magnus Melin [:mkmelin]]

(In reply to Alessandro Castellani (:aleca) (PTO to 17th Jan 2020, sporadically reading bugmail) from comment #6)

This is kinda of a unique case since I don't see this CE being used anywhere else, being very specific of the messenger compose, so setting IDs here doesn't seem too bad.
I need those IDs for quick focus selection and for the label control="" attribute for accessibility and focus,

Well you can set ids, if you just set them to something unique, i.e. random, or with a global ever increasing counter, like addrTo-333

Besides that, patch looks ok

Comment 10

[Alessandro Castellani (:aleca)]

Well you can set ids, if you just set them to something unique, i.e. random, or with a global ever increasing counter, like addrTo-333

Ah, you're right, that's a good point.
I think in this case it's preferred to leave those input fields statically written in the XHTML file as it allows us to easily reuse the recipients CE somewhere else without worrying about IDs.

Comment 11

[Magnus Melin [:mkmelin]]

Not sure I follow.
I guess we could land this and handle the id thing separately.

Comment 12

[Thomas D.]

(In reply to Alessandro Castellani (:aleca) (PTO to 17th Jan 2020, sporadically reading bugmail) from comment #10)

Well you can set ids, if you just set them to something unique, i.e. random, or with a global ever increasing counter, like addrTo-333

Ah, you're right, that's a good point.
I think in this case it's preferred to leave those input fields statically written in the XHTML file as it allows us to easily reuse the recipients CE somewhere else without worrying about IDs.

I like that approach (static inputs in XHTML with stable, explicitly defined IDs) in terms of transparency, future maintenance, add-ons, etc.
The alternative of creating dynamic IDs inside the CE looks like a transparency loss and maintenance risk.

Comment 13

[Magnus Melin [:mkmelin]]

On the contrary, defining many things in the markup is more places to change for each possible future change.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1f8ec0da7271
Wrap the recipients area of the compose window into a Custom Element. r=mkmelin