Bug 213139

Summary: Automation.computeElementLayout should return iframe-relative element rects (when coordinate system is 'Page')
Product: WebKit Reporter: BJ Burg <bburg>
Component: WebDriverAssignee: BJ Burg <bburg>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, cgarcia, hi, lmoura, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch hi: review+

Description BJ Burg 2020-06-12 10:44:29 PDT 
Currently it's relative to top-level browsing context, which does not match the non-normative note (https://w3c.github.io/webdriver/#get-element-rect):

"""
The Get Element Rect command returns the dimensions and coordinates of the given web element. The returned value is a dictionary with the following members:

x
X axis position of the top-left corner of the web element relative to the current browsing context’s document element in CSS pixels.
y
Y axis position of the top-left corner of the web element relative to the current browsing context’s document element in CSS pixels.
"""

However the remote end says to "let coordinates be the result of the 'calculate the absolute position' algorithm, which does not seem to align with the note in the case where the main frame is scrolled:

"""
To calculate the absolute position of element:
1. Let rect be the value returned by calling getBoundingClientRect.
2. Let window be the associated window of current top-level browsing context.
3. Let x be (scrollX of window + rect’s x coordinate).
4. Let y be (scrollY of window + rect’s y coordinate).
5. Return a pair of (x, y).
"""

It seems to me that adding the scrollX of the top window is unnecessary to achieve the desired result.

In any case other browsers return iframe-relative coordinates from Get Element Rect.
Comment 1 BJ Burg 2020-06-12 10:45:15 PDT 
<rdar://problem/55042445>
Comment 2 BJ Burg 2020-06-12 11:02:30 PDT 
I filed a spec bug to clarify here: https://github.com/w3c/webdriver/issues/1533
Comment 3 BJ Burg 2020-06-12 11:03:57 PDT 
I think the correct fix is to alter the semantics of the 'Page' coordinate system to provide iframe-relative coordinates. The LayoutViewport coordinate system is designed to provide top-level browsing context client coordinates that would be usable for Element Click / Actions chains.
Comment 4 BJ Burg 2020-06-12 11:04:26 PDT 
(In reply to Brian Burg from comment #3)
> I think the correct fix is to alter the semantics of the 'Page' coordinate
> system to provide iframe-relative coordinates. The LayoutViewport coordinate
> system is designed to provide top-level browsing context client coordinates
> that would be usable for Element Click / Actions chains.

Based on an audit of uses in webkitgtkdriver and safaridriver, this change should not regress any other endpoints.
Comment 5 BJ Burg 2020-06-12 12:07:36 PDT 
The obvious fix to use the equivalent of elem.getBoundingClientRect() seems to do the trick so far, running some more tests to check for fallout.
Comment 6 BJ Burg 2020-06-12 15:30:07 PDT 
Created attachment 401794 [details]
Patch
Comment 7 Devin Rousso 2020-06-12 15:58:51 PDT 
Comment on attachment 401794 [details]
Patch

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

r=me, nice fix :)

> Source/WebKit/ChangeLog:14
> +        Covered by existing WPT test suite. The semantics of this command are under review, as

I always hated "WPT test" ... it's so redundant 😞

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:689
>      case CoordinateSystem::LayoutViewport:

NIT: we should wrap this `case` in `{` and `}` so `elementBoundsInRootCoordinates` is local to just this `case` (i'd also be fine if you inlined it)

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:747
> +        auto inViewCenterPointInRootCoordinates = convertPointFromFrameClientToRootView(frameView, elementInViewCenterPoint);

Ditto (:689)
Comment 8 BJ Burg 2020-06-12 23:22:56 PDT 
Committed r262997: <https://trac.webkit.org/changeset/262997>
Comment 9 Lauro Moura 2020-06-22 20:50:29 PDT 
Two selenium test cases from `position_and_size_tests.py` started failing after this change, but I think we may add them as failures while this spec ambiguity is sorted out.

The first one, testShouldScrollPageAndGetCoordinatesOfAnElementThatIsOutOfViewPort, uses `element.location` and `element.location_once_scrolled_into_view`, and seems to assume the former to reflect the non-normative behavior of returning the location on the page instead on the current browsing context. Similarly, the Java version of the tests finishes with:

assertThat(getLocationOnPage(By.id("box"))).isEqualTo(new Point(10, 5010));

The second one, testShouldGetCoordinatesOfAnElementWithFixedPosition, uses a fixed element originally at y=0, scrolls, and tries to see if its `elem.location` changed (it does not, with the current behavior, as the position in the viewport does not change).

I'll mark them as Fail, linking to this bug for reference.