Specify scrollIntoView arguments when clicking element

RESOLVED FIXED in Firefox 51

Status

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: ato, Assigned: ato)

Tracking

(Blocks 1 bug)

Version 3
mozilla53
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox51 fixed, firefox52 fixed, firefox53 fixed)

Details

Attachments

(6 attachments)

(Assignee)

Description

3 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 years ago
Assignee: nobody → ato
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Blocks: webdriver
(Assignee)

Comment 5

3 years ago
mozreview-review
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`.
(Assignee)

Comment 6

3 years ago
mozreview-review
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`.
(Assignee)

Comment 7

3 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

3 years ago
mozreview-review
Comment on attachment 8810543 [details]
Bug 1317386 - Scroll element into view at the bottom;

https://reviewboard.mozilla.org/r/92798/#review94010
Attachment #8810543 - Flags: review?(dburns) → review+

Comment 23

3 years ago
mozreview-review
Comment on attachment 8810544 [details]
Bug 1317386 - Check pointer interactability upon interaction;

https://reviewboard.mozilla.org/r/92800/#review94014
Attachment #8810544 - Flags: review?(dburns) → review+

Comment 24

3 years ago
mozreview-review
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 *`
Attachment #8810545 - Flags: review?(dburns) → review+

Comment 25

3 years ago
mozreview-review
Comment on attachment 8810546 [details]
Bug 1317386 - Add test for overlay element after scroll;

https://reviewboard.mozilla.org/r/92804/#review94022
Attachment #8810546 - Flags: review?(dburns) → review+

Comment 26

3 years ago
mozreview-review
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

3 years ago
mozreview-review
Comment on attachment 8810808 [details]
Bug 1317386 - Swap expectation of which button causes scroll;

https://reviewboard.mozilla.org/r/93146/#review94034
Attachment #8810808 - Flags: review?(dburns) → review+
(Assignee)

Comment 28

3 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 35

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

https://reviewboard.mozilla.org/r/93144/#review94538
Attachment #8810807 - Flags: review?(dburns) → review+

Comment 36

3 years ago
Pushed by atolfsen@mozilla.com:
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
(Assignee)

Comment 38

3 years ago
Sheriffs: Test-only uplift because Marionette is guarded behind a flag.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
You need to log in before you can comment on or make changes to this bug.