If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Improve documentation for EventUtils.synthesizeMouse()

NEW
Unassigned

Status

Testing
Mochitest
7 years ago
a year ago

People

(Reporter: msucan, Unassigned)

Tracking

({dev-doc-needed})

Trunk
x86_64
Linux
dev-doc-needed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
Created attachment 479409 [details] [diff] [review]
mochitest

Mochitest showing the problem.
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
(Reporter)

Comment 3

7 years ago
(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.
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?
(Reporter)

Comment 5

7 years ago
(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.
(Reporter)

Comment 6

7 years ago
Created attachment 479423 [details] [diff] [review]
proposed patch

Proposed fix and test code.
Assignee: nobody → mihai.sucan
Attachment #479409 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #479423 - Flags: review?(rcampbell)
-    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.
(Reporter)

Comment 8

7 years ago
(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!
(Reporter)

Comment 9

7 years ago
Created attachment 479448 [details] [diff] [review]
updated patch

As per discussion with Robert: updating the patch to make the if() change less confusing. Will submit this to the tryserver.
Attachment #479423 - Attachment is obsolete: true
Attachment #479448 - Flags: review?(rcampbell)
Attachment #479423 - Flags: review?(rcampbell)
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.
(Reporter)

Comment 11

7 years ago
The tryserver results look good. Results:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mihai.sucan@gmail.com-119714b5c5d2/
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/mihai.sucan@gmail.com-119714b5c5d2/
Attachment #479448 - Flags: review?(rcampbell) → review+
Keywords: checkin-needed
Whiteboard: [land-post-fx4b7]
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.
Attachment #479448 - Flags: feedback-
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 on attachment 479448 [details] [diff] [review]
updated patch

recanting r+ until we figure out how we want to proceed with this.
Attachment #479448 - Flags: review+ → review-
Keywords: checkin-needed → dev-doc-needed
Whiteboard: [land-post-fx4b7] → [back to discussion]

Comment 16

7 years ago
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.
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.
Summary: EventUtils.synthesizeMouse() throws for type: click → Improve documentation for EventUtils.synthesizeMouse()
Whiteboard: [back to discussion]
(Reporter)

Updated

6 years ago
Assignee: mihai.sucan → nobody
(Reporter)

Updated

6 years ago
Status: ASSIGNED → NEW
There is currently no documentation at all for EventUtils.synthesizeMouse() or any of the other methods of EventUtils.
You need to log in before you can comment on or make changes to this bug.