Web Automation: elements larger than the viewport have incorrect in-view center point

• Component: WebDriver
• Status: RESOLVED FIXED

[BJ Burg]

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

[BJ Burg]

<rdar://problem/48737122>

[BJ Burg]

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

[BJ Burg]

Created attachment 367478 [details]
WIP v2

[BJ Burg]

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

Attachment 367478 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:586:  elementBounds_frame_client is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:587:  elementBounds_mainframe_root is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:588:  elementBounds_mainframe_document is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:589:  elementBounds_mainframe_viewport is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:629:  inViewCenterPoint_mainframe_root is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:630:  inViewCenterPoint_mainframe_document is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:631:  inViewCenterPoint_mainframe_viewport is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 7 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[BJ Burg]

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.

[BJ Burg]

Created attachment 369207 [details]
Patch

[EWS Watchlist]

Comment on attachment 369207 [details]
Patch

Attachment 369207 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12120114

New failing tests:
legacy-animation-engine/compositing/reflections/load-video-in-reflection.html

[EWS Watchlist]

Created attachment 369241 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Devin Rousso]

Created attachment 369584 [details]
Patch

Rebase, plus some minor style changes

[Simon Fraser (smfr)]

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]

Created attachment 369659 [details]
Patch

[EWS Watchlist]

Comment on attachment 369659 [details]
Patch

Attachment 369659 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12169065

New failing tests:
http/tests/misc/repeat-open-cancel.html

[EWS Watchlist]

Created attachment 369676 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Devin Rousso]

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]

Created attachment 369812 [details]
Patch

[Darin Adler]

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]

Created attachment 369913 [details]
Patch

[EWS Watchlist]

Attachment 369913 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:609:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Devin Rousso]

Created attachment 369936 [details]
Patch

[EWS Watchlist]

Attachment 369936 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:609:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 369936 [details]
Patch

Clearing flags on attachment: 369936

Committed r245320: <https://trac.webkit.org/changeset/245320>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.