Bug 1171979 prepares for toolkit's bug 1162050. We should use the HTML Drag and Drop API instead - http://developer.mozilla.org/En/DragDrop/Drag_and_Drop (and remove our copy of nsDragAndDrop.js)

(In reply to Magnus Melin from comment #0) > Bug 1171979 prepares for toolkit's bug 1162050. > > We should use the HTML Drag and Drop API instead - > http://developer.mozilla.org/En/DragDrop/Drag_and_Drop (and remove our copy > of nsDragAndDrop.js) Are there ongoing any changes in the current Thunderbird beta (50.0b3) that might affect button dragging? Just found that the dragstart event on my QuickFolders toolbar doesn't fire anymore...

Well bug 1162050 landed. Not sure if that would affect button dragging or not.

Yes(In reply to Magnus Melin from comment #2) > Well bug 1162050 landed. Not sure if that would affect button dragging or > not. Yes that was it - I found myself in the meantime. I will check my dragdrop events as well and make sure they are replaced. https://developer.mozilla.org/en-US/docs/Web/Events turned out to be very helpful, as it shows which of the events will be deprecated.

Comment on attachment 9121269 [details] [diff] [review] Bug-1226362_remove-nsDragAndDrop-usage-chat-0.patch Review of attachment 9121269 [details] [diff] [review]: ----------------------------------------------------------------- Code looks sane to me, but I can't say I understand all the changes. Magnus' review & testing here should be enough.

Comment on attachment 9121269 [details] [diff] [review] Bug-1226362_remove-nsDragAndDrop-usage-chat-0.patch Review of attachment 9121269 [details] [diff] [review]: ----------------------------------------------------------------- Looks to be working pretty ok, but I get this if I happen to drag the actual text node JavaScript error: chrome://messenger/content/chat/imAccounts.js, line 710: TypeError: aEvent.target.closest is not a function I think you need to do this check ``` onDragStart(aEvent) { let target = aEvent.target; if (target.nodeType != Node.ELEMENT_NODE) { target = target.parentNode; } let accountElement = target.closest( 'richlistitem[is="chat-account-richlistitem"]' ); ``` And probably the same for ondragover and drop ::: mail/components/im/content/imAccounts.js @@ +716,5 @@ > if (gAccountManager.accountList.itemCount == 1) { > throw new Error("Can't drag while there is only one account!"); > } > > + aEvent.dataTransfer.effectAllowed = "all"; should this also be "move"?

Comment on attachment 9121883 [details] [diff] [review] Bug-1226362_remove-nsDragAndDrop-usage-addresss-0.patch Review of attachment 9121883 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/addrbook/content/abDragDrop.js @@ +97,2 @@ > > + event.dataTransfer.effectAllowed = "all"; copyMove or copy, depending on case, like it was before?

Comment on attachment 9122572 [details] [diff] [review] Bug-1226362_remove-nsDragAndDrop-usage-messenger-0.patch Review of attachment 9122572 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailCore.js @@ +859,5 @@ > * > * @param aAttachment the attachment object > * @return the TransferData > */ > +function CreateAttachmentTransferData(event, attachmentData, index) { I think it would be preferable if this took event, attachments as arguments Also, return value is now void. Please update the documentation. @@ +874,2 @@ > > + if (attachmentData.url && name) { could just do an early return when not set

Comment on attachment 9122964 [details] [diff] [review] Bug-1226362_remove-nsDragAndDrop-usage-messenger-1.patch Review of attachment 9122964 [details] [diff] [review]: ----------------------------------------------------------------- At least dragging a file from the file manager to compose (attach area) doesn't work. JavaScript error: chrome://messenger/content/messengercompose/MsgComposeCommands.js, line 6811: TypeError: data.exists is not a function ::: mail/base/content/mailCore.js @@ +853,5 @@ > return displayName.replace(/(.)\1{9,}/g, "$1…$1"); > } > > /** > * Create a TransferData object for a message attachment, either from the This is now incorrect. @@ +856,5 @@ > /** > * Create a TransferData object for a message attachment, either from the > * message reader or the composer. > * > + * @param event the associated event @param {Event} event - The associated event. @@ +857,5 @@ > * Create a TransferData object for a message attachment, either from the > * message reader or the composer. > * > + * @param event the associated event > + * @param attachments the array of attachment objects @param {nsIMsgAttachment} attachments - The attachments to setup @@ +862,2 @@ > */ > +function CreateAttachmentTransferData(event, attachments) { maybe we should rename the function too, setupDataTransfer perhaps? @@ +874,2 @@ > > + var name = attachment.name || attachment.displayName; let's make it let instead of var now that we're touching this, for the variables below too @@ +874,4 @@ > > + var name = attachment.name || attachment.displayName; > + > + if (!attachment.url) { || !name, so you can skip the if(name) below ::: mail/base/content/msgHdrView.js @@ +3390,1 @@ > for (let item of selection) { just do for (let item of target.parentNode.selectedItems) { ::: mail/components/compose/content/MsgComposeCommands.js @@ +6508,5 @@ > * Adjust the drop target when dragging from the attachment bucket onto itself > * by picking the nearest possible insertion point (generally, between two > * list items). > * > + * @param event the drag-and-drop event being performed @param {Event} event - ................ @@ +6708,5 @@ > let attachments = []; > + let dt = event.dataTransfer; > + let dataList = []; > + let count = 0; > + while (count < dt.mozItemCount) { should be a for loop, no? @@ +6718,5 @@ > + let dragObj = { > + data, > + type: flavour, > + }; > + dataList.push(dragObj); please inline dragObj dataList.push({data, type: flavour}); I'd assume the if (data) check is redundant when you already check the flavour is there

Comment on attachment 9123038 [details] [diff] [review] Bug-1226362_remove-nsDragAndDrop-usage-messenger-2.patch Review of attachment 9123038 [details] [diff] [review]: ----------------------------------------------------------------- I'll attach an updated patch. ::: mail/base/content/mailCore.js @@ +859,3 @@ > * > + * @param {Event} event - The associated event. > + * @param {nsIMsgAttachment} attachments - The attachments to setup {nsIMsgAttachment[]} ::: mail/components/compose/content/MsgComposeCommands.js @@ +6508,5 @@ > * Adjust the drop target when dragging from the attachment bucket onto itself > * by picking the nearest possible insertion point (generally, between two > * list items). > * > + * @param {Event} event - the drag-and-drop event being performed nit: double space @@ +6711,5 @@ > + let types = Array.from(dt.mozTypesAt(count)); > + for (let flavour of flavours) { > + if (types.includes(flavour)) { > + let data = dt.mozGetDataAt(flavour, count); > + dataList.push({ data, type: flavour }); Hmm, we don't need to make that type to confuse stuff @@ +6720,2 @@ > for (let dataListObj of dataList) { > + let data = dataListObj.data; let's change this to for (let { data, favour} of dataList) { @@ +6736,2 @@ > try { > + size = data.fileSize; Need to do an (data instanceof Ci.nsIFile) call to make this a file you can access fileSize of Dragged files from the filenmanager didn't get a size now.