1663041 - Drag and drop of recipient pills annihilates random pills when there's an error pill

Status: RESOLVED FIXED

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

Description

[Thomas D. (:thomas8)]

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

My first attempts of dragging pills around were just eating up most of the pills most of the time. Turns out that if you have just one error pill in the mix, the result of drag and drop becomes unpredictable, either existing or dragged pills are randomly getting lost and should not...

STR

1. compose and have one red error pill (e.g. on German keyboard, if you don't hold AltGr long enough and press q key where the @ is third-level character, you'll just get q):
   To: john <[e-mail]>, error <fooqbar.com>, jane <[e-mail]>
   CC: [e-mail], [e-mail]

2. drag 1@... and 2@... from CC and drop them behind jane@ (onto address input of to)

3. delete last pills in To: so that error pill is last pill, now drop pills after the error pill

Actual result:

• jane deleted! (pill after error pill replaced with first dropped pill) :-O
• 1 and 2 now in To (ok)
• if error pill is last and you drop after that, first dropped pill will disappear
• if you keep dragging pills around into the field with the error pill, almost every move you're going to lose another pill, as long as the error pill is still around. Odd...

Expected result:

• Please don't lose pills on drag and drop

Something's wrong here. Just having an error pill shouldn't eat random pills...

Comment 1

[Thomas D. (:thomas8)]

Kushil, could you have a look?

(Interesting, cloning a bug doesn't even CC the former assignee if he wasn't CCed before)

Comment 2

[Khushil Mistry [:khushil324]]

It's actually a problem with MailServices.headerParser.makeFromDisplayAddress. When we copy/paste john <[e-mail]>, error <fooqbar.com>, jane <[e-mail]> to the input row, you will just see the two address pills getting created. john <[e-mail]> and error <fooqbar.com>. The third pill is missing.
When we drag and drop, it removes the existing pills in that row, combines the addresses of the existing pills, and dropped pills(in the order according to the drop position) and creates all the pills again. It's the same as the scenario I mentioned above. Now I am checking makeFromDisplayAddress and try to solve this issue.

Comment 3

[Magnus Melin [:mkmelin]]

I don't think makeFromDisplayAddress will be the right place to fix, and that's designed to handle attack cases too (which is probably why it's removing junk like this.)
We could just disallow drop if there is any wrong pills in the mix I think. I'm not even sure why we allow to leave invalid pills there to begin with, except for to be able to edit them.

Comment 4

[Khushil Mistry [:khushil324]]

I found another fix, let's see. It should work, Not touching makeFromDisplayAddress.

Comment 5

[Khushil Mistry [:khushil324]]

Attachment: Bug-1663041_Dnd-pills-problem-error-pill-0.patch

Comment 6

[Khushil Mistry [:khushil324]]

Attachment: Bug-1663041_Dnd-pills-problem-error-pill-1.patch

This patch also solve the problem from the bug 1663055.

Comment 7

[Khushil Mistry [:khushil324]]

Attachment: Screenshot 2020-09-04 at 1.01.29 PM.png

I am seeing the problem with this patch. The input area where pills are dropped is focused but the cursor is in another row. When I type, it's being typed in the row where pills are dropped but cursor is still in the previously selected row.

Comment 8

[Alessandro Castellani [:aleca]]

Comment on attachment 9173933 [details] [diff] [review]
Bug-1663041_Dnd-pills-problem-error-pill-1.patch

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

This fixes the moving of "error" pills, but it needs some fixes.
Also, the selection is not maintained, I'm not sure if you were trying to fix that here as well.

::: mail/base/content/mailWidgets.js
@@ +2299,5 @@
>        }
>  
>        // Create pills for all the combined addresses.
> +      let addressInput = addressContainer.querySelector(".address-input");
> +      addressInput.focus();

No need to declare a variable if we don't use it anywhere else.
Also, let's always specify the ".address-input[recipienttype]" selector otherwise we risk to select an input inside a pill.
Suggestion: addressContainer.querySelector(".address-input[recipienttype]").focus();

@@ +2301,5 @@
>        // Create pills for all the combined addresses.
> +      let addressInput = addressContainer.querySelector(".address-input");
> +      addressInput.focus();
> +      let recipientType = addressContainer
> +        .querySelector(".address-input")

Also here: ".address-input[recipienttype]"

Comment 9

[Khushil Mistry [:khushil324]]

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

Also, the selection is not maintained, I'm not sure if you were trying to
fix that here as well.

It's working fine for me. Whichever pills are selected, are set as selected after drop also.

Comment 10

[Khushil Mistry [:khushil324]]

Attachment: Bug-1663041_Dnd-pills-problem-error-pill-2.patch

Comment 11

[Alessandro Castellani [:aleca]]

Comment on attachment 9174315 [details] [diff] [review]
Bug-1663041_Dnd-pills-problem-error-pill-2.patch

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

This works for error pills, but unfortunately the selection is not maintained for me.
Magnus, Thomas, could you take a look at this?

Comment 12

[Alessandro Castellani [:aleca]]

Attachment: Screen record from 2020-09-08 11.06.12.gif

Here's a screencast

Comment 13

[Magnus Melin [:mkmelin]]

Selection seems to be kept for me. Actually it's kept too much: if i drag 2 addresses from To to Cc, and then after drop click a third one in cc, then drag that to To, the other two will also get dragged to to. The click (without holding shift) should have cleared the selection and then only dragged the one pill which was clicked+dragged.

Comment 14

[Thomas D. (:thomas8)]

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

Selection seems to be kept for me. Actually it's kept too much: if i drag 2 addresses from To to Cc, and then after drop click a third one in cc, then drag that to To, the other two will also get dragged to to. The click (without holding shift) should have cleared the selection and then only dragged the one pill which was clicked+dragged.

+1, exactly that's what I'm seeing also.

Found bug 1664039 when testing this.

Comment 15

[Khushil Mistry [:khushil324]]

Alex, can you check why the selection is not being persisted with this patch in your machine?
We are doing this here: if the address is from the selected addressed then selectedAddresses.includes(address) will be true and function awAddRecipientsArray should make it selected. We do it for all the pills.

for (let address of combinedAddresses) {
  awAddRecipientsArray(
    recipientType,
    [address],
    selectedAddresses.includes(address)
  );
}

Comment 16

[Alessandro Castellani [:aleca]]

Attachment: Bug-1663041_Dnd-pills-problem-error-pill.patch

I found the problem that was causing the erratic deselection of pills.

Using the this.removeSelectedPills() method is not necessary in this case because the user is not deleting pills, but we are before recreating them.
That method has some very refined conditions that Thomas created to properly manage the focus when deleting pills, which was interfering with the createDNDPills method.

Another issue was the fact that we were moving the focus on the input field.
That causes the removal of the selection from all the pills, since we have that method onFocus in the field, which is correct.

We should move the focus on the first selected pills we just moved, to guarantee consistency.
Here's an updated patch for you to test.

One last thing to implement before the r+ here is to prevent moving all the currently selected pills if the drag action starts from a pill that isn't selected.
So, for example:

• You have 6 pills, 3 are selected.
• You start the drag action from a selected pill, the 3 pills are dragged.
• You start the drag action from an unselected pill, only the newly clicked pill is dragged and the other 3 previously selected pills are unselected.

Comment 17

[Khushil Mistry [:khushil324]]

Attachment: Bug-1663041_Dnd-pills-problem-error-pill-3.patch

The patch you attached was working fine for me. I have updated the patch as you have asked: One last thing to implement before the r+ here is to prevent moving all the currently selected pills if the drag action starts from a pill that isn't selected.

Comment 18

[Alessandro Castellani [:aleca]]

Comment on attachment 9175201 [details] [diff] [review]
Bug-1663041_Dnd-pills-problem-error-pill-3.patch

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

Beautiful!
This works perfectly. r+

Just add a small comment and then this is ready to land.
Great work!

::: mail/base/content/mailWidgets.js
@@ +2148,4 @@
>        this.addEventListener("dragstart", event => {
>          let targetPill = event.originalTarget.closest("mail-address-pill");
>          if (targetPill && !targetPill.hasAttribute("selected")) {
> +          for (let pill of this.getAllSelectedPills()) {

Let's add a small comment here:
// Deselect all previously selected pills if the drag action starts from a non selected pill.

Comment 19

[Khushil Mistry [:khushil324]]

Attachment: Bug-1663041_Dnd-pills-problem-error-pill-4.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ffabcc86cff9170e6750890408ce53c245d51334

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5f196afbd35f
Fix drag & drop of recipient pills when there's an error pill. r=aleca

Comment 21

[Rob Lemley [:rjl]]

Does this need uplift?

Comment 22

[Khushil Mistry [:khushil324]]

Comment on attachment 9175434 [details] [diff] [review]
Bug-1663041_Dnd-pills-problem-error-pill-4.patch

[Approval Request Comment]
Regression caused by (bug #): 1601749
User impact if declined: Drag and Drop operation of address pills with the error pill will not show correct behaviour.
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky): Low

Comment 23

[Wayne Mery (:wsmwk)]

Comment on attachment 9175434 [details] [diff] [review]
Bug-1663041_Dnd-pills-problem-error-pill-4.patch

[Triage Comment]
Approved for beta

Walt, can you check this during testing?

Comment 24

[Rob Lemley [:rjl]]

Thunderbird 81.0b4:
https://hg.mozilla.org/releases/comm-beta/rev/bc58408893f2

Comment 25

[WaltS48 [:walts48]]

I'm not sure of the expected results, but I'm not able to add any addresses behind a red error pill. The cursor turns red.

Drag and drop appears to be working correctly. Not a feature I use a lot.

Tested using 81.0b4 on Windows10 with a test profile

Comment 26

[Alessandro Castellani [:aleca]]

The scope of this bug was to prevent the deletion/merging of error pills when another pill gets dropped on top of it.

I'm not able to add any addresses behind a red error pill. The cursor turns red.

What do you mean?
After adding an incorrect address, and the red pill is created, are you unable to type another address?
If with "behind" you mean before, we currently don't have the ability to type addresses before or between pills.

Comment 27

[Thomas D. (:thomas8)]

(In reply to WaltS48 [:walts48] from comment #25)

I'm not sure of the expected results, but I'm not able to add any addresses behind a red error pill. The cursor turns red.

Drag and drop appears to be working correctly. Not a feature I use a lot.

Tested using 81.0b4 on Windows10 with a test profile

Wfm on 81.0b4 (32-bit) on Win10 with a possibly dirty profile (so it should definitely work on clean profile).
I can type another address after the last pill if that's an error pill, and I can drag and drop both proper or error pill and it will be inserted correctly after error pill, regardless of the position of the error pill. It all works.

Comment 28

[WaltS48 [:walts48]]

Sorry, chalk this one up to the testers inexperience with pills.

It appears I only typed "Mxyzptlk" and didn't hit enter to create a pill and just started typing again.

I can type and add more addresses after creating that error pill.

Comment 29

[Alessandro Castellani [:aleca]]

Awesome, thanks for checking.

Comment 30

[Thomas D. (:thomas8)]

[OT]

(In reply to WaltS48 [:walts48] from comment #28)

Sorry, chalk this one up to the testers inexperience with pills.
It appears I only typed "Mxyzptlk" and didn't hit enter to create a pill and just started typing again.

We don't fire testers after going astray, but we send them to do penance... :-p

Can you please head over to Bug 1665483 Comment 5 and try to reproduce that, which is similar case. Maybe you'll succeed... User provided screenshot with a valid address, focus moved to body, but pill not created, still plain text in the input. That's technically impossible, as we create pill on blur (loss of focus in input box). However, her case is better than yours assuming that the screenshot is original and not photoshopped for sanitizing her private address. Which might be a question to ask...

Comment 31

[Wayne Mery (:wsmwk)]

Comment on attachment 9175434 [details] [diff] [review]
Bug-1663041_Dnd-pills-problem-error-pill-4.patch

[Triage Comment]
approved for esr78

Comment 32

[Rob Lemley [:rjl]]

Thunderbird 78.3.0:
https://hg.mozilla.org/releases/comm-esr78/rev/87408bc2d416

Comment 33

[Richard Leger]

Thank you for this new feature long awaited for and now landed.

Bravo to the devs and all teams that make this possible and happen.

Job done! Well done!