583604 - Synthesizing events doesn't return any information if event has been successfully processed

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

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

This is a kinda bad behavior because failures we are seeing in our tests are not the ones we would have to fix. In case of the safe browsing pages it is not possible to click on elements in the first 200ms. There is no response that the event hasn't been processed instead we get a failure while waiting for the result of the synthesize event.

Since a couple of versions back EventUtils.js supports the synthesizeMouseExpectEvent and synthesizeKeyExpectEvent, which allow us to check if the synthesized event has really happened. That shouldn't be that hard to enable for Mozmill 1.4.2. Would be perfect to slip this in after the next beta or even before. I will have a patch ready hopefully soon.

Comment 1

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

Attachment: WIP v1

Work in progress. Clint, it would be great to get some feedback from you. Right now I only pass in an expectedEvent value but no expectedTarget. The latter one should probably also done once we have to operate on trees and have to check for a specific cell. What do you think? For such a case I would simply wrap the function parameter into an object like {event: "focus", target: searchbar}. Further I have removed the dependency to the events.js function, which doesn't make sense. As in other areas we are too smart here in automatically focusing elements. We should really do this manually.

Still to do:
* Update controller.type to use EventUtils directly.
* Pass in expectedTarget element (if undefined use the first parameter)

Comment 2

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

Attachment: WIP v1.1

Gna, that was a diff against the wrong branch. Here the full patch.

Comment 3

[cmtalbert]

Comment on attachment 461947 [details] [diff] [review]
WIP v1.1

In general, this looks like a good idea and a good approach. I like the way it gets rid of a bunch of repeated code.  But, it's too invasive of a change for 1.4.2 if we're really going to ship 1.4.2 in the near term.

I don't think we have enough benefit from this change to counterbalance the risk.

Comment 4

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

(In reply to comment #3)
> In general, this looks like a good idea and a good approach. I like the way it
> gets rid of a bunch of repeated code.  But, it's too invasive of a change for
> 1.4.2 if we're really going to ship 1.4.2 in the near term.

Do you mean the removal of how the focus is set, is the invasive change? That's the only thing I could think about. All other stuff is only removing duplicated code. The new feature to check the results for synthesized events is optional and no existing test will be affected by this patch.

I can revert the focus handling as it was before and we can target that part for Mozmill 2.0. In that way there is really no risk.

Comment 5

[cmtalbert]

We'll take this but only if we can get the tests passing.  Currently 3 tests fail (on linux) and the checked in test fails on linux.

Comment 6

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

Attachment: Patch v1

Ok, I have finalized the patch with the following changes:

1. I don't have that much time to completely remove the dependency of the triggerKeyEvent call and the combined check for focus. That should be really something for the next big refactoring which will be part of Mozmill 2.0.

2. I have created a new controller.mouseEvent function as a central member for mouse events. That will give us the flexibility to simulate any kind of event. Users aren't limited to our existent functions, i.e. simulating a triple-click.

3. As an addition the expectedEvent parameter has been added to all synthesize class members. It's optional and current tests, which do not use this parameter, are not affected. If it is specified we check the type and target members to forward it to EventUtils.

4. It comes with some tests. I will still have to figure out why the one case fails on Linux. But IMO I simply check for the wrong target event.

Clint, would be great if we can already start the review process, to have it land soon. An updated test will be delivered as soon as possible.

Comment 7

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

Attachment: Patch v1.1

Silly mistake. isNan() is not a function. It should be isNaN().

Comment 8

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

Attachment: Patch v1.2

This patch now completely fixes the other silly mistakes from the last version. I have also added some more tests to make sure everything works as expected.

The failure on Linux for the test I have supplied is simply that no select event is raised when hitting Ctrl+A, which is suspicious. Instead a keypress event is created.

Comment 9

[cmtalbert]

I've wrestled with this patch some more, and it is still failing more tests with the patch than without it.  I really don't think this is something we want to take for 1.4.2.  I'm going to minus it, as I think it is just too risky to take at this point.

Comment 10

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

(In reply to comment #9)
> I've wrestled with this patch some more, and it is still failing more tests
> with the patch than without it.  I really don't think this is something we want
> to take for 1.4.2.  I'm going to minus it, as I think it is just too risky to
> take at this point.

I have spent a lot of time in the last days on that patch to make sure no existing test is affected by the changes introduced with this patch. Compared to the first WIP I have removed any code, which has been changed the behavior of the API. Only the (1,1)->(2,2) mouse event click position update, which I have taken over from the Mochitests, is left. The new expected event parameter is completely optional and doesn't affect any test.

Clint, it would have been great if we could have the same way of discussion as what you had with Andrew and Jeff in the office in the last days. I feel sad about the current and quick decision without any ongoing conversation and real background to focus on. Also I haven't seen any test-run report from you for the latest patch. So I can't say anything why you are getting more failures with this patch applied. Could be an issue on your machine only. Please give me that information so we have data to work with. Thanks. 

The new possibilities introduced with this patch would be a big step forward for our Mozmill test work and also the time we can safe for investigating test failures.

Comment 11

[cmtalbert]

(In reply to comment #10)

> Clint, it would have been great if we could have the same way of discussion as
> what you had with Andrew and Jeff in the office in the last days. I feel sad
> about the current and quick decision without any ongoing conversation and real
> background to focus on. Also I haven't seen any test-run report from you for
> the latest patch. So I can't say anything why you are getting more failures
> with this patch applied. Could be an issue on your machine only. Please give me
> that information so we have data to work with. Thanks. 
There hasn't been any other discussion in the office.  It's all been in email and bugs.  We need to ship 1.4.2.  We need to ship it now.

I told you when we +'d this change that I didn't want it, and it would be held to a much higher standard than anything else because it is changing the core mozmill event generation code, which we all know is fragile.  Jeff reproduced this error, and even changing your code and working with it I still can't decipher what is going on here; I have no idea why this is failing (you're not doing anything wrong) and so I think we need to take this after 1.4.2 so that we have more time for debugging and for making this more robust code.
> 
> The new possibilities introduced with this patch would be a big step forward
> for our Mozmill test work and also the time we can safe for investigating test
> failures.
I understand that, but I also can't have it destabilizing the event generation system for all other Mozmill users in the meantime.  It will have to wait until the next dot release.

Comment 12

[cmtalbert]

Comment on attachment 462931 [details] [diff] [review]
Patch v1.2

>diff --git a/mozmill/mozmill/extension/resource/modules/controller.js b/mozmill/mozmill/extension/resource/modules/controller.js
>index 3a520ba..6f3d21f 100644
>@@ -168,28 +168,28 @@ var MozMillController = function (window) {
> 
> MozMillController.prototype.sleep = utils.sleep;
> 
>@@ -213,199 +213,134 @@ MozMillController.prototype.open = function(url)
> 
>-MozMillController.prototype.click = function(elem, left, top)
>-{
>-  var element = elem.getNode();
>-  if (!element){
>-    throw new Error("could not find element " + elem.getInfo());
>-    return false;
>-  }
>+/**
>+ * Synthesize a general mouse event on the given element
>+ */
>+MozMillController.prototype.mouseEvent = function(aTarget, aOffsetX, aOffsetY,
>+                                                  aEvent, aExpectedEvent) {
> 
>-  if (element.tagName == "menuitem") {
>-    element.click();
>-    return true;
>+  var element = aTarget.getNode();
>+  if (!element) {
>+    throw new Error(arguments.callee.name + ": could not find element " + elem.getInfo());
This will fail, there is no longer an elem in this function.  You should use aTarget ^ instead.

>+  if (aExpectedEvent && aExpectedEvent.event) {
>+    var target = aExpectedEvent.target ? aExpectedEvent.target.getNode(): element;
>+    EventUtils.synthesizeMouseExpectEvent(element, aOffsetX, aOffsetY, aEvent,
>+      target, aExpectedEvent.event, "controller.mouseEvent()",
>+      element.ownerDocument.defaultView
We need to be sure that we document the structure of the exepcted event object.  This code is making a lot of assumptions about how that object is put together. You have two methods of failure here.  If the aExpectedEvent doesn't exist, then we will default to the old way of using synthesizeMouse, which is correct.  If the aExpectedEvent object is not properly formatted (i.e. it exists but (aExepected.event) is false, then we will fall back to using the old method as well.  Is that what you want?  Or should we throw an error in that case?  I tend to think we should throw an error telling the test writer that their exepectedEvent is not properly constructed because otherwise, this will constitute a silent failure.

This comment applies to everywhere you do this in this patch.

> MozMillController.prototype.rightclick = function(){
>-  frame.log({function:'rightclick - Deprecation Warning', message:'Controller.rightclick should be renamed to Controller.rightClick'});
>+  frame.log({function:'rightclick - Deprecation Warning',
>+            message:'controller.rightclick should be renamed to Controller.rightClick'});
>   this.rightClick.apply(this, arguments);
For future reference, I would really prefer code cleanup things like this be in their own patches.  It makes it so much easier to review.  

>diff --git a/mozmill/mozmill/extension/resource/stdlib/EventUtils.js b/mozmill/mozmill/extension/resource/stdlib/EventUtils.js
>index 0329d47..6267e41 100644
>--- a/mozmill/mozmill/extension/resource/stdlib/EventUtils.js
>+++ b/mozmill/mozmill/extension/resource/stdlib/EventUtils.js
>@@ -1,15 +1,13 @@
> // Export all available functions for Mozmill
> var EXPORTED_SYMBOLS = ["sendMouseEvent", "sendChar", "sendString", "sendKey",
>-                         "__doEventDispatch", "_parseModifiers", "synthesizeMouse",
>-                         "synthesizeMouseScroll", "synthesizeKey", "_expectEvent",
>-                         "_checkExpectedEvent", "synthesizeMouseExpectEvent",
>-                         "synthesizeDragStart", "synthesizeDrop",
>-                         "disableNonTestMouseEvents", "_getDOMWindowUtils",
>-                         "synthesizeComposition", "synthesizeText",
>-                         "synthesizeQuerySelectedText", "synthesizeQueryTextContent",
>-                         "synthesizeQueryCaretRect", "synthesizeQueryTextRect",
>-                         "synthesizeQueryEditorRect", "synthesizeCharAtPoint",
>-                         "synthesizeSelectionSet"];
>+                        "synthesizeMouse", "synthesizeMouseScroll", "synthesizeKey",
>+                        "synthesizeMouseExpectEvent", "synthesizeKeyExpectEvent",
>+                        "synthesizeDragStart", "synthesizeDrop", "synthesizeText",
>+                        "disableNonTestMouseEvents", "synthesizeComposition", 
>+                        "synthesizeQuerySelectedText", "synthesizeQueryTextContent",
>+                        "synthesizeQueryCaretRect", "synthesizeQueryTextRect",
>+                        "synthesizeQueryEditorRect", "synthesizeCharAtPoint",
>+                        "synthesizeSelectionSet"];
Why are you removing exports from this code? Is this just another cleanup step?

So, I ran this again on master, and it passes more tests than it failed.  That's a little disconcerting as it had the opposite behavior when run against the 1.5 branch and I don't think the patch has changed at all.  I have a feeling that this is going to have a huge fall out in terms of follow on bugs, so I encourage you to test this thoroughly before you check it in.  If it has a huge fallout in terms of follow on bugs, we'll back this out from 1.5 and just take it on master.

r=ctalbert with the changes and questions answered from above (particuarly about how to handle improperly constructed exepectedEvent objects.)

Comment 13

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

(In reply to comment #12)
> >+MozMillController.prototype.mouseEvent = function(aTarget, aOffsetX, aOffsetY,
> >+                                                  aEvent, aExpectedEvent) {
> > 
> >-  if (element.tagName == "menuitem") {
> >-    element.click();
> >-    return true;
> >+  var element = aTarget.getNode();
> >+  if (!element) {
> >+    throw new Error(arguments.callee.name + ": could not find element " + elem.getInfo());
> This will fail, there is no longer an elem in this function.  You should use
> aTarget ^ instead.

Good catch. Thanks.

> We need to be sure that we document the structure of the exepcted event object.

Will be done. Even in the controller.js file itself.

>  This code is making a lot of assumptions about how that object is put
> together. You have two methods of failure here.  If the aExpectedEvent doesn't
> exist, then we will default to the old way of using synthesizeMouse, which is
> correct.  If the aExpectedEvent object is not properly formatted (i.e. it
> exists but (aExepected.event) is false, then we will fall back to using the old
> method as well.  Is that what you want?  Or should we throw an error in that
> case?  I tend to think we should throw an error telling the test writer that
> their exepectedEvent is not properly constructed because otherwise, this will
> constitute a silent failure.

Yeah, that would make sense. When the object has been specified we should throw an error if the type does not exist. The target will still be optional and use the current element as default.

> > MozMillController.prototype.rightclick = function(){
> >-  frame.log({function:'rightclick - Deprecation Warning', message:'Controller.rightclick should be renamed to Controller.rightClick'});
> >+  frame.log({function:'rightclick - Deprecation Warning',
> >+            message:'controller.rightclick should be renamed to Controller.rightClick'});
> >   this.rightClick.apply(this, arguments);
> For future reference, I would really prefer code cleanup things like this be in
> their own patches.  It makes it so much easier to review.  

That was by accident. I will remove that part. We should even completely get rid of this function.

> >-                         "__doEventDispatch", "_parseModifiers", "synthesizeMouse",
> >-                         "synthesizeMouseScroll", "synthesizeKey", "_expectEvent",
> >-                         "_checkExpectedEvent", "synthesizeMouseExpectEvent",
>
> Why are you removing exports from this code? Is this just another cleanup step?

The only exports which get removed are the ones with a _ prefix. Those shouldn't be exported at all. 

I will upload a new patch soonish.

Comment 14

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

Attachment: Patch v2

Patch with review comments addressed.

Comment 15

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

Comment on attachment 481253 [details] [diff] [review]
Patch v2

Sorry, missed to update the test. :) New version upcoming once it works.

Comment 16

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

Attachment: Patch v2.1

Updated patch with working tests and fixed problems catched by the tests. Yay!

Comment 17

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

Attachment: Patch v2.2

Not sure why the test itself got lost from the patch queue. Now it is attached.

Comment 18

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

Comment on attachment 481474 [details] [diff] [review]
Patch v2.2

>-  if (isNaN(left)) { left = 1; }
>-  if (isNaN(top)) { top = 1; }
>+  // If no offset is given we have we will use (2,2) as position, because of
>+  // rounded borders (identical to Mochitests)
>+  if (isNaN(aOffsetX))
>+    aOffsetX = 2;
>+  if (isNaN(aOffsetY))
>+    aOffsetY = 2;

Given the posting from dbaron on the dev-apps-firefox list we should probably change it to use (6,6) for now or even better calculate the center of the element. Clint, what do you think?

Comment 19

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

Attachment: Patch v2.3

As talked with Clint on IRC we also want to synthesize a click at the center of the element when no offset is given. This patch adds it.

Comment 20

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

Landed on hotfix-1.5.1 branch:
http://github.com/mozautomation/mozmill/commit/12bf6079104e37df1c7858d83c6df6a75efb6560

Comment 21

[Aaron Train [:aaronmt]]

Verified  fixed with 1.5.1rc1

Comment 22

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

Pushed to master:
https://github.com/mozautomation/mozmill/commit/746c992ecbba943a0ca3946cd29a5472850a6194