Bug 179129 – Web Automation: inViewCenterPoint should not include topContentInset when computed in viewport coordinates

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

This blocks adoption of inViewCenterPoint algorithm by safaridriver. Adopting it is a regression without fixing this.

[Blaze Burg]

Created attachment 325595 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/35297038>

[Simon Fraser (smfr)]

Comment on attachment 325595 [details]
Patch

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

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:578
>      if (containerElement) {

The code above uses coreElement->clientRect(). Note the comment above it:

// Note that this is not web-exposed, and does not use the same coordinate system as getBoundingClientRect() and friends.
IntRect Element::clientRect() const

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:580
>              inViewCenter = WebCore::IntPoint(coreFrameView->clientToDocumentPoint(clientCenterPoint.value()));

So i think clientToDocumentPoint is the wrong way to convert this point.

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:583
> +                inViewCenter.value().moveBy(WebCore::IntPoint(0, -coreFrameView->topContentInset()));

I never want to see this kind of manual coordinate conversion. We should always go through FrameView helpers like "convert foo to bar".

[Blaze Burg]

Comment on attachment 325595 [details]
Patch

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

>> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:578
>>      if (containerElement) {
> 
> The code above uses coreElement->clientRect(). Note the comment above it:
> 
> // Note that this is not web-exposed, and does not use the same coordinate system as getBoundingClientRect() and friends.
> IntRect Element::clientRect() const

Okay, this should be easy to convert to snappedIntRect(boundingClientRect())...

>> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:580
>>              inViewCenter = WebCore::IntPoint(coreFrameView->clientToDocumentPoint(clientCenterPoint.value()));
> 
> So i think clientToDocumentPoint is the wrong way to convert this point.

...and then clientToDocumentPoint will make sense.

>> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:583
>> +                inViewCenter.value().moveBy(WebCore::IntPoint(0, -coreFrameView->topContentInset()));
> 
> I never want to see this kind of manual coordinate conversion. We should always go through FrameView helpers like "convert foo to bar".

Let me preface by saying I'm super ignorant of layout code. So please humor me, I want to understand better :)

I'm not aware of any existing conversions that go from content rects to what are essentially client rects. The most sensible name I could think up is contentsToClient

[Blaze Burg]

(In reply to Brian Burg from comment #4)
> Comment on attachment 325595 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325595&action=review
> 
> >> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:578
> >>      if (containerElement) {
> > 
> > The code above uses coreElement->clientRect(). Note the comment above it:
> > 
> > // Note that this is not web-exposed, and does not use the same coordinate system as getBoundingClientRect() and friends.
> > IntRect Element::clientRect() const
> 
> Okay, this should be easy to convert to
> snappedIntRect(boundingClientRect())...
> 
> >> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:580
> >>              inViewCenter = WebCore::IntPoint(coreFrameView->clientToDocumentPoint(clientCenterPoint.value()));
> > 
> > So i think clientToDocumentPoint is the wrong way to convert this point.
> 
> ...and then clientToDocumentPoint will make sense.
> 
> >> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:583
> >> +                inViewCenter.value().moveBy(WebCore::IntPoint(0, -coreFrameView->topContentInset()));
> > 
> > I never want to see this kind of manual coordinate conversion. We should always go through FrameView helpers like "convert foo to bar".
> 
> Let me preface by saying I'm super ignorant of layout code. So please humor
> me, I want to understand better :)
> 
> I'm not aware of any existing conversions that go from content rects to what
> are essentially client rects. The most sensible name I could think up is
> contentsToClient

Sorry, comment got clipped off. I looked at this again (and again and again). My notes from today:

For the clientRect (viewport case), I think we can just use the client rect as-is to send back to WebDriver land. In the non-viewport case, I just need to go from client to document/contents rect, I think?

For the inViewCenterPoint (viewport case), we already return a client point so I don't think anything needs to be done, which is why the code is wrong right now. For the non-viewport case, we need to convert to document/contents rect as well.

[Simon Fraser (smfr)]

My general thoughts:

This code has an "useViewportCoordinates" branch, and the question there is what that means. Ideally it would explicitly refer to the visual or layout viewports (probably the second). For that you'd use something like FrameView::documentToClientPoint() because "client" coordinates are nominally layout viewport coordinates. Do this after you've converted the point to the root document/view.

If not viewport coordinates, what are they?

[Simon Fraser (smfr)]

Also, generally contentsToView() is the thing that deals with topContentInset (via documentScrollPositionRelativeToViewOrigin).

[Blaze Burg]

(In reply to Simon Fraser (smfr) from comment #6)
> My general thoughts:
> 
> This code has an "useViewportCoordinates" branch, and the question there is
> what that means. Ideally it would explicitly refer to the visual or layout
> viewports (probably the second). For that you'd use something like
> FrameView::documentToClientPoint() because "client" coordinates are
> nominally layout viewport coordinates. Do this after you've converted the
> point to the root document/view.
> 
> If not viewport coordinates, what are they?

Someday, if we support WebDriver for iOS, then this will need to support returning visual viewport coordinates so that simulated OS events hit on the right thing.

My plan is to proceed to make useViewportCoordinates mean "layout viewport" for now, since that's the only thing I can reasonably test on Mac. When the time comes, and it's reasonable to test in a context where visual viewports are used, I can add a new VisualViewport mode for the command.

[Blaze Burg]

Created attachment 326297 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 326297 [details]
Patch

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

Is this covered by WPT?

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.messages.in:33
> +    ComputeElementLayout(uint64_t pageID, uint64_t frameID, String nodeHandle, bool scrollIntoViewIfNeeded, uint32_t coordinateSystem, uint64_t callbackID)

Can't we use enums?

[Blaze Burg]

Comment on attachment 326297 [details]
Patch

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

>> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.messages.in:33
>> +    ComputeElementLayout(uint64_t pageID, uint64_t frameID, String nodeHandle, bool scrollIntoViewIfNeeded, uint32_t coordinateSystem, uint64_t callbackID)
> 
> Can't we use enums?

I had assumed not, but if it works then I'll switch it.

[Blaze Burg]

(In reply to Simon Fraser (smfr) from comment #10)
> Comment on attachment 326297 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326297&action=review
> 
> Is this covered by WPT?

Not right now. It progresses about 20 Selenium tests for safaridriver.

If we shim WPT browser-internal methods to reuse this code, then it would be tested that way.

[Blaze Burg]

Created attachment 326774 [details]
Patch

Re-EWS after using enum instead of int32_t.

[Blaze Burg]

Committed r224789: <https://trac.webkit.org/changeset/224789>