RESOLVED FIXED 195696
Web Automation: elements larger than the viewport have incorrect in-view center point
https://bugs.webkit.org/show_bug.cgi?id=195696
Summary Web Automation: elements larger than the viewport have incorrect in-view cent...
Blaze Burg
Reported 2019-03-13 13:25:51 PDT
This seems to be an omission in the specification. While it does mention that the in-view center point (IVCP) must be within the viewport, the algorithm never intersects the element bounding box with the viewport rect. Spec issue filed here: https://github.com/w3c/webdriver/issues/1402
Attachments
WIP patch (12.85 KB, patch)
2019-03-14 13:54 PDT, Blaze Burg
no flags
WIP v2 (31.13 KB, patch)
2019-04-15 17:14 PDT, Blaze Burg
no flags
Patch (35.35 KB, patch)
2019-05-06 17:01 PDT, Blaze Burg
no flags
Archive of layout-test-results from ews211 for win-future (13.67 MB, application/zip)
2019-05-06 23:31 PDT, EWS Watchlist
no flags
Patch (37.90 KB, patch)
2019-05-10 13:06 PDT, Devin Rousso
no flags
Patch (38.12 KB, patch)
2019-05-11 13:14 PDT, Devin Rousso
no flags
Archive of layout-test-results from ews214 for win-future (13.78 MB, application/zip)
2019-05-12 09:48 PDT, EWS Watchlist
no flags
Patch (38.16 KB, patch)
2019-05-13 19:31 PDT, Devin Rousso
no flags
Patch (36.98 KB, patch)
2019-05-14 17:36 PDT, Devin Rousso
no flags
Patch (38.78 KB, patch)
2019-05-15 00:50 PDT, Devin Rousso
no flags
Blaze Burg
Comment 1 2019-03-13 13:26:11 PDT
Blaze Burg
Comment 2 2019-03-14 13:54:04 PDT
Created attachment 364682 [details] WIP patch This is an attempt to fix some hit testing issues I ran into in more complicated content such as scrolling. I need to run full test suites to verify this doesn't regress anything. There are also other unexpected 'element not interactable' errors I've run into, but these may be unrelated to the bugs fixed in this patch.
Simon Fraser (smfr)
Comment 3 2019-03-14 14:06:17 PDT
Comment on attachment 364682 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=364682&action=review > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1550 > + WebCore::FloatRect visualViewportRect = page.customFixedPositionRect(); // Content coordinates. This is wrong; customFixedPositionRect is layoutViewport, not visualViewport. > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1551 > + visualViewportRect.setLocation(WebCore::FloatPoint::zero()); visualViewportRect.setLocation({ }) should work. > Source/WebKit/UIProcess/Automation/ios/WebAutomationSessionIOS.mm:180 > +static TextStream& operator<<(TextStream& ts, const TouchInteraction& interaction) TouchInteraction is an enum so you can pass by value. > Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:480 > +// Returns the in-view center point of an element in client coordinates. At some future point "client" coordinates might be layoutViewport coords, which might allow the layout viewport center to not be visible. > Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:494 > + FloatRect elementRect = FloatRect(firstElementRect->x(), firstElementRect->y(), firstElementRect->width(), firstElementRect->height()); = { firstElementRect->x(), firstElementRect->y(), firstElementRect->width(), firstElementRect->height() } We should probably give DOMRectReadOnly a FloatRect operator. > Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:495 > + FloatRect visibleElementRect = intersection(viewportRect, elementRect); You're assuming element.getClientRects() are in mainFrameView.visualViewportRect() coordinates, which is true now but may not always be.
Blaze Burg
Comment 4 2019-04-15 17:14:15 PDT
Blaze Burg
Comment 5 2019-04-15 17:16:52 PDT
(In reply to Simon Fraser (smfr) from comment #3) > Comment on attachment 364682 [details] > WIP patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=364682&action=review > > > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1550 > > + WebCore::FloatRect visualViewportRect = page.customFixedPositionRect(); // Content coordinates. > > This is wrong; customFixedPositionRect is layoutViewport, not visualViewport. OK > > > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1551 > > + visualViewportRect.setLocation(WebCore::FloatPoint::zero()); > > visualViewportRect.setLocation({ }) should work. OK > > > Source/WebKit/UIProcess/Automation/ios/WebAutomationSessionIOS.mm:180 > > +static TextStream& operator<<(TextStream& ts, const TouchInteraction& interaction) > > TouchInteraction is an enum so you can pass by value. OK > > > Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:480 > > +// Returns the in-view center point of an element in client coordinates. > > At some future point "client" coordinates might be layoutViewport coords, > which might allow the layout viewport center to not be visible. See question below. > > Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:494 > > + FloatRect elementRect = FloatRect(firstElementRect->x(), firstElementRect->y(), firstElementRect->width(), firstElementRect->height()); > > = { firstElementRect->x(), firstElementRect->y(), firstElementRect->width(), > firstElementRect->height() } > > We should probably give DOMRectReadOnly a FloatRect operator. ¯\_(ツ)_/¯ > > > Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:495 > > + FloatRect visibleElementRect = intersection(viewportRect, elementRect); > > You're assuming element.getClientRects() are in > mainFrameView.visualViewportRect() coordinates, which is true now but may > not always be. Should code for this situation be guarded by this, which I found in TreeScope.cpp? > if (settings.visualViewportEnabled() && settings.clientCoordinatesRelativeToLayoutViewport()) { I'll add that in when I'm able to test it, but for the sake of getting this landed sometime, I might make a separate bug.
EWS Watchlist
Comment 6 2019-04-15 17:17:41 PDT Comment hidden (obsolete)
Blaze Burg
Comment 7 2019-04-15 17:18:58 PDT
This new patch is not ready to land as I'm unable to test it on iOS at this time. On Mac, it only causes one WPT test regression on Mac (which is actually a separate, newly-revealed bug). For iOS, I need to substitute contentsToRootView with the manual frame client->root view conversion code. This is currently in a comment block so I don't forget the gist of it. Comments and feedback welcome. I'm hoping to put out a final patch tomorrow.
Blaze Burg
Comment 8 2019-05-06 17:01:36 PDT
EWS Watchlist
Comment 9 2019-05-06 23:31:43 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 10 2019-05-06 23:31:45 PDT Comment hidden (obsolete)
Devin Rousso
Comment 11 2019-05-10 13:06:05 PDT
Created attachment 369584 [details] Patch Rebase, plus some minor style changes
Simon Fraser (smfr)
Comment 12 2019-05-10 18:04:03 PDT
Comment on attachment 369584 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369584&action=review > Source/WebCore/platform/ScrollView.cpp:850 > + return viewToContents(IntPoint(point)); roundedIntPoint > Source/WebCore/platform/ScrollView.cpp:858 > + return contentsToView(IntPoint(point)); roundedIntPoint > Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:554 > + auto& frame = frameView->frame(); > + clientRect.scale(frame.pageZoomFactor() * frame.frameScaleFactor()); > + clientRect.moveBy(frameView->contentsScrollPosition()); > + return clientRect; Maybe add a comment to say why contentsToRootView doesn't work for you in this case. > Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:565 > + auto& frame = frameView->frame(); > + clientPoint.scale(frame.pageZoomFactor() * frame.frameScaleFactor()); > + clientPoint.moveBy(frameView->contentsScrollPosition()); > + return clientPoint; Ditto.
Devin Rousso
Comment 13 2019-05-11 13:14:17 PDT
EWS Watchlist
Comment 14 2019-05-12 09:48:53 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 15 2019-05-12 09:48:56 PDT Comment hidden (obsolete)
Devin Rousso
Comment 16 2019-05-13 17:02:09 PDT
Comment on attachment 369584 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369584&action=review >> Source/WebCore/platform/ScrollView.cpp:850 >> + return viewToContents(IntPoint(point)); > > roundedIntPoint This actually causes WebDriver WPT failures, specifically all of the odd numbered wpt/tests/element_click/center_point.py::test_css_pixel_rounding tests.
Devin Rousso
Comment 17 2019-05-13 19:31:13 PDT
Darin Adler
Comment 18 2019-05-13 19:58:51 PDT
Comment on attachment 369812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369812&action=review > Source/WebCore/dom/DOMRectReadOnly.h:55 > + operator FloatRect() const { return FloatRect(m_x, m_y, m_width, m_height); } Not sure it’s a good idea to have a lossy operation that converts doubles to floats be implicit; typically we only want lossless operations to be implicit and lossy ones to have names
Devin Rousso
Comment 19 2019-05-14 17:36:50 PDT
EWS Watchlist
Comment 20 2019-05-14 17:39:09 PDT Comment hidden (obsolete)
Devin Rousso
Comment 21 2019-05-15 00:50:15 PDT
EWS Watchlist
Comment 22 2019-05-15 00:52:47 PDT Comment hidden (obsolete)
WebKit Commit Bot
Comment 23 2019-05-15 01:37:08 PDT
Comment on attachment 369936 [details] Patch Clearing flags on attachment: 369936 Committed r245320: <https://trac.webkit.org/changeset/245320>
WebKit Commit Bot
Comment 24 2019-05-15 01:37:10 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.