Bug 154133 – Refine SimulatedMouseEvent to support Event.isTrusted

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Jiewen Tan]

Add new create method/constructor to SimulatedMouseEvent to be called by bindings exclusively.

Also, separate EventDispatcher::dispatchSimulatedClick into two: one for bindings, and the other for user agent.

[Radar WebKit Bug Importer]

<rdar://problem/24616246>

[Jiewen Tan]

Created attachment 271132 [details]
Patch

[Jiewen Tan]

Created attachment 271133 [details]
Patch

[Darin Adler]

Comment on attachment 271133 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271133&action=review

This patch is OK, but I have a better idea of how to do this. You can land it as-is, but then I see a way to make this cleaner.

> Source/WebCore/dom/EventDispatcher.cpp:171
> +    if (creationOptions == SimulatedClickCreationOptions::FromUserAgent) {
> +        if (mouseEventOptions == SendMouseOverUpDownEvents)
> +            dispatchEvent(element, SimulatedMouseEvent::create(eventNames().mouseoverEvent, element->document().defaultView(), underlyingEvent, element));
> +
> +        if (mouseEventOptions != SendNoEvents)
> +            dispatchEvent(element, SimulatedMouseEvent::create(eventNames().mousedownEvent, element->document().defaultView(), underlyingEvent, element));
> +        element->setActive(true, visualOptions == ShowPressedLook);
> +        if (mouseEventOptions != SendNoEvents)
> +            dispatchEvent(element, SimulatedMouseEvent::create(eventNames().mouseupEvent, element->document().defaultView(), underlyingEvent, element));
> +        element->setActive(false);
> +
> +        // always send click
> +        dispatchEvent(element, SimulatedMouseEvent::create(eventNames().clickEvent, element->document().defaultView(), underlyingEvent, element));
> +    } else if (creationOptions == SimulatedClickCreationOptions::FromBindings) {
> +        SimulatedMouseEventInit init;
> +        init.bubbles = true;
> +        init.cancelable = true;
> +        init.view = element->document().defaultView();
> +        init.isSimulated = true;
> +        init.underlyingEvent = underlyingEvent;
> +        init.target = element;
> +
> +        if (mouseEventOptions == SendMouseOverUpDownEvents)
> +            dispatchEvent(element, SimulatedMouseEvent::createForBindings(eventNames().mouseoverEvent, init));
> +
> +        if (mouseEventOptions != SendNoEvents)
> +            dispatchEvent(element, SimulatedMouseEvent::createForBindings(eventNames().mousedownEvent, init));
> +        element->setActive(true, visualOptions == ShowPressedLook);
> +        if (mouseEventOptions != SendNoEvents)
> +            dispatchEvent(element, SimulatedMouseEvent::createForBindings(eventNames().mouseupEvent, init));
> +        element->setActive(false);
> +
> +        // always send click
> +        dispatchEvent(element, SimulatedMouseEvent::createForBindings(eventNames().clickEvent, init));
> +    }

This seems excessive. I don’t think we need to make any changes to SimulatedMouseEvent itself (although changing PassRefPtr to RefPtr&& is nice!). Instead, I think the code should just use a private helper function, dispatchSimulatedMouseEvent that would say:

    auto event = SimulatedMouseEvent::create(<arguments>);
    if (creationOptions == SimulatedClickCreationOptions::FromBindings)
        event.get().setUntrusted();
    dispatchEvent(element, event.get());

I know that’s not the pattern we use elsewhere, but I think it’s fine here. No need to create the events in an entirely different way and pretend they were created directly by JavaScript when that’s not the real way the code flows.

I also think that SimulatedMouseEvent should be moved out of the public headers so no one is tempted to use it in a different way. It’s should be used only by this function, dispatchSimulatedClick. So what I would suggest is that we move this function and the SimulatedMouseEvent class into a new file, SimulatedClick.h/cpp. The header file would have only one function, dispatchSimulatedClick, and the implementation file would contain the entire SimulatedMouseEvent class, which would be removed from MouseEvent.h and MouseEvent.cpp. Note that this header would also be included only by Element.cpp!

I would also put SimulatedClickCreationOptions into SimulatedClick.h instead of SimulatedClickOptions.h. The options are for the functions that code uses directly, the ones in the Element class. Whereas the SimulatedClickCreationOptions is only for use inside Element.cpp.

We could also consider renaming this function. I’m not sure that “dispatch” is a great name for a function that creates new events and dispatches them. The other “dispatch” functions take an existing event and dispatch it. I think a better name for the function would be “simulateClick”.

> Source/WebCore/dom/EventDispatcher.h:42
> +void dispatchSimulatedClick(Element*, Event* underlyingEvent, SimulatedClickMouseEventOptions, SimulatedClickVisualOptions, SimulatedClickCreationOptions);

The element argument should be Element&, not Element*.

[Jiewen Tan]

Created attachment 271248 [details]
Patch for landing

[WebKit Commit Bot]

Attachment 271248 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/SimulatedClick.cpp:51:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/dom/SimulatedClick.cpp:53:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/SimulatedClick.cpp:55:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

Created attachment 271269 [details]
Patch

[WebKit Commit Bot]

Attachment 271269 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/SimulatedClick.cpp:51:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/dom/SimulatedClick.cpp:53:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/SimulatedClick.cpp:55:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 271269 [details]
Patch

Rejecting attachment 271269 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 271269, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
m/SimulatedClick.cpp
patching file Source/WebCore/dom/SimulatedClick.h
patching file Source/WebCore/html/HTMLElement.cpp
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/imported/blink/fast/events/event-trusted-expected.txt
patching file LayoutTests/imported/blink/fast/events/event-trusted.html

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Darin Adler']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/832061

[Darin Adler]

Comment on attachment 271269 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271269&action=review

> Source/WebCore/dom/Element.h:462
> +    void dispatchSimulatedClickForBindings(Event* underlyingEvent, SimulatedClickMouseEventOptions = SendNoEvents, SimulatedClickVisualOptions = ShowPressedLook);

I suggest we remove the options arguments from this function and instead have it always use the options "SendNoEvents" and "DoNotShowPressedLook", since the only caller wants those options. We can always add the options later if needed.

> Source/WebCore/dom/SimulatedClick.cpp:47
> +    virtual ~SimulatedMouseEvent()
> +    {
> +    }

This is not needed and should just be removed.

> Source/WebCore/dom/SimulatedClick.cpp:77
> +static void simulateClickInternal(const AtomicString& eventType, Element& element, Event* underlyingEvent, SimulatedClickCreationOptions creationOptions)

I’d name this simulateMouseEvent.

> Source/WebCore/dom/SimulatedClick.cpp:104
> +    // always send click

We should remove this comment, unless we replace it with a longer one that says *why* we always send a click event. I think there’s no need for a comment.

> Source/WebCore/dom/SimulatedClick.h:36
> +enum SimulatedClickCreationOptions {

Should change this to an enum class.

[WebKit Commit Bot]

Comment on attachment 271269 [details]
Patch

Rejecting attachment 271269 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 271269, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
m/SimulatedClick.cpp
patching file Source/WebCore/dom/SimulatedClick.h
patching file Source/WebCore/html/HTMLElement.cpp
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/imported/blink/fast/events/event-trusted-expected.txt
patching file LayoutTests/imported/blink/fast/events/event-trusted.html

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Darin Adler']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/832086

[Jiewen Tan]

Created attachment 271352 [details]
Patch for landing

[Jiewen Tan]

Created attachment 271359 [details]
Patch for landing

[WebKit Commit Bot]

Attachment 271359 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/SimulatedClick.cpp:47:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/dom/SimulatedClick.cpp:49:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/SimulatedClick.cpp:51:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

Committed r196598: <http://trac.webkit.org/changeset/196598>