Bug 174616 - WebDriver: implement advance user interactions
Summary: WebDriver: implement advance user interactions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebDriver (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 184913
Blocks: 166679
  Show dependency treegraph
 
Reported: 2017-07-18 00:40 PDT by Carlos Garcia Campos
Modified: 2018-05-09 23:52 PDT (History)
4 users (show)

See Also:


Attachments
Patch (65.32 KB, patch)
2018-04-25 06:32 PDT, Carlos Garcia Campos
bburg: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.65 MB, application/zip)
2018-04-25 10:42 PDT, EWS Watchlist
no flags Details
Updated patch (91.55 KB, patch)
2018-05-09 06:57 PDT, Carlos Garcia Campos
bburg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2017-07-18 00:40:42 PDT
https://www.w3.org/TR/webdriver/#actions
Comment 1 Carlos Garcia Campos 2018-04-25 06:32:35 PDT
Created attachment 338723 [details]
Patch
Comment 2 EWS Watchlist 2018-04-25 06:35:41 PDT
Attachment 338723 [details] did not pass style-queue:


ERROR: Source/WebDriver/Session.h:113:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.h:114:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.h:199:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.cpp:1915:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.cpp:1948:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.h:106:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.h:107:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:2095:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebDriver/Session.cpp:2098:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:2131:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:2287:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 11 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 EWS Watchlist 2018-04-25 10:42:30 PDT
Comment on attachment 338723 [details]
Patch

Attachment 338723 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7452385

New failing tests:
http/tests/security/contentSecurityPolicy/video-with-http-url-allowed-by-csp-media-src-star.html
Comment 4 EWS Watchlist 2018-04-25 10:42:41 PDT
Created attachment 338752 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 5 Brian Burg 2018-05-03 09:56:20 PDT
Comment on attachment 338723 [details]
Patch

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

r=me Looks good, but I want to move element-based pointer origin into the protocol rather than doing it driver-side. Do you want to code that up now, or land this as-is and refactor it later?

> Source/WebDriver/ChangeLog:21
> +        (WebDriver::Session::virtualKeyForKeySequence): Add more kay codes includes in the spec.

Nit: key

> Source/WebDriver/Session.cpp:2114
> +        computeInViewCenterPointOfElements(WTFMove(elements), WTFMove(elementOrigins), WTFMove(completionHandler));

Nice.

> Source/WebDriver/Session.cpp:2187
> +                        state->setDouble(ASCIILiteral("duration"), action.duration.value());

Is duration guaranteed to have a value given these preconditions?

> Source/WebDriver/Session.cpp:2200
> +                                currentState.origin.x = action.x.value();

Per our discussion earlier, I think it is incorrect to resolve element locations on the driver side since they may change in the course of a multi-tick interaction. Instead, currentState.origin would have a symbolic x/y if the type is Element. On the WebKit side it would convert that into a layout computation.

> Source/WebDriver/Session.cpp:2203
> +                            case PointerOrigin::Type::Pointer:

This case can be merged with above case.

> Source/WebDriver/Session.cpp:2216
> +                                state->setDouble(ASCIILiteral("duration"), action.duration.value());

This seems like the correct way to do what I called out above re: Action::Type::None.

> Source/WebDriver/WebDriverService.cpp:1834
> +static std::optional<Vector<Action>> processInputActionSequence(Session& session, JSON::Value& actionSequenceValue, std::optional<String>& errorMessage)

I would use return type of

Expected<Vector<Action>, String>

then you can get rid of the errorMessage out parameter.

> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:206
> +            m_client.simulateMouseInteraction(m_page, MouseInteraction::Move, b.pressedMouseButton.value_or(MouseButton::NoButton), b.location.value_or(WebCore::IntPoint()), WTFMove(eventDispatchFinished));

Let's land this separately since it affects safaridriver as well, and don't want to hold it up on the above questions.
Comment 6 Carlos Garcia Campos 2018-05-04 03:21:26 PDT
(In reply to Brian Burg from comment #5)
> Comment on attachment 338723 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338723&action=review
> 
> r=me Looks good, but I want to move element-based pointer origin into the
> protocol rather than doing it driver-side. Do you want to code that up now,
> or land this as-is and refactor it later?

Thanks for the review. I started to move the origin handling to the WebKit side yesterday, but it's more complicated than I expected. I'm not sure I'll be able to continue working on it today. If I see it's going to take long, I'll land this patch and leave the origin for a follow up.

> > Source/WebDriver/ChangeLog:21
> > +        (WebDriver::Session::virtualKeyForKeySequence): Add more kay codes includes in the spec.
> 
> Nit: key
> 
> > Source/WebDriver/Session.cpp:2114
> > +        computeInViewCenterPointOfElements(WTFMove(elements), WTFMove(elementOrigins), WTFMove(completionHandler));
> 
> Nice.

But useless in the end :-(

> > Source/WebDriver/Session.cpp:2187
> > +                        state->setDouble(ASCIILiteral("duration"), action.duration.value());
> 
> Is duration guaranteed to have a value given these preconditions?

Yes.

> > Source/WebDriver/Session.cpp:2200
> > +                                currentState.origin.x = action.x.value();
> 
> Per our discussion earlier, I think it is incorrect to resolve element
> locations on the driver side since they may change in the course of a
> multi-tick interaction. Instead, currentState.origin would have a symbolic
> x/y if the type is Element. On the WebKit side it would convert that into a
> layout computation.

Indeed.

> > Source/WebDriver/Session.cpp:2203
> > +                            case PointerOrigin::Type::Pointer:
> 
> This case can be merged with above case.
> 
> > Source/WebDriver/Session.cpp:2216
> > +                                state->setDouble(ASCIILiteral("duration"), action.duration.value());
> 
> This seems like the correct way to do what I called out above re:
> Action::Type::None.
> 
> > Source/WebDriver/WebDriverService.cpp:1834
> > +static std::optional<Vector<Action>> processInputActionSequence(Session& session, JSON::Value& actionSequenceValue, std::optional<String>& errorMessage)
> 
> I would use return type of
> 
> Expected<Vector<Action>, String>
> 
> then you can get rid of the errorMessage out parameter.
> 
> > Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:206
> > +            m_client.simulateMouseInteraction(m_page, MouseInteraction::Move, b.pressedMouseButton.value_or(MouseButton::NoButton), b.location.value_or(WebCore::IntPoint()), WTFMove(eventDispatchFinished));
> 
> Let's land this separately since it affects safaridriver as well, and don't
> want to hold it up on the above questions.

Ok, it makes sense.
Comment 7 Carlos Garcia Campos 2018-05-09 06:57:18 PDT
Created attachment 339961 [details]
Updated patch

This is a new patch moving the origin handling to the WebKit layer side. Asking for review again, because I changed several things.
Comment 8 EWS Watchlist 2018-05-09 06:58:38 PDT
Attachment 339961 [details] did not pass style-queue:


ERROR: Source/WebDriver/Session.h:113:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.h:114:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.cpp:1915:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.cpp:1948:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:181:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1419:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.h:119:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.h:147:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.h:106:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.h:107:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/Automation/WebAutomationSession.h:141:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:2095:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebDriver/Session.cpp:2124:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:2253:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 14 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Brian Burg 2018-05-09 09:14:47 PDT
Comment on attachment 339961 [details]
Updated patch

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

r=me, great work!

> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:273
> +void SimulatedInputDispatcher::run(uint64_t frameID, Vector<SimulatedInputKeyFrame>&& keyFrames, HashSet<Ref<SimulatedInputSource>>& inputSources, AutomationCompletionHandler&& completionHandler)

I am not sure whether the spec allows interacting with elements from different frames in the same interaction; it's a bit vague. The actions commands will fail if there is not a current browsing context.

EDIT: I think this is correct, because the "get a known element" algorithm only looks up elements in the current browsing context. I think this might trip up some users, but let's go with this for now.

> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.h:119
> +        virtual void viewportInViewCenterPointOfElement(WebPageProxy&, uint64_t frameID, const String& nodeHandle, Function<void (std::optional<WebCore::IntPoint>, std::optional<AutomationCommandError>)>&&) = 0;

This name is a bit awkward, but I can live with it, assuming it computes the in-view centre point of the element in viewport coordinate space. If it does something else then please clarify :)

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:981
> +    // Start at 2 and use only even numbers to not conflict with m_nextViewportInViewCenterPointOfElementCallbackID.

I'm not a fan of tricks like this. Would it be possible to just have the existing Automation command use the Client API?
Comment 10 Radar WebKit Bug Importer 2018-05-09 09:19:24 PDT
<rdar://problem/40093941>
Comment 11 Carlos Garcia Campos 2018-05-09 22:55:28 PDT
(In reply to Brian Burg from comment #9)
> Comment on attachment 339961 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339961&action=review
> 
> r=me, great work!

Thanks!

> > Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:273
> > +void SimulatedInputDispatcher::run(uint64_t frameID, Vector<SimulatedInputKeyFrame>&& keyFrames, HashSet<Ref<SimulatedInputSource>>& inputSources, AutomationCompletionHandler&& completionHandler)
> 
> I am not sure whether the spec allows interacting with elements from
> different frames in the same interaction; it's a bit vague. The actions
> commands will fail if there is not a current browsing context.
> 
> EDIT: I think this is correct, because the "get a known element" algorithm
> only looks up elements in the current browsing context. I think this might
> trip up some users, but let's go with this for now.

Right, get a known element says we should look in the current browsing context, but it will be the same for the whole sequence anyway.

> > Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.h:119
> > +        virtual void viewportInViewCenterPointOfElement(WebPageProxy&, uint64_t frameID, const String& nodeHandle, Function<void (std::optional<WebCore::IntPoint>, std::optional<AutomationCommandError>)>&&) = 0;
> 
> This name is a bit awkward, but I can live with it, assuming it computes the
> in-view centre point of the element in viewport coordinate space. If it does
> something else then please clarify :)

That's exactly what it does. It doesn't scroll the view to make the element visible, because the spec doesn't say we should. So, in case the element is not in viewport a target out of bounds error will be generated.

> > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:981
> > +    // Start at 2 and use only even numbers to not conflict with m_nextViewportInViewCenterPointOfElementCallbackID.
> 
> I'm not a fan of tricks like this. Would it be possible to just have the
> existing Automation command use the Client API?

I'm not sure what you mean exactly. At first I tried to use the computeElementLayout directly, but that requires to use the backend dispatcher, a sequence id, and the backend callback. But we only want to send the message to the web process and get the response as part of an ongoing command.
Comment 12 Carlos Garcia Campos 2018-05-09 23:52:42 PDT
Committed r231632: <https://trac.webkit.org/changeset/231632>