Bug 174616 – WebDriver: implement advance user interactions

• Component: WebDriver
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

https://www.w3.org/TR/webdriver/#actions

[Carlos Garcia Campos]

Created attachment 338723 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Blaze Burg]

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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

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.

[EWS Watchlist]

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.

[Blaze Burg]

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?

[Radar WebKit Bug Importer]

<rdar://problem/40093941>

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Committed r231632: <https://trac.webkit.org/changeset/231632>