WebDriver: use in-view center point for clicks instead of bounding box center point

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

The center of the element bounding box is not always part of the element, like in multiline links, for example.

https://w3c.github.io/webdriver/webdriver-spec.html#dfn-in-view-center-point

[Carlos Garcia Campos]

This causes test testCanClickOnALinkThatOverflowsAndFollowIt to fail

___________________________________________________________________ testCanClickOnALinkThatOverflowsAndFollowIt[WebKitGTK] ___________________________________________________________________

driver = <selenium.webdriver.webkitgtk.webdriver.WebDriver (session="074c8710-f8bd-429e-99e2-f4035435f0c8")>

    def testCanClickOnALinkThatOverflowsAndFollowIt(driver):
        driver.find_element(By.ID, "overflowLink").click()
>       WebDriverWait(driver, 3).until(EC.title_is("XHTML Test Page"))

>       raise TimeoutException(message, screen, stacktrace)
E       TimeoutException: Message:

[Carlos Garcia Campos]

Created attachment 316448 [details]
Patch

[Build Bot]

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


ERROR: Source/WebDriver/Session.h:136:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:583:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 9 files


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

[BJ Burg]

Comment on attachment 316448 [details]
Patch

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

r- while we figure out the details of "in-view" and "obscured". For Mac/iOS, we need to make DOMRect.h a private header. I'll put up a revised patch that does that.

> Source/WebKit/UIProcess/Automation/Automation.json:429
> +                { "name": "inViewCenterPoint", "$ref": "Point", "description": "The in-view center point for the requested element. Specified in page or viewport coordinates based on the useViewportCoordinates rameter." }

Nit: rameter -> parameter

I don't like this name, but 'interactableCenterPoint' isn't much better. However, this directly maps to the spec concept, so maybe it should just be left alone. :|

My issue is that the name is inaccurate. For a <span> of text that line wraps, the entire element could be "in-view" and yet the center of the bounding box (i.e., the center point of part of an element that is inside the viewport-in this case) may not necessarily intersect one of the element's client rects.  So it's really inViewCenterPointOfFirstClientRect, or something.

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:523
> +    WebCore::IntPoint inViewCenter;

This is missing the check of whether the element is "in view" at all. According to the spec, if elementsFromPoint(inViewCenterPoint(elem.getClientRects()[0])) does not contain elem, then it is not considered in-view. There is some confusion of how to compute this since it seems kind of circular (the precondition of inViewCenterPoint(e) is that 'e' is in view).

Alternatively, we could ignore this precondition and separately report in the command result whether
- the element is "in view" per spec
- the element is "obscured" per spec

Note: Element.elementsFromPoint just landed in TOT. So we could use it. :)

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:525
> +    if (auto* clientRect = clientRectList->item(0)) {

Is it always the case that the first client rect is within the viewport?

[Carlos Garcia Campos]

(In reply to Brian Burg from comment #4)
> Comment on attachment 316448 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316448&action=review
> 
> r- while we figure out the details of "in-view" and "obscured". For Mac/iOS,
> we need to make DOMRect.h a private header. I'll put up a revised patch that
> does that.
> 
> > Source/WebKit/UIProcess/Automation/Automation.json:429
> > +                { "name": "inViewCenterPoint", "$ref": "Point", "description": "The in-view center point for the requested element. Specified in page or viewport coordinates based on the useViewportCoordinates rameter." }
> 
> Nit: rameter -> parameter
> 
> I don't like this name, but 'interactableCenterPoint' isn't much better.
> However, this directly maps to the spec concept, so maybe it should just be
> left alone. :|

Yes, I simply used the spec name.

> My issue is that the name is inaccurate. For a <span> of text that line
> wraps, the entire element could be "in-view" and yet the center of the
> bounding box (i.e., the center point of part of an element that is inside
> the viewport-in this case) may not necessarily intersect one of the
> element's client rects.  So it's really inViewCenterPointOfFirstClientRect,
> or something.
> 
> > Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:523
> > +    WebCore::IntPoint inViewCenter;
> 
> This is missing the check of whether the element is "in view" at all.

I know, for now I just wanted to get a better click point, and I planned to do the in-view in a follow up. I thought we would that in a previous step, maybe using an js atom to do the check, so I assumed at this point we would know the element is in view for sure.

> According to the spec, if
> elementsFromPoint(inViewCenterPoint(elem.getClientRects()[0])) does not
> contain elem, then it is not considered in-view. There is some confusion of
> how to compute this since it seems kind of circular (the precondition of
> inViewCenterPoint(e) is that 'e' is in view).

elementsFromPoint? Do you mean id the center point is obscured?

> Alternatively, we could ignore this precondition and separately report in
> the command result whether
> - the element is "in view" per spec
> - the element is "obscured" per spec
> 
> Note: Element.elementsFromPoint just landed in TOT. So we could use it. :)

Ah, I'll check it.

> > Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:525
> > +    if (auto* clientRect = clientRectList->item(0)) {
> 
> Is it always the case that the first client rect is within the viewport?

I assumed that at this point scrollIntoView has always been called. Maybe we should only return the in view center point when scroll view parameter is true.

[Carlos Garcia Campos]

Created attachment 316641 [details]
Patch

[Build Bot]

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


ERROR: Source/WebDriver/Session.h:136:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:493:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebDriver/Session.cpp:583:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 11 files


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

[Carlos Garcia Campos]

(In reply to Brian Burg from comment #4)
> Comment on attachment 316448 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316448&action=review
> 
> r- while we figure out the details of "in-view" and "obscured". For Mac/iOS,
> we need to make DOMRect.h a private header. I'll put up a revised patch that
> does that.
> 
> > Source/WebKit/UIProcess/Automation/Automation.json:429
> > +                { "name": "inViewCenterPoint", "$ref": "Point", "description": "The in-view center point for the requested element. Specified in page or viewport coordinates based on the useViewportCoordinates rameter." }
> 
> Nit: rameter -> parameter
> 
> I don't like this name, but 'interactableCenterPoint' isn't much better.
> However, this directly maps to the spec concept, so maybe it should just be
> left alone. :|
> 
> My issue is that the name is inaccurate. For a <span> of text that line
> wraps, the entire element could be "in-view" and yet the center of the
> bounding box (i.e., the center point of part of an element that is inside
> the viewport-in this case) may not necessarily intersect one of the
> element's client rects.  So it's really inViewCenterPointOfFirstClientRect,
> or something.
> 
> > Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:523
> > +    WebCore::IntPoint inViewCenter;
> 
> This is missing the check of whether the element is "in view" at all.
> According to the spec, if
> elementsFromPoint(inViewCenterPoint(elem.getClientRects()[0])) does not
> contain elem, then it is not considered in-view. There is some confusion of
> how to compute this since it seems kind of circular (the precondition of
> inViewCenterPoint(e) is that 'e' is in view).

Well, actually it's possible in case the element has a hidden visibility in its style. In that case we might have the client rect in the view, but elementsFromPoint() will not include the element in the list.

> Alternatively, we could ignore this precondition and separately report in
> the command result whether
> - the element is "in view" per spec
> - the element is "obscured" per spec

In the end I've made the inViewCenterPoint optional, so when not present is because the element is not in the view (not interactable) and added isObscured to report the click intercepted error. This fixed several visibility tests too.

> Note: Element.elementsFromPoint just landed in TOT. So we could use it. :)

Yes, perfect timing!

> > Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:525
> > +    if (auto* clientRect = clientRectList->item(0)) {
> 
> Is it always the case that the first client rect is within the viewport?

This is now caught by elementsFromPoint() :-)

[Carlos Garcia Campos]

Created attachment 316772 [details]
Try to fix mac builds

[Build Bot]

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


ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:493:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebDriver/Session.cpp:583:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.h:136:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 16 files


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

[BJ Burg]

Comment on attachment 316772 [details]
Try to fix mac builds

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

r=me

Does this progress any Selenium python tests?

> Source/WebDriver/Session.cpp:1043
> +        if (!inViewCenter) {

Very nice!

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:477
> +    // §11.1 Element Interactability.

Very nice.

[Radar WebKit Bug Importer]

<rdar://problem/33626463>

[Simon Fraser (smfr)]

Comment on attachment 316772 [details]
Try to fix mac builds

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

> Source/WebCore/page/FrameView.h:476
> +    WEBCORE_EXPORT FloatSize documentToClientOffset() const;

I would prefer not to export this, but to export something that does the coordinate conversion you want (which I think is "client to contents point").

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:553
> +        centerPoint.move(-clientOffset);
> +        inViewCenter = WebCore::IntPoint(centerPoint);
> +
> +        if (useViewportCoordinates)
> +            inViewCenter = coreFrameView->contentsToRootView(inViewCenter.value());
> +    }

I'm not sure this is correct with page scale and page zoom. It's also worth testing in an RTL document.

[Carlos Garcia Campos]

(In reply to Brian Burg from comment #11)
> Comment on attachment 316772 [details]
> Try to fix mac builds
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316772&action=review
> 
> r=me
> 
> Does this progress any Selenium python tests?

Indeed.

-FAIL test/selenium/webdriver/common/click_tests.py::testCanClickOnALinkThatOverflowsAndFollowIt[WebKitGTK]
-FAIL test/selenium/webdriver/common/click_tests.py::testShouldThrowExceptionIfElementIsObscured[WebKitGTK]

This is not part of selenium, I added it manually to check the isObscured. It uses the example from the spec and then tries to click the button checking that exception should be thrown.

-FAIL test/selenium/webdriver/common/form_handling_tests.py::testShouldThrowAnExceptionWhenSelectingAnUnselectableElement[WebKitGTK]
-FAIL test/selenium/webdriver/common/visibility_tests.py::testShouldNotBeAbleToClickOnAnElementThatIsNotDisplayed[WebKitGTK]
-FAIL test/selenium/webdriver/common/visibility_tests.py::testShouldNotBeAbleToToggleAnElementThatIsNotDisplayed[WebKitGTK]
-FAIL test/selenium/webdriver/common/visibility_tests.py::testShouldNotBeAbleToSelectAnElementThatIsNotDisplayed[WebKitGTK]
-FAIL test/selenium/webdriver/support/event_firing_webdriver_tests.py::test_should_fire_click_event[WebKitGTK]


> > Source/WebDriver/Session.cpp:1043
> > +        if (!inViewCenter) {
> 
> Very nice!
> 
> > Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:477
> > +    // §11.1 Element Interactability.
> 
> Very nice.

[Carlos Garcia Campos]

Created attachment 316934 [details]
Updated patch

[Build Bot]

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


ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:493:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebDriver/Session.cpp:583:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.h:136:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 17 files


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

[Carlos Garcia Campos]

Brian, Simon, could you review this again, please?

[BJ Burg]

Comment on attachment 316934 [details]
Updated patch

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

LGTM, Simon can you take another glance at coordinate-related code?

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:498
> +        // Element is not the first one in the list.

Very nice.

[Carlos Garcia Campos]

Committed r220314: <http://trac.webkit.org/changeset/220314>