Closed Bug 846007 Opened 11 years ago Closed 11 years ago

Mutt test dnd_chrome.js appears to hang when run unless mouse movement issued

Categories

(Testing Graveyard :: Mozmill, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jfrench, Assigned: andrei)

References

()

Details

(Whiteboard: [mozmill-2.0][ateamtrack: p=mozmill q=2013q2 m=4][sprint2013-34])

Attachments

(4 files, 6 obsolete files)

I'm observing that dnd_chrome.js hangs the mutt test run, unless some sort of mouse movement is issued to the system during that particular test. If, in a subsequent mutt run you jiggle the mouse during dnd_chrome, the test will actually continue and pass.

A full synopsis of the test client config, the steps to reproduce, and the error has been attached.
Jonathan, can you see in which step it is hanging? You might want to run with --console-level set to debug to get all the information.
Component: Mozmill Tests → Mozmill
Priority: P4 → --
Product: Mozilla QA → Testing
Sure, I will attach a --console-level debug output of the individual test. It looks like mozelement failing to find a particular element ID, "item1". 

Interestingly, mozmill considers the test a fail in both test conditions; including when you jiggle the mouse to allow the test to complete. However mutt - considers and marks the same test a pass. So mozmill and mutt reporting, appear to disagree.
And to that point, here is an excerpt from the mutt output, on a run where you jiggled the mouse to let it continue (the same test condition that mozmill reports a fail)

Running JS Tests
[1;34mTEST-START[0m | js\assertions\expect_assert.js | testExpect
[1;32mTEST-PASS[0m | js\assertions\expect_assert.js | testExpect
[1;34mTEST-START[0m | js\assertions\expect_assert.js | testAssert
[1;32mTEST-PASS[0m | js\assertions\expect_assert.js | testAssert
TEST-END | js\assertions\expect_assert.js | finished in 73ms
[1;34mTEST-START[0m | js\controller\contextmenu.js | setupModule
[1;34mTEST-START[0m | js\controller\contextmenu.js | testMenu
[1;32mTEST-PASS[0m | js\controller\contextmenu.js | testMenu
TEST-END | js\controller\contextmenu.js | finished in 1435ms
[1;34mTEST-START[0m | js\controller\dnd_chrome.js | setupModule
[1;34mTEST-START[0m | js\controller\dnd_chrome.js | test
[1;32mTEST-PASS[0m | js\controller\dnd_chrome.js | test *******************
TEST-END | js\controller\dnd_chrome.js | finished in 1448ms
[1;34mTEST-START[0m | js\controller\dnd_content.js | setupModule
[1;34mTEST-START[0m | js\controller\dnd_content.js | test
[1;32mTEST-PASS[0m | js\controller\dnd_content.js | test
Attached file mozmill console debug
Console level debug of failing test, run with mozmill 2.0rc1 w/clean master.
That's bad, indeed. We should file a separate bug to get Mutt correctly recognize the failure. Otherwise the usefulness of the test framework is in question. Would you mind to file it?

So in this case we should figure out why item1 is not available. We are using a local test page for testing, right? I wonder how it is related to the fix in bug 760410.

In all the cases we should not stall the test. If that happens I would love to hear which line of code is responsible for.

Thanks for the detailed information, Jon. Just one info for you: You might want to use the mozmill-env instead which colors the log output. Just execute run.cmd and run setup_development.py inside to update to mozmill 2.0.

I hope it's ok to assign this bug for you?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Sure, I will enter a new separate bug, for Mutt appearing not to recognize the failure.

Can anyone else reproduce this bug though, of dnd_chrome.js appearing to hang? 

I am concerned it may be just me or my particular system. I will also try running it in mozmill-env to see if that produces any different results. 

I will also attach a screen grab of the state the browser is in when it stopped/hung in the test. I'm not sure if I will be able to fix it, I can try investigating it further and maybe it will need to be handed off to someone. My first thought is that the element ID "item1" would actually be there if the page loaded properly, but some other deeper architectural event is preventing the page from loading.
Attached image mutt screen grab
Screen grab of dnd_chrome.js in its hung state, running under Mutt. This is just prior to any "wiggling" of the mouse to get Mutt to continue. If you let the test sit, eventually Mutt's jsbridge times out the test.
(In reply to Jonathan French (:jfrench) from comment #6)
> Sure, I will enter a new separate bug, for Mutt appearing not to recognize
> the failure.

Reported as bug 846351.
I see this only happening on Windows.

The weird thing I found is that the element appears to be dragged to the mouse position instead of the target element. This can be seen on slower machines (I'm running Linux and Windows as VM's).

Under windows it appears not to register the mouse position until you move it slightly, then the reb box is drawn under it fast and the test passes.
Something what I see on Linux is that the dragged data is not released anymore and is always carried to the next tests. Not sure if that has an influence here.
Under windows the sendMouseEvent is not triggered until you manually move the mouse over the window.

This might be a deeper problem somewhere, since that is directly calling nsIDOMWindowUtils

https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/stdlib/EventUtils.js#L247
Status: NEW → ASSIGNED
Assignee: nobody → andrei.eftimie
What's happening with Mozmill 1.5? Does it work with this version?
Mozmill 1.5 exhibits the same behaviour.
Please check if the latest version of EventUtils.js fixes it. You can find it here:
http://hg.mozilla.org/mozilla-central/filelog/feccfce43b59/testing/mochitest/tests/SimpleTest/EventUtils.js

I think in any case we might want to upgrade. There are tons of very helpful features in there. So lets spun off a separate bug for it.
Same problem with latest EventUtils.js version
Whiteboard: [sprint2013-34]
Finally some good news:

controller.dragToElement() is based on EventUtils.synthesizeDrop() which has been removed from the latest EventuUtils.

I've found a newer version of synthesizeDrop() from before it was removed, and updated controller.dragToElement() with that code and we no longer need to move the mouse to pass under windows.

Needs more testing, but it looks promising.
Depends on: 874895
The updated controller.dragToElement() uses some new methods from EventUtils (mainly synthesizeMouseAtCenter() -- which we probably _could_ live without).

Opened bug 874895 for the EventUtils.js upgrade. Created a new bug for it since its scope is so much bigger than this bug. Simply upgrading the library causes lots of failures in other tests due to methods that have been removed, renamed or replaced.

It might be worth doing it now since it supports lots of new touch events.
Attached patch patch v1 (obsolete) — Splinter Review
Updated and adapted controller.dragToElement() from a newer version of synthesizeDrop [1].
No testsruns since dragToElement is only used in 3 mutt tests.

* Removed custom offsets and also updated dnd_content_chrome.js to not use them.

[1] http://hg.mozilla.org/mozilla-central/file/feccfce43b59/testing/mochitest/tests/SimpleTest/ChromeUtils.js#l218
Attachment #753808 - Flags: review?(hskupin)
I just tried the patch above, along with "patch 1" for bug 874895 for EventUtils.js and I see the same; dnd_chrome.js test now passes cleanly when no mouse movement occurs.

I tested it about a dozen times to make sure the results were consistent, running all the tests enabled in mutt/mutt/tests/js/controller/tests.ini.

I also did a run and actually did the opposite, issuing some random mouse movement for interest. That was fine also.
Comment on attachment 753808 [details] [diff] [review]
patch v1

Review of attachment 753808 [details] [diff] [review]:
-----------------------------------------------------------------

Good to see that you were able to get this working. Nice work. I haven't checked the patch yet on my own but there are a couple of things we should clarify and update first. Please see my comments inline.

::: mozmill/mozmill/extension/resource/driver/controller.js
@@ +823,5 @@
>   * Adapted from EventUtils' synthesizeDrop()
>   */
> +MozMillController.prototype.dragToElement = function (src, dest, dragData,
> +                                                      dropEffect, aWindow,
> +                                                      aDestWindow) {

We cannot change the signature of a public method. You will have to keep all formerly used arguments. We can rename them to make it clearer to understand.

@@ -828,5 @@
>    srcElement = src.getNode();
>    destElement = dest.getNode();
>    aWindow = aWindow || srcElement.ownerDocument.defaultView;
> -  offsetX = offsetX || 20;
> -  offsetY = offsetY || 20;

Why do you remove that? The offset is important and can't be hard-coded values as also mentioned below.

@@ +830,5 @@
>    aWindow = aWindow || srcElement.ownerDocument.defaultView;
> +  aDestWindow = aDestWindow || destElement.ownerDocument.defaultView;
> +
> +  var gWindowUtils = aDestWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                .getInterface(Ci.nsIDOMWindowUtils);

Please don't use the Hungarian notation for locally created variables. We don't do this anywhere else.

Also make sure to add 'var' to all declarations here so we do not pollute the global scope.

@@ +855,5 @@
>  
> +  ds.startDragSession();
> +
> +  try {
> +    // need to use real mouse action

Is that a TODO or just a comment? Please make this a bit clearer.

@@ +864,5 @@
> +    var x = rect.width / 2;
> +    var y = rect.height / 2;
> +
> +    EventUtils.synthesizeMouse(srcElement, x, y, { type: "mousemove" }, aWindow);
> +    EventUtils.synthesizeMouse(srcElement, x+10, y+10, { type: "mousemove" }, aWindow);

I wonder why we need two mousemove events here. Do you know why? Also why does the second one have hardcoded values? Have you found some tests like browser chrome tests where the same code is used?

@@ +865,5 @@
> +    var y = rect.height / 2;
> +
> +    EventUtils.synthesizeMouse(srcElement, x, y, { type: "mousemove" }, aWindow);
> +    EventUtils.synthesizeMouse(srcElement, x+10, y+10, { type: "mousemove" }, aWindow);
> +    aWindow.removeEventListener("dragstart", trapDrag, true);

I don't feel well here that we directly remove the event listener again. Usually it should be removed from within the callback. How would we make sure that the event has been processed before we continue?

@@ +870,5 @@
> +
> +    var event = aDestWindow.document.createEvent("DragEvents");
> +    event.initDragEvent("dragenter", true, true, aDestWindow, 0, 0, 0, 0, 0,
> +                        false, false, false, false, 0, null, dataTransfer);
> +    gWindowUtils.dispatchDOMEventViaPresShell(destElement, event, true);

We don't have to check the return value here?

@@ +872,5 @@
> +    event.initDragEvent("dragenter", true, true, aDestWindow, 0, 0, 0, 0, 0,
> +                        false, false, false, false, 0, null, dataTransfer);
> +    gWindowUtils.dispatchDOMEventViaPresShell(destElement, event, true);
> +
> +    var event = aDestWindow.document.createEvent("DragEvents");

nit: redeclaration of event.

@@ +884,5 @@
>  
> +    var event = aWindow.document.createEvent("DragEvents");
> +    event.initDragEvent("drop", true, true, aWindow, 0, 0, 0, 0, 0,
> +                        false, false, false, false, 0, null, dataTransfer);
> +    gWindowUtils.dispatchDOMEventViaPresShell(destElement, event, true);

We don't have to check the return value here?

@@ +890,4 @@
>  
> +    return dataTransfer.dropEffect;
> +  } finally {
> +    ds.endDragSession(true);

We also have to unregister formerly registered event handlers if those haven't been removed e.g. due to an exception.
Attachment #753808 - Flags: review?(hskupin) → review-
Attached patch patch v2 (obsolete) — Splinter Review
Addressed issues.

And some comments/answers:


> ::: mozmill/mozmill/extension/resource/driver/controller.js
> @@ +823,5 @@
> >   * Adapted from EventUtils' synthesizeDrop()
> >   */
> > +MozMillController.prototype.dragToElement = function (src, dest, dragData,
> > +                                                      dropEffect, aWindow,
> > +                                                      aDestWindow) {
> 
> We cannot change the signature of a public method. You will have to keep all formerly used arguments. We can rename them to make it clearer to understand.

We still need to add a new argument to the function.
Reenabled the offset arguments.
Also renamed them to be more clear in the rest of the method who is what.

> @@ -828,5 @@
> >    srcElement = src.getNode();
> >    destElement = dest.getNode();
> >    aWindow = aWindow || srcElement.ownerDocument.defaultView;
> > -  offsetX = offsetX || 20;
> > -  offsetY = offsetY || 20;
> 
> Why do you remove that? The offset is important and can't be hard-coded values as also mentioned below.

Reenabled them.
I initially took them out because we don't really have any use for them at the moment.

> @@ +855,5 @@
> >  
> > +  ds.startDragSession();
> > +
> > +  try {
> > +    // need to use real mouse action
> 
> Is that a TODO or just a comment? Please make this a bit clearer.

Removed that altogether. 

> @@ +864,5 @@
> > +    var x = rect.width / 2;
> > +    var y = rect.height / 2;
> > +
> > +    EventUtils.synthesizeMouse(srcElement, x, y, { type: "mousemove" }, aWindow);
> > +    EventUtils.synthesizeMouse(srcElement, x+10, y+10, { type: "mousemove" }, aWindow);
> 
> I wonder why we need two mousemove events here. Do you know why? Also why does the second one have hardcoded values? Have you found some tests like browser chrome tests where the same code is used?

No idea, that's what the original code had.
Tested a bit and it works just as well with only 1 mousemove event.
Removed the superfluous one.

> @@ +865,5 @@
> > +    var y = rect.height / 2;
> > +
> > +    EventUtils.synthesizeMouse(srcElement, x, y, { type: "mousemove" }, aWindow);
> > +    EventUtils.synthesizeMouse(srcElement, x+10, y+10, { type: "mousemove" }, aWindow);
> > +    aWindow.removeEventListener("dragstart", trapDrag, true);
> 
> I don't feel well here that we directly remove the event listener again. Usually it should be removed from within the callback. How would we make sure that the event has been processed before we continue?

We're removing the event listener int he callback now.

> @@ +870,5 @@
> > +
> > +    var event = aDestWindow.document.createEvent("DragEvents");
> > +    event.initDragEvent("dragenter", true, true, aDestWindow, 0, 0, 0, 0, 0,
> > +                        false, false, false, false, 0, null, dataTransfer);
> > +    gWindowUtils.dispatchDOMEventViaPresShell(destElement, event, true);
> 
> We don't have to check the return value here?

I'm not sure how/where that would help us.
Attachment #753808 - Attachment is obsolete: true
Attachment #756582 - Flags: review?(hskupin)
Is there anything we would have to update here given that the new version of the EventUtils module has now been landed?
This is good as it is, applies correctly and fixes the problem.
Comment on attachment 756582 [details] [diff] [review]
patch v2

Review of attachment 756582 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozmill/mozmill/extension/resource/driver/controller.js
@@ +825,5 @@
>   */
> +MozMillController.prototype.dragToElement = function (aSrc, aDest, aOffsetX,
> +                                                      aOffsetY, aDragData,
> +                                                      aDropEffect, aSourceWindow,
> +                                                      aDestWindow) {

As mentioned last time please keep the right order of parameters. We cannot mix them up here. Any new parameter has to be added at the end of the list.

@@ +833,5 @@
> +  var offsetY = aOffsetY || 10;
> +  var srcWindow = aSourceWindow || srcElement.ownerDocument.defaultView;
> +  var destWindow = aDestWindow || destElement.ownerDocument.defaultView;
> +
> +  var gWindowUtils = destWindow.QueryInterface(Ci["nsIInterfaceRequestor"])

As mentioned last time please use Ci.XYZ. This applies to a couple of similar instances.

@@ +846,2 @@
>  
> +    if (!aDragData) return;

What type is aDragData? Would you mind to add a JSDoc comment to the function? If it's an array what about an empty list? 

Also please keep our style. New line and brackets.

@@ +864,5 @@
> +    srcWindow.addEventListener("dragstart", trapDrag, true);
> +    EventUtils.synthesizeMouseAtCenter(srcElement, { type: "mousedown" }, srcWindow);
> +    var rect = srcElement.getBoundingClientRect();
> +    var x = rect.width / 2 + offsetX;
> +    var y = rect.height / 2 + offsetY;

Those are the target coordinates right? Why do we use the elements center for it? That will make it hard to find the right drop position if the element gets dynamically update. I think we should start from the top left corner.

@@ +874,5 @@
> +    event.initDragEvent("dragover", true, true, destWindow, 0, 0, 0, 0, 0,
> +                        false, false, false, false, 0, null, dataTransfer);
> +    event.initDragEvent("drop", true, true, srcWindow, 0, 0, 0, 0, 0,
> +                        false, false, false, false, 0, null, dataTransfer);
> +    var x = gWindowUtils.dispatchDOMEventViaPresShell(destElement, event, true);

I don't think 'x' has to be defined here.
Attachment #756582 - Flags: review?(hskupin) → review-
(In reply to Henrik Skupin (:whimboo) from comment #24)
> Comment on attachment 756582 [details] [diff] [review]
> patch v2
> 
> Review of attachment 756582 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mozmill/mozmill/extension/resource/driver/controller.js
> @@ +825,5 @@
> >   */
> > +MozMillController.prototype.dragToElement = function (aSrc, aDest, aOffsetX,
> > +                                                      aOffsetY, aDragData,
> > +                                                      aDropEffect, aSourceWindow,
> > +                                                      aDestWindow) {
> 
> As mentioned last time please keep the right order of parameters. We cannot
> mix them up here. Any new parameter has to be added at the end of the list.

This is completely counter-intuitive.
We only use this method in 3 places right now.
2 of them use only the first 2 arguments.
The third also uses a custom offset (imho useless in that specific test).

Since we _are_ changing the API, doesn't it make sense to do that *now*?
Either keep this in sync with the original method: 
http://hg.mozilla.org/mozilla-central/file/feccfce43b59/testing/mochitest/tests/SimpleTest/ChromeUtils.js#l218

Or order them in another way that makes more sense for us (if we feel the need for that).
But I see no reason to mix them up in a unorganized way.

The order you are requesting here is:
> aSrc, aDest, aOffsetX, aOffsetY, aSrcWindow, aDropEffect, aDragData, aDestWindow

aDestWindow _should_ come after aSrcWindow
Flags: needinfo?(hskupin)
(In reply to Andrei Eftimie from comment #25)
> > As mentioned last time please keep the right order of parameters. We cannot
> > mix them up here. Any new parameter has to be added at the end of the list.
> 
> This is completely counter-intuitive.
> We only use this method in 3 places right now.
> 2 of them use only the first 2 arguments.
> The third also uses a custom offset (imho useless in that specific test).

So it's fine with you when we are blowing up tools and tests for others, who also make use of Mozmill for their testing? An API exists to provide stability and when we want to change something, we have to do it in a way which is kinda backward compatible. Mixing parameters is totally out of scope for that. Something we could do is to mark the current method as deprecated and introduce a new one with another (and better) name - if that works. 

It still stands, the patch can't go in as it is.

> Since we _are_ changing the API, doesn't it make sense to do that *now*?

Everything we have changed is backward compatible. This change isn't.
Flags: needinfo?(hskupin)
Attached patch patch v3 (obsolete) — Splinter Review
Thanks for the clarification.
It does make sense to be backwards compatible if others are consuming this method.

Fixed various issues:
- kept the order of the arguments as requested
- added JSDOC comment
- I was wrongly applying the offset to the source element. It should have been used on the destination element. Amended that. Also using them from top/left instead of center
Attachment #756582 - Attachment is obsolete: true
Attachment #762059 - Flags: review?(hskupin)
Comment on attachment 762059 [details] [diff] [review]
patch v3

Review of attachment 762059 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozmill/mozmill/extension/resource/driver/controller.js
@@ +815,5 @@
> + /**
> +  * Drag an element to the specified offset on another element, firing mouse and
> +  * drag events. Adapted from ChromeUtils.js synthesizeDrop()
> +  *
> +  * @param  {Element} aSrc Source element to be dragged

The type here is MozElement. Also lets move the description of the parameter to the next line so that it starts with the { char.

@@ +824,5 @@
> +  * @param  {String} [aDropEffect="move"] Effect used for the drop event
> +  *                              Possible values: copy, move, link, none
> +  * @param  {Array} [aDragData] An array holding custom drag data to be used during
> +  *                           the drag event. Format:
> +  *                           [{ type: "text/plain", "Text to drag"}, ...]

The indentation needs to be fixed here.

@@ +826,5 @@
> +  * @param  {Array} [aDragData] An array holding custom drag data to be used during
> +  *                           the drag event. Format:
> +  *                           [{ type: "text/plain", "Text to drag"}, ...]
> +  *
> +  * @param  {DOMWindow} [aDestWindow] Destination window where the drag ends

Why an empty line above? It should be before the @returns line.

@@ +838,5 @@
> +  var destElement = aDest.getNode();
> +  var offsetX = aOffsetX || 10;
> +  var offsetY = aOffsetY || 10;
> +  var srcWindow = aSourceWindow || srcElement.ownerDocument.defaultView;
> +  var destWindow = aDestWindow || destElement.ownerDocument.defaultView;

Meanwhile I really don't see the benefit in specifying the destination window. As shown here we should retrieve it from the aDest element. Same for the source Window. Why do we need separately specified windows? We can only use the window the element is contained in.

@@ +841,5 @@
> +  var srcWindow = aSourceWindow || srcElement.ownerDocument.defaultView;
> +  var destWindow = aDestWindow || destElement.ownerDocument.defaultView;
> +
> +  var windowUtils = destWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                .getInterface(Ci.nsIDOMWindowUtils);

Still an indentation issue.

@@ +873,5 @@
> +    srcWindow.addEventListener("dragstart", trapDrag, true);
> +    EventUtils.synthesizeMouseAtCenter(srcElement, { type: "mousedown" }, srcWindow);
> +    var rect = srcElement.getBoundingClientRect();
> +    var x = rect.width / 2;
> +    var y = rect.height / 2;

I wonder if we should do the same for the destination element. As used below with 10px by default we will fail if the element is only e.g. 8px wide.
Attachment #762059 - Flags: review?(hskupin) → review-
Attached patch patch v4 (obsolete) — Splinter Review
(In reply to Henrik Skupin (:whimboo) from comment #28)
> Comment on attachment 762059 [details] [diff] [review]
> patch v3
> 
> Review of attachment 762059 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +838,5 @@
> > +  var destElement = aDest.getNode();
> > +  var offsetX = aOffsetX || 10;
> > +  var offsetY = aOffsetY || 10;
> > +  var srcWindow = aSourceWindow || srcElement.ownerDocument.defaultView;
> > +  var destWindow = aDestWindow || destElement.ownerDocument.defaultView;
> 
> Meanwhile I really don't see the benefit in specifying the destination
> window. As shown here we should retrieve it from the aDest element. Same for
> the source Window. Why do we need separately specified windows? We can only
> use the window the element is contained in.

I think you are right.
In the original method you needed to specify it if the Destination Element would be in a different window:
http://hg.mozilla.org/mozilla-central/file/feccfce43b59/testing/mochitest/tests/SimpleTest/ChromeUtils.js#l218
We can just infer the window from the element.

So we can safely remove that.
We would also be able to remove the srcWindow... but lets keep that for backwards-compatibility.

> @@ +873,5 @@
> > +    srcWindow.addEventListener("dragstart", trapDrag, true);
> > +    EventUtils.synthesizeMouseAtCenter(srcElement, { type: "mousedown" }, srcWindow);
> > +    var rect = srcElement.getBoundingClientRect();
> > +    var x = rect.width / 2;
> > +    var y = rect.height / 2;
> 
> I wonder if we should do the same for the destination element. As used below
> with 10px by default we will fail if the element is only e.g. 8px wide.

I hope I understood this.
Changed the default offset values to be in the center of the destElement.
I agree, looks better this way.

And fixed nits.
Attachment #762059 - Attachment is obsolete: true
Attachment #763622 - Flags: review?(hskupin)
Comment on attachment 763622 [details] [diff] [review]
patch v4

Review of attachment 763622 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozmill/mozmill/extension/resource/driver/controller.js
@@ +829,5 @@
> +*        Source element to be dragged
> +* @param {MozElement} aDest
> +*        Destination element over which the drop occurs
> +* @param {Number} [aOffsetX]
> +*        Relative x offset for dropping on the aDest element

When it is optional please mark which default value a parameter gets. Same for the next two entries.

@@ +833,5 @@
> +*        Relative x offset for dropping on the aDest element
> +* @param {Number} [aOffsetY]
> +*        Relative y offset for dropping on the aDest element
> +* @param {DOMWindow} [aSourceWindow]
> +*        Deprecated. Window is inferred from the aSrc element.

Please mark that with @deprecated

@@ +842,5 @@
> +*        An array holding custom drag data to be used during the drag event
> +*        Format: [{ type: "text/plain", "Text to drag"}, ...]
> +*
> +* @returns {String}
> +*          the captured dropEffect

Return lines are usually short and have no parameter name. So you can have everything in one line.

@@ +846,5 @@
> +*          the captured dropEffect
> +*/
> +MozMillController.prototype.dragToElement = function (aSrc, aDest, aOffsetX,
> +                                                      aOffsetY, aSourceWindow,
> +                                                      aDropEffect, aDragData) {

Hm, why do we implement all the logic here and not for MozMillElement? This method should be marked as deprecated too and should forward to the MozElement implementation.

@@ +848,5 @@
> +MozMillController.prototype.dragToElement = function (aSrc, aDest, aOffsetX,
> +                                                      aOffsetY, aSourceWindow,
> +                                                      aDropEffect, aDragData) {
> +  var srcElement = aSrc.getNode();
> +  var destElement = aDest.getNode();

Before operating on those elements the method should check if those are really existent. That's similar to other actions on the element.

@@ +850,5 @@
> +                                                      aDropEffect, aDragData) {
> +  var srcElement = aSrc.getNode();
> +  var destElement = aDest.getNode();
> +  var srcWindow = srcElement.ownerDocument.defaultView;
> +  var destWindow = destElement.ownerDocument.defaultView;

It might be that the source and/or the target are windows. So those two lines would fail. You might want to check that.

@@ +886,5 @@
> +    EventUtils.synthesizeMouseAtCenter(srcElement, { type: "mousedown" }, srcWindow);
> +    var rect = srcElement.getBoundingClientRect();
> +    var x = rect.width / 2;
> +    var y = rect.height / 2;
> +    EventUtils.synthesizeMouse(srcElement, x + 10, y + 10, { type: "mousemove" }, srcWindow);

Why do we still have to add 10px for both x and y? Is that the target position for the move? Why can't we issue a move to the final destination of the destElement?

@@ +899,5 @@
> +    windowUtils.dispatchDOMEventViaPresShell(destElement, event, true);
> +
> +    var rect = destElement.getBoundingClientRect();
> +    var offsetX = aOffsetX || rect.width / 2;
> +    var offsetY = aOffsetY || rect.height / 2;

This will not work in case offset is required to be 0.
Attachment #763622 - Flags: review?(hskupin) → review-
Attached patch patch v4 (obsolete) — Splinter Review
Moved the implementation to MozElement.
Marked the method from controller as deprecated.
Resolved nits, added more checks to ensure elements are available and valid.
We now move the source element directly over the destination element.
Attachment #763622 - Attachment is obsolete: true
Attachment #764112 - Flags: review?(hskupin)
Comment on attachment 764112 [details] [diff] [review]
patch v4

Review of attachment 764112 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +154,5 @@
> +  }
> +
> +  if (!aElement || aElement.constructor.name !== "MozMillElement") {
> +    throw new Error("Destination element needs to be a MozMillElement");
> +  }

Why this check? All of our elements are of type MozMillElement as base class.

@@ +157,5 @@
> +    throw new Error("Destination element needs to be a MozMillElement");
> +  }
> +
> +  var srcElement = this.element;
> +  var destElement = aElement.getNode();

To lessen the confusion I would call both Node and not Element.

@@ +160,5 @@
> +  var srcElement = this.element;
> +  var destElement = aElement.getNode();
> +  var destRect = destElement.getBoundingClientRect();
> +  var destCoords = {x: (!isNaN(aOffsetX)) ? aOffsetX : destRect.width / 2,
> +                    y: (!isNaN(aOffsetY)) ? aOffsetY : destRect.height / 2}

Please remove the exclamation mark in the check and flip the cases for better readability.

@@ +165,5 @@
> +
> +  var srcWindow = srcElement.ownerDocument ? srcElement.ownerDocument.defaultView
> +                                           : srcElement;
> +  var destWindow = destElement.ownerDocument ? destElement.ownerDocument.defaultView
> +                                             : destElement;

I would move this up to the declaration of the nodes, so that it is located side by side.

@@ +171,5 @@
> +  var windowUtils = destWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                              .getInterface(Ci.nsIDOMWindowUtils);
> +  var ds = Cc["@mozilla.org/widget/dragservice;1"].getService(Ci.nsIDragService);
> +
> +  var dataTransfer;

nit: any reason for this extra empty row above?

@@ +199,5 @@
> +  try {
> +    srcWindow.addEventListener("dragstart", trapDrag, true);
> +    EventUtils.synthesizeMouseAtCenter(srcElement, { type: "mousedown" }, srcWindow);
> +    EventUtils.synthesizeMouse(destElement, destCoords.x, destCoords.y, { type: "mousemove" }, destWindow);
> +    var event = destWindow.document.createEvent("DragEvents");

Here I would add a blank row above to separate the blocks. Can we already define all the event data as first item under this try and only call dispatchDOMEventViaPresShell in the middle of the synthesizeMouse actions? I feel that would be way cleaner.

::: mutt/mutt/tests/js/controller/dnd_chrome.js
@@ +17,2 @@
>  
> +  bar.dragToElement(box);

Nice one! That looks fantastic.

Just one question. How would this work for a textfield and with a text selection? Do we drag the text correctly onto the target? Another test which makes use of form.html would be nice.

::: mutt/mutt/tests/js/controller/dnd_content_chrome.js
@@ +17,4 @@
>  
>    let link = new elementslib.ID(controller.tabs.activeTab, "link");
>    controller.waitForElement(link);
> +  link.dragToElement(urlbar, 100, 20);

Can we kill the coordinates now that we drop at the middle of the target?
Attachment #764112 - Flags: review?(hskupin) → review-
Attached patch patch v5 (obsolete) — Splinter Review
Fixed what was asked in the last review and some comments: 

(In reply to Henrik Skupin (:whimboo) from comment #32)
> Comment on attachment 764112 [details] [diff] [review]
> patch v4
> 
> Review of attachment 764112 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +199,5 @@
> > +  try {
> > +    srcWindow.addEventListener("dragstart", trapDrag, true);
> > +    EventUtils.synthesizeMouseAtCenter(srcElement, { type: "mousedown" }, srcWindow);
> > +    EventUtils.synthesizeMouse(destElement, destCoords.x, destCoords.y, { type: "mousemove" }, destWindow);
> > +    var event = destWindow.document.createEvent("DragEvents");
> 
> Here I would add a blank row above to separate the blocks. Can we already
> define all the event data as first item under this try and only call
> dispatchDOMEventViaPresShell in the middle of the synthesizeMouse actions? I
> feel that would be way cleaner.

This does not work. 
We fail to correctly generate the event.
Left them in this order.

Separated the event block.

> ::: mutt/mutt/tests/js/controller/dnd_chrome.js
> @@ +17,2 @@
> >  
> > +  bar.dragToElement(box);
> 
> Nice one! That looks fantastic.
> 
> Just one question. How would this work for a textfield and with a text
> selection? Do we drag the text correctly onto the target? Another test which
> makes use of form.html would be nice.

Appears to be more intricate then initially thought.
Opened bug 885745 for this.
Dragging a selection appears to need some other mechanism then we have atm.
Attachment #764112 - Attachment is obsolete: true
Attachment #765930 - Flags: review?(hskupin)
Comment on attachment 765930 [details] [diff] [review]
patch v5

Review of attachment 765930 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine. There are just two nits which have to be solved before we can land it.

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +138,5 @@
> + * @param {Number} [aOffsetX=aElement.width/2]
> + *        Relative x offset for dropping on aElement
> + * @param {Number} [aOffsetY=aElement.height/2]
> + *        Relative y offset for dropping on aElement
> + * @param {String} [aDropEffect="move"]

You missed to update the jsdoc for the window parameters.

::: mutt/mutt/tests/js/controller/dnd_content_chrome.js
@@ +17,4 @@
>  
>    let link = new elementslib.ID(controller.tabs.activeTab, "link");
>    controller.waitForElement(link);
> +  link.dragToElement(urlbar);

This test is still disabled. If it works now, please re-enable in the manifest.
Attachment #765930 - Flags: review?(hskupin) → review-
Attached patch patch v6Splinter Review
Good catch. Reenabled dnd_content_chrome.js it passes on all platforms.
Also added JSDOC comments for the window params.
Attachment #765930 - Attachment is obsolete: true
Attachment #767087 - Flags: review?(hskupin)
OS: Windows XP → All
Hardware: x86 → All
Attachment #767087 - Flags: review?(hskupin) → review+
Pushed to master:
https://github.com/mozilla/mozmill/commit/7273f9003e510c669417ddfed974f0b0aa45c4fd
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [sprint2013-34] → [mozmill-2.0][ateamtrack: p=mozmill q=2013q2 m=4][sprint2013-34]
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: