[chromedriver] Use center of first ClientRect when moving mouse to an element.

[chromedriver] Use center of first ClientRect when moving mouse to an element.

Previously we used the center of the bounding box, but this can cause problems if the center of the bounding box is outside the element.

This CL also changes the behavior of the GetElementLocationOnceScrolledIntoView command in the same way.

BUG=

Committed: https://crrev.com/089c3ddeec583fd9e11564727a9695b70657e681
Cr-Commit-Position: refs/heads/master@{#311423}

[samuong, 2015-01-12 04:22:13]

samuong@chromium.org changed reviewers:
+ stgao@chromium.org

[chromium-reviews, 2015-01-12 22:11:10]

Thanks Sam! Two things:

1) didn't we want to move to the center (of the first ClientRect) when no
offset is specified (not even [0,0]) but start from the top-left (of the
first ClientRect) when an offset is specified (so, for example, it would be
exactly top-left corner if the offset was 0,0) ?

2) in the description you say "match the behavior of FirefoxDriver" because
you know that FirefoxDriver uses the first ClientRect? (I actually don't
know if it does, I only experimentally know that one test passed on
FirefoxDriver somehow)

Seva

On Sun, Jan 11, 2015 at 8:22 PM, <samuong@chromium.org> wrote:

> Reviewers: Shuotao,
>
> Description:
> [chromedriver] Use center of first ClientRect when moving mouse to an
> element.
>
> Previously we used the center of the bounding box, but this can cause
> problems
> if the center of the bounding box is outside the element. This makes
> ChromeDriver match the behavior of FirefoxDriver.
>
> This CL also changes the behavior of the GetElementLocationOnceScrolled
> IntoView
> command in the same way.
>
> BUG=
>
> Please review this at https://codereview.chromium.org/848483002/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+54, -17 lines):
>   M chrome/test/chromedriver/element_commands.cc
>   M chrome/test/chromedriver/element_util.h
>   M chrome/test/chromedriver/element_util.cc
>   M chrome/test/chromedriver/test/run_py_tests.py
>   M chrome/test/chromedriver/window_commands.cc
>   A chrome/test/data/chromedriver/multiline.html
>
>
> Index: chrome/test/chromedriver/element_commands.cc
> diff --git a/chrome/test/chromedriver/element_commands.cc
> b/chrome/test/chromedriver/element_commands.cc
> index f6907343ef83a6bbde96407bb325fc73ab2b28d0..
> 3eb07a2b8979e0f6ce6fc2b7b1289ee5741ba1fc 100644
> --- a/chrome/test/chromedriver/element_commands.cc
> +++ b/chrome/test/chromedriver/element_commands.cc
> @@ -445,7 +445,7 @@ Status ExecuteGetElementLocationOnceScrolledIntoView(
>      scoped_ptr<base::Value>* value) {
>    WebPoint location;
>    Status status = ScrollElementIntoView(
> -      session, web_view, element_id, &location);
> +      session, web_view, element_id, nullptr, &location);
>    if (status.IsError())
>      return status;
>    value->reset(CreateValueFrom(location));
> Index: chrome/test/chromedriver/element_util.cc
> diff --git a/chrome/test/chromedriver/element_util.cc
> b/chrome/test/chromedriver/element_util.cc
> index 9f02b4956cd43e6aef989d993487bdafaf12c483..
> e8714d3d33ef980e43ef819db7bf1560e687bc5e 100644
> --- a/chrome/test/chromedriver/element_util.cc
> +++ b/chrome/test/chromedriver/element_util.cc
> @@ -577,14 +577,21 @@ Status ScrollElementIntoView(
>      Session* session,
>      WebView* web_view,
>      const std::string& id,
> +    const WebPoint* offset,
>      WebPoint* location) {
> -  WebSize size;
> -  Status status = GetElementSize(session, web_view, id, &size);
> +  WebRect region;
> +  Status status = GetElementRegion(session, web_view, id, &region);
>    if (status.IsError())
>      return status;
> -  return ScrollElementRegionIntoView(
> -      session, web_view, id, WebRect(WebPoint(0, 0), size),
> +  status = ScrollElementRegionIntoView(session, web_view, id, region,
>        false /* center */, std::string(), location);
> +  if (status.IsError())
> +    return status;
> +  if (offset)
> +    location->Offset(offset->x, offset->y);
> +  else
> +    location->Offset(region.size.width / 2, region.size.height / 2);
> +  return Status(kOk);
>  }
>
>  Status ScrollElementRegionIntoView(
> Index: chrome/test/chromedriver/element_util.h
> diff --git a/chrome/test/chromedriver/element_util.h
> b/chrome/test/chromedriver/element_util.h
> index 0fbd766179b2d9c1689e76d5d7478fcb1577936c..
> 142fd646c7801a11b24b556ee5d0c3436fcf54ab 100644
> --- a/chrome/test/chromedriver/element_util.h
> +++ b/chrome/test/chromedriver/element_util.h
> @@ -131,6 +131,7 @@ Status ScrollElementIntoView(
>      Session* session,
>      WebView* web_view,
>      const std::string& element_id,
> +    const WebPoint* offset,
>      WebPoint* location);
>
>  // |element_id| refers to the element which is to be scrolled into view.
> Index: chrome/test/chromedriver/test/run_py_tests.py
> diff --git a/chrome/test/chromedriver/test/run_py_tests.py
> b/chrome/test/chromedriver/test/run_py_tests.py
> index e41c0268b82f07d582f59d5ba933dd37ff0429ab..
> fd46b0652a1a92994eafcfc67457b38279b735e3 100755
> --- a/chrome/test/chromedriver/test/run_py_tests.py
> +++ b/chrome/test/chromedriver/test/run_py_tests.py
> @@ -562,6 +562,29 @@ class ChromeDriverTest(ChromeDriverBaseTest):
>      self._driver.MouseMoveTo(div, 10, 10)
>      self.assertEquals(1, len(self._driver.FindElements('tag name',
> 'br')))
>
> +  def testMoveToElementAndClick(self):
> +    self._driver.Load(self.GetHttpUrlForFile('/
> chromedriver/multiline.html'))
> +
> +    # Check that link element spans two lines and that the first
> ClientRect is
> +    # above the second.
> +    link = self._driver.FindElements('tag name', 'a')[0]
> +    client_rects = self._driver.ExecuteScript(
> +        'return arguments[0].getClientRects();', link)
> +    self.assertEquals(2, len(client_rects))
> +    self.assertTrue(client_rects[0]['bottom'] < client_rects[1]['top'])
> +
> +    # Check that the center of the link's bounding ClientRect is outside
> the
> +    # element.
> +    bounding_client_rect = self._driver.ExecuteScript(
> +        'return arguments[0].getBoundingClientRect();', link)
> +    center = bounding_client_rect['left'] + bounding_client_rect['width']
> / 2
> +    self.assertTrue(client_rects[1]['right'] < center)
> +    self.assertTrue(center < client_rects[0]['left'])
> +
> +    self._driver.MouseMoveTo(link)
> +    self._driver.MouseClick()
> +    self.assertTrue(self._driver.GetCurrentUrl().endswith('#top'))
> +
>    def testMouseClick(self):
>      div = self._driver.ExecuteScript(
>          'document.body.innerHTML = "<div>old</div>";'
> Index: chrome/test/chromedriver/window_commands.cc
> diff --git a/chrome/test/chromedriver/window_commands.cc
> b/chrome/test/chromedriver/window_commands.cc
> index 8a62f896d254f1369b267d6aa464c025cc20bf4f..
> 9a3c40512fa32d40afa8c6e46dcc08131266fa7d 100644
> --- a/chrome/test/chromedriver/window_commands.cc
> +++ b/chrome/test/chromedriver/window_commands.cc
> @@ -470,22 +470,15 @@ Status ExecuteMouseMoveTo(
>
>    WebPoint location;
>    if (has_element) {
> -    Status status = ScrollElementIntoView(
> -        session, web_view, element_id, &location);
> +    WebPoint offset(x_offset, y_offset);
> +    Status status = ScrollElementIntoView(session, web_view, element_id,
> +        has_offset ? &offset : nullptr, &location);
>      if (status.IsError())
>        return status;
>    } else {
>      location = session->mouse_position;
> -  }
> -
> -  if (has_offset) {
> -    location.Offset(x_offset, y_offset);
> -  } else {
> -    WebSize size;
> -    Status status = GetElementSize(session, web_view, element_id, &size);
> -    if (status.IsError())
> -      return status;
> -    location.Offset(size.width / 2, size.height / 2);
> +    if (has_offset)
> +      location.Offset(x_offset, y_offset);
>    }
>
>    std::list<MouseEvent> events;
> Index: chrome/test/data/chromedriver/multiline.html
> diff --git a/chrome/test/data/chromedriver/multiline.html
> b/chrome/test/data/chromedriver/multiline.html
> new file mode 100644
> index 0000000000000000000000000000000000000000..
> c7bf5ffab1f27e9bdb9afe4a2ab386486ceea814
> --- /dev/null
> +++ b/chrome/test/data/chromedriver/multiline.html
> @@ -0,0 +1,13 @@
> +<!DOCTYPE html>
> +<html lang="en">
> +  <head>
> +    <meta charset="utf-8">
> +    <title>A multi-line link</title>
> +  </head>
> +  <body>
> +    <p style="max-width: 11em">
> +      This test page contains <a href="#top">a link</a> that spans two
> lines.
> +      The center of the bounding box of the link is not inside the link
> element.
> +    </p>
> +  </body>
> +</html>
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stgao, 2015-01-14 01:47:20]

Overall, the code lgtm.

It is good to support those corner cases.

https://codereview.chromium.org/848483002/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/element_util.cc (right):

https://codereview.chromium.org/848483002/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/element_util.cc:583: Status status =
GetElementRegion(session, web_view, id, &region);
Not sure if there will be side-effect on screenshot or the like. What's the
difference between GetElementRegion and GetElementSize?

https://codereview.chromium.org/848483002/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/element_util.cc:593: location->Offset(region.size.width
/ 2, region.size.height / 2);
It may worth a check if this change has unexpected side-effect, because it
changed the returned location of this function (compared to without this patch).

[samuong, 2015-01-14 04:35:05]

Seva, for 1) we'll be adding the offsets if there are any (see
element_util.cc:591) and using the center of the first ClientRect if there isn't
(see element_util.cc:593). For 2) I might've been wrong about the behavior of
FirefoxDriver so I've removed this from the description. I think I misread your
comment from your other bug report...

https://codereview.chromium.org/848483002/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/element_util.cc (right):

https://codereview.chromium.org/848483002/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/element_util.cc:583: Status status =
GetElementRegion(session, web_view, id, &region);
On 2015/01/14 01:47:20, Shuotao wrote:
> Not sure if there will be side-effect on screenshot or the like. What's the
> difference between GetElementRegion and GetElementSize?

GetElementRegion gets a WebRect (i.e. both a WebPoint and a WebSize), and
GetElementSize just gets a WebSize. There are edge cases where these functions
fall back to other behavior, but this is what they do in the usual case:

The size of the size from GetElementSize() is the size of the whole element's
bounding box (as determined by getBoundingClientRects()) while the size from
GetElementRegion() is just the size of the first ClientRect from
getClientRects().

The WebPoint from GetElementRegion is the top-left corner of the first
ClientRect, relative to the top-left corner of the bounding box.

I think there's scope to simplify this, but I want to wait until after the spec
is more concrete on how to handle this.

https://codereview.chromium.org/848483002/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/element_util.cc:593: location->Offset(region.size.width
/ 2, region.size.height / 2);
On 2015/01/14 01:47:20, Shuotao wrote:
> It may worth a check if this change has unexpected side-effect, because it
> changed the returned location of this function (compared to without this
patch).

I will be running some presubmits later to find as many of these as possible.

[samuong, 2015-01-14 05:38:46]

The CQ bit was checked by samuong@chromium.org

[commit-bot: I haz the power, 2015-01-14 05:38:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/848483002/40001

[commit-bot: I haz the power, 2015-01-14 07:04:24]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-14 07:05:26]

Patchset 3 (id:??) landed as
https://crrev.com/089c3ddeec583fd9e11564727a9695b70657e681
Cr-Commit-Position: refs/heads/master@{#311423}

[chromium-reviews, 2015-01-14 09:08:20]

Ah, I guess you got it right with/without offsets. I didn't read your
change carefully enough before, somehow.. So it sounds reasonable.

I am now thinking about two edge cases:
1. Element is much lager than browser's viewport. In that case if we are
trying to scroll the center of the element (or its first clientrect) into
view, we may want to pass true /* center */ to the
ScrollElementRegionIntoView atom, and not false /* center */.

2. What if the offset (x,y) that user passed has one or both negative
coordinates? Say (-1, -1). Don't we want to scroll into view the
(elementTop-1, elementLeft-1) point of the page? (or, as usual, as close to
that point as possible)

On Tue, Jan 13, 2015 at 8:35 PM, <samuong@chromium.org> wrote:

> Seva, for 1) we'll be adding the offsets if there are any (see
> element_util.cc:591) and using the center of the first ClientRect if there
> isn't
> (see element_util.cc:593). For 2) I might've been wrong about the behavior
> of
> FirefoxDriver so I've removed this from the description. I think I misread
> your
> comment from your other bug report...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.