Wrap the recipients area of the compose window into a Custom Element
Categories
(Thunderbird :: Message Compose Window, enhancement, P2)
Tracking
(Not tracked)
People
(Reporter: aleca, Assigned: aleca)
References
Details
Attachments
(1 file, 3 obsolete files)
49.30 KB,
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
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?
Assignee | ||
Updated•6 years ago
|
Comment 2•6 years ago
|
||
Assignee | ||
Comment 3•6 years ago
•
|
||
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 themail-address-pill
own CE? - Should we maybe consider moving the
mail-recipients-area
and themail-address-pill
into a dedicated file inside thecomponents/compose/
folder? Maybe aMsgComposeWidgets.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•6 years ago
|
||
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•6 years ago
|
||
Assignee | ||
Comment 6•6 years ago
|
||
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?
Assignee | ||
Comment 7•6 years ago
|
||
I updated the patch in order to keep all those input fields with IDs statically written in the XHTML file.
Assignee | ||
Comment 8•6 years ago
|
||
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•6 years ago
|
||
(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 thelabel 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
Assignee | ||
Comment 10•6 years ago
|
||
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•6 years ago
|
||
Not sure I follow.
I guess we could land this and handle the id thing separately.
Updated•6 years ago
|
Comment 12•6 years ago
|
||
(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•6 years ago
|
||
On the contrary, defining many things in the markup is more places to change for each possible future change.
Assignee | ||
Updated•6 years ago
|
Comment 14•6 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/1f8ec0da7271
Wrap the recipients area of the compose window into a Custom Element. r=mkmelin
Updated•6 years ago
|
Description
•