Bug 155606 – Web Automation: implement Automation.performMouseInteraction

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Blaze Burg]

Do it!

[Radar WebKit Bug Importer]

<rdar://problem/25227576>

[Radar WebKit Bug Importer]

<rdar://problem/25227587>

[Blaze Burg]

Created attachment 274334 [details]
Proposed Fix

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 6 files


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

[Timothy Hatcher]

Comment on attachment 274334 [details]
Proposed Fix

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

> Source/WebKit2/UIProcess/Cocoa/WebAutomationSessionCocoa.mm:47
> +    IntRect windowRect;
> +    page.rootViewToWindow(IntRect(viewPosition, IntSize()), windowRect);
> +    IntPoint windowPosition = windowRect.location();
> +
> +    // FIXME: This may need to include any still-pressed Shift, Ctrl, Alt, Meta keys from a previous key press command.
> +    NSEventModifierFlags modifiers = 0;
> +    NSWindow *window = page.platformWindow();
> +    NSEvent *simulatedEvent = [NSEvent mouseEventWithType:NSMouseMoved location:windowPosition modifierFlags:modifiers timestamp:[NSDate timeIntervalSinceReferenceDate] windowNumber:window.windowNumber context:nil eventNumber:0 clickCount:0 pressure:0.0f];
> +    [window sendEvent:simulatedEvent];

Looks fine. Maybe we should consider using WebPage::simulateMouseDown, WebPage::simulateMouseUp, WebPage::simulateMouseMotion instead?

[Blaze Burg]

Comment on attachment 274334 [details]
Proposed Fix

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

>> Source/WebKit2/UIProcess/Cocoa/WebAutomationSessionCocoa.mm:47
>> +    [window sendEvent:simulatedEvent];
> 
> Looks fine. Maybe we should consider using WebPage::simulateMouseDown, WebPage::simulateMouseUp, WebPage::simulateMouseMotion instead?

I would need to talk to Wenson or Enrica to understand the tradeoffs here. There's no equivalent for key presses so I'm not sure we want to do some interactions via UIProcess and others in WebProcess. I can't really validate whether either approach works until all mouse commands work. It may be the case that the code needs different platform helper implementation for UIKit vs AppKit.

[Blaze Burg]

Comment on attachment 274334 [details]
Proposed Fix

Investigating a new approach that combines all the mouse interactions into one command.

[Blaze Burg]

Created attachment 274728 [details]
Proposed Fix (v2)

[Blaze Burg]

Comment on attachment 274728 [details]
Proposed Fix (v2)

This is an interdiff from the v1 patch. Neither applies to trunk cleanly at the moment.

[Joseph Pecoraro]

Comment on attachment 274728 [details]
Proposed Fix (v2)

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

> Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:492
> +    WebEvent::Modifiers keyModifiers = (WebEvent::Modifiers)0;

Is this cast needed?

> Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:502
> +        keyModifiers = (WebEvent::Modifiers)(enumValue | keyModifiers);

Is this cast needed? Seems like this could be: keyModified |= enumValue.

> Source/WebKit2/UIProcess/Cocoa/WebAutomationSessionCocoa.mm:124
> +
> +#endif // USE(APPKIT)

Nit: An extra newline in here.

Would be nice to support Force Click in here in the future!

[Joseph Pecoraro]

Comment on attachment 274728 [details]
Proposed Fix (v2)

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

> Source/WebKit2/UIProcess/Cocoa/WebAutomationSessionCocoa.mm:113
> +        [eventsToBeSent addObject:[NSEvent mouseEventWithType:upEventType location:windowPosition modifierFlags:modifiers timestamp:timestamp windowNumber:windowNumber context:nil eventNumber:0 clickCount:2 pressure:0.0f]];
> +    }

Nit: Missing a break here.

[Blaze Burg]

(In reply to comment #10)
> Comment on attachment 274728 [details]
> Proposed Fix (v2)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274728&action=review
> 
> > Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:492
> > +    WebEvent::Modifiers keyModifiers = (WebEvent::Modifiers)0;
> 
> Is this cast needed?
> 
> > Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:502
> > +        keyModifiers = (WebEvent::Modifiers)(enumValue | keyModifiers);
> 
> Is this cast needed? Seems like this could be: keyModified |= enumValue.

According to Xcode, it is.

> > Source/WebKit2/UIProcess/Cocoa/WebAutomationSessionCocoa.mm:124
> > +
> > +#endif // USE(APPKIT)
> 
> Nit: An extra newline in here.
> 
> Would be nice to support Force Click in here in the future!

[Blaze Burg]

Created attachment 275035 [details]
Rebased Patch

[Timothy Hatcher]

Comment on attachment 275035 [details]
Rebased Patch

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

> Source/WebKit2/UIProcess/Automation/Automation.json:194
> +                { "name": "handle", "$ref": "BrowsingContextHandle", "description": "The browsing context to be interacted with." },

We might want to always use a name of "browsingContextHandle", so it is clear that it is not a frame handle, etc.

> Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:611
> +    platformSimulateMouseInteraction(*page, viewPosition, parsedInteraction.value(), parsedButton.value(), keyModifiers);

We should add a FIXME about adding force click (pressure).

[Blaze Burg]

Created attachment 275069 [details]
Rebased

[Blaze Burg]

Created attachment 275079 [details]
Rebased

[Blaze Burg]

Created attachment 275080 [details]
Rebased

[Blaze Burg]

Comment on attachment 275080 [details]
Rebased

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

> Source/WebKit2/UIProcess/Cocoa/WebAutomationSessionCocoa.mm:49
> +        objc_setAssociatedObject(event, [_WKAutomationSession class], m_sessionIdentifier, OBJC_ASSOCIATION_RETAIN_NONATOMIC);

It looks like we can't use _WKAutomationSession for this purpose, since it is guarded by WK_API_ENABLED (which is false if building for 32-bit). Maybe we should just use a static const char* casted to void*? It would need to have the same address when used in separately compiled and linked libraries.

It's common to use a selector as the key, but I'm not sure how many selectors are useable by both WK2 internally and by its clients.

[Blaze Burg]

Created attachment 275081 [details]
Rebased

[Blaze Burg]

Committed: <http://trac.webkit.org/changeset/198792>