Closed Bug 1601749 Opened 5 years ago Closed 4 years ago

Implement Drag&Drop functionality for the mail-address-pill Custom Element - Ability to reorder recipient addresses/pills in To, Cc and Bcc via mouse

Categories

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

Product:
Thunderbird
Component:
Message Compose Window
Type:
enhancement

Tracking

(thunderbird_esr78 fixed, thunderbird81 fixed)

Status:
RESOLVED FIXED
Milestone:
82 Branch
Tracking Flags:
Tracking Status
thunderbird_esr78 --- fixed
thunderbird81 --- fixed

People

(Reporter: aleca, Assigned: khushil324)

References

Details

Attachments

(1 file, 6 obsolete files)

Bug-1601749_DnD-mail-address-pill-6.patch
(deleted), patch
aleca
: review+
wsmwk
: approval-comm-beta+
wsmwk
: approval-comm-esr78+
Details | Diff | Splinter Review
  • Allow drag&drop of single or multiple pills between recipient containers (To, Cc, etc.).
  • Allow manual reordering of pills within the same recipient container.

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

  • Allow drag&drop of single or multiple pills between recipient containers (To, Cc, etc.).
  • Allow manual reordering of pills within the same recipient container.

Reordering pills by drag and drop will require insertion points (for dropping), and there should be a keyboard equivalent for the sake of access. The UX concept of inter-pill insertion points both for drag n drop and keyboard usage is explored in bug 1603051, including interaction with navigation/selection patterns etc.

Priority: -- → P3

Khushil has worked on dnd, so assigning this to him.

Assignee: alessandro → khushil324
Attached patch Bug-1601749_DnD-mail-address-pill-0.patch (obsolete) (deleted) — — Splinter Review

Currently, "Allow manual reordering of pills within the same recipient container" part is not working exactly. They can just put the pill at the end dragging from the middle.

Attachment #9165187 - Flags: feedback?(alessandro)

Just want feedback if we are moving in a right direction.

Status: NEW → ASSIGNED
Comment on attachment 9165187 [details] [diff] [review] Bug-1601749_DnD-mail-address-pill-0.patch Review of attachment 9165187 [details] [diff] [review]: ----------------------------------------------------------------- This is a good start, thanks for working on this. In terms of objectives, this is a quick list of expected behaviour for this feature: - An indicator should appear to reflect the dropping location in between existing pills. - Click and drag on a pill that is not currently selected, should select it and start the drag motion. - When multiple pills are selected, they should all be moved. - Dropping a pill on the recipient label (To, Cc, etc.) should paste it in first position. - Dropping a pill on a label of a closed recipient should open the recipient and add the pill, like it happens when dropping an address from the Address Book sidebar. Let me know if you have any questions, and if you think would be useful for me to do some quick visual prototypes to reflect these requirements. Thanks again for working on this!
Attachment #9165187 - Flags: feedback?(alessandro) → feedback+
Summary: Implement Drag&Drop functionality for the mail-address-pill Custom Element → Implement Drag&Drop functionality for the mail-address-pill Custom Element - Ability to reorder recipient addresses/pills in To,Cc,Bcc,etc...
Attached patch Bug-1601749_DnD-mail-address-pill-1.patch (obsolete) (deleted) — — Splinter Review
Attachment #9165187 - Attachment is obsolete: true
Attachment #9171650 - Flags: review?(alessandro)
Comment on attachment 9171650 [details] [diff] [review] Bug-1601749_DnD-mail-address-pill-1.patch Review of attachment 9171650 [details] [diff] [review]: ----------------------------------------------------------------- Amazing work, this is coming along pretty great. I'm gonna focus on this first review on the UI and the interactions, and after we nail those I'm gonna do a thorough code review, I hope you don't mind. 1. We should improve the drop indicator to let the user know where the pill is gonna be moved. Right now, using the border doesn't properly communicate that, and also makes the whole section moves up and down due to the changes in eight. I propose to add this pseudo element to all address pills: ``` .address-pill::before { display: block; content: ''; position: relative; width: 3px; background-color: Highlight; height: 15px; border-radius: 2px; margin-inline: -10px 5px; transition: all .2s ease; transform: scaleY(0); } ``` And then use the drop indicator to animate its reveal: ``` .drop-indicator::before { transform: scaleY(1); } ``` What do you think? Maybe you could ask a ui-r to Richard to be sure things look good. 2. Dropping a pill above the Label of an opened recipient field (eg. To) duplicates the pill. I think we should prevent users from adding the same recipient twice in the same addressing field, and it might happen that users trying to move a pill in first position could accidentally drop it above the label. Maybe this is something we could deal in a follow up bug to not make this too complicated. 3. Dragging multiple pills creates the "ghost" dragged element always from the first pill, even if I start the drag from another pill. This is a bit weird since the drop indicator correctly follows the position of the mouse cursor, but the ghost element is in another location. Great work!
Attachment #9171650 - Flags: review?(alessandro) → feedback?

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

  1. Dragging multiple pills creates the "ghost" dragged element always from
    the first pill, even if I start the drag from another pill.
    This is a bit weird since the drop indicator correctly follows the position
    of the mouse cursor, but the ghost element is in another location.

All other issues got resolved including "the ghost element is in another location."
But I am facing issue in:
"Dragging multiple pills creates the "ghost" dragged element always from the first pill, even if I start the drag from another pill."

Now the "ghost" dragged element will be the pill which is being dragged but the problem is it will show only one pill even if we have selected multiple pills. You can add "ghost" image/element when "dragstart" is being called like event.dataTransfer.setDragImage(targetPill, xOffset, yOffset). We are able to add only one element which should be an element from the DOM. I tried to create a div element and insert all the selected pills deep clone but it was showing nothing as "ghost" image/element.

Attached patch Bug-1601749_DnD-mail-address-pill-2.patch (obsolete) (deleted) — — Splinter Review
Attachment #9171650 - Attachment is obsolete: true
Attachment #9171650 - Flags: feedback?
Attachment #9171845 - Flags: ui-review?(richard.marti)
Attachment #9171845 - Flags: review?(alessandro)

Comment on attachment 9171845 [details] [diff] [review]
Bug-1601749_DnD-mail-address-pill-2.patch

Looks good. When I drag a pill to the end of the last pill, I have no real feedback, no drop indicator is shown. It would be better to show one, if possible, to get a better feedback where the pill will land.

Attachment #9171845 - Flags: ui-review?(richard.marti) → ui-review+

What I now noticed is, that dropping the address over the chevron doesn't work. It would be good when then the popup opens and the address can be placed directly on an entry in this popup to add this row like it happens with the To and Cc buttons.

Comment on attachment 9171845 [details] [diff] [review] Bug-1601749_DnD-mail-address-pill-2.patch Review of attachment 9171845 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +2152,5 @@ > + } > + let selectedPills = this.getAllSelectedPills(); > + if (!selectedPills.length) { > + return; > + } Is this condition ever true? Since we're adding the "selected" attribute to the clicked pill, we will always have at least 1 selected pill. Did you encounter any case where a dragstart event started with 0 selected items? @@ +2161,5 @@ > + }); > + > + this.findTargetInput = event => { > + let targetInput = event.originalTarget.closest(".address-input"); > + // If original target is pill, then we need to navigate for the input element. Is this comment correct? Based on the condition it seems you're checking if the label inside the pill was the selected element. @@ +2166,5 @@ > + if ( > + !targetInput && > + event.originalTarget.classList.contains("pill-label") > + ) { > + targetInput = event.originalTarget.parentElement.nextElementSibling; It's better to avoid navigating the markup without referencing a class as the pill structure might change and we would break this. @@ +2196,5 @@ > + targetPill.classList.add("drop-indicator"); > + } > + let targetInput = this.findTargetInput(event); > + // Add the indicator style for address-container. > + if (targetInput && targetInput.parentElement) { I think it would be better to check if the parentElement is the one expected by referencing a class. @@ +2207,5 @@ > + if ( > + targetLabel && > + targetLabel.parentElement && > + targetLabel.parentElement.nextElementSibling > + ) { Also this condition is too generic and is prone to breakage if recipient structure changes. @@ +2226,5 @@ > + } > + > + let targetInput = this.findTargetInput(event); > + // Remove the indicator style for address-container. > + if (targetInput && targetInput.parentElement) { Same here, which parentElement are we expecting to find? @@ +2236,5 @@ > + let targetLabel = this.findLabelContainer(event); > + if ( > + targetLabel && > + targetLabel.parentElement && > + targetLabel.parentElement.nextElementSibling Same here @@ +2279,5 @@ > + ...selectedAddresses > + ); > + } > + this.appendDNDAddresses( > + this.getAllSelectedPills()[0], What's the purpose of passing only the first selected pill? Wouldn't be easier to simply pass all the currently selected pills to the `appendDNDAddresses` method and extract the addresses in there? @@ +2338,5 @@ > + /** > + * Move the DND pills email address to another addressing row. > + * > + * @param {Element} pill - The first selected pill element to move. > + * opened. Is this "opened." necessary? it's seems like a typo in the comment. @@ +2342,5 @@ > + * opened. > + * @param {string} targetFieldType - The target recipient type, > + * e.g. "addr_to". > + * @param {string[]} combinedAddresses - The array of email adressed in right to order to move. > + * e.g. "addr_to". Update with "The array of email addresses to move." and remove the `e.g. "addr_to".` which is not related to this parameter. @@ +2344,5 @@ > + * e.g. "addr_to". > + * @param {string[]} combinedAddresses - The array of email adressed in right to order to move. > + * e.g. "addr_to". > + * @param {Element[]} rowPills - The pills element in the input row to remove. > + */ "The pill elements to remove". @@ +2346,5 @@ > + * e.g. "addr_to". > + * @param {Element[]} rowPills - The pills element in the input row to remove. > + */ > + appendDNDAddresses(pill, targetFieldType, combinedAddresses, rowPills) { > + this.removeSelectedPills(pill, "next", false, true); When dropping an address from the Contacts sidebar, calling this method triggers this error: `JavaScript error: chrome://messenger/content/mailWidgets.js, line 2846: TypeError: can't access property "closest", pill is undefined` This doesn't happen during a regular drag and drop, but only when a new address is added. ::: mail/components/compose/content/addressingWidgetOverlay.js @@ +983,4 @@ > } > > /** > + * Show the container row of an hidden recipient (Cc, Bcc, etc.). Wrong comment.
Attachment #9171845 - Flags: review?(alessandro) → feedback+

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

Is this condition ever true?
Since we're adding the "selected" attribute to the clicked pill, we will
always have at least 1 selected pill.
Did you encounter any case where a dragstart event started with 0 selected
items?

Yes, when the user tries to drag the input area. We have dragstart event listener on the whole MailRecipientsArea so this condition is necessary.

I have cleaned up the patch now. It's more readable and moduler now.

Attached patch Bug-1601749_DnD-mail-address-pill-3.patch (obsolete) (deleted) — — Splinter Review
Attachment #9171845 - Attachment is obsolete: true
Attachment #9172088 - Flags: review?(alessandro)
Attached patch Bug-1601749_DnD-mail-address-pill-4.patch (obsolete) (deleted) — — Splinter Review
Attachment #9172088 - Attachment is obsolete: true
Attachment #9172088 - Flags: review?(alessandro)
Attachment #9172091 - Flags: review?(alessandro)
Comment on attachment 9172091 [details] [diff] [review] Bug-1601749_DnD-mail-address-pill-4.patch Review of attachment 9172091 [details] [diff] [review]: ----------------------------------------------------------------- There's a little glitch happening when I drop the selected pill on top of itself. The pill gets added before the last one in the list. I think nothing should happen if the moved pill is only 1 and it's dropped above itself. ::: mail/base/content/mailWidgets.js @@ +2180,5 @@ > + ".address-label-container" > + ); > + // labelContainer.nextElementSibling would be the address container. > + return labelContainer ? labelContainer.nextElementSibling : null; > + }; Both these methods to retrieve the address container seem a bit too convoluted, and I think we can simplify the checks you do later and remove these methods. @@ +2199,5 @@ > + this.findAddressContainerFromPill(event) || > + this.findAddressContainerFromLabel(event); > + if (addressContainer && addressContainer.classList) { > + addressContainer.classList.add("drag-address-container"); > + } We can simplify this condition by navigating up to the main address-row container, and then use the querySelector to get the address-container. Something like this: ``` let addressRow = event.originalTarget.closest(".address-row"); if (addressRow) { addressRow .querySelector(".address-container") .classList.add("drag-address-container"); } ``` @@ +2218,5 @@ > + this.findAddressContainerFromPill(event) || > + this.findAddressContainerFromLabel(event); > + if (addressContainer && addressContainer.classList) { > + addressContainer.classList.remove("drag-address-container"); > + } Same for this condition. @@ +2239,5 @@ > + this.createDNDPills( > + addressContainer, > + !!targetPill, > + targetPill ? targetPill.fullAddress : null > + ); Also this condition can be simplified by grabbing directly the address-container. ``` let addressContainer = event.originalTarget.closest( ".address-container" ); ``` Then, if the condition is truty, let's remove the drag-address-container class and interrupt the execution of the method with a return, so we don't need to run another condition if not necessary. @@ +2243,5 @@ > + ); > + } > + > + // Used when drop is over the label container. > + addressContainer = this.findAddressContainerFromLabel(event); We can replace this with: ``` addressContainer = event.originalTarget.closest(".address-row").querySelector(".address-container"); ``` And also here we can remove the drag-address-container class if the condition is truty. @@ +2251,5 @@ > + > + // Remove the indicator style for the address container. > + if (addressContainer && addressContainer.classList) { > + addressContainer.classList.remove("drag-address-container"); > + } After those edits, this condition is not necessary anymore. @@ +2260,5 @@ > + * Move the dragged pills to another addressing row. > + * > + * @param {string} addressContainer - The address container on which pills are dropped. > + * @param {boolean} appendStart - The boolean value for the selected addresses to append to > + * the existing addresses at the end or start. Mh, this sounds strange to read. Let's update with: "If the selected addresses should be appended to the end or the start of existing addresses." @@ +2261,5 @@ > + * > + * @param {string} addressContainer - The address container on which pills are dropped. > + * @param {boolean} appendStart - The boolean value for the selected addresses to append to > + * the existing addresses at the end or start. > + * @param {string} targetAdress - The adress before which all selected addresses should Small typo, "address". @@ +2275,5 @@ > + ); > + > + // Remove all the the duplicate existing addresses. > + let loop = true; > + while (loop) { I don't understand the necessity of this while loop. Isn't the for loop enough to splice all the selected addresses? @@ +2278,5 @@ > + let loop = true; > + while (loop) { > + loop = false; > + for (let address of selectedAddresses) { > + const index = existingAddresses.indexOf(address); let @@ +2292,5 @@ > + // before that pill. > + if (targetAdress) { > + // Remove the duplicates addresses. > + for (let address of selectedAddresses) { > + const index = existingAddresses.indexOf(address); let @@ +2311,5 @@ > + combinedAddresses = selectedAddresses.concat(existingAddresses); > + } else { > + // Pills are dropped at the end of all the existing pills. > + combinedAddresses = existingAddresses.concat(selectedAddresses); > + } Let's simplify the callback condition with a simple: ``` } else { combinedAddresses = appendStart ? selectedAddresses.concat(existingAddresses) : existingAddresses.concat(selectedAddresses); }
Attachment #9172091 - Flags: review?(alessandro)

You may want to consider:

  • when dragging selection on a non selected pill, the indicator for destination would appear befor the targeted pill and upon release would insert/move the selected pills before the targeted one. Some people may not easily point between two pills with a mouse, that might help them (loosy target)
  • draging selection on any labels (To, Cc,etc) would show target indicator at the begining of the field (if displayed and if pills or no pills already present) to insert selected pills there upon release, in front of any other non selected pills (if any present in the field)
  • if selected pill(s) are cut via keyboard and cursor moved via keyboard arrow, then the indicator for targeting paste shall appear in front of the non selected pills for possible paste via keyboard, the indicator to target paste moving as you press on left or right arrow to target a destination for paste... something to explore perhaps... when moving indicator with arrow if you reach and of field and continue to press right arrow target indicator would move to the next displayed field... when you reach the end of last disolayed field and continue to press it woul move at the begining of first dispayed field... and vice-versa...
Attached patch Bug-1601749_DnD-mail-address-pill-5.patch (obsolete) (deleted) — — Splinter Review
Attachment #9172091 - Attachment is obsolete: true
Attachment #9172347 - Flags: review?(alessandro)

(In reply to Richard Leger from comment #19)

when dragging selection on a non selected pill, the indicator for destination would appear befor the targeted pill and upon release would insert/move the selected pills before the targeted one. Some people may not easily point between two pills with a mouse, that might help them (loosy target)

Yup, this is already happening in the patch.

draging selection on any labels (To, Cc,etc) would show target indicator at the begining of the field (if displayed and if pills or no pills already present) to insert selected pills there upon release, in front of any other non selected pills (if any present in the field)

We will consider this, maybe in a follow up bug since the labels are also a drop recipient for new addresses coming from the contact sidebar,a nd those are added at the end of the field.

if selected pill(s) are cut via keyboard and cursor moved via keyboard arrow, then the indicator for targeting paste shall appear in front of the non selected pills for possible paste via keyboard, the indicator to target paste moving as you press on left or right arrow to target a destination for paste... something to explore perhaps... when moving indicator with arrow if you reach and of field and continue to press right arrow target indicator would move to the next displayed field... when you reach the end of last disolayed field and continue to press it woul move at the begining of first dispayed field... and vice-versa...

I'm not sure about this. Probably we can explore it in another bug if we will ever allow the typing cursor in between pills.
For now, we're focusing on mouse interaction with drag and drop.
Keyboard reordering will come later.

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

I'm not sure about this. Probably we can explore it in another bug if we will ever allow the typing cursor in between pills.
For now, we're focusing on mouse interaction with drag and drop.
Keyboard reordering will come later.

That's Bug 1603051 (already linked up in "See also" here) - Recipient pills UX: Allow insertion and reordering of pills (via inter-pill insertion points)
...where I have explored keyboard reordering in detail, and how it would interact with our existing keyboard methods of handling pills.

Comment on attachment 9172347 [details] [diff] [review] Bug-1601749_DnD-mail-address-pill-5.patch Review of attachment 9172347 [details] [diff] [review]: ----------------------------------------------------------------- This is looking very very good, just a few nits to fix. Be sure to launch a try-run to see if any test was affected by this. Great work! ::: mail/base/content/mailWidgets.js @@ +2150,5 @@ > + if (targetPill && !targetPill.hasAttribute("selected")) { > + targetPill.toggleAttribute("selected"); > + } > + let selectedPills = this.getAllSelectedPills(); > + // If anything other than adress pill is being dragged, function will return. Typo: "address". Let's update this comment with: "Interrupt if no address pill is being dragged" @@ +2224,5 @@ > + addressContainer, > + !!targetPill, > + targetPill ? targetPill.fullAddress : null > + ); > + if (addressContainer.classList) { Do we need this condition? Did you encounter any case where the element didn't have a classList? The very fact that we're selecting that element by it's class should make it pretty safe as we're already inside the condition to be sure that element exists. @@ +2236,5 @@ > + .closest(".address-row") > + .querySelector(".address-container"); > + if (addressContainer) { > + this.createDNDPills(addressContainer, true, null); > + if (addressContainer.classList) { Same question as above. @@ +2249,5 @@ > + * > + * @param {string} addressContainer - The address container on which pills are dropped. > + * @param {boolean} appendStart - If the selected addresses should be appended to the end > + * or the start of existing addresses. > + * @param {string} targetaddress - The adress before which all selected addresses should Typo: "address". Also, be sure to return the text of these comments when reaching the 80ch limit. @@ +2252,5 @@ > + * or the start of existing addresses. > + * @param {string} targetaddress - The adress before which all selected addresses should > + * be dropped. > + */ > + createDNDPills(addressContainer, appendStart, targetaddress) { Nit: let's be consistent with variable naming and use camel case also for the last attribute. targetAddress. ::: mail/components/compose/content/addressingWidgetOverlay.js @@ +989,5 @@ > + * @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) { Nit: recipientType
Attachment #9172347 - Flags: review?(alessandro) → feedback+
Comment on attachment 9172585 [details] [diff] [review] Bug-1601749_DnD-mail-address-pill-6.patch Review of attachment 9172585 [details] [diff] [review]: ----------------------------------------------------------------- Outstanding work! And the try-run looks good. r+
Attachment #9172585 - Flags: review?(alessandro) → review+
Keywords: checkin-needed-tb

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/73e9d8f66346
Implement Drag&Drop functionality for the mail-address-pill Custom Element. r=aleca DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED
status-thunderbird81: --- → affected
status-thunderbird_esr78: --- → affected
Target Milestone: --- → 82 Branch

When should we add ESR uplift request for this?

You can ask beta+esr approval now. It should go through beta before esr.

Comment on attachment 9172585 [details] [diff] [review]
Bug-1601749_DnD-mail-address-pill-6.patch

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: Drag and Drop functionality for address pills is a frequently asked feature from the Thunderbird users.
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky): Low

Attachment #9172585 - Flags: approval-comm-esr78?
Attachment #9172585 - Flags: approval-comm-beta?

Comment on attachment 9172585 [details] [diff] [review]
Bug-1601749_DnD-mail-address-pill-6.patch

[Triage Comment]
Approved for beta

Attachment #9172585 - Flags: approval-comm-beta? → approval-comm-beta+

If this tests well, then perhaps we take this in 78.2.2

Depends on: 1663041
Blocks: 1663055
Blocks: 1663057

(In reply to Wayne Mery (:wsmwk) from comment #32)

If this tests well, then perhaps we take this in 78.2.2

Would fix at least bug 1663041 before shipping - losing pills after drop isn't nice, and having an error pill isn't very special

I've filed some nits and corners for polishing, but nevertheless:

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

Outstanding work!

+1, indeed. Khushil, this is AWESOME, thank you! Users will love it.

Comment on attachment 9172585 [details] [diff] [review]
Bug-1601749_DnD-mail-address-pill-6.patch

[Triage Comment]
Approved for esr78.

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

(In reply to Richard Leger from Bug 1663041 comment 33)

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!

My comment above was meant to be published here!
Thank you for drag&drop feature!

Regressions: 1691842
Summary: Implement Drag&Drop functionality for the mail-address-pill Custom Element - Ability to reorder recipient addresses/pills in To,Cc,Bcc,etc... → Implement Drag&Drop functionality for the mail-address-pill Custom Element - Ability to reorder recipient addresses/pills in To, Cc and Bcc via mouse
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: