WebDriver on non-iOS ports cannot perform ActionChain which has scrolling down to the element and click it

• Component: WebDriver
• Status: RESOLVED FIXED

[Tomoki Imai]

Created attachment 391731 [details]
HTML and selenium script to reproduce the issue

Summary:

WebDriver on non-iOS ports cannot perform ActionChains which scrolls down to the element and click it.
Our investigation suggests that the issue comes from convertRectFromFrameClientToRootView, which was added in bug 195696.
convertRectFromFrameClientToRootView and the other calculation looks correct when frame delegates scrolling, but not when it doesn't delegate scrolling.

How to reproduce:
- Build WebKitGTK
- Unzip the attached package. It contains HTML file long-many-links.html and selenium script actionchains.py.
- Replace "/path_to_your_webkit_repo" in actionchains.py with your preference.
- Host long-many-links.html in your localhost:8080. You can edit actionchains.py if you want to use the other setting.
- Run actionchains.py with python3 selenium installed.

Expected behavior:
The script scrolls down the webpage, and then clicks "link 10".
No exceptions occurs.

Actual behavior:
The script scrolls down the webpage, and MoveTargetOutOfBoundsException occurs.
<class 'selenium.common.exceptions.MoveTargetOutOfBoundsException'>
('', None, None)

Environment:
- Ubuntu 18.04
- WebKitGTK trunk r257372
- python 3.6.9
- selenium 3.141.0 (installed by pip)

Our investigation:
We first noticed that "resultElementBounds" and "resultInViewCenterPoint" has negative y or some smaller value than expected.
It looks like we adjust the y-value by scroll position twice.
https://trac.webkit.org/browser/webkit/trunk/Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp#L646

Next thing we see is that "convertRectFromFrameClientToRootView" function. As the name suggested, we expect that it returns RootView coordinate.
When the frame delegates scrolling, it seems to calculate an absolute coordinate from layout coordinate.
The returned value, for example "elementBoundsInRootCoordinates",  is passed to "absoluteTo..." functions.
https://trac.webkit.org/browser/webkit/trunk/Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp#L585
On the other hand, when frame doesn't delegate scrolling, we suspect that "convertRectFromFrameClientToRootView" returns RootView coordinate.
RootView coordinate is not documented, but we guess it's coordinate which (0, 0) is the left-top corner of the root widget, which is different from absolute coordinate.
https://trac.webkit.org/browser/webkit/trunk/Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp#L585

So our suspicion is that this issue is caused by the difference of which coordinates "convertRectFromFrameClientToRootView", depending on delegating scrolling.

[Radar WebKit Bug Importer]

<rdar://problem/59859491>

[BJ Burg]

Thanks for the detailed investigation! I will try this test case on Mac to see if it's affected. If so, this may be related or the same as some other Perform Actions bugs that are currently open. Otherwise, it may be specific to delegated scrolling and I may not be able to directly validate a fix.


(It would be great to have WPT WebDriver test results for WebKit EWS...)

[BJ Burg]

I am testing out a patch for this.

[BJ Burg]

Created attachment 401569 [details]
Patch

[Devin Rousso]

Comment on attachment 401569 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401569&action=review

r=me

> Source/WebKit/UIProcess/Automation/mac/WebAutomationSessionMac.mm:140
> +    IntPoint locationInView = WebCore::IntPoint(locationInViewport.x(), locationInViewport.y() + page.topContentInset());

Do we want to do this in a caller function?

Aside: this code makes me wish there was a `IntPoint IntPoint::move(int dx, int dy);` that does this :(

[Darin Adler]

Comment on attachment 401569 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401569&action=review

>> Source/WebKit/UIProcess/Automation/mac/WebAutomationSessionMac.mm:140
>> +    IntPoint locationInView = WebCore::IntPoint(locationInViewport.x(), locationInViewport.y() + page.topContentInset());
> 
> Do we want to do this in a caller function?
> 
> Aside: this code makes me wish there was a `IntPoint IntPoint::move(int dx, int dy);` that does this :(

Seems strange to have both IntPoint and WebCore::IntPoint on the same line. Either we need to specify the namespace or we don’t.

Here’s another way to write this:

    auto locationInView = locationInViewport) + IntPoint { 0, page.topContentInset() };

Or could use IntSize instead of IntPoint for the right hand size of the +. Sadly can’t write it without specifying a type at all.

[Darin Adler]

Comment on attachment 401569 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401569&action=review

>>> Source/WebKit/UIProcess/Automation/mac/WebAutomationSessionMac.mm:140
>>> +    IntPoint locationInView = WebCore::IntPoint(locationInViewport.x(), locationInViewport.y() + page.topContentInset());
>> 
>> Do we want to do this in a caller function?
>> 
>> Aside: this code makes me wish there was a `IntPoint IntPoint::move(int dx, int dy);` that does this :(
> 
> Seems strange to have both IntPoint and WebCore::IntPoint on the same line. Either we need to specify the namespace or we don’t.
> 
> Here’s another way to write this:
> 
>     auto locationInView = locationInViewport) + IntPoint { 0, page.topContentInset() };
> 
> Or could use IntSize instead of IntPoint for the right hand size of the +. Sadly can’t write it without specifying a type at all.

auto locationInView = locationInViewport + IntPoint { 0, page.topContentInset() };

[BJ Burg]

Committed r262861: <https://trac.webkit.org/changeset/262861>

[BJ Burg]

*** Bug 212521 has been marked as a duplicate of this bug. ***