Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me) Reporter
	Description • 7 years ago

+++ This bug was initially created as a clone of Bug #1370403 +++

When we perform a two clicks in quick succession (640ms?) via performActions in Marionette, we should see a dblclick MouseEvent but we don't.

See https://github.com/mozilla/geckodriver/issues/661

The expected sequence of events is mousedown,mouseup,click,mousedown,mouseup,click,dblclick

	muthuraj90ec Assignee
	Comment 1 • 7 years ago

(In reply to Maja Frydrychowicz (:maja_zf) from comment #0)
> +++ This bug was initially created as a clone of Bug #1370403 +++
> 
> When we perform a two clicks in quick succession (640ms?) via performActions
> in Marionette, we should see a dblclick MouseEvent but we don't.
> 
> See https://github.com/mozilla/geckodriver/issues/661
> 
> The expected sequence of events is
> mousedown,mouseup,click,mousedown,mouseup,click,dblclick

Hi I would like to work on this, could if please assign this to me, if the owner is not decided yet.

Thanks

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me) Reporter
	Comment 2 • 7 years ago

Sounds good. We'll assign it to you when you submit your first patch for it.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me) Reporter
	Comment 3 • 7 years ago

Regarding event.synthesizeMouseAtPoint, you may want to consider passing the `clickCount` option with mousedown/mouseup rather then synthesizing a dblclick event directly, for example:

```
event.synthesizeMouseAtPoint(
            inputState.x,
            inputState.y,
            Object.assign({}, mousedownEvent, {clickCount: 2}),
            win);
```

clickCount is 1 by default.

I've found that the following sequence of calls gets us the events we want as described in Comment 0:
mousedown, mouseup, mousedown with clickCount 2, mouseup with countCount 2.

I think the approach here will be to set some global `firstClick` flag to indicate that one click has already happened (listen for a click event); that flag should flip after 640ms (use a ONE_SHOT nsITimer, perhaps). When we're about to perform a mousedown or mouseup, if a click has recently happened then we should set clickCount to 2 when synthesizing those events.

One thing to keep in mind is that when a user does something like "mousedown,mouseup,mousedown,pause 10 seconds,mouseup", the browser still gets a click and therefore a dblclick as long as the properties of the second mouseup as the same as the second mousedown (coordinates, ctrl, shift, etc). So when the actions code see a second mousedown while firstClick is true, the timer should be cancelled so the firstClick flag doesn't expire until we produce a MouseEvent with different properties. 

Andreas, setting n-i on you in case you have anything to add here -- feel free to clear it, otherwise.

	muthuraj90ec Assignee
	Comment 4 • 7 years ago

I have a question regarding the clickCount property in mouseevent. I manually generated double click on the test_actions_wdspec.html page, logging the event to console (please find screen shot attached). I did not see the property clickCount in the generated event, but i did notice a property called detail which changed from 1 to 2 in the second mousedown, meaning the order of generated events was:
{type:mousedown, detail:1}
{type:mouseup, detail:1}
{type:click, detail:1}
{type:mousedown, detail:2}
{type:mouseup, detail:2}
{type:dblclick, detail:2}

Am i missing something?

Thanks

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me) Reporter
	Comment 5 • 7 years ago

clickCount is just an option that is used by the `opts` object passed to `synthesizeMouseAtPoint`. That object doesn't correspond exactly to a DOM MouseEvent.

	Andreas Tolfsen ❲:ato❳
	Comment 6 • 7 years ago

(In reply to Maja Frydrychowicz (:maja_zf) from comment #3)

> I've found that the following sequence of calls gets us the events
> we want as described in Comment 0: mousedown, mouseup, mousedown
> with clickCount 2, mouseup with countCount 2.

Excellent find!

> I think the approach here will be to set some global `firstClick`
> flag to indicate that one click has already happened (listen for
> a click event); that flag should flip after 640ms (use a ONE_SHOT
> nsITimer, perhaps). When we're about to perform a mousedown or
> mouseup, if a click has recently happened then we should set
> clickCount to 2 when synthesizing those events.

I think this approach would work.

	muthuraj90ec Assignee
	Comment 7 • 7 years ago

Few Clarifying questions before proceeding,

What is the best way to listen for the "click" mouseevent? I have seen win.addEventListener("someevent", dosomethingcallback()) in listener.js, but wanted to know the right way to listen for the mouse "click" to set event.DoubleClickTracker.setMouseClick();

Pending work : use ONE_SHOT timer to flip the event.firstClick to false..

	muthuraj90ec Assignee
	Comment 8 • 7 years ago

pasting previous comment here (+ more tests):

dblclickevent.patch

Few Clarifying questions before proceeding,

What is the best way to listen for the "click" mouseevent? I have seen win.addEventListener("someevent", dosomethingcallback()) in listener.js, but wanted to know the right way to listen for the mouse "click" to set event.DoubleClickTracker.setMouseClick();

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me) Reporter
	Comment 9 • 7 years ago

Comment on attachment 8903875 [details] [diff] [review]
dbclickevent.patch

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

This is on the right track.

* Re win.addEvenListener -- yes, that's how I would do it.
* Tests look good, overall, but I've made some small suggestions.

::: testing/marionette/event.js
@@ +17,4 @@
>  const {ElementNotInteractableError} =
>      Cu.import("chrome://marionette/content/error.js", {});
>  
> +const timer = Components.classes["@mozilla.org/timer;1"]

name this dblclickTimer, or something similar

@@ +77,5 @@
> +    event.DoubleClickTracker.resetMouseClick();
> +  },
> +};
> +
> +event.DoubleClickTracker = {

Consider defining this as a class. See action.InputState as an example.

@@ +90,5 @@
> +  resetMouseClick: () => {
> +    event.DoubleClickTracker.firstClick = false;
> +  },
> +  startTimer: () => {
> +    timer.initWithCallback(flipFirstMouseClick,

If you pass `resetMouseClick` as the first argument, that will also work. (In other words, you don't need to define flipFirstMouseClick at all.)

::: testing/web-platform/tests/webdriver/tests/actions/mouse.py
@@ +50,4 @@
>      assert expected == filtered_events[1:]
>  
>  
> +def test_dblclick_at_coordinates(session, test_actions_page, mouse_chain):

All the tests ending with "_at_coordinates" should verify the coordinates in the events: perhaps generalize [1] to reuse it in all these tests.

[1] https://dxr.mozilla.org/mozilla-central/rev/632e42dca494ec3d90b70325d9c359f80cb3f38a/testing/web-platform/tests/webdriver/tests/actions/mouse.py#36-40

@@ +74,5 @@
> +    filtered_events = [filter_dict(e, expected[0]) for e in events]
> +    assert expected == filtered_events[1:]
> +
> +
> +def test_dblclick_with_delay_at_coordinates(session, test_actions_page, mouse_chain):

This test doesn't have to be "_at_coordinates". You can just move the point to the "outer" element center instead of checking coordinates. That way the test is focused on the delay. (Same comment applied to the "no doubleclick" test below.

I'd also rename the test to something like "test_dblclick_with_pause_after_second_click"

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me) Reporter
	Comment 10 • 7 years ago

You might want to request Level 1 Commit access so you can push your changes to the try server yourself. I can vouch for you. The instructions are here: https://www.mozilla.org/en-US/about/governance/policies/commit/

	muthuraj90ec Assignee
	Comment 11 • 7 years ago

(In reply to Maja Frydrychowicz (:maja_zf) from comment #10)
> You might want to request Level 1 Commit access so you can push your changes
> to the try server yourself. I can vouch for you. The instructions are here:
> https://www.mozilla.org/en-US/about/governance/policies/commit/

Sure, One more thing, What is the best way to debug through (breakpoints) marionette codebase, when running the mouse.py tests, Are there any useful commands/best practices that I could use ?
Thanks

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me) Reporter
	Comment 12 • 7 years ago

The tools for debugging marionette during web-platform-test runs is not ideal. 

You can try the `debugger;` statement: Firefox should give you a prompt to start the tests and then break at "debugger;". https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/debugger

If that doesn't work, you can resort to printing: the `dump()` statement should work anywhere, e.g. dump("my message");

	muthuraj90ec Assignee
	Comment 13 • 7 years ago

Please note, the new tests which were added to check the double click functionality passes only if the interval between clicks is atmost 120ms
+    dblclickTimer.initWithCallback(event.DoubleClickTracker.resetMouseClick,
+        120, Ci.nsITimer.TYPE_ONE_SHOT);

If i use 640ms the wpt tests for dblclick is failing..

Also I am registering addEventListener("click", event.DoubleClickTracker.setMouseClick, false);

in function registerSelf() in listener.js, as this function seem to get called when a window is setup initially..

Thanks

	Andreas Tolfsen ❲:ato❳
	Comment 14 • 7 years ago

Comment on attachment 8907897 [details] [diff] [review]
dblclickimpl.patch

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

Doing a drive-by feedback on this patch, and it’s looking really good.  Most of my comments are to do with style and naming.

::: testing/marionette/event.js
@@ +72,5 @@
>  };
>  
> +event.DoubleClickTracker = {
> +  firstClick: false,
> +  isMouseClicked: () => {

In object literals functions can be made using this shorthand:

> event.DoubleClickTracker = {
>   isMouseClicked() {
>     …
>   }
> };

So you can drop all the assignments of arrow functions to properties here.

@@ +79,5 @@
> +  setMouseClick: () => {
> +    event.DoubleClickTracker.firstClick = true;
> +    event.DoubleClickTracker.startTimer();
> +  },
> +  resetMouseClick: () => {

FWIW I think “Mouse” is superfluous here.  The name of the object is already DoubleClickTracker, so it’s obvious from a event.DoubleClickTracker.isMouseClicked() call that it has to do with the mouse.

Maybe this can be shortened to just setClick, resetClick, isClicked?  WDYT?

@@ +84,5 @@
> +    event.DoubleClickTracker.firstClick = false;
> +  },
> +  startTimer: () => {
> +    dblclickTimer.initWithCallback(event.DoubleClickTracker.resetMouseClick,
> +        120, Ci.nsITimer.TYPE_ONE_SHOT);

Where does the 120 ms value come from?

Can you also add this as a constant somewhere, with a somewhat descriptive name instead of using magical number values?

::: testing/marionette/listener.js
@@ +485,5 @@
>    logger.debug(`Register listener.js for window ${msg.value}`);
>  
> +  // Listen for click event to indicate one click has happened, so actions
> +  // code can send dblclick event
> +  addEventListener("click", event.DoubleClickTracker.setMouseClick, false);

Drop third argument since that behaviour is implicit.

	muthuraj90ec Assignee
	Comment 15 • 7 years ago

Comment on attachment 8907897 [details] [diff] [review]
dblclickimpl.patch

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

::: testing/marionette/event.js
@@ +79,5 @@
> +  setMouseClick: () => {
> +    event.DoubleClickTracker.firstClick = true;
> +    event.DoubleClickTracker.startTimer();
> +  },
> +  resetMouseClick: () => {

I think it makes sense, event.DoubleClickTracker.isClicked, setClick, resetClick makes more sense, will make that change thanks..

@@ +84,5 @@
> +    event.DoubleClickTracker.firstClick = false;
> +  },
> +  startTimer: () => {
> +    dblclickTimer.initWithCallback(event.DoubleClickTracker.resetMouseClick,
> +        120, Ci.nsITimer.TYPE_ONE_SHOT);

Initially, i had the value 640ms, which is if a second click is encountered within 640ms, clickCount of the mouseEvent obj must be flipped to 2, before synthesizing the events, but i noticed the wpt tests which were added to test the dblclick were failing, as it was generating dblclick at places where it shouldn't have.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me) Reporter
	Comment 16 • 7 years ago

Comment on attachment 8907897 [details] [diff] [review]
dblclickimpl.patch

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

Thanks for the update, this is great. 

Re 640ms, I think the issues with extra clicks might be due to overlapping timer intervals. Think about how the timer and firstClick behave when you have many clicks in a short interval. See my comments inline.

::: testing/marionette/action.js
@@ +1265,5 @@
>          let mouseEvent = new action.Mouse("mouseup", a.button);
>          mouseEvent.update(inputState);
> +        if (event.DoubleClickTracker.isMouseClicked()) {
> +          mouseEvent = Object.assign({},
> +              mouseEvent, {clickCount: 2});

I think you want to resetMouseClick as well after the second click completes, and cancel the timer that's been started, otherwise you might get extra dblclicks when the mouse clicks again before the timer interval is up.

::: testing/marionette/event.js
@@ +76,5 @@
> +  isMouseClicked: () => {
> +    return event.DoubleClickTracker.firstClick;
> +  },
> +  setMouseClick: () => {
> +    event.DoubleClickTracker.firstClick = true;

Here, I think you only want to start the timer if firstClick is false. Otherwise, resetClick might get called early after a third click. (Imagine 3 clicks close together, then one more a bit later but within the time limit for double-clicking.)

::: testing/marionette/listener.js
@@ +485,5 @@
>    logger.debug(`Register listener.js for window ${msg.value}`);
>  
> +  // Listen for click event to indicate one click has happened, so actions
> +  // code can send dblclick event
> +  addEventListener("click", event.DoubleClickTracker.setMouseClick, false);

Andreas also pointed out to me that we probably don't need to wait until the listener is registered to start listening for the click, so could you try putting the call to addEventListener in global scope?

	muthuraj90ec Assignee
	Comment 17 • 7 years ago

Thanks for the explanation!
For the mouse.py wpt tests, I have included a pause of 650ms between dblclick tests, so that the timer do not interfere in consecutive tests..

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me) Reporter
	Comment 18 • 7 years ago

Comment on attachment 8909564 [details] [diff] [review]
dblclickimpl.patch

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

Ah, good idea to listen for dblclick, too. Some style nits and a suggestion to improve the tests.

::: testing/marionette/event.js
@@ +20,5 @@
> +const dblclickTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +
> +//  A callback will be initialized to be fired after the set timeout in ms,
> +//  which will reset the firstClick flag in event.DoubleClickTracker
> +const firstClickResetForDblClick = 640;

Change the comment to something like "Max interval between two clicks that should result in a dblclick (in ms)" 

Change the identifier to something like DBLCLICK_INTERVAL

@@ +80,5 @@
> +  isClicked() {
> +    return event.DoubleClickTracker.firstClick;
> +  },
> +  setClick() {
> +    if (event.DoubleClickTracker.firstClick === false) {

if (!event.DoubleClickTracker.firstClick)

::: testing/web-platform/tests/webdriver/tests/actions/mouse.py
@@ +55,4 @@
>      assert expected == filtered_events[1:]
>  
>  
> +def test_dblclick_at_coordinates(session, test_actions_page, mouse_chain):

Parametrize this test to check couple of different pauses between clicks: 0, 100, 500

@@ +64,5 @@
> +    # As the interval between two clicks to generate a dblclick is
> +    # 640ms
> +    mouse_chain \
> +        .pointer_move(div_point["x"], div_point["y"]) \
> +        .pause(650) \

Rather than waiting 650ms before starting each double-click test, try using the new_session fixture (see ../conftest.py) instead of the session+test_actions_page+mouse_chain fixtures. That should make the double-click tests independent.

@@ +84,5 @@
> +    filtered_events = [filter_dict(e, expected[0]) for e in events]
> +    assert expected == filtered_events[1:]
> +
> +
> +def test_dblclick_with_pause_after_second_click(session, test_actions_page, mouse_chain):

This test should be renamed to ...pause_after_second_pointerdown

@@ +95,5 @@
> +            .pointer_move(int(center["x"]), int(center["y"])) \
> +            .pause(650) \
> +            .click() \
> +            .pointer_down() \
> +            .pause(650) \

Please set the 640ms constant globally, then write your pauses as something like `_DBLCLICK_INTERVAL + 10`.

	Andreas Tolfsen ❲:ato❳
	Comment 19 • 7 years ago

FYI you might want to run this patch against

	% ./mach lint testing/marionette

to ensure it passes the lint job in CI.

Could you also link to a try run?

	muthuraj90ec Assignee
	Comment 20 • 7 years ago

Used new_session to run dblclick test independently, please verify if the usage is right.

Also I noticed when parametrizing the test
@pytest.mark.parametrize("pauses_between_clicks", [0])
def test_dblclick_at_coordinates(dblclick_session, mouse_chain_for_dblclick, pauses_between_clicks):

if i include @pytest.mark.parametrize("pauses_between_clicks", [0, 100, 500])

I am hitting a timeout (error msg below):
0:34.00 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:54374) "1506308809841	Marionette	INFO	New connections will no longer be accepted"
 0:34.06 TEST_END: Thread-TestrunnerManager-1 TIMEOUT, expected OK


is there is global timeout setting under which all tests should complete? 

Other wise the tests pass fine, after opening the browser for the dblclick tests

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me) Reporter
	Comment 21 • 7 years ago

Comment on attachment 8911639 [details] [diff] [review]
dblclickimpl.patch

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

For timeout, see http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/testing/web-platform/tests/webdriver/tests/actions/special_keys.py#1

I think you can simplify the changes around new_session, but the overall usage looks fine. 

When you rebase onto latest central, you may find that the new_session fixture throws an error when you navigate to a url. This is fixed in https://github.com/w3c/web-platform-tests/pull/7461

Thanks!

::: testing/web-platform/tests/webdriver/tests/actions/conftest.py
@@ +14,4 @@
>          {"pointerType": "mouse"})
>  
>  
> +@pytest.fixture

Removing autouse will break a bunch of other actions tests. 

Instead, it makes sense to move the dblclick tests into their own module, and that way you should be able to override the release_actions fixture as in https://docs.pytest.org/en/latest/fixture.html#override-a-fixture-on-a-test-module-level

@@ +36,5 @@
> +
> +@pytest.fixture
> +def dblclick_session(new_session):
> +    _, session = new_session({"alwaysMatch": {"unhandledPromptBehavior": "dismiss"}})
> +    session.url = inline(

Please navigate to the test_actions_wdspec.html url rather than using inline, so you can avoid duplicating its code here. 

You're using new_session correctly, although I don't think you can pass an empty capabilities dict.

	muthuraj90ec Assignee
	Comment 22 • 7 years ago

(In reply to Andreas Tolfsen ‹:ato› from comment #19)
> FYI you might want to run this patch against
> 
> 	% ./mach lint testing/marionette
> 
> to ensure it passes the lint job in CI.
> 
> Could you also link to a try run?

Yes, I just got the Level 1 Try access, after cleaning up the wpt will push for a try run. Thanks

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me) Reporter
	Comment 23 • 7 years ago

Hey Muthuraj, we haven't heard from you in a while. I hope all is well. Do you still intend to finish up this patch?

	muthuraj90ec Assignee
	Comment 24 • 7 years ago

(In reply to Maja Frydrychowicz (:maja_zf) from comment #23)
> Hey Muthuraj, we haven't heard from you in a while. I hope all is well. Do
> you still intend to finish up this patch?

Hello Maja_zf, Due to other priorities at work i do not have enough time to finish up the wpt test for dblclick, and submit to try server. It would be helpful if you could help me complete this one. Thanks

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me) Reporter
	Comment 28 • 7 years ago
mozreview-review

Comment on attachment 8932543 [details]
Bug 1385476 - Synthesize dblclick MouseEvent when performing webdriver actions;

https://reviewboard.mozilla.org/r/203592/#review209038

Re-recording the r+ I had given to the changes submitted by muthuraj90ec

	Andreas Tolfsen ❲:ato❳
	Comment 31 • 7 years ago
mozreview-review

Comment on attachment 8932544 [details]
Bug 1385476 - Test double-click, starting a new session for each test;

https://reviewboard.mozilla.org/r/203594/#review209066

::: commit-message-3b6c3:3
(Diff revision 2)
> +This is a modification of the tests proposed by our contributor,
> +muthuraj90ec.

It’s also normal to add a "Thanks-to: Jane Doe <foo@example>" next
to MozReview-Commit-ID in this case.

::: testing/web-platform/tests/webdriver/tests/actions/modifier_click.py:18
(Diff revision 2)
> +    request.addfinalizer(mod_click_session.actions.release)
> +
> +
> +@pytest.fixture
> +def mod_click_session(new_session, url, add_browser_capabilites):
> +    _, session = new_session({"capabilities": {"alwaysMatch": add_browser_capabilites({})}})

If you’re not adding any new browser capabilities, couldn’t this
be called without arguments?

::: testing/web-platform/tests/webdriver/tests/actions/modifier_click.py:23
(Diff revision 2)
> +    _, session = new_session({"capabilities": {"alwaysMatch": add_browser_capabilites({})}})
> +    session.url = url("/webdriver/tests/actions/support/test_actions_wdspec.html")
> +
> +    return session
> +
> +@pytest.fixture

Two lines between functions, otherwise the wpt linter which doesn’t
run on m-c will complain after upstreaming.

::: testing/web-platform/tests/webdriver/tests/actions/mouse_dblclick.py:35
(Diff revision 2)
> +@pytest.mark.parametrize("click_pause", [0, 200])
> +def test_dblclick_at_coordinates(dblclick_session, mouse_chain, click_pause):

I foresee that the 200 ms delay test could become intermittent.
Make sure you trigger a few Wd jobs on Linux debug before integrating.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me) Reporter
	Comment 32 • 7 years ago
mozreview-review

Comment on attachment 8932544 [details]
Bug 1385476 - Test double-click, starting a new session for each test;

https://reviewboard.mozilla.org/r/203594/#review209416

::: testing/web-platform/tests/webdriver/tests/actions/modifier_click.py:18
(Diff revision 2)
> +    request.addfinalizer(mod_click_session.actions.release)
> +
> +
> +@pytest.fixture
> +def mod_click_session(new_session, url, add_browser_capabilites):
> +    _, session = new_session({"capabilities": {"alwaysMatch": add_browser_capabilites({})}})

As far as I can tell, they have to be added in explicitly like this. I believe wptserve doesn't work correctly, otherwise.

::: testing/web-platform/tests/webdriver/tests/actions/mouse_dblclick.py:35
(Diff revision 2)
> +@pytest.mark.parametrize("click_pause", [0, 200])
> +def test_dblclick_at_coordinates(dblclick_session, mouse_chain, click_pause):

Will do.

	Pulsebot
	Comment 35 • 7 years ago

Pushed by mjzffr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4391f6b7a338
Synthesize dblclick MouseEvent when performing webdriver actions; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/337fbc9e9bf9
Test double-click, starting a new session for each test; r=ato

	Bogdan Tara[:bogdan_tara | bogdant]
	Comment 36 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4391f6b7a338
https://hg.mozilla.org/mozilla-central/rev/337fbc9e9bf9