[webkit-reviews] review denied: [Bug 174863] WebDriver: use in-view center point for clicks instead of bounding box center point : [Attachment 316448] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 174863: WebDriver: use in-view center point for clicks instead of bounding
box center point
https://bugs.webkit.org/show_bug.cgi?id=174863

Attachment 316448: Patch

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




--- Comment #4 from Brian Burg <bburg at apple.com> ---
Comment on attachment 316448
  --> https://bugs.webkit.org/attachment.cgi?id=316448
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?