1705690 - Remove event.originalTarget from mailWidgets.js, recipients area drag and drop code cleanup, fix uncaught TypeError when missing address row drop target

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, task)

Description

[Thomas D. (:thomas8)]

• Coming from bug 1705038, the drag and drop code for dragging pills or text in composition's recipient area is unnecessarily verbose, has some duplicated code, unnecessarily makes resizeInputField() run twice, is poorly and wrongly documented, and uses event.originalTarget all over without need (which we want to remove anyway per bug 1699809, but each occurence must be checked before changing).
• When accidentally dropping pills minimally outside of an address row, there's an Uncaught TypeError because it won't find a row (even if event.target is used).

STR

1. compose, drag a pill and accidentally drop it exactly between To and CC row (or on the very right margin) when none of the two rows shows drop indicator (the 1px blue border indicator is pretty easy to miss).
2. Observe error console

Actual

Uncaught TypeError: can't access property "querySelector", event.originalTarget.closest(...) is null
    connectedCallback chrome://messenger/content/mailWidgets.js:2380
mailWidgets.js:2380:12

Expected

• no uncaught type error

Comment 1

[Thomas D. (:thomas8)]

Attachment: 1705690_recipientsArea_DnD_cleanup.diff

Bug 1705690 - Remove event.originalTarget from mailWidgets.js, recipients area drag and drop code cleanup, fix uncaught TypeError. r=aleca

• In mailWigdets.js, replace all instances of event.originalTarget with
  event.target (see bug 1699809).
• Full code cleanup of composition's recipients area drag and drop code,
  eliminate unnecessarily verbose, duplicate, and no-op code (resizeInputField() runs twice) .
• Correct and improve documentation of drag and drop in comments.
• For the scenario of dropping pills outside address row drop target by accident,
  fix Uncaught TypeError: can't access property "querySelector",
  event.originalTarget.closest(...) is null
  connectedCallback chrome://messenger/content/mailWidgets.js:2380

Comment 2

[Alessandro Castellani [:aleca]]

Comment on attachment 9216375 [details] [diff] [review]
1705690_recipientsArea_DnD_cleanup.diff

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

Good improvements, thanks for taking care of this.
It needs some small fixes.

::: mail/base/content/mailWidgets.js
@@ +2337,5 @@
>            }
>  
> +          // Dropped data should be plain text (images are handled elsewhere).
> +          // Unfortunately, we currently only support dropping text directly
> +          // into the row input, which is pretty limiting.

If we want to leave a comment highlighting a missing feature I think we should rewrite with something more positive.
"FIXME: Implement the ability to <explain needed feature>"

@@ +2345,5 @@
> +
> +        // Pills have been dropped ("text/pills").
> +        // If drop event on a pill, remove its drop indicator style.
> +        let targetPill = event.target.closest("mail-address-pill");
> +        targetPill?.classList.remove("drop-indicator");

This doesn't seem to be necessary since we don't actually move the pill but we clear and re-add the array of addresses.

@@ +2346,5 @@
> +        // Pills have been dropped ("text/pills").
> +        // If drop event on a pill, remove its drop indicator style.
> +        let targetPill = event.target.closest("mail-address-pill");
> +        targetPill?.classList.remove("drop-indicator");
> +        let targetAddressContainer = event.target.closest(".address-container");

This should be declared right above the condition in which is used.

@@ +2368,5 @@
> +        // Otherwise if dropped elsewhere on the row (e.g. on the row label),
> +        // append pills before existing pills.
> +        this.createDNDPills(
> +          addressContainer,
> +          targetPill ? true : targetAddressContainer ? false : true,

I wish we could use this, but our eslint setup doesn't accept nested ternary expressions.
Since we need a boolean, I think we can replace that with "targetPill || !targetAddressContainer"

Comment 3

[Thomas D. (:thomas8)]

Attachment: 1705690_recipientsArea_DnD_cleanup.diff

Perfect review, thanks. This patch addresses all issues of comment 2. FIXME didn't feel right because there's nothing broken; I've added a simple reference to bug 1706187 instead.

Comment 4

[Alessandro Castellani [:aleca]]

Comment on attachment 9216934 [details] [diff] [review]
1705690_recipientsArea_DnD_cleanup.diff

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

Looks good!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a93215d684f5
Remove event.originalTarget from mailWidgets.js, recipients area drag and drop code cleanup, fix uncaught TypeError. r=aleca