1317386 - Specify scrollIntoView arguments when clicking element

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Andreas Tolfsen ❲:ato❳]

Marionette needs to specify the arguments with which to call scrollIntoView when clicking an element as defined in https://github.com/w3c/webdriver/pull/440.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1317386 - Scroll element into view at the bottom;

When scrolling an element into view using `Element.scrollIntoView`,
use the `{block: "end", inline: "nearest"}` scroll position arguments,
which are equivalent to `Element.scrollIntoView(false)`.  This is what
other WebDriver implementations have used for a while, and we meant to
change to this sooner.

This ensures that the element appears at the bottom of the viewport
rather than the top, where overlaying menus with fixed style position
may more frequently appear.

In the future we might consider replacing this with `{block: "center"}`
which is specified in the CSSOM specification, but not yet implemented
in any browsers.

This implements https://github.com/w3c/webdriver/pull/440, which should
fix https://github.com/mozilla/geckodriver/issues/327.

Review commit: https://reviewboard.mozilla.org/r/92798/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92798/

Comment 2

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1317386 - Check pointer interactability upon interaction;

Checking for general interactability will also consider keyboard
interactability, which has not yet been implemented.  On interacting with
an element by clicking, we should only test for pointer interactability.

Review commit: https://reviewboard.mozilla.org/r/92800/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92800/

Comment 3

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1317386 - Rewrite in-view centre point calcaulation;

The old calcaulation to determine an element's in-view centre point was
wrong as pointed out in https://github.com/w3c/webdriver/issues/425,
and this is an implementation of the proposed algorithm which passes
real-world tests.

This also addresses https://github.com/w3c/webdriver/pull/441 which
checks if the `DOMRect` sequence returned from `getClientRects` is empty,
as it may be if the element's `display` style property is `none`.

Review commit: https://reviewboard.mozilla.org/r/92802/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92802/

Comment 4

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1317386 - Add test for overlay element after scroll;

Add a test that checks that `scrollIntoView({block: "end", inline:
"nearest"})` works by placing the element at the bottom of the viewport,
where the overlay does not cover it.

Review commit: https://reviewboard.mozilla.org/r/92804/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92804/

Comment 5

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810546 [details]
Bug 1317386 - Add test for overlay element after scroll;

https://reviewboard.mozilla.org/r/92804/#review92920

::: testing/marionette/harness/marionette/tests/unit/test_click.py:46
(Diff revision 1)
> +link.addEventListener("click", () => {
> +  window.clicked = true;
> +  return false;
> +});

```js
link.addEventListener("click", () => window.clicked = true);
```

should probably be sufficient, as the default return value from a function is `undefined` which is falsy.

::: testing/marionette/harness/marionette/tests/unit/test_click.py:97
(Diff revision 1)
> +        link = self.marionette.find_element(By.TAG_NAME, "a")
> +        link.click()
> +        self.assertTrue(self.marionette.execute_script("return window.clicked", sandbox=None))
> +
>  
>  class TestClick(TestLegacyClick):

Add test for new pointer interactable scenario that the legacy click cannot support, e.g. clicking something that is obscured by an element with `position: absolute`.

Comment 6

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810545 [details]
Bug 1317386 - Rewrite in-view centre point calcaulation;

https://reviewboard.mozilla.org/r/92802/#review92918

::: testing/marionette/element.js:958
(Diff revision 1)
> +    top: Math.max(0, Math.min(rect.x, rect.x + rect.width)),
> +    bottom: Math.min(win.innerWidth, Math.max(rect.x, rect.x + rect.width)),

s/top/left/ and s/bottom/right/

::: testing/marionette/element.js:958
(Diff revision 1)
> +    top: Math.max(0, Math.min(rect.x, rect.x + rect.width)),
> +    bottom: Math.min(win.innerWidth, Math.max(rect.x, rect.x + rect.width)),

```js
const {max, min} = Math;
```

::: testing/marionette/element.js:967
(Diff revision 1)
> +    top: Math.max(0, Math.min(rect.y, rect.y + rect.height)),
> +    bottom: Math.min(win.innerHeight, Math.max(rect.y, rect.y + rect.height)),
> +  };
> +
> +  return {
> +    x: 0.5 * (x.top + x.bottom),

s/top/left/ and s/bottom/right/

::: testing/marionette/element.js:1007
(Diff revision 1)
>    // filter out non-interactable elements
>    let rv = [];
>    for (let el of tree) {
>      if (win.getComputedStyle(el).opacity === "1") {
>        rv.push(el);
>      }
>    }

Could probably be compacted using `Array.filter`, which means we could get rid of `rv`.

Comment 7

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810546 [details]
Bug 1317386 - Add test for overlay element after scroll;

https://reviewboard.mozilla.org/r/92804/#review92920

> Add test for new pointer interactable scenario that the legacy click cannot support, e.g. clicking something that is obscured by an element with `position: absolute`.

Addressed in a follow-up patch as I discovered there was a bug with `element.isPointerInteractable` when writing a test for this.  It was looking at whether there were _any_ interactable elements under the cursor instead of comparing the passed in `el` to the top of the stack.

Comment 8

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1317386 - Test pointer interactability of first element in paint order;

When returning the tree of interactable elemenets under the cursor, we
want to test the first one against the passed element, not that there
are _any_ interactable elements under the cursor.

Review commit: https://reviewboard.mozilla.org/r/93144/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93144/

Comment 9

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1317386 - Swap expectation of which button causes scroll;

When we scroll the element into the bottom of the viewport, it is the
first button that does not cause a scroll.

Review commit: https://reviewboard.mozilla.org/r/93146/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93146/

Comment 10

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810543 [details]
Bug 1317386 - Scroll element into view at the bottom;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92798/diff/1-2/

Comment 11

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810544 [details]
Bug 1317386 - Check pointer interactability upon interaction;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92800/diff/1-2/

Comment 12

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810545 [details]
Bug 1317386 - Rewrite in-view centre point calcaulation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92802/diff/1-2/

Comment 13

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810546 [details]
Bug 1317386 - Add test for overlay element after scroll;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92804/diff/1-2/

Comment 14

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810807 [details]
Bug 1317386 - Test pointer interactability of first element in paint order;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93144/diff/1-2/

Comment 15

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810808 [details]
Bug 1317386 - Swap expectation of which button causes scroll;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93146/diff/1-2/

Comment 16

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810543 [details]
Bug 1317386 - Scroll element into view at the bottom;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92798/diff/2-3/

Comment 17

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810544 [details]
Bug 1317386 - Check pointer interactability upon interaction;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92800/diff/2-3/

Comment 18

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810545 [details]
Bug 1317386 - Rewrite in-view centre point calcaulation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92802/diff/2-3/

Comment 19

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810546 [details]
Bug 1317386 - Add test for overlay element after scroll;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92804/diff/2-3/

Comment 20

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810807 [details]
Bug 1317386 - Test pointer interactability of first element in paint order;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93144/diff/2-3/

Comment 21

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810808 [details]
Bug 1317386 - Swap expectation of which button causes scroll;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93146/diff/2-3/

Comment 22

[David Burns :automatedtester]

Comment on attachment 8810543 [details]
Bug 1317386 - Scroll element into view at the bottom;

https://reviewboard.mozilla.org/r/92798/#review94010

Comment 23

[David Burns :automatedtester]

Comment on attachment 8810544 [details]
Bug 1317386 - Check pointer interactability upon interaction;

https://reviewboard.mozilla.org/r/92800/#review94014

Comment 24

[David Burns :automatedtester]

Comment on attachment 8810545 [details]
Bug 1317386 - Rewrite in-view centre point calcaulation;

https://reviewboard.mozilla.org/r/92802/#review94020

::: testing/marionette/element.js:969
(Diff revision 3)
> +    top: max(0, min(rect.y, rect.y + rect.height)),
> +    bottom: min(win.innerHeight, max(rect.y, rect.y + rect.height)),
> +  };
> +
> +  return {
> +    x: 0.5 * (x.left + x.right),

This would read better if it were `/2` instead of `0.5 *`

Comment 25

[David Burns :automatedtester]

Comment on attachment 8810546 [details]
Bug 1317386 - Add test for overlay element after scroll;

https://reviewboard.mozilla.org/r/92804/#review94022

Comment 26

[David Burns :automatedtester]

Comment on attachment 8810807 [details]
Bug 1317386 - Test pointer interactability of first element in paint order;

https://reviewboard.mozilla.org/r/93144/#review94030

::: testing/marionette/harness/marionette/tests/unit/test_click.py:108
(Diff revision 3)
>  
>      @skip("fails with spec compatible interactability checks")
>      def test_clicking_an_element_that_is_not_displayed_raises(self):
>          pass
> +
> +    def test_click_element_obscured_by_absolute_positioned_element(self):

Can you add a test where the element center is just outside the viewport and the element can't be scrolled to.

e.g.

```
<div style=transform:translateX(-105px), width=200px>&nbsp;</div>

```

Comment 27

[David Burns :automatedtester]

Comment on attachment 8810808 [details]
Bug 1317386 - Swap expectation of which button causes scroll;

https://reviewboard.mozilla.org/r/93146/#review94034

Comment 28

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810807 [details]
Bug 1317386 - Test pointer interactability of first element in paint order;

https://reviewboard.mozilla.org/r/93144/#review94030

> Can you add a test where the element center is just outside the viewport and the element can't be scrolled to.
> 
> e.g.
> 
> ```
> <div style=transform:translateX(-105px), width=200px>&nbsp;</div>
> 
> ```

Good idea.

Comment 29

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810543 [details]
Bug 1317386 - Scroll element into view at the bottom;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92798/diff/3-4/

Comment 30

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810544 [details]
Bug 1317386 - Check pointer interactability upon interaction;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92800/diff/3-4/

Comment 31

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810545 [details]
Bug 1317386 - Rewrite in-view centre point calcaulation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92802/diff/3-4/

Comment 32

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810546 [details]
Bug 1317386 - Add test for overlay element after scroll;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92804/diff/3-4/

Comment 33

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810807 [details]
Bug 1317386 - Test pointer interactability of first element in paint order;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93144/diff/3-4/

Comment 34

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8810808 [details]
Bug 1317386 - Swap expectation of which button causes scroll;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93146/diff/3-4/

Comment 35

[David Burns :automatedtester]

Comment on attachment 8810807 [details]
Bug 1317386 - Test pointer interactability of first element in paint order;

https://reviewboard.mozilla.org/r/93144/#review94538

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f44d49bb142
Scroll element into view at the bottom; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/132052948ab1
Check pointer interactability upon interaction; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/59009e044524
Rewrite in-view centre point calcaulation; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/d8ad355ff155
Add test for overlay element after scroll; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/bd737369c386
Test pointer interactability of first element in paint order; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/a2fa061b5903
Swap expectation of which button causes scroll; r=automatedtester

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0f44d49bb142
https://hg.mozilla.org/mozilla-central/rev/132052948ab1
https://hg.mozilla.org/mozilla-central/rev/59009e044524
https://hg.mozilla.org/mozilla-central/rev/d8ad355ff155
https://hg.mozilla.org/mozilla-central/rev/bd737369c386
https://hg.mozilla.org/mozilla-central/rev/a2fa061b5903

Comment 38

[Andreas Tolfsen ❲:ato❳]

Sheriffs: Test-only uplift because Marionette is guarded behind a flag.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/154f73dc0407
https://hg.mozilla.org/releases/mozilla-aurora/rev/6393e7ca57dd
https://hg.mozilla.org/releases/mozilla-aurora/rev/aa77464b82d3
https://hg.mozilla.org/releases/mozilla-aurora/rev/54e23725434e
https://hg.mozilla.org/releases/mozilla-aurora/rev/229a089df414
https://hg.mozilla.org/releases/mozilla-aurora/rev/0d440483f80c

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/406d6147e9df
https://hg.mozilla.org/releases/mozilla-beta/rev/30ffa3343e2e
https://hg.mozilla.org/releases/mozilla-beta/rev/997cba09c701
https://hg.mozilla.org/releases/mozilla-beta/rev/0b360c1f9917
https://hg.mozilla.org/releases/mozilla-beta/rev/d40798d68d03
https://hg.mozilla.org/releases/mozilla-beta/rev/e521208d0ee5