600559 - Improve documentation for EventUtils.synthesizeMouse()

Status: NEW

(Testing :: Mochitest, defect)

Description

[Mihai Sucan [:msucan]]

STR: write a test that tries to synthesize a click mouse event to an element in a web page that you open in a tab.

Expected result: the click event is synthesized.

Actual result: the synthesizeMouse() method throws an exception.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: mochitest

Mochitest showing the problem.

Comment 2

[(not currently active) Ted Mielczarek]

I don't know that synthesizeMouse handles click events. You probably want to use sendMouseEvent:
http://mxr.mozilla.org/mozilla-central/search?string=sendMouseEvent
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#19

Comment 3

[Mihai Sucan [:msucan]]

(In reply to comment #2)
> I don't know that synthesizeMouse handles click events. You probably want to
> use sendMouseEvent:
> http://mxr.mozilla.org/mozilla-central/search?string=sendMouseEvent
> http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#19

Thanks Ted!

I bumped into this synthesizeMouse() inconvenience that I just got the chance to report ... a month ago or so, when I started hacking on mochitests and when I was requested by a reviewer to send the click event with synthesizeMouse(). My background is as a web dev and as such I used type:click.

Only today I learned from the IDL of nsIDOMWindowUtils that its own sendMouseEvent() method (used by EventUtils.synthesizeMouse) requires click events to be sent as mousedown and mouseup.

What I'd suggest is making a minor change to make it convenient for new comers to mochitests: add an if to EventUtils.synthesizeMouse() to check if type == click. In that case, it should send mousedown and mouseup. These trigger the click event, as desired in the mochitest I provided.

Not sure if you guys agree with such a suggestion. If you want, I can provide a patch.

Comment 4

[(not currently active) Ted Mielczarek]

It looks like the default behavior of synthesizeMouse is to send mousedown/mouseup, so maybe you could modify it to just silently handle type:click as it does the default?

Comment 5

[Mihai Sucan [:msucan]]

(In reply to comment #4)
> It looks like the default behavior of synthesizeMouse is to send
> mousedown/mouseup, so maybe you could modify it to just silently handle
> type:click as it does the default?

Yup, that would be nice. I'll submit a patch that does this.

Comment 6

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed fix and test code.

Comment 7

[Martijn Wargers (dead)]

-    if (aEvent.type) {
-      utils.sendMouseEvent(aEvent.type, left, top, button, clickCount, modifiers);
+    if (!aEvent.type || aEvent.type == "click") {
+      utils.sendMouseEvent("mousedown", left, top, button, clickCount, modifiers);
+      utils.sendMouseEvent("mouseup", left, top, button, clickCount, modifiers);
     }
     else {
-      utils.sendMouseEvent("mousedown", left, top, button, clickCount, modifiers);
-      utils.sendMouseEvent("mouseup", left, top, button, clickCount, modifiers);
+      utils.sendMouseEvent(aEvent.type, left, top, button, clickCount, modifiers);
     }
   }

Why is this ok? You're changing the function. When no event.type is passed to this function, you're calling:
utils.sendMouseEvent(aEvent.type, left, top, button, clickCount, modifiers);
with the patch, which will throw.
I don't think this patch is good.
You probably want to only change the 
if (aEvent.type) {
line into:
if (aEvent.type && aEvent.type != 'click') {
or something like that.

Make sure to test the 2 scenarios in the test code:
- Without aEvent.type
- With aEvent.type='click'
With aEvent.type ='some random type' should still throw.

Also make sure that existing mochitests will still pass with the patch applied. You could use the tryserver for that, I guess.

Comment 8

[Mihai Sucan [:msucan]]

(In reply to comment #7)
> -    if (aEvent.type) {
> -      utils.sendMouseEvent(aEvent.type, left, top, button, clickCount,
> modifiers);
> +    if (!aEvent.type || aEvent.type == "click") {
> +      utils.sendMouseEvent("mousedown", left, top, button, clickCount,
> modifiers);
> +      utils.sendMouseEvent("mouseup", left, top, button, clickCount,
> modifiers);
>      }
>      else {
> -      utils.sendMouseEvent("mousedown", left, top, button, clickCount,
> modifiers);
> -      utils.sendMouseEvent("mouseup", left, top, button, clickCount,
> modifiers);
> +      utils.sendMouseEvent(aEvent.type, left, top, button, clickCount,
> modifiers);
>      }
>    }
> 
> Why is this ok? You're changing the function. When no event.type is passed to
> this function, you're calling:
> utils.sendMouseEvent(aEvent.type, left, top, button, clickCount, modifiers);
> with the patch, which will throw.
> I don't think this patch is good.
> You probably want to only change the 
> if (aEvent.type) {
> line into:
> if (aEvent.type && aEvent.type != 'click') {
> or something like that.

I believe that what you suggest is equivalent to what the code does in the proposed patch.

> Also make sure that existing mochitests will still pass with the patch applied.
> You could use the tryserver for that, I guess.

True, we'll have to run this through a trybuild.

Thanks for your comment!

Comment 9

[Mihai Sucan [:msucan]]

Attachment: updated patch

As per discussion with Robert: updating the patch to make the if() change less confusing. Will submit this to the tryserver.

Comment 10

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 479448 [details] [diff] [review]
updated patch

According to sendMouseEvent, a click event here is only one half of an actual click. mousedown + mouseup (or mousedown | up % 2) will produce a click.

I didn't see any senders of synthesizeMouseEvent with eventType = click in mxr.

I believe this should work, but would like to see a complete run through try first.

Comment 11

[Mihai Sucan [:msucan]]

The tryserver results look good. Results:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-119714b5c5d2/

Comment 12

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 479448 [details] [diff] [review]
updated patch

successful build through try. Results were here:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-119714b5c5d2/

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 479448 [details] [diff] [review]
updated patch

I don't like this change. synthesizeMouse intentionally triggers widget-level events (mousedown/mouseup), and adding support for specifying higher-level events (like "click") is just confusing. We should instead fix the documentation to make it clear what the difference is between synthesizeMouse/sendMouseEvent and the event levels.

Comment 14

[Rob Campbell [:rc] (:robcee)]

I definitely agree that we should add some more documentation around this to clear up any potential confusion. If we sent a "click" event, I'd expect a click event or its equivalent composite events to be sent. As it is currently, it's confusing.

Maybe a better approach would be a separate method called "synthesizeClick" that sent the composite mousedown+mouseup events in sequence. That way, we don't further clutter up synthesizeMouse with extra logic and have an explicit place to attach our "this is not a system click" documentation that will be helpful for people who expect their clicks to be more webby.

Comment 15

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 479448 [details] [diff] [review]
updated patch

recanting r+ until we figure out how we want to proceed with this.

Comment 16

[Neil Deakin]

I agree with Gavin here. People will confuse this into thinking it fires a 'click' event when it does not do that.

The default behaviour if no type is specified is to do a mousedown/mouseup. The documentation should just be improved.

Comment 17

[Rob Campbell [:rc] (:robcee)]

Thanks for the additional comments, Neil. Gavin had pretty-much already sold me on it, but it's good to hear backup.

Sounds like a trip to MDC is in order.

Comment 18

[Martijn Wargers (dead)]

There is currently no documentation at all for EventUtils.synthesizeMouse() or any of the other methods of EventUtils.