1147700 - Add optional parameters to ChromeUtils.synthesizeDrop to specify drop position, modifiers, and avoid overwriting dragData and/or preventDefault.

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Tooru Fujisawa [:arai]]

Separated from bug 1146193.

In the test added by bug 1146193, a function similar to ChromeUtils.synthesizeDrop is used, but there are several differences:
  * keyboard modifier in initDragEvent parameter (ctrlKey/altKey/shiftKey/metaKey)
  * position in initDragEvent parameter (screenX/screenY/clientX/clientY)
  * don't overwrite data in dataTransfer
  * don't call preventDefault/stopPropagation

So, I'd like to add parameter(s) to synthesizeDrop to allow above.

Also, currently synthesizeDrop uses all 0 as screenX/screenY/clientX/clientY, but it should be better to use center of destElement by default, and overwrite them with optional parameter to use different position.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add aDragEvent parameter to ChromeUtils.synthesizeDrop to modify DragEvent, and set default position to center of destElement.

Added sendDragEvent in EventUtils.js since almost same code for drag event is written thrice in ChromeUtils.synthesizeDrop.

Only browser_tabDrop.js supposes ChromeUtils.synthesizeDrop to drop on (0,0), so make it passing parameter with clientX/clientY/screenX/screenY = 0.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bbe660678e6

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Do not overwrite dataTransfer in ChromeUtils.synthesizeDrop if dragData is null.

Currently ChromeUtils.synthesizeDrop always overwrites data in dataTransfer, but to test dragstart handler (or something else which sets dataTransfer), we need to leave it untouched.

Comment 3

[Neil Deakin]

Comment on attachment 8583603 [details] [diff] [review]
Part 1: Add aDragEvent parameter to ChromeUtils.synthesizeDrop to modify DragEvent, and set default position to center of destElement.

>       // A drop type of "link" onto an existing tab would normally trigger a
>       // load in that same tab, but tabbrowser code in _getDragTargetTab treats
>       // drops on the outer edges of a tab differently (loading a new tab
>-      // instead). The events created by synthesizeDrop have all of their
>+      // instead). Make events created by synthesizeDrop to have all of their

Remove 'to'

>+    var destRect = destElement.getBoundingClientRect();
>+    var destClientX = destRect.width / 2;
>+    var destClientY = destRect.height / 2;
>+    var destScreenX = aDestWindow.screenX + destRect.left + destClientX;
>+    var destScreenY = aDestWindow.screenY + destRect.top + destClientY;

clientX and clientY should be relative to the viewport, no? Shouldn't this be:

  var destClientX = destRect.left + destRect.width / 2

and the rest fixed up accordingly.

Comment 4

[Neil Deakin]

Comment on attachment 8583605 [details] [diff] [review]
Part 2: Do not overwrite dataTransfer in ChromeUtils.synthesizeDrop if dragData is null.

>   function dragAndDrop(tab1, tab2, copy) {
>-    let ctrlKey = copy;
>-    let altKey = copy;
>+    let rect = tab1.getBoundingClientRect();
>+    let event = {
>+      ctrlKey: copy,
>+      altKey: copy,
>+      clientX: rect.width / 2 + 10,
>+      clientY: rect.width / 2,
>+    };
> 

Similar comment as the other patch.


>     dataTransfer.dropEffect = dropEffect || "move";
>-    event.preventDefault();
>-    event.stopPropagation();
>-  }
>+    if (dragData) {
>+      event.preventDefault();
>+      event.stopPropagation();
>+    }
>+  };

This should cancel the event all the time to avoid doing any of the default behaviour which we don't want.

Comment 5

[Tooru Fujisawa [:arai]]

Thank you for reviewing :D

(In reply to Neil Deakin from comment #4)
> >     dataTransfer.dropEffect = dropEffect || "move";
> >-    event.preventDefault();
> >-    event.stopPropagation();
> >-  }
> >+    if (dragData) {
> >+      event.preventDefault();
> >+      event.stopPropagation();
> >+    }
> >+  };
> 
> This should cancel the event all the time to avoid doing any of the default
> behaviour which we don't want.

I'd like to test the default behaviour of tab drag-and-drop in browser_tabReorder.js test.
Isn't this function appropriate for that purpose?

Comment 6

[Neil Deakin]

The test seems to work ok without the default behaviour. Which default behaviour do you think you need?

Comment 7

[Tooru Fujisawa [:arai]]

I'd like to test dragstart handler in tabbrowser-tabs binding, which sets tab data to dataTransfer:
  https://hg.mozilla.org/mozilla-central/file/da2f28836843/browser/base/content/tabbrowser.xml#l5077
and the test doesn't work for me if I remove `if (dragData)` condition, because dragstart handler is not called with that change.

Comment 8

[Neil Deakin]

OK, that isn't the default behaviour though. What you want is to just remove the call to stopPropagation, which doesn't need to be called regardless.

Comment 9

[Tooru Fujisawa [:arai]]

Wow, thank you for pointing out my misunderstanding and letting me know the solution :D
Yeah, the test works with that change, I'll post updated patch after some more tests.

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add aDragEvent parameter to ChromeUtils.synthesizeDrop to modify DragEvent, and set default position to center of destElement.

addressed review comments :)

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=26bb5cbbac26

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Do not overwrite dataTransfer in ChromeUtils.synthesizeDrop if dragData is null.

tests for customizableui require calling stopPropagation to suppress dragstart handler.

https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/head.js#188
> function simulateItemDrag(toDrag, target) {
>   let docId = toDrag.ownerDocument.documentElement.id;
>   let dragData = [[{type: 'text/toolbarwrapper-id/' + docId,
>                     data: toDrag.id}]];
>   synthesizeDragStart(toDrag.parentNode, dragData);
>   synthesizeDrop(target, target, dragData);
> }

https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizeMode.jsm#1526
>   _onDragStart: function(aEvent) {
>     __dumpDragData(aEvent);
>     let item = aEvent.target;
> ...
>     dt.mozSetDataAt(kDragDataTypePrefix + documentId, draggedItem.id, 0);
>     dt.effectAllowed = "move";
> ...

failed try run for patches without stopPropagation call:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0ac016a456c

So I added `if (dragData)` condition only to stopPropagation, instead of removing it.

Comment 12

[Neil Deakin]

Comment on attachment 8588088 [details] [diff] [review]
Part 1: Add aDragEvent parameter to ChromeUtils.synthesizeDrop to modify DragEvent, and set default position to center of destElement.

>+    var destScreenX = aDestWindow.screenX + destClientX;
>+    var destScreenY = aDestWindow.screenY + destClientY;
>+    if ("clientX" in aDragEvent && !("screenX" in aDragEvent)) {
>+      aDragEvent.screenX = aDestWindow.screenX + aDragEvent.clientX;
>+    }
>+    if ("clientY" in aDragEvent && !("screenY" in aDragEvent)) {
>+      aDragEvent.screenY = aDestWindow.screenY + aDragEvent.clientY;
>+    }

I think these should use the window's mozInnerScreenX/Y rather than screenX/Y.

Comment 13

[Neil Deakin]

Comment on attachment 8588090 [details] [diff] [review]
Part 2: Do not overwrite dataTransfer in ChromeUtils.synthesizeDrop if dragData is null.

>     event.preventDefault();
>-    event.stopPropagation();
>+    if (dragData) {
>+      event.stopPropagation();
>+    }
>   }

We don't really want different behaviour in each case though, so please file a bug on removing this inconsistency.

Comment 14

[Tooru Fujisawa [:arai]]

Thank you for reviewing :D

https://hg.mozilla.org/integration/mozilla-inbound/rev/697925b7e024
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa7e6ceab5f3

I'll file a bug for customizableui tests shortly.

Comment 15

[Tooru Fujisawa [:arai]]

Filed as bug 1154140.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/697925b7e024
https://hg.mozilla.org/mozilla-central/rev/fa7e6ceab5f3