https://www.w3.org/TR/webdriver/#actions
Created attachment 338723 [details] Patch
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 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
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 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.
(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.
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.
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 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?
<rdar://problem/40093941>
(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.
Committed r231632: <https://trac.webkit.org/changeset/231632>