Summary: | Web Automation: implement Automation.performMouseInteraction | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | BJ Burg <bburg> | ||||||||||||||||
Component: | WebKit2 | Assignee: | BJ Burg <bburg> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | bburg, benjamin, cdumez, cmarcelo, commit-queue, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Attachments: |
|
Description
BJ Burg
2016-03-17 15:35:24 PDT
Created attachment 274334 [details]
Proposed Fix
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 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 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 on attachment 274334 [details]
Proposed Fix
Investigating a new approach that combines all the mouse interactions into one command.
Created attachment 274728 [details]
Proposed Fix (v2)
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 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 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. (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! Created attachment 275035 [details]
Rebased Patch
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). Created attachment 275069 [details]
Rebased
Created attachment 275079 [details]
Rebased
Created attachment 275080 [details]
Rebased
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. Created attachment 275081 [details]
Rebased
Committed: <http://trac.webkit.org/changeset/198792> |