From ede84d522d97965c8f9f718b802d5b78904b70b0 Mon Sep 17 00:00:00 2001 From: BJ Burg Date: Mon, 18 Dec 2017 16:56:24 +0000 Subject: [PATCH] Merge r225367 - Web Automation: computeElementLayout does not correctly translate iframe client coordinates to main frame coordinates https://bugs.webkit.org/show_bug.cgi?id=180213 Reviewed by Simon Fraser. The current implementation computes points in terms of the frame in which the element is located. However, WebDriver expects coordinates to be relative to the top-level document since these coordinates are used for generating click events, among other things. To convert from frame client coordinates to main frame client coordinates, round-trip both inViewCenterPoint and elementBounds to root view coordinates and back to the main frame's contents/client coordinates. Then convert this to page coordinates if needed. This progresses several tests in the Selenium Python test suite: - event_firing_webdriver_tests.py::test_should_fire_navigation_events - frame_switching_tests.py::testShouldBeAbleToClickInAFrameThatRewritesTopWindowLocation - frame_switching_tests.py::testShouldBeAbleToSwitchToTheTopIfTheFrameIsDeletedFromUnderUs - frame_switching_tests.py::testShouldBeAbleToSwitchToTheTopIfTheFrameIsDeletedFromUnderUsWithFrameIndex - frame_switching_tests.py::testShouldBeAbleToSwitchToTheTopIfTheFrameIsDeletedFromUnderUsWithWebelement - frame_switching_tests.py::testShouldNotBeAbleToDoAnythingTheFrameIsDeletedFromUnderUs - position_and_size_tests.py::testShouldGetCoordinatesOfAnInvisibleElement * WebProcess/Automation/WebAutomationSessionProxy.cpp: (WebKit::WebAutomationSessionProxy::computeElementLayout): Get both the frame and main frame FrameViews and convert coordinates to the root view. This is somewhat lossy as clientToDocument* deals with FloatPoints but contentsToRootView deals with IntPoints. For the purposes of WebDriver, lossiness is not a problem since integer values are expected anyway. The imperative nature of the coordinate calculations is difficult to debug, so I converted this function to only assign to each variable once. --- Source/WebKit/ChangeLog | 36 +++++++++++++++++++ .../Automation/WebAutomationSessionProxy.cpp | 23 +++++++----- 2 files changed, 50 insertions(+), 9 deletions(-) diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog index e474fd23e51c..bb7103370085 100644 --- a/Source/WebKit/ChangeLog +++ b/Source/WebKit/ChangeLog @@ -1,3 +1,39 @@ +2017-11-30 Brian Burg + + Web Automation: computeElementLayout does not correctly translate iframe client coordinates to main frame coordinates + https://bugs.webkit.org/show_bug.cgi?id=180213 + + + Reviewed by Simon Fraser. + + The current implementation computes points in terms of the frame in which the element is located. + However, WebDriver expects coordinates to be relative to the top-level document since + these coordinates are used for generating click events, among other things. + + To convert from frame client coordinates to main frame client coordinates, round-trip + both inViewCenterPoint and elementBounds to root view coordinates and back + to the main frame's contents/client coordinates. Then convert this to page coordinates if needed. + + This progresses several tests in the Selenium Python test suite: + + - event_firing_webdriver_tests.py::test_should_fire_navigation_events + - frame_switching_tests.py::testShouldBeAbleToClickInAFrameThatRewritesTopWindowLocation + - frame_switching_tests.py::testShouldBeAbleToSwitchToTheTopIfTheFrameIsDeletedFromUnderUs + - frame_switching_tests.py::testShouldBeAbleToSwitchToTheTopIfTheFrameIsDeletedFromUnderUsWithFrameIndex + - frame_switching_tests.py::testShouldBeAbleToSwitchToTheTopIfTheFrameIsDeletedFromUnderUsWithWebelement + - frame_switching_tests.py::testShouldNotBeAbleToDoAnythingTheFrameIsDeletedFromUnderUs + - position_and_size_tests.py::testShouldGetCoordinatesOfAnInvisibleElement + + * WebProcess/Automation/WebAutomationSessionProxy.cpp: + (WebKit::WebAutomationSessionProxy::computeElementLayout): + Get both the frame and main frame FrameViews and convert coordinates to the root view. + This is somewhat lossy as clientToDocument* deals with FloatPoints but contentsToRootView + deals with IntPoints. For the purposes of WebDriver, lossiness is not a problem since + integer values are expected anyway. + + The imperative nature of the coordinate calculations is difficult to debug, so I converted + this function to only assign to each variable once. + 2017-11-14 Carlos Garcia Campos Move JSONValues to WTF and convert uses of InspectorValues.h to JSONValues.h diff --git a/Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp b/Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp index e47374a6800c..070a8caba215 100644 --- a/Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp +++ b/Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp @@ -572,32 +572,37 @@ void WebAutomationSessionProxy::computeElementLayout(uint64_t pageID, uint64_t f return; } - WebCore::FloatRect elementBounds = coreElement->boundingClientRect(); - - auto* coreFrameView = frame->coreFrame()->view(); + WebCore::FrameView* frameView = frame->coreFrame()->view(); + WebCore::FrameView* mainView = frame->coreFrame()->mainFrame().view(); + WebCore::IntRect frameElementBounds = roundedIntRect(coreElement->boundingClientRect()); + WebCore::IntRect rootElementBounds = mainView->rootViewToContents(frameView->contentsToRootView(frameElementBounds)); + WebCore::IntRect resultElementBounds; switch (coordinateSystem) { case CoordinateSystem::Page: - elementBounds = coreFrameView->clientToDocumentRect(elementBounds); + resultElementBounds = WebCore::IntRect(mainView->clientToDocumentRect(WebCore::FloatRect(rootElementBounds))); break; case CoordinateSystem::LayoutViewport: // The element bounds are already in client coordinates. + resultElementBounds = rootElementBounds; break; case CoordinateSystem::VisualViewport: ASSERT_NOT_REACHED(); break; } - std::optional inViewCenterPoint; + std::optional resultInViewCenterPoint; bool isObscured = false; if (containerElement) { - inViewCenterPoint = elementInViewClientCenterPoint(*containerElement, isObscured); - if (inViewCenterPoint.has_value()) { + std::optional frameInViewCenterPoint = elementInViewClientCenterPoint(*containerElement, isObscured); + if (frameInViewCenterPoint.has_value()) { + WebCore::IntPoint rootInViewCenterPoint = mainView->rootViewToContents(frameView->contentsToRootView(WebCore::IntPoint(frameInViewCenterPoint.value()))); switch (coordinateSystem) { case CoordinateSystem::Page: - inViewCenterPoint = coreFrameView->clientToDocumentPoint(inViewCenterPoint.value()); + resultInViewCenterPoint = WebCore::IntPoint(mainView->clientToDocumentPoint(rootInViewCenterPoint)); break; case CoordinateSystem::LayoutViewport: // The point is already in client coordinates. + resultInViewCenterPoint = rootInViewCenterPoint; break; case CoordinateSystem::VisualViewport: ASSERT_NOT_REACHED(); @@ -606,7 +611,7 @@ void WebAutomationSessionProxy::computeElementLayout(uint64_t pageID, uint64_t f } } - WebProcess::singleton().parentProcessConnection()->send(Messages::WebAutomationSession::DidComputeElementLayout(callbackID, enclosingIntRect(elementBounds), WebCore::IntPoint(inViewCenterPoint.value()), isObscured, String()), 0); + WebProcess::singleton().parentProcessConnection()->send(Messages::WebAutomationSession::DidComputeElementLayout(callbackID, resultElementBounds, resultInViewCenterPoint.value(), isObscured, String()), 0); } void WebAutomationSessionProxy::selectOptionElement(uint64_t pageID, uint64_t frameID, String nodeHandle, uint64_t callbackID)