Closed Bug 1663041 Opened 1 year ago Closed 1 year ago

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

Categories

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

Tracking

(thunderbird_esr78 fixed, thunderbird81 fixed)

RESOLVED FIXED
82 Branch
Tracking Status
thunderbird_esr78 --- fixed
thunderbird81 --- fixed

People

(Reporter: thomas8, Assigned: khushil324)

References

Details

Attachments

(3 files, 5 obsolete files)

+++ 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 <john@example.com>, error <fooqbar.com>, jane <jane@example.com>
    CC: 1@example.com, 2@example.com

  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...

Kushil, could you have a look?

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

Flags: needinfo?(khushil324)
Assignee: nobody → khushil324
Flags: needinfo?(khushil324)

It's actually a problem with MailServices.headerParser.makeFromDisplayAddress. When we copy/paste john <john@example.com>, error <fooqbar.com>, jane <jane@example.com> to the input row, you will just see the two address pills getting created. john <john@example.com> 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.

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.

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

Attachment #9173931 - Flags: review?(alessandro)
Status: NEW → ASSIGNED

This patch also solve the problem from the bug 1663055.

Attachment #9173931 - Attachment is obsolete: true
Attachment #9173931 - Flags: review?(alessandro)
Attachment #9173933 - Flags: review?(alessandro)

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 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]"
Attachment #9173933 - Flags: review?(alessandro)

(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.

Attachment #9173933 - Attachment is obsolete: true
Attachment #9174315 - Flags: review?(alessandro)
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?
Attachment #9174315 - Flags: review?(alessandro) → feedback+
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(bugzilla2007)

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.

Flags: needinfo?(mkmelin+mozilla)
Blocks: 1664039

(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.

Flags: needinfo?(bugzilla2007)

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)
  );
}
Flags: needinfo?(alessandro)

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.
Flags: needinfo?(alessandro)
Attachment #9175012 - Flags: feedback?(khushil324)

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.

Attachment #9174315 - Attachment is obsolete: true
Attachment #9175012 - Attachment is obsolete: true
Attachment #9175012 - Flags: feedback?(khushil324)
Attachment #9175201 - Flags: review?(alessandro)
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.
Attachment #9175201 - Flags: review?(alessandro) → review+

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

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED

Does this need uplift?

Flags: needinfo?(khushil324)
Target Milestone: --- → 82 Branch

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

Flags: needinfo?(khushil324)
Attachment #9175434 - Flags: approval-comm-esr78?
Attachment #9175434 - Flags: approval-comm-beta?

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?

Flags: needinfo?(wls220spring)
Attachment #9175434 - Flags: approval-comm-beta? → approval-comm-beta+
Blocks: 1663055

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

Flags: needinfo?(wls220spring)

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.

Flags: needinfo?(wls220spring)

(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.

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.

Flags: needinfo?(wls220spring)

Awesome, thanks for checking.

[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 on attachment 9175434 [details] [diff] [review]
Bug-1663041_Dnd-pills-problem-error-pill-4.patch

[Triage Comment]
approved for esr78

Attachment #9175434 - Flags: approval-comm-esr78? → approval-comm-esr78+

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!

You need to log in before you can comment on or make changes to this bug.