Bug 195696 - Web Automation: elements larger than the viewport have incorrect in-view center point
Summary: Web Automation: elements larger than the viewport have incorrect in-view cent...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebDriver (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brian Burg
URL:
Keywords: InRadar
Depends on:
Blocks: 197896
  Show dependency treegraph
 
Reported: 2019-03-13 13:25 PDT by Brian Burg
Modified: 2019-05-15 01:37 PDT (History)
14 users (show)

See Also:


Attachments
WIP patch (12.85 KB, patch)
2019-03-14 13:54 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
WIP v2 (31.13 KB, patch)
2019-04-15 17:14 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Patch (35.35 KB, patch)
2019-05-06 17:01 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews211 for win-future (13.67 MB, application/zip)
2019-05-06 23:31 PDT, Build Bot
no flags Details
Patch (37.90 KB, patch)
2019-05-10 13:06 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (38.12 KB, patch)
2019-05-11 13:14 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews214 for win-future (13.78 MB, application/zip)
2019-05-12 09:48 PDT, Build Bot
no flags Details
Patch (38.16 KB, patch)
2019-05-13 19:31 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (36.98 KB, patch)
2019-05-14 17:36 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (38.78 KB, patch)
2019-05-15 00:50 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 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
Comment 1 Brian Burg 2019-03-13 13:26:11 PDT
<rdar://problem/48737122>
Comment 2 Brian Burg 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.
Comment 3 Simon Fraser (smfr) 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.
Comment 4 Brian Burg 2019-04-15 17:14:15 PDT
Created attachment 367478 [details]
WIP v2
Comment 5 Brian Burg 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.
Comment 6 Build Bot 2019-04-15 17:17:41 PDT Comment hidden (obsolete)
Comment 7 Brian Burg 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.
Comment 8 Brian Burg 2019-05-06 17:01:36 PDT
Created attachment 369207 [details]
Patch
Comment 9 Build Bot 2019-05-06 23:31:43 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2019-05-06 23:31:45 PDT Comment hidden (obsolete)
Comment 11 Devin Rousso 2019-05-10 13:06:05 PDT
Created attachment 369584 [details]
Patch

Rebase, plus some minor style changes
Comment 12 Simon Fraser (smfr) 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.
Comment 13 Devin Rousso 2019-05-11 13:14:17 PDT
Created attachment 369659 [details]
Patch
Comment 14 Build Bot 2019-05-12 09:48:53 PDT Comment hidden (obsolete)
Comment 15 Build Bot 2019-05-12 09:48:56 PDT Comment hidden (obsolete)
Comment 16 Devin Rousso 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.
Comment 17 Devin Rousso 2019-05-13 19:31:13 PDT
Created attachment 369812 [details]
Patch
Comment 18 Darin Adler 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
Comment 19 Devin Rousso 2019-05-14 17:36:50 PDT
Created attachment 369913 [details]
Patch
Comment 20 Build Bot 2019-05-14 17:39:09 PDT Comment hidden (obsolete)
Comment 21 Devin Rousso 2019-05-15 00:50:15 PDT
Created attachment 369936 [details]
Patch
Comment 22 Build Bot 2019-05-15 00:52:47 PDT Comment hidden (obsolete)
Comment 23 WebKit Commit Bot 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>
Comment 24 WebKit Commit Bot 2019-05-15 01:37:10 PDT
All reviewed patches have been landed.  Closing bug.