515776 - Drag and drop doesn't work for chrome and content elements

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Merike Sell (:merike)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; et; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3 (.NET CLR 3.5.30729)
Build Identifier: 

Currently controller.dragDropElemToElem doesn't support xul elements. Even if the reference to ownerDocument.body is removed it doesn't trigger drag and drop. Mozmill simulates it with mousedown-mousemove-mouseup, but xul listens for draggesture and other drag events and therefore ignores drag and drop.

Reproducible: Always

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Merike, would it be possible that you can create a tiny Mozmill test for Firefox which demonstrates this behavior? That would be great.

Comment 2

[Merike Sell (:merike)]

Attachment: Xul drag and drop test

This should drag second button on bookmarks bar to tabbar which either opens getting started page on a tab already open or on a new one depending on drop position. Command succeeds but nothing happens.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Clint and Mikeal, I hope it is ok for you when I add this bug onto our P1 wishlist too. I missed that yesterday and hope that you can take care of it. If it's not doable just move it to qae-p2.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As talked here my list of things we would need:

Requirements
* D&D of elements in XUL and content
* Dropping elements to the same or another browser window
* Drop at a given position (e.g. inserting as text in an input field)

Available functions inside EventUtils.js
* http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#437
* http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#492

Comment 5

[cmtalbert]

We said that we wanted this for 1.4.2, but in talking with Heather more, it does seem like this will end up being wasted work after we take on the webdriver stuff.  

Do we really need this in the short term or can it wait for native events?

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

At least for our Firefox tests it's not the highest priority. We still have a couple of other tests to implement and D&D isn't used that often. So trying to get the webdriver feature implemented, sounds like a better plan. This will be part of the next major release? If yes, can you just add a [mozmill-2.0] whiteboard entry?

Merike, what about tests for calendar? How important is it for you?

Comment 7

[Merike Sell (:merike)]

Well drag and drop can be used for
 * dragging event to a different day
 * dragging event start or end time
 * rearranging calendars in calendar list
I'd suspect that first two are used relatively often and the last one not so much.

And even then there are other areas that can be automated before. So it's a nice to have but probably not worth duplicating any work.

When are said native events supposed to be available?

Comment 8

[Joachim Herb]

I am trying to create test cases for bug 519956 (see e.g. Bug 519956 comments 133, 143):

I tried to add the following test case to http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/message-header/test-message-header.js:

function test_customize_header_toolbar_functions(){
  var defaultButtons = get_header_toolbar_default_buttons();
  var mcCHT = open_customization_dialog();
  
  defaultButtons.reverse();
  defaultButtons.forEach( function(strButton) {
      //mcCHT.window.alert(strButton);
      var button = mc.eid(strButton);
      mcCHT.dragDropElemToElem(button, mcCHT.eid("palette-box"));
      //mc.mouseDown(button);
      mc.sleep(1000);
      //mc.mouseUp(mc.eid("header-view-toolbar"));
      //mcCHT.mouseOver(mcCHT.eid("palette-box"));
      //mcCHT.mouseUp(mcCHT.eid("palette-box"));
      //mcCHT.sleep(1000);
    }
  )
}

function get_header_toolbar_default_buttons() {
  var strDefaultButtons = mc.eid("header-view-toolbar").
                            node.getAttribute("defaultset");
  var defaultButtons = strDefaultButtons.split(",");
  return defaultButtons;
}

function open_customization_dialog() {
  mc.rightClick(new elementslib.ID(mc.window.document, 
                                   "header-view-toolbar"));
  mc.click(new elementslib.ID(mc.window.document, 
                              "CustomizeHeaderToolbar")); 
  
  // For this to work also CustomizeToolbarOverlay had to be patched:
  // new windowtype "mail:customize"
  var mcCHT = wh.wait_for_existing_window("mail:customize");
  return mcCHT;
}

I will add the resulting error message as attachment to this bug.

Comment 9

[Joachim Herb]

Attachment: Error message for drag and drop test case

Just another comment: I also tried the out-commented version of the test case with mouseDown/mouseMove/mouseUp which didn't have any effect.

Comment 10

[Joachim Herb]

Attachment: Now also with the stderr output.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

With bug 578130 we have landed the recent updates for EventUtils.js for Mozmill. I wonder if there are improvements which could help us with that bug.

Comment 12

[Joachim Herb]

It looks like there are no changes. A sequence with mouseDown(), mouseOver(), mouseUp() still does not move xul elements (i.e. toolbar buttons) around.

This is the installed version of the command line mozmill:
mozmill 1.4.2b1

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #12)
> It looks like there are no changes. A sequence with mouseDown(), mouseOver(),
> mouseUp() still does not move xul elements (i.e. toolbar buttons) around.

Right. That's not an initiated D&D action when you are using synthesized events, because you will have to specify the transfer object.

Our team has to focus on Panorama tests for Q4 this year. So having a solution we could use would be awesome. If it's not a fix we can get for Mozmill 1.5.2 it would be great if you could support us by implementing an helper function in our test repository.

But finally this missing feature is getting more important and we really miss it. I know it's not the final solution but as long as we do not have the native events (webdriver) API included, we could at least implement tests.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As seen while working on the Panorama tests it also doesn't work for content elements. Updating summary.

Comment 15

[cmtalbert]

Heather is actively working on this, and we are planning to take it for 1.5.2.

Comment 16

[cmtalbert]

Heather, what's the status on this?

Comment 17

[Heather Arthur [:harth]]

 EventUtils' sythesizeDrop works for html and xul elements, I have that and a tabview test in my dnd branch: https://github.com/harthur/mozmill/blob/dnd/mozmill/mozmill/extension/resource/modules/controller.js#L1130

I'm working on testcases for the html and xul cases, After that we'll need to try to get it to work in the case of dragging content to chrome and vv.

synthesizeDrop uses both mouse and HTML dnd events, this is the only way to get it working for html, xul, and tabcandy (which uses pure mouse events instead of the HTML dnd API).

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Is there any way so we do not have to set the flavor of the dragged data?

Comment 19

[Heather Arthur [:harth]]

(In reply to comment #18)
> Is there any way so we do not have to set the flavor of the dragged data?

setting the drag data is optional

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sure but do you think that we do not have to specify the flavor in nearly all of our cases? What about reordering bookmarks in the bookmarks toolbar? I'm sure that they have another flavor.

Comment 21

[Heather Arthur [:harth]]

Attachment: controller.dragToElement() and test cases

This patch replaces controller.dragDropElemToElem() with controller.dragToElement(). The function synthesizes both mouse and drag events. It does not yet work across windows (i.e. dragging from content to chrome).

There are also a couple testcases. I put the html and xul files in the extension so the testcases could access them from a chrome:// url, I couldn't get the local file server hosting to work.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #21)
> There are also a couple testcases. I put the html and xul files in the
> extension so the testcases could access them from a chrome:// url, I couldn't
> get the local file server hosting to work.

See http://hg.mozilla.org/qa/mozmill-tests/file/5e234391c916/firefox/testTabbedBrowsing/testOpenInBackground.js#l48. It's fairly trivial to do.

Comment 23

[Heather Arthur [:harth]]

(In reply to comment #22)
> (In reply to comment #21)
> > There are also a couple testcases. I put the html and xul files in the
> > extension so the testcases could access them from a chrome:// url, I couldn't
> > get the local file server hosting to work.
> 
> See
> http://hg.mozilla.org/qa/mozmill-tests/file/5e234391c916/firefox/testTabbedBrowsing/testOpenInBackground.js#l48.
> It's fairly trivial to do.

I've tried that and it didn't work unfortunately, they didn't seem to be hosted.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #23)
> I've tried that and it didn't work unfortunately, they didn't seem to be
> hosted.

Where have you placed those files? A subfolder of the mozmill/tests folder? Probably the relative path was not correctly set?

Comment 25

[Heather Arthur [:harth]]

(In reply to comment #24)
> (In reply to comment #23)
> > I've tried that and it didn't work unfortunately, they didn't seem to be
> > hosted.
> 
> Where have you placed those files? A subfolder of the mozmill/tests folder?
> Probably the relative path was not correctly set?

It works for the html file but not the xul file. Works fine in 3.6 so I think this has to do with remote XUL being disabled.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #25)
> It works for the html file but not the xul file. Works fine in 3.6 so I think
> this has to do with remote XUL being disabled.

Oh, excellent argument! That's correct. Support for remote XUL has been removed. So yeah, worth putting the test file into the extension itself.

Comment 27

[Heather Arthur [:harth]]

Attachment: controller.dragToElement() and test cases

new patch makes the window argument optional (infers it from the source element). Bonus, found that this works for dragging content to chrome, within the same main window at least, so added a testcase for that.

Comment 28

[cmtalbert]

Comment on attachment 502957 [details] [diff] [review]
controller.dragToElement() and test cases

>diff --git a/mozmill/mozmill/extension/content/test/test.js b/mozmill/mozmill/extension/content/test/test.js
>new file mode 100644
>index 0000000..d1bb5fe
>--- /dev/null
>+++ b/mozmill/mozmill/extension/content/test/test.js
I don't really like that this test is now a part of mozmill by virtue of being in the extension, but I don't see any other way to do it, so it's ok for now.  Let's try to keep the tests out of the core code if we can help it in the future.  For this one, I know its here because we turned off XUL in content in Ffx 4 so this is the only way you can serve it.
>diff --git a/mozmill/mozmill/extension/resource/modules/controller.js b/mozmill/mozmill/extension/resource/modules/controller.js
>index daac8d9..a31837d 100644
>--- a/mozmill/mozmill/extension/resource/modules/controller.js
>+++ b/mozmill/mozmill/extension/resource/modules/controller.js
>@@ -1125,36 +1125,57 @@ MozMillController.prototype.mouseMove = function (doc, start, dest) {
>   return true;
> }
> 
>+// Drag an element to the specified offset on another element, firing mouse and drag events
>+//  win must be the window both elements are in. Adapted from EventUtils' synthesizeDrop()
win?  I think that should be aWindow?  Is that the parameter you're referring to?
>+MozMillController.prototype.dragToElement = function(src, dest, offsetX,
>+    offsetY, aWindow, dropEffect, dragData) {
>+  srcElement = src.getNode();
>+  destElement = dest.getNode();
>+  aWindow = aWindow || srcElement.ownerDocument.defaultView;
>+  offsetX = offsetX || 20;
>+  offsetY = offsetY || 20;
>+
>+  var dataTransfer;
>+
>+  aWindow.addEventListener("dragstart", trapDrag, true);
>+  EventUtils.synthesizeMouse(srcElement, 2, 2, { type: "mousedown" }, aWindow);
>+  EventUtils.synthesizeMouse(srcElement, 11, 11, { type: "mousemove" }, aWindow);
Please comment the magic numbers           ^^^^^
What are they, why are they 2 and 11?  etc.

>+  EventUtils.synthesizeMouse(srcElement, offsetX, offsetY, { type: "mousemove" }, aWindow);
>+  aWindow.removeEventListener("dragstart", trapDrag, true);
>+
>+  var event = aWindow.document.createEvent("DragEvents");
>+  event.initDragEvent("dragenter", true, true, aWindow, 0, 0, 0, 0, 0, false, false, false, false, 0, null, dataTransfer);
>+  destElement.dispatchEvent(event);
>+
>+  var event = aWindow.document.createEvent("DragEvents");
>+  event.initDragEvent("dragover", true, true, aWindow, 0, 0, 0, 0, 0, false, false, false, false, 0, null, dataTransfer);
>+  if (destElement.dispatchEvent(event)) {
>+    EventUtils.synthesizeMouse(destElement, offsetX, offsetY, { type: "mouseup" }, aWindow);
>+    return "none";
Why do you return the string "none"?  Why not return something standard like null or undefined or...something.  

r+ with those changes and questions answered

Comment 29

[Heather Arthur [:harth]]

> >+// Drag an element to the specified offset on another element, firing mouse and drag events
> >+//  win must be the window both elements are in. Adapted from EventUtils' synthesizeDrop()
> win?  I think that should be aWindow?  Is that the parameter you're referring
> to?

good catch, yeah.

> >+    EventUtils.synthesizeMouse(destElement, offsetX, offsetY, { type: "mouseup" }, aWindow);
> >+    return "none";
> Why do you return the string "none"?  Why not return something standard like
> null or undefined or...something.  

"none" is one of the accepted dropEffects.

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #28)
> >+++ b/mozmill/mozmill/extension/content/test/test.js
> I don't really like that this test is now a part of mozmill by virtue of being
> in the extension, but I don't see any other way to do it, so it's ok for now. 
> Let's try to keep the tests out of the core code if we can help it in the
> future.  For this one, I know its here because we turned off XUL in content in
> Ffx 4 so this is the only way you can serve it.

We should probably move the test to resource:// instead of having it in chrome://. Wouldn't that be better (and also work)?

Comment 31

[cmtalbert]

(In reply to comment #29)
> 
> > >+    EventUtils.synthesizeMouse(destElement, offsetX, offsetY, { type: "mouseup" }, aWindow);
> > >+    return "none";
> > Why do you return the string "none"?  Why not return something standard like
> > null or undefined or...something.  
> 
> "none" is one of the accepted dropEffects.
Ah ok.  Thanks for explaining that -- probably worth a comment in the code.

(In reply to comment #30)
> (In reply to comment #28)
> > >+++ b/mozmill/mozmill/extension/content/test/test.js
> We should probably move the test to resource:// instead of having it in
> chrome://. Wouldn't that be better (and also work)?
No, you can't serve a UI out of a resource:// URL to my knowledge.  That only works for javascript.  And if you can, I wouldn't trust it as a test.  Besides with it as a standard part of chrome:// we can use it for other tests if need be and be confident of it working the same way.

Besides, whether it is resource:// or chrome:// it is still part of the extension, which is what I was objecting to.

Comment 32

[Heather Arthur [:harth]]

(In reply to comment #30)
> (In reply to comment #28)
> > >+++ b/mozmill/mozmill/extension/content/test/test.js
> > I don't really like that this test is now a part of mozmill by virtue of being
> > in the extension, but I don't see any other way to do it, so it's ok for now. 
> > Let's try to keep the tests out of the core code if we can help it in the
> > future.  For this one, I know its here because we turned off XUL in content in
> > Ffx 4 so this is the only way you can serve it.
> 
> We should probably move the test to resource:// instead of having it in
> chrome://. Wouldn't that be better (and also work)?

I don't think it belongs in resource:// anymore than chrome://

Comment 33

[Heather Arthur [:harth]]

1.5.2:
https://github.com/mozautomation/mozmill/commit/7dd15a8cf3436e1dfdeab92c104bffbc049cd545

master:
https://github.com/mozautomation/mozmill/commit/1d8b3eb9b2bea3b67ba1fc333726746e1b8bfe84

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Anthony, can you please check if that fix completely enables us to create drag&drop tests for Panorama?

Comment 35

[u279076]

(In reply to comment #34)
> Anthony, can you please check if that fix completely enables us to create
> drag&drop tests for Panorama?

I will as soon as I have a free moment.  I'm currently working on 3 Firefox releases and Mozmill failures in tandem.  If you need this checked now, please find someone else to test.  I probably will not have time until late this week, early next week.

Comment 36

[u279076]

Attachment: Proof of Concept v1

Here is a WIP POC.  So far it "works".  The first tab in the group kind of wobbles.  I can't seem to find an intelligent way to determine valid space (ie. offsets) to drag this element to in #content.

Any suggestions?

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 506829 [details] [diff] [review]
Proof of Concept v1

No need to attach patches to the implementation bug. If you have problems with specifics of the drag and drop action, which should be included file new bugs, and mark them as dependent. If it doesn't work at all please reopen.

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #36)
> Here is a WIP POC.  So far it "works".  The first tab in the group kind of
> wobbles.  I can't seem to find an intelligent way to determine valid space (ie.
> offsets) to drag this element to in #content.

An optional drop position is absolutely necessary for us. It that not provided by tha API? As far as I can see it is available. You simply have to add more parameter as given here:

https://github.com/mozautomation/mozmill/commit/7dd15a8cf3436e1dfdeab92c104bffbc049cd545#L4R1130

Also check the existing tests in the patch how those can help us to verify this feature.

Comment 39

[u279076]

Well...

https://github.com/mozautomation/mozmill/commit/7dd15a8cf3436e1dfdeab92c104bffbc049cd545#L4R1159

https://github.com/mozautomation/mozmill/commit/7dd15a8cf3436e1dfdeab92c104bffbc049cd545#L4R1169

...looks like the offsets are used for both the source and destination elements.  While this might work in theory, it's not necessarily what I want.  What I really want is to be able to detect empty space in content of the Panorama view where a tab can be dragged to create a new tabgroup.

If this is not possible with the current API, I can file a bug for it.

Comment 40

[Heather Arthur [:harth]]

(In reply to comment #36)
> Created attachment 506829 [details] [diff] [review]
> Proof of Concept v1
> 
> Here is a WIP POC.  So far it "works".  The first tab in the group kind of
> wobbles.  I can't seem to find an intelligent way to determine valid space (ie.
> offsets) to drag this element to in #content.
> 
> Any suggestions?

Unfortunately you'll have to pick some offsets that appear to work. For my test I did:

controller.dragToElement(tab, content, 500, 600);

which will drag to an offset of 500 and 600 on the #content element. If you don't drag far enough you'll get that wobbly effect as the drop action is cancelled because you didn't drag to a drop location. You just have to play around with the numbers and see what works visually.

Comment 41

[u279076]

So, as a temporary workaround, I think I can live with that.  However, I think we should try to find some way to detect valid drop space.  I can file a bug for that if no one objects.

Comment 42

[Heather Arthur [:harth]]

(In reply to comment #39)
> 
> ...looks like the offsets are used for both the source and destination
> elements.  While this might work in theory, it's not necessarily what I want. 
> What I really want is to be able to detect empty space in content of the
> Panorama view where a tab can be dragged to create a new tabgroup.
> 
> If this is not possible with the current API, I can file a bug for it.

Detecting empty space in general isn't easy, you'd have to get all the elements in the page and use their bounding boxes to calculate it somehow.

Comment 43

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #39)
> ...looks like the offsets are used for both the source and destination
> elements.  While this might work in theory, it's not necessarily what I want. 
> What I really want is to be able to detect empty space in content of the
> Panorama view where a tab can be dragged to create a new tabgroup.

This is clearly not part of Mozmill. It's the job of the underlying shared module. 

> If this is not possible with the current API, I can file a bug for it.

Yes, please file a bug but not for Mozmill.

Given the above information I can see that it works as expected. Can we mark the bug as verified?

Comment 44

[u279076]

(In reply to comment #43)
> Can we mark the bug as verified?

Not yet. I still haven't verified it working with a set of offsets.  Let me test that and make it verified.

I'll file a bug for the shared module too.

Comment 45

[u279076]

Marking this verified.  An offset of 500, 600 seems to work.  I'll file a follow up bug to investigate improvements to this API through a shared module.

Thanks.

Comment 46

[Joachim Herb]

I would like to ask if this patch makes a test possible like I described in comment #8: Can a toolbutton be dragged from the toolbar palette and dropped onto a toolbar? The toolbar palette is in a different window than the toolbar.