Closed Bug 1609647 Opened 5 years ago Closed 5 years ago

Add "Move to To/Cc/Bcc" to recipient pill context menu

Categories

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

Product:
Thunderbird
Component:
Message Compose Window
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 75.0

People

(Reporter: jorgk-bmo, Assigned: aleca)

References

Details

(Keywords: ux-efficiency, ux-minimalism)

Attachments

(1 file, 5 obsolete files)

1609647-recipient-context.diff
(deleted), patch
aleca
: review+
Details | Diff | Splinter Review

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

As per bug 440377 comment #216 and before and after.

Assignee: nobody → alessandro
Status: NEW → ASSIGNED
Priority: P2 → P1
Version: unspecified → Trunk

Alexandro,

Would the same feature be available for a selection of pills? e.g via selection context menu?

To allow move of multiple pills selected (from various fields) at once?

May be nice to have as well perhaps...

Regards,

(In reply to Richard Leger from comment #1)

Would the same feature be available for a selection of pills? e.g via selection context menu?

Yes, absolutely.

Attached patch 1609647-recipient-context.diff (obsolete) (deleted) — — Splinter Review
Attachment #9126536 - Flags: review?(mkmelin+mozilla)
Attachment #9126536 - Flags: feedback?(bugzilla2007)
Attachment #9126536 - Attachment description: 1609647-recipient-context.diff → 1609647-recipient-context.patch

Thomas:
1609647-recipient-context.diff → 1609647-recipient-context.patch

Why?
diff and patch are handled in the same way by bugzilla, mercurial, and treeherder.
diff is technically the correct extension that should be used since it works in Phabricator, which we will probably start using sooner or later.

Comment on attachment 9126536 [details] [diff] [review] 1609647-recipient-context.diff (In reply to Alessandro Castellani (:aleca) from comment #4) > `diff` is technically the correct extension that should be used since it works in Phabricator, which we will probably start using sooner or later. Ah ok, I see.
Attachment #9126536 - Attachment description: 1609647-recipient-context.patch → 1609647-recipient-context.diff
Comment on attachment 9126536 [details] [diff] [review] 1609647-recipient-context.diff Review of attachment 9126536 [details] [diff] [review]: ----------------------------------------------------------------- Thank you! I am very happy to see this tackled, and this looks promising and seems to work well. I like the first-level contextual actions which legacy users will appreciate. We only offer this for the most likely candidates (not for rare Reply-To and friends), I think that's ok. As usual, we need to cover some edge cases, e.g. cross-field selections and newsgroups. Moved pills should stay selected after moving for a visual success feedback and potential recovery from wrong moves. ::: mail/components/compose/content/addressingWidgetOverlay.js @@ +745,5 @@ > + * @param {Event} event - The DOM event. > + */ > +function onEmailAddressPillPopupShowing(element, event) { > + // Reset previously disabled menuitems. > + for (let menuitem of event.target.querySelectorAll(".pill-action-move")) { Elegant. @@ +754,5 @@ > + switch (pill.getAttribute("recipienttype")) { > + case "addr_to": > + event.target > + .querySelector("#moveAddressPillTo") > + .setAttribute("disabled", true); **Edge case handling** 1) Cross-field selections: This is good, but not good enough for cross-field selections, where it becomes inconsistent. If I select one pill each from To, CC, BCC, and BCC happens to have the last focus, I can move all 3 pills to To or CC (net move of 2 pills, 1 from target row just remains in-place), but not to BCC, which is a random restriction given that the other two cases work although they also contain selected cells from the target field. I think we should only disable anything for the following cases: - If there is no selected pill at all, disable all move-actions - If theres is no selected pill *outside* the row which currently has focus, disable that move-action to that row. E.g. 2 pills selected in To, but nothing selected in CC or BCC, then "move-to-To" must be disabled, as moving a selection of only(!) To recipients to To does not make sense because it doesn't change anything, but we want to be lenient and allow to move cross-selection of (1 To and 1 CC) into CC which will only move the 1 To as a net result. Disabling for cross-type selections including the target row would imo impose unnecessary and potentially difficult restriction on the user. To check for selected pill in rows other than focus row, just use querySelector([selected]....) to check if there's at least one selected pill in the other row. 2) when (regular or cross-) selections include at least one pill with a newsgroup-type-address, we should disable all move actions to To, CC, BCC, because these are for email addresses only. @@ +780,5 @@ > + * @param {string} recipient - The target recipient type, e.g. "addr_to". > + */ > +function moveAddressPill(element, recipient) { > + // Store all the selected addresses inside an array. > + let allAddresses = []; Varibale name: Pls use `let selectedPills = []` (Not all, selected only, and it's pill elements.) @@ +788,5 @@ > + allAddresses.push(pill.fullAddress); > + } > + > + // Remove all the selected pills. > + deleteAddressPill(element); Can we please rename this method to "deleteSelectedPills(...)"? Irritates every time to see singular, and somehow suggests that we'd delete focused but not selected pills, which we don't. @@ +791,5 @@ > + // Remove all the selected pills. > + deleteAddressPill(element); > + > + // Create new addressing pills inside the target recipient row. > + awAddRecipientsArray(recipient, allAddresses); selectedPills @@ +795,5 @@ > + awAddRecipientsArray(recipient, allAddresses); > + > + // Move focus to the target recipient input field. > + let label = document.getElementById(recipient); > + document.getElementById(label.getAttribute("control")).focus(); Auto-focusing rowInput is very irritating, because now it's hard to visually see the success of moving pills, and also to recover from errors like accidentally moving them to the wrong field. I think we want to keep all those pills which have been successfully moved selected. For focus, if the original focus was on a selected pill, keep it, otherwise focus last selected pill after the move. ::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl @@ +15,5 @@ > *[other] { $type } input field with { $count } addresses > } > + > +pill-action-edit = > + .label = Edit Contact NOT "Edit Contact" because that's reserved for editing contact cards in AB. I think here we want "Edit address" @@ +24,5 @@ > + .accesskey = t > + > +pill-action-move-cc = > + .label = Move to Cc: > + .accesskey = c Hmm. We have &Copy and Move to &CC (two actions with access key 'C'). Unique access keys might be better imo, but we definitely want the best access keys for these move actions. Could we change the access key for Copy to something else (maybe 'o') because there's Ctrl+C for copy so using keyboard to get copy from context menu is pointless and discouraged? Well, that interferes with toolkit defaults. Or we can just leave it as-is and user needs to press C twice, then enter for "move to Cc"
Attachment #9126536 - Flags: feedback?(bugzilla2007) → feedback+

(In reply to Thomas D. from comment #7)

Auto-focusing rowInput is very irritating, because now it's hard to visually
see the success of moving pills, and also to recover from errors like
accidentally moving them to the wrong field.

Would CTRL+Z allow to quickly cancel selected pills move in case of error... returning pills to original location in selected state... as it was just before triggering move?

Would Undo action also be made available in context menu of pills selection still focus after move?

This might be handy for fast recovery in case of major end-user error spotted immediately :-)

Comment on attachment 9126536 [details] [diff] [review] 1609647-recipient-context.diff Review of attachment 9126536 [details] [diff] [review]: ----------------------------------------------------------------- This has bitrotted ::: mail/components/compose/content/addressingWidgetOverlay.js @@ +739,5 @@ > > /** > + * Disable the context menu item that matches the current addressing row. > + * > + * @param {XULElement} element - The element from which the context menu was Let's keep it at Element ::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl @@ +19,5 @@ > + .label = Edit Contact > + .accesskey = e > + > +pill-action-move-to = > + .label = Move to To: Maybe these should be in the form "Move to the To field"
Attachment #9126536 - Flags: review?(mkmelin+mozilla)

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

Comment on attachment 9126536 [details] [diff] [review]
::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
@@ +19,5 @@
(...)

+pill-action-move-to =

  • .label = Move to To:

Maybe these should be in the form "Move to the To field"

From an end-user point of view, I would suggest to keep "Move to To:" because if make sufficient sense and is easier to read (also take less space)...

It would be read along other possible actions anyway... so unlikely to cause any confusion...
(...)
Move to To:
Move to Cc:
Move to Bcc:
(...)

You could eventually even ditch the trailing colon ':' ...
(...)
Move to To
Move to Cc
Move to Bcc
Move to Reply-To
(...)

You could also separate and regroup some of the actions within horizontal lines in the context menu to make it easier to read/reach when a long list of options possible... I don't know if that is already in place or in the pipeline...

Edit Address
Edit Contact

Move to To
Move to Cc
Move to Bcc
Move to Reply-To

Delete

Uh, so many comments, I'll try to answer everyone while I update the patch.

Thomas

Moved pills should stay selected after moving for a visual success feedback and potential recovery from wrong moves.

Good point, I'll do that.

Cross-field selections

I'll handle this with these options:

  • Disable all if a newsgroups or followups pill is currently selected.
  • Disable the current one if all the selected pills are part of the same addressing row (all pills from To).
  • Don't disable anything if there's cross selection as it makes things easier for us and it doesn't block any action.

Varibale name: Pls use let selectedPills = [] (Not all, selected only, and it's pill elements.)

We're not selecting pills here but pulling the address string out. I'll rename it to selectedAddresses.

Can we please rename this method to "deleteSelectedPills(...)"?

Sure.

Auto-focusing rowInput is very irritating...

As you suggested before, we'll keep the focus on the moved pills.

I think here we want "Edit address"

Sure.

Richard

Would CTRL+Z allow to quickly cancel selected pills move in case of error...

I'm not sure about that as we would need to store every single user action and keep a history of what they're doing in order to enable undos.
I think by keeping the moved pills selected, users will immediately see if the action was successful, and having those elements still selected will allow him to put them back where they were.

You could also separate and regroup some of the actions within horizontal lines in the context menu...

Already did that :)

Magnus

Maybe these should be in the form "Move to the To field"

I think we should keep it as "Move to To", etc, as it's short and easy to read. Also, it matches the format we're currently using in the Address Book sidebar (Add to To, Add to Cc, etc).

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

Would CTRL+Z allow to quickly cancel selected pills move in case of error...

I'm not sure about that as we would need to store every single user action and keep a history of what they're doing in order to enable undos.

At least the last action if not all...

I think by keeping the moved pills selected, users will immediately see if the action was successful, and having those elements still selected will allow him to put them back where they were.

Your suggestion is not possible if pills were selected from various fields and moved at once...

CTRL+Z would be a must to have not urgent and be implemented last in a follow-up bug...

In Mail tab if I move a message into a sub-folder I can press CTRL+Z to put it back were it was... CTRL+Z may not be easy to implement put should be possible as last feature implemented...

It would be awckward not to allow Undo for selected pills moved in Mail Compose Window...

Mh, yeah, if we limit the Undo to only the most recent pill movement, I think it could be implemented.
I'll see if it's doable in this patch, otherwise I'll do a follow up.

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

Awesome! Looking forward to next iteration.

Attached patch 1609647-recipient-context.diff (obsolete) (deleted) — — Splinter Review

Patch updated.

This is a quick recap of the context menu when pills are selected:

  • Disable all items if a newsgroups or followup pill is currently selected.
  • Disable the current item target if all the currently selected pills are part of the same addressing row (eg. all pills are from the To field).
  • Don't disable anything if there's cross selection (various pills are selected from multiple addressing rows) as it makes things easier for us and it doesn't block any action for the user.

I tried to shrink the code as much as possible by using map() and filter() but without success.
I don't why but I couldn't properly filter that array and ended up using a simple for() loop.

Attachment #9126536 - Attachment is obsolete: true
Attachment #9126863 - Flags: review?(mkmelin+mozilla)
Attachment #9126863 - Flags: feedback?(bugzilla2007)
Attached patch 1609647-recipient-context.diff (obsolete) (deleted) — — Splinter Review

A little update to move the focus to the last selected pill.
Sorry for the noise.

Attachment #9126863 - Attachment is obsolete: true
Attachment #9126863 - Flags: review?(mkmelin+mozilla)
Attachment #9126863 - Flags: feedback?(bugzilla2007)
Attachment #9126864 - Flags: review?(mkmelin+mozilla)
Attachment #9126864 - Flags: feedback?(bugzilla2007)
Comment on attachment 9126864 [details] [diff] [review] 1609647-recipient-context.diff Review of attachment 9126864 [details] [diff] [review]: ----------------------------------------------------------------- Thank you very much Alex for fine-tuning the behaviour; I'm aware that some of these things are non-trivial. I haven't tested this but it looks functional and matching the specs (feedback+ only for that). I have some questions about code distribution, micro-performance and alternative notations. ::: mail/base/content/mailWidgets.js @@ +2198,5 @@ > * > * @param {HTMLElement} element - The original autocomplete input that > * generated the pill. > * @param {Array} address - The array containing the recipient's info. > + * @param {boolean} select - If the newly generate pill should be selected. typo: generated ::: mail/components/compose/content/addressingWidgetOverlay.js @@ +377,5 @@ > * and drops in the array of addresses. > * > * @param aRecipientType Type of recipient, e.g. "addr_to". > * @param aAddressArray An array of recipient addresses (strings) to add. > + * @param {boolean} select - If the newly generate pills should be selected. typo: generated @@ +728,5 @@ > > /** > * Delete the selected pill(s). > * > * @param {XULElement} element - The element from which the context menu was If we know that this is always the label, for clarity this should read: - The label element... @@ +736,1 @@ > // element is the pill's <label>, get the pill. Let's be more informative whilst we are here... // element is the <label> of the focused pill, get the pill itself. @@ +738,5 @@ > document.getElementById("recipientsContainer").removeSelectedPills(pill); > } > > /** > + * Disable the context menu item that matches the current addressing row. * Handle disabling of "Move to ..." context menu items according to the types of selected pills. @@ +744,5 @@ > + * @param {Element} element - The element from which the context menu was > + * opened. > + * @param {Event} event - The DOM event. > + */ > +function onEmailAddressPillPopupShown(element, event) { I'd prefer emailAddressPillOnPopupShown(), but it's a matter of taste. @@ +754,5 @@ > + // Add the recipient type of the pill where the context menu was opened. > + // We need to do this since the selection of the pill happens after the > + // context menu has been opened. > + let selectedTypes = [ > + element.closest("mail-address-pill").getAttribute("recipienttype"), I'm failing to understand the explanation, and I think this is wrong. As I explained elsewhere, with mouse or keyboard it's very possible to trigger the context menu from "satellite focus" (focused, but unselected pill), so the type of the focused pill does not matter unless if it's part of selection, whose types you check below. E.g.: User triggers "move-to" from satellite focus pill in newsgroup field, then move-to should be enabled if there's no other pill selected in newsgroup. @@ +758,5 @@ > + element.closest("mail-address-pill").getAttribute("recipienttype"), > + ]; > + > + // Add all the recipient types of the selected pills. > + for (let type of document let pill @@ +760,5 @@ > + > + // Add all the recipient types of the selected pills. > + for (let type of document > + .getElementById("recipientsContainer") > + .getAllSelectedPills()) { Micro-performance: You are iterating all selected pills (which might be hundreds, even hundreds of the same type) just to find out if *one* of each type exists. I'd suspect we'd be better off looping all existing rows/types (small, limited number) and bailing out when we find *one* selected pill of that type using querySelector(), then add it to the types array. After each row, check if types.length > 1 and bail out if true, no need to check any other rows if we check news types in advance. Something like that. @@ +768,5 @@ > + } > + > + // If Newsgroups or Followups are part of the selection, disable everything. > + if ( > + selectedTypes.includes("addr_newsgroups") || Maybe better to use a class to get news types so that custom news types can be covered? And since newsgroup fields typically have zero or 1 address, can't we check this at the beginning so we skip checking any other rows if this fails? @@ +806,5 @@ > + > +/** > + * Move the selected pills email address to another addressing row. > + * > + * @param {XULElement} element - The element from which the context menu was XUL is on its way out: `{Element}` @@ +810,5 @@ > + * @param {XULElement} element - The element from which the context menu was > + * opened. > + * @param {string} recipient - The target recipient type, e.g. "addr_to". > + */ > +function moveAddressPill(element, recipient) { Suggested rename: moveSelectedPills(...) Shouldn't such functions live in mailWidgets.js? I'm failing to see the structural difference between removeSelectedPills() and moveSelectedPills() that would force us to keep them in different files. I have the same question for most of the pills stuff which lives in addressingWidgetOverlay.js. The random and intertwined distribution of functionality between the two files does not look very sustainable. I'd consider these functions part of the internal implementation of the recipients area custom element, and we should avoid getting the "recipientsContainer" from outside by ID when we can get it from inside the current recipient area custom element. Any outside function makes it harder to re-use the entire recipient area for other things like mailing lists. If you still want to keep this function here, using .closest(...) to get the area would mitigate a bit. If we really need an outer layer of functions which is hooked up to the xhtml, I think they should just call the inner functions from mailWidgets.js, so that we bundle all the inner functionality there. @@ +812,5 @@ > + * @param {string} recipient - The target recipient type, e.g. "addr_to". > + */ > +function moveAddressPill(element, recipient) { > + // Store all the selected addresses inside an array. > + let selectedAddresses = []; Have you tried this? Maybe easier notation. I don't know which way is better wrt micro-performance. let selectedAddresses = [...document.getElementById("recipientsContainer").getAllSelectedPills()].map(pill => pill.fullAddress); There are several ways of doing this, e.g. let nodelist = document.getElementById("recipientsContainer").getAllSelectedPills(); - nodeList.forEach(pill => selectedAddresses.push(pill.address)) - let selectedAddresses = Array.from(nodeList).map(pill => pill.fullAddress) @@ +826,5 @@ > + // Create new addressing pills inside the target recipient row and maintain > + // the current selection. > + awAddRecipientsArray(recipient, selectedAddresses, true); > + > + // Move focus to the last selected pill. Yeah, maybe we can get away with that and skip remembering the original selected focus pill if any.
Attachment #9126864 - Flags: feedback?(bugzilla2007) → feedback+
Comment on attachment 9126864 [details] [diff] [review] 1609647-recipient-context.diff Review of attachment 9126864 [details] [diff] [review]: ----------------------------------------------------------------- After moving an addr from To to Bcc, going directly to move more, next address in To still allows move to To ::: mail/base/content/mailWidgets.js @@ +2203,2 @@ > */ > + createRecipientPill(element, address, select = false) { instead of passing a select option, I'd make this return the pill, and let the caller do the select ::: mail/components/compose/content/addressingWidgetOverlay.js @@ +772,5 @@ > + selectedTypes.includes("addr_newsgroups") || > + selectedTypes.includes("addr_followup") > + ) { > + for (let menuitem of event.target.querySelectorAll(".pill-action-move")) { > + menuitem.setAttribute("disabled", true); would be "true" But better just use menuitem.disabled = true which is also right for html @@ +786,5 @@ > + switch (selectedTypes[0]) { > + case "addr_to": > + event.target > + .querySelector("#moveAddressPillTo") > + .setAttribute("disabled", true); same for these ::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl @@ +15,5 @@ > *[other] { $type } input field with { $count } addresses > } > + > +pill-action-edit = > + .label = Edit address We capitalize each word in labels (not to/for and such) Anyway, shouldn't show this item for when it's a multiselect
Attachment #9126864 - Flags: review?(mkmelin+mozilla)

Thanks both for the feedback and review. While I'm updating everything accordingly, here's some comments:

I'm failing to understand the explanation, and I think this is wrong.

I did that as a workaround to avoid the issue of not having the pill selected when the right click happens.
Unfortunately, the context menu gets triggered before the pill gets selected if clicking with the right mouse button on an unselected pill.
I solved this issue by triggering the update of the context menu only on specific occasions, and avoided this "hack".

Micro-performance: You are iterating all selected pills (which might be hundreds, even hundreds of the same type) just to find out if one of each type exists.

Good point, I changed it.

Shouldn't such functions live in mailWidgets.js? I'm failing to see the structural difference between removeSelectedPills() and moveSelectedPills() that would force us to keep them in different files.

Yes, I think some of those methods can be moved inside the CE.
I think this is mostly a leftover from the old code, where we had all the addressing functions inside the addressingWidgetOverlay.
I think I'll move the moveSelectedPills() method inside the CE, but for a more targeted code clean up we can maybe deal with it in a dedicated bug.

let selectedAddresses = [...document.getElementById("recipientsContainer").getAllSelectedPills()].map(pill => pill.fullAddress);

This is good, thanks.

Attached patch 1609647-recipient-context.diff (obsolete) (deleted) — — Splinter Review
Attachment #9126864 - Attachment is obsolete: true
Attachment #9127171 - Flags: review?(mkmelin+mozilla)
Attachment #9127171 - Flags: feedback?(bugzilla2007)
Comment on attachment 9127171 [details] [diff] [review] 1609647-recipient-context.diff Review of attachment 9127171 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +2198,5 @@ > * > * @param {HTMLElement} element - The original autocomplete input that > * generated the pill. > * @param {Array} address - The array containing the recipient's info. > + * @return {XULElement} The newly created pill. {Element} should be enough @@ +2386,5 @@ > + if (pill.isEditing) { > + return; > + } > + > + if (pill.hasAttribute("selected") && event.which == 3) { which is deprecated. can you switch to key? I'm not sure what 3 means @@ +2459,5 @@ > + * Move the selected pills email address to another addressing row. > + * > + * @param {Element} element - The element from which the context menu was > + * opened. > + * @param {string} recipient - The target recipient type, e.g. "addr_to". instead of recipient, maybe targetFieldType?
Attachment #9127171 - Flags: review?(mkmelin+mozilla) → review+
Attached patch 1609647-recipient-context.diff (obsolete) (deleted) — — Splinter Review

Patch updated and try run launched: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e3144aee87d6a4764b73ae13713a441fecbff2f1

which is deprecated. can you switch to key? I'm not sure what 3 means

event.key doesn't carry which button is pressed on the mouse as I need to listen to the right click.
I've updated it with event.button.

Attachment #9127171 - Attachment is obsolete: true
Attachment #9127171 - Flags: feedback?(bugzilla2007)
Attachment #9127321 - Flags: review+
Attached patch 1609647-recipient-context.diff (deleted) — — Splinter Review

Fixed a tiny eslint error spotted from the try run.

Attachment #9127321 - Attachment is obsolete: true
Attachment #9127370 - Flags: review+
Keywords: checkin-needed-tb
Target Milestone: --- → Thunderbird 75.0

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/1355f132539c
Add 'Move to To/Cc/Bcc' to recipient pill context menu. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED
Comment on attachment 9127370 [details] [diff] [review] 1609647-recipient-context.diff Review of attachment 9127370 [details] [diff] [review]: ----------------------------------------------------------------- Hmmm. Maybe this was landing a bit too fast...? It probably works, but code isn't quite optimized. We should really stop adding more recipient area functions outside the mail-recipient-area CE. Inflationary use of .querySelectorAll(...) where just querySelector(...) would suffice, which is a performance loss. Inconsistent context menu item IDs. Some other nits. Probably better to clean up now whilst we're still into this; otherwise cleanup may never happen. Just saying. ::: mail/base/content/mailWidgets.js @@ +2400,5 @@ > + if (pill.isEditing) { > + return; > + } > + > + if (pill.hasAttribute("selected") && event.button == 2) { You have an almost identical condition running emailAddressPillOnPopupShown() again below, isn't there something wrong/duplicated? If this is really needed, it needs a comment for explanation. ::: mail/components/compose/content/addressingWidgetOverlay.js @@ +745,5 @@ > +function emailAddressPillOnPopupShown() { > + let menu = document.getElementById("emailAddressPillPopup"); > + > + // Reset previously disabled menuitems. > + for (let menuitem of menu.querySelectorAll( menu.querySelectorAll(".pill-action-move, .pill-action-edit").forEach( menuitem => menuitem.disabled = false ); @@ +763,5 @@ > + // If Newsgroups or Followups are part of the selection, disable everything. > + if ( > + document.querySelectorAll( > + `mail-address-pill[recipienttype="addr_newsgroups"][selected]` > + ).length || That's still checking for unlimited numbers of matching elements just to find if there is one. We're done if we find just one which is selected: if ( document.querySelector( "mail-address-pill[recipienttype=addr_newsgroups][selected]" ) || document.querySelector( "mail-address-pill[recipienttype=addr_followup][selected]" ) ) { menu.querySelectorAll(".pill-action-move").forEach( menuitem => menuitem.disabled = true ); } That said, searching the entire composition document for pills is odd and potentially error-prone in the future. You wanted to have this function inside the mail-recipient-area CE, which would be much better and safer. Also, I think we shouldn't use backticks without need when it's not a template string; should use outer single quotes, or outer double quotes and just omit inner quotes, which is legitimate for selectors. @@ +768,5 @@ > + document.querySelectorAll( > + `mail-address-pill[recipienttype="addr_followup"][selected]` > + ).length > + ) { > + for (let menuitem of menu.querySelectorAll(".pill-action-move")) { use .forEach as above @@ +776,5 @@ > + } > + > + let selectedTypes = []; > + // Add all the recipient types of the selected pills. > + for (let row of document.querySelectorAll(".address-row:not(.hidden)")) { maybe use .forEach()? @@ +777,5 @@ > + > + let selectedTypes = []; > + // Add all the recipient types of the selected pills. > + for (let row of document.querySelectorAll(".address-row:not(.hidden)")) { > + if (row.querySelectorAll("mail-address-pill[selected]").length) { dito: querySelector to find first selected pill will do, no need to go for all. @@ +781,5 @@ > + if (row.querySelectorAll("mail-address-pill[selected]").length) { > + selectedTypes.push( > + row > + .querySelector(`input[is="autocomplete-input"][recipienttype]`) > + .getAttribute("recipienttype") wouldn't it be easier to have this in the loop: let selectedPill = row.querySelector("mail-address-pill[selected]"); if (selectedPill) { selectedTypes.push(selectedPill.getAttribute("recipienttype")); } @@ +815,5 @@ > + * @param {string} targetFieldType - The target recipient type, e.g. "addr_to". > + */ > +function moveSelectedPills(element, targetFieldType) { > + document > + .getElementById("recipientsContainer") We should really stop hardcoding this ID, and have those functions inside the CE. ::: mail/components/compose/content/messengercompose.xhtml @@ +770,5 @@ > </menupopup> > > > <menupopup id="emailAddressPillPopup" class="emailAddressPopup"> > + <menuitem id="editAddressPill" We used to make such IDs more descriptive and unique by including a hint to the parent, e.g. something like: id="pillContext_editAddressPill" Same for all context menu IDs below. At least, ID naming patterns should be consistent - why some menus have "menu_..." and others don't? ::: mail/themes/shared/mail/messengercompose.css @@ +530,1 @@ > padding-block: 4px; Is this causing the pill input border to expand on editing, which in turn causes the entire row to expand vertically? Can we try to avoid such shaking of the UI?
Attachment #9127370 - Flags: review-

(In reply to Thomas D. from comment #25)
Please, avoid giving an r- on a bug that was approved by the technical director.
It's confusing and doesn't bring any benefit.

You can add a follow-up patch to clean up what just landed.

You have an almost identical condition running
emailAddressPillOnPopupShown() again below, isn't there something

Nothing wrong there as the first condition is necessary to interrupt the toggle of the selected attribute in case this is already selected, and we need to trigger the visual update of the context menu.
The second condition is in case the code went through and only the detection of which button was clicked needs to be verified.

That's still checking for unlimited numbers of matching elements just to
find if there is one.
We're done if we find just one which is selected

Sure, we can update it if we want, but the newsgroups and newsletters will always have a limited amount of pills.
I don't see anyone sending the same email to 100+ newsletters.

Also, I think we shouldn't use backticks.

We're following the Prettier code formatting which requires backticks.

use .forEach as above

Nope, forEach is slower than for.

We should really stop hardcoding this ID, and have those functions inside
the CE.

Some of these functions will only be used in this location, since the addressingWidgetOverlay.js file is only for the messenger compose window.
Is not a mistake per se retrieving the container via its ID, but as I said, you can do a follow up to clean up the code.

::: mail/themes/shared/mail/messengercompose.css
@@ +530,1 @@

padding-block: 4px;

Is this causing the pill input border to expand on editing, which in turn
causes the entire row to expand vertically? Can we try to avoid such shaking
of the UI?

This is actually fixing the issue as we're styling only the first child of the address-container, as you can see from the CSS diff.

Attachment #9127370 - Flags: review-
Blocks: 1636393
Severity: normal → N/A
Component: Composition → Message Compose Window
Product: MailNews Core → Thunderbird
Blocks: 1698461
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: