Bug 213139 - Automation.computeElementLayout should return iframe-relative element rects (when coordinate system is 'Page')
Summary: Automation.computeElementLayout should return iframe-relative element rects (...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebDriver (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-12 10:44 PDT by BJ Burg
Modified: 2020-06-22 20:50 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.80 KB, patch)
2020-06-12 15:30 PDT, BJ Burg
hi: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.