1691842 - Dragging addresses from contact sidebar to a closed addressing label also moves selected pills

Status: RESOLVED FIXED

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

Description

[Alessandro Castellani [:aleca]]

To reproduce:

• Create a pill in the To field.
• Select the created pill.
• Open the Contacts Sidebar and drag some addresses above the closed Cc label.

Expected result:

• The new addresses should be added to the newly visible field, and the selected pill in the To field should be kept selected and untouched.

Actual result:

• The selected pill in the To field is moved to the newly opened field and it's unselected.

Also, this JS error appears:

JavaScript error: chrome://messenger/content/mailWidgets.js, line 2899:
TypeError: can't access property "closest", element is undefined

This issue doesn't happen if the target recipient field is already opened. It happens only if the drop target is the closed recipient label.

Comment 1

[Thomas D. (:thomas8)]

Cunning! Good catch, Alex!

Maybe Alice could find the regression range?

Comment 2

[Thomas D. (:thomas8)]

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

This issue doesn't happen if the target recipient field is already opened. It happens only if the drop target is the closed recipient label.

https://searchfox.org/comm-central/search?q=dropAddressPill&redirect=false

ondrop on closed label triggers dropAddressPill(), which gets the selected pills as that's the only thing which it is expecting so far.
That looks wrong because it should certainly get the data from the drag-n-drop action and not just pills always. Or at least to look at the drag source before getting the pills. The error only occurs when no pill selected, because moveSelectedPills expects a pill or subpart of that.
Interestingly, the address from contacts side bar also always gets dropped in (I observed that a display name with @ was quoted sometimes, and sometimes not, where not quoted in pills display is the correct thing ).

Maybe regressed by Bug 1601749 which introduced dropAddressPill() function.

Uncaught TypeError: can't access property "closest", element is undefined
moveSelectedPills chrome://messenger/content/mailWidgets.js:2910
dropAddressPill chrome://messenger/content/messengercompose/addressingWidgetOverlay.js:1083
ondrop chrome://messenger/content/messengercompose/messengercompose.xhtml:1
mailWidgets.js:2910:18

Comment 3

[Thomas D. (:thomas8)]

Btw, we should deselect pills when contacts side bar gets focus - it's confusing to have two active selections.
I wonder if we could find a more generic algorithm for recipient area deselection as soon as the focus moves to another composition element outside the recipient area.

Comment 4

[Alessandro Castellani [:aleca]]

Yeah, bug 1601749 did it.
I'll fix this.

Comment 5

[Thomas D. (:thomas8)]

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

Btw, we should deselect pills when contacts side bar gets focus - it's confusing to have two active selections.
I wonder if we could find a more generic algorithm for recipient area deselection as soon as the focus moves to another composition element outside the recipient area.

I have a perfectly working patch for this (in bug 1663062, see comment 6).

Comment 6

[Thomas D. (:thomas8)]

Fixing dual selection is bug 1663062 (filed by me 7 months ago), so I have attached my patch there.
Per Bug 1663062 Comment 3, attachment 9210511 [details] [diff] [review] will prevent this bug from occuring by preventing the dual selection.
However, the console error from comment 0 is still there, so the internals of drag and drop are still wrong and must be fixed.

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

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

Btw, we should deselect pills when contacts side bar gets focus - it's confusing to have two active selections.
I wonder if we could find a more generic algorithm for recipient area deselection...

I have a perfectly working patch for this.

Comment 7

[Thomas D. (:thomas8)]

Function names matter: Why we should use only generic event handling hooks in the layout layer, e.g. myElementIdOnEvent()

This bug confirms the importance of using generic names for event handler functions called from the layout layer, as always advocated for by myself and accepted best practice for TB:

Wrong: specific 'action' function name (as opposed to generic event handler hook) and code in the layout layer

                         ondrop="dropAddressPill(this, 'addressRowTo', 'addr_to')"
	                 ondragover="event.preventDefault();"

The very moment you add an "action" function into your "ondrop" attribute, you are already starting to forget that this is a generic event handler which will fire according to the event and not according what you wish the event to do.
Then in the code, there'll be no trace that this a generic event handler, which makes it much harder to figure out where things go wrong:

/**
 * Move the selected pills to the container row of an hidden recipient (Cc, Bcc, etc.)
 * in drag and drop operation.
 *
 * @param {XULElement} label - The clicked label to hide.
 * @param {string} rowID - The ID of the container to reveal.
 * @param {string} recipientType - The recipient type for dropped pills to move.
 */
function dropAddressPill(label, rowID, recipientType) {

Semantic discrepancy completed:

• The function is actually a generic ondrop handler for a specific set of elements, so it should be called recipientLabelOnDrop().
• Instead, the chosen function name is unaware of the target (a recipient label) and of its generic event handler qualities: dropAddressPill() falsely suggests that this will only handle dropped pills ("text/pills"), whereas many types can be dropped on the label (e.g. addresses from contacts sidebar which are not pills, this bug).
• The function description is also unaware of the target and is all but oblivious of event handling. "Move the selected pills..." now sounds almost like a regular "action" function, as opposed to a generic ondrop handler.
• As a result of this semantic negligence and confusion, the function forgot that anything (various DataTransfer.types) can be dropped onto the label, and never handled that case, which is the other half of this bug (apart from bug 1663062: it should not be possible to select both pills and contacts in the sidebar simultaneously).

Right: Generic event handler hooks in the layout layer, specific actions only in the code layer

                             ondrop="recipientLabelonDrop(event);"
                             ondragover="recipientLabelonDragOver(event);"

/**
 *  Handle the dragover event on a recipient disclosure label.
 *
 *  @param {Event} - The DOM dragover event on a recipient disclosure label.
 */
function recipientLabelonDragover(event) {
  // Prevent dragover event's default action (which resets the current drag
  // operation to "none").
  event.preventDefault();
}

/**
 *  Handle the drop event on a recipient disclosure label.
 *
 *  @param {Event} - The DOM drop event on a recipient disclosure label.
 */
function recipientLabelonDrop(event) {
  if (event.dataTransfer.types.includes("text/pills")) {
    // If the dragged data includes the type "text/pills", we believe that
    // the user is dragging our own pills, so we try to move the selected pills
    // to the address row of the recipient label they were dropped on (Cc, Bcc,
    // etc.), which will also show the row if needed. If there are no selected
    // pills (so "text/pills" was generated elsewhere), moveSelectedPills() will
    // bail out and we'll do nothing.
    document
      .getElementById("recipientsContainer")
      .moveSelectedPills(event.target.id);
  }
}

As a rule of thumb, if your generic event handler function needs more than just event as an argument, there's most likely something wrong. Also note that all the other arguments were actually superfluous, even for the subsequent action functions. Instead of passing in this, you can always retrieve event.target in the event handler.

Comment 8

[Thomas D. (:thomas8)]

Attachment: 1691842_fixAndNormalizeRecipientLabelOnDrop+OnDragOver.diff

Bug 1691842 - Fix and normalize onDrop and onDragOver handlers of recipient disclosure labels. r=aleca

• Use generic names for event handler hooks in the layout layer so that we will
  actually know and consider what the function is really about.
• Stop trying to move selected pills when something other than "text/pills" has
  been dropped onto a closed recipient label, e.g. addresses from contacts
  sidebar. I've fixed the worst of the original privacy/ux-error-prevention
  fallout in bug 1663062 by preventing dual selection of pills and contacts.
  But even without pills selected, this was still causing a console error.
• moveSelectedPills(): Add early return if there are no pills selected.

Comment 9

[Thomas D. (:thomas8)]

I've cleaned up moveSelectedPills (now with only one argument, targetFieldType) in Bug 1700278, which we're also touching here. That bug also dropped a line of that function, let pill = element.closest("mail-address-pill");, which I understand was causing the error described in comment 0 / comment 2. After bug 1700278, the error then moved down to this line: selectedPills[selectedPills.length - 1].focus();. This patch is now preventing such errors in two ways:

• We no longer try to move pills when the drag-data does not have pills, so by default we won't get into moveSelectedPills() if no pills are involved. This change on its own will also prevent the privacy violation threat of this bug (as does bug 1663062 on its own by preventing dual selection of pills and contacts in sidebar).
• For added safety, even if some unexpected drag data would have "text/pills" without any pills selected, moveSelectedPills () will bail out early if there are no pills selected, so any such data type errors can no longer occur here.

Comment 10

[Thomas D. (:thomas8)]

Each of my patches (in this bug 1691842 and bug 1663062) on their own prevent the privacy violation of pills getting dragged along with addresses from contacts sidebar. We could try to uplift a slim variant of this bug 1691842 to TB 78.

Comment 11

[Alessandro Castellani [:aleca]]

Comment on attachment 9216640 [details] [diff] [review]
1691842_fixAndNormalizeRecipientLabelOnDrop+OnDragOver.diff

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

Looks good, thanks.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5065,5 @@
> + *  Handle the dragover event on a recipient disclosure label.
> + *
> + *  @param {Event} - The DOM dragover event on a recipient disclosure label.
> + */
> +function recipientLabelonDragover(event) {

nit: maybe call this `recipientLabelOnDragover` to respect camelCase?

@@ +5076,5 @@
> + *  Handle the drop event on a recipient disclosure label.
> + *
> + *  @param {Event} - The DOM drop event on a recipient disclosure label.
> + */
> +function recipientLabelonDrop(event) {

nit: recipientLabelOnDrop

Comment 12

[Thomas D. (:thomas8)]

Attachment: 1691842_fixAndNormalizeRecipientLabelOnDrop+OnDragOver.diff

Great, thanks! Of course, let's respect camelCase!

r=aleca from comment 11

As indicated in dependency, this should be landed on top of the patch of Bug 1663062.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/04c8a960d231
Fix and normalize onDrop and onDragOver handlers of recipient disclosure labels. r=aleca