898378 - Mutt test failure "keypress() with pressed shift key succeeded. - '' should equal 'F'" in "mutt/mutt/tests/js/controller/synthesize_events.js"

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect, P1)

Description

[Andrei Eftimie]

Failure seen on Nightly from 2013-07-26
Build from 2013-07-25 works fine

Getting an accurate regression range now.
Nightly pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=76fb8f815ac7

Comment 1

[Andrei Eftimie]

Accurate pushlog from inbound builds:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=949a5e125a8d

See bug 501496

I think me might need to update our library code

Comment 2

[Andrei Eftimie]

We're actually not closing a context menu in textExpectedEvents.js
https://github.com/mozilla/mozmill/blob/master/mutt/mutt/tests/js/testController/testExpectedEvents.js#L50

Comment 3

[Andrei Eftimie]

*Sigh*, updating EventUtils does not solve the issue.
The context menu referenced above is not getting closed by hitting ESC.
It seems to be stuck: I've put up a sleep, and I can't even manually close is by pressing ESC.

We can hack this error to go away by issuing a restart between the tests, but that would only avoid the real problem.

Comment 4

[Andrei Eftimie]

Attachment: testcase.js

Simplified testcase

Comment 5

[Andrei Eftimie]

Attachment: testcase.js

Masayuki since bug 501496 has landed we're getting a failure in our automated tests.

Attached is a testcase.
You can run the file via mozmill:
> pip install mozmill
> mozmill --v // mozmill 2.0rc4
> mozmill -t testcase.js -b %path/to/firefox/binary%

We are failing to close a contextmenu by pressing VK_ESCAPE.
I am not sure if this is a regression, or as you say in bug 501496 comment 85 that we might have a hacky eventhandler that might cause problems.

Mozmill's code is on github: https://github.com/mozilla/mozmill
The keypress event gets handled by EventUtils.js here: https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/stdlib/EventUtils.js#L255

Could you please look into this. Thanks

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Does the testcase causes keydown event of escape key? nsXULPopupManager handles keydown event for non-printable key operation like pressing escape key.

Comment 7

[Andrei Eftimie]

We are issuing a keypress event through nsIDOMWindowUtils
Just to make sure, should we switch to issuing all/most keydown/press/up events through nsXULPopupManager?
Or are only some specific type of events that should go through nsXULPopupManager?

Thanks for the fast response!

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

>   controller.keypress(null, "VK_ESCAPE", {type: "keypress"});

If |{type: "keypress"}| this is an argument of EventUtils.synthesizeKey(), it causes *only* keypress event. If so, there is no keydown event, which causes nsXULPopupManager failing to close any popup. Any testcases should dispatch a set of keydown, keypress (if necessary), keyup because one of the key event handlers of them might cause a bug even if you want to test one of the key events.

http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsXULPopupManager.cpp?rev=d26846484b7c#2201
http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsXULPopupManager.cpp?rev=d26846484b7c#1964

Comment 9

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

Hm, that sounds like a blocker then for Mozmill 2.0. Thanks for the information Masayuki!

Andrei, can you please focus on this bug the next days? Also it would be good to know if that is something we have to fix for Mozmill 1.5.

Comment 10

[Andrei Eftimie]

Attachment: 0001-Bug-898378-Triggered-a-keydown-event-before-a-keypre.patch

As per Masayuki's explanation, I've fixed our mutt test by issuing a keydown event when trying to close the context menu.

I haven't found any problems in 1.5 (we haven't seen anything in our daily runs)
If we do, we'll have to make sure our tests also issue a keydown before a keypress.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I believe that for testing key operation, a set of keydown/keypress(only when keydown isn't consumed)/keyup should be fired because combination of each key event handlers may cause unexpected behavior. E.g., two or more actions occur for one key operation.

Comment 12

[Andrei Eftimie]

We're failing here not when testing a specific event, but when dismissing a context menu after a test. I've initially triggered both a keydown then a keypress, but it seems to work just as well with only a keydown.

I guess we could leave it with both events fired for increased stability.

Comment 13

[Andrei Eftimie]

Attachment: 0001-Bug-898378-Triggered-a-keydown-event-before-a-keypre.patch

Updated to trigger both keydown and keypress for increased stability.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 790171 [details] [diff] [review]
0001-Bug-898378-Triggered-a-keydown-event-before-a-keypre.patch

> +  controller.keypress(lname, "VK_ESCAPE", {type: "keydown"});
>    controller.keypress(lname, "VK_ESCAPE", {type: "keypress"});

If keydown is consumed by somebody, the keypress event shouldn't be fired since it's the new correct behavior.

Comment 15

[Andrei Eftimie]

Comment on attachment 790158 [details] [diff] [review]
0001-Bug-898378-Triggered-a-keydown-event-before-a-keypre.patch

Uhm, reverted to initial patch where we only trigger the keydown.

Comment 16

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

Wow, that sounds complicated and I hope at some time we will get native events for Mozmill. At latest when we move to Marionette. Looks like there is no easy way to generalize the keypress actions. But good to know how that works now. Thanks Masayuki.

Comment 17

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

Comment on attachment 790158 [details] [diff] [review]
0001-Bug-898378-Triggered-a-keydown-event-before-a-keypre.patch

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

Andrei, for the future please keep by our patch version names and don't take the default as what Bugzilla proposes. That makes it hard to find the right revisions of the patch for diff.

::: mutt/mutt/tests/js/testController/testExpectedEvents.js
@@ +47,4 @@
>    expect.throws(function () {
>      controller.rightClick(lname, 2, 2, {type: "click"});
>    }, "Error", "Opening a context menu shouldn't raise a click event.");
> +  controller.keypress(lname, "VK_ESCAPE", {type: "keydown"});

Please update the test so we do not use controller.keypress but %elem%.keypress. It might be helpful to spun off a new bug for all the other tests which make use of controller.* keyboard and mouse actions.

Also not sure what happened here and why the modifier has been added to this function call. Removing it should also fix our problem.

@@ +51,4 @@
>  
>    expect.doesNotThrow(function () {
>      controller.rightClick(lname, 2, 2, {type: "contextmenu"});
> +  }, "Error", "Opening a context menu does faire a contextmenu event.");

Something is broken with that message. Can you please fix the 'faire' part?

Comment 18

[Andrei Eftimie]

Opened bug 905939 for updating all controller.* deprecated methods.

Comment 19

[Andrei Eftimie]

Attachment: patch v3

Removed the modifier altogether. Good idea!
We just want to dismiss the context menu, we don't really care in these 2 cases about the specific event.

Updated code from deprecated controller.method to elem.method
All instances expect elem.type which we seem to miss, opened bug 905959 to fix that.

And fixed message typo.

Running a complete mutt testjs run, if we pass all tests, we get 0 in the final reporting, if we have at least 1 failure we got correct readings. Tested and it is not related to this bug, will investigate and file one appropriately.

Comment 20

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

Comment on attachment 791213 [details] [diff] [review]
patch v3

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

Landed on master:
https://github.com/mozilla/mozmill/commit/fe9ba4b43c7ffdbf455bc39fa6d2911fabdd147f