Closed Bug 1147700 Opened 5 years ago Closed 5 years ago

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

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: arai, Assigned: arai)

Details

Attachments

(2 files, 2 obsolete files)

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.
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
Attachment #8583603 - Flags: review?(enndeakin)
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.
Attachment #8583605 - Flags: review?(enndeakin)
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.
Attachment #8583603 - Flags: review?(enndeakin) → review-
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.
Attachment #8583605 - Flags: review?(enndeakin)
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?
Flags: needinfo?(enndeakin)
The test seems to work ok without the default behaviour. Which default behaviour do you think you need?
Flags: needinfo?(enndeakin)
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.
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.
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.
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.
Assignee: nobody → arai.unmht
Attachment #8583605 - Attachment is obsolete: true
Attachment #8588090 - Flags: review?(enndeakin)
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.
Attachment #8588088 - Flags: review?(enndeakin) → review+
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.
Attachment #8588090 - Flags: review?(enndeakin) → review+
Filed as bug 1154140.
https://hg.mozilla.org/mozilla-central/rev/697925b7e024
https://hg.mozilla.org/mozilla-central/rev/fa7e6ceab5f3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.