Bug 155606 - Web Automation: implement Automation.performMouseInteraction
Summary: Web Automation: implement Automation.performMouseInteraction
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-03-17 15:35 PDT by BJ Burg
Modified: 2016-03-29 13:21 PDT (History)
11 users (show)

See Also:


Attachments
Proposed Fix (14.01 KB, patch)
2016-03-17 15:42 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (v2) (18.78 KB, patch)
2016-03-22 23:04 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Rebased Patch (26.20 KB, patch)
2016-03-28 12:02 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Rebased (25.07 KB, patch)
2016-03-28 17:36 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Rebased (27.00 KB, patch)
2016-03-28 20:12 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Rebased (27.04 KB, patch)
2016-03-28 20:57 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Rebased (26.93 KB, patch)
2016-03-28 21:18 PDT, BJ Burg
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2016-03-17 15:35:24 PDT
Do it!
Comment 1 Radar WebKit Bug Importer 2016-03-17 15:36:02 PDT
<rdar://problem/25227576>
Comment 2 Radar WebKit Bug Importer 2016-03-17 15:36:31 PDT
<rdar://problem/25227587>
Comment 3 BJ Burg 2016-03-17 15:42:47 PDT
Created attachment 274334 [details]
Proposed Fix
Comment 4 WebKit Commit Bot 2016-03-17 15:46:00 PDT
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.
Comment 5 Timothy Hatcher 2016-03-18 14:28:30 PDT
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?
Comment 6 BJ Burg 2016-03-18 15:22:50 PDT
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.
Comment 7 BJ Burg 2016-03-18 16:15:31 PDT
Comment on attachment 274334 [details]
Proposed Fix

Investigating a new approach that combines all the mouse interactions into one command.
Comment 8 BJ Burg 2016-03-22 23:04:59 PDT
Created attachment 274728 [details]
Proposed Fix (v2)
Comment 9 BJ Burg 2016-03-22 23:05:57 PDT
Comment on attachment 274728 [details]
Proposed Fix (v2)

This is an interdiff from the v1 patch. Neither applies to trunk cleanly at the moment.
Comment 10 Joseph Pecoraro 2016-03-25 17:02:08 PDT
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!
Comment 11 Joseph Pecoraro 2016-03-25 17:04:44 PDT
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.
Comment 12 BJ Burg 2016-03-25 17:39:34 PDT
(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!
Comment 13 BJ Burg 2016-03-28 12:02:05 PDT
Created attachment 275035 [details]
Rebased Patch
Comment 14 Timothy Hatcher 2016-03-28 13:32:39 PDT
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).
Comment 15 BJ Burg 2016-03-28 17:36:18 PDT
Created attachment 275069 [details]
Rebased
Comment 16 BJ Burg 2016-03-28 20:12:04 PDT
Created attachment 275079 [details]
Rebased
Comment 17 BJ Burg 2016-03-28 20:57:17 PDT
Created attachment 275080 [details]
Rebased
Comment 18 BJ Burg 2016-03-28 21:02:03 PDT
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.
Comment 19 BJ Burg 2016-03-28 21:18:52 PDT
Created attachment 275081 [details]
Rebased
Comment 20 BJ Burg 2016-03-29 13:21:05 PDT
Committed: <http://trac.webkit.org/changeset/198792>