Honor "moz:useNonSpecCompliantPointerOrigin" capability to allow usage of non-spec behavior

RESOLVED FIXED in Firefox 59

Status

defect
P2
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

(Depends on 1 bug, Blocks 1 bug, {regression})

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

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox58 unaffected, firefox59 fixed, firefox60 fixed)

Details

()

Attachments

(5 attachments)

As originally filed:
https://github.com/mozilla/geckodriver/issues/1123

The change on bug 1393831 caused a regression in calculating the center point for a large image inside an "overflow: scroll" div in the following example:

https://output.jsbin.com/moyiqipapa/1

Here the test:

    def test_wrong_center_point(self):
        self.driver.get("https://output.jsbin.com/moyiqipapa/1")
        pic = self.driver.find_element_by_id("picture")
        action = ActionChains(self.driver)
        action.move_to_element_with_offset(pic, 1500, 150).click().perform()

Before the change: (1508, 231.10000610351562)
After the change:  (-578, -1425.1999969482422)

Maja, could you have a look at this regression? It would be great if we don't ship Firefox 58 with it.
Flags: needinfo?(mjzffr)
For now we backed out the changes from bug 1393831 on mozilla-beta (Firefox 58) to not affect users from this API change. We will have to check what specifically is happening here, and if we should allow a fallback via a custom capability.

I will minimize the code soon, so it should become easier to figure out.
Posted file selenium testcase
Here is a stripped down version of the HTML testcase, and the usage of action chains with `move_to_element()` and `move_to_element_with_offset()`.

Both methods should result in the same behavior but the latter always fails with a bounding box failure like:

MoveTargetOutOfBoundsException: Message: (-356, -570.5) is out of bounds of viewport width (1280) and height (851)

I'm not sure where those values are coming from. Maybe Maya can explain. I will attach a tracelog shortly.
Based on the trace log in Comment 3, the call to `action.move_to_element_with_offset(pic, 0, 0).click().perform()` sends a pointerMove command with offset coordinates -1000, -1000: 

> {"duration":250,"origin":{"element-6066-11e4-a52e-4f735466cecf":"5c8ba930-a8e0-4146-ac01-7e41c92469fc"},"type":"pointerMove","x":-1000,"y":-1000}

Why -1000, -1000? My guess is that the Selenium client is calculating how to express the "top-left" coords in terms of the element center point. This doesn't work anymore because, in accordance with the spec, Marionette uses the *in-view* center-point when an element is the origin of a move.

So these -1000 offsets are applied to the in-view center point and thus the pointer destination is (-356, -570.5), which is outside the viewport and we get an error.

Conclusion #1: the selenium client implementation needs to change to be spec-compliant.

In the other call, `action.move_to_element(pic).click().perform()`, the client sense a pointerMove command with offset coordinates 0, 0: 

> {"duration":250,"origin":{"element-6066-11e4-a52e-4f735466cecf":"5c8ba930-a8e0-4146-ac01-7e41c92469fc"},"type":"pointerMove","x":0,"y":0}

Why does the Selenium client send 0, 0 in this case? No idea. I guess they take the absence of an offset to mean "use the element center"? Either way, that's a design decision at the client level and has nothing to do with Marionette.

Conclusion #2: the selenium client has an intentional yet surprising difference in behaviour between no offset coordinates and and offset of 0,0

Going back to our face-to-face discussion from Wednesday, I agree that the right thing to do for now is to hide the spec-compliant "in-view center point" behaviour behind a capability to give users some flexibility while waiting for clients to (hopefully) become spec-compliant, but I would not say that this is a regression in Marionette.
Flags: needinfo?(mjzffr)
Those are great findings, maja_zf.

Your last finding where Selenium explicitly sends an X/Y offset of
0,0 for move_to_element(pic).click(), could be explained by Selenium
previously calculating the move target from the top left corner and
that the shim wants to patch over this inconsistency with WebDriver
conforming implementations.

I can’t find a good explanation for the -1000,-1000 in the first
example however.
The testcase include `<img style="width: 2000px; height: 2000px;"  ...` so I'm guessing that's what the -1000,-1000 is based on.
[Mass Change 2018-01-15] Moving bugs to backlog
Priority: -- → P3
Great find and thank you Maja. As discussed we will take this bug to make sure that Marionette will honor the newly introduced capability with the name "moz:useNonSpecCompliantPointerOrigin", which will be available with the next geckodriver release (see bug 1430123 for details).

The behavior as filed as broken first, we identified and can say for sure that this is what drivers and Selenium binding should handle in the future. But unless at least all Selenium bindings have been updated, we need a way to allow users to turn this new behavior off. 

Geckodriver will pass this capability to Marionette. Now we only have to make sure that Marionette honors it.

As given by Maja that shouldn't be too hard to implement.

Andreas, and David, should we add a specific Marionette unit test to check that the old behavior still works, or should we just ignore and not test it at all? 

This should be a P2 bug so that we don't break people, but can offer this workaround.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Depends on: 1430123
Flags: needinfo?(dburns)
Flags: needinfo?(ato)
Priority: P3 → P2
Summary: Wrong center point calculation of elements inside a "overflow: scroll" container → Honor "moz:useNonSpecCompliantPointerOrigin" capability to allow usage of non-spec behavior
If by old, you mean the spec conformant behaviour, then yes we will
want a test for that in WPT.  I don’t know off hand if the action
tests in WPT covers it.
Flags: needinfo?(ato)
Well, we should. And I will use the minified HTML testcase from this bug for testing that.
Flags: needinfo?(dburns)
(In reply to Andreas Tolfsen ‹:ato› from comment #9)
> If by old, you mean the spec conformant behaviour, then yes we will
> want a test for that in WPT.  I don’t know off hand if the action
> tests in WPT covers it.

By old, I mean the old behavior eq. non conformant. We cannot put those into wpt, so as such have to use Marionette unit tests.
Hm, so I implemented two tests as run by Marionette unit tests, but as I figured out we do not use the new actions but still legacyactions. :/ Is that intended, or why didn't we also update the client to make use of the actions work as done by Maja a while ago? 

So if we want to have fallback tests, this would be a requirement and I don't think that I have time for that.
Flags: needinfo?(mjzffr)
Flags: needinfo?(ato)
Right, the Marionette Python client doesn’t have a client API for
the new actions API because the tests for it all live in WPT.  There
are a few other minor example where the Marionette client is missing
bits and pieces.
Flags: needinfo?(ato)
So there is bug 1354578 to move Marionette unit tests to the new API and get rid of the legacy actions. As said earlier without that work done I cannot write unit tests for this bug, and as such it would have to be pushed untested for now.
Depends on: 1354578
Comment on attachment 8943949 [details]
Bug 1429338 - Marionette has to honor "moz:useNonSpecCompliantPointerOrigin" capability.

https://reviewboard.mozilla.org/r/214284/#review219998

::: testing/marionette/action.js:990
(Diff revision 1)
>      for (let tickActions of chain) {
>        await action.dispatchTickActions(
>            tickActions,
>            action.computeTickDuration(tickActions),
> -          window);
> +          window,
> +          specCompatPointerOrigin);

Not sure if passing this all through is a great idea due to the amount of changes which have to be done here. Maybe we could set a global flag in this file, and individual commands pick this up?
You can use _send_message in your tests with an actions payload (use the payloads from the tracelogs we got from the selenium test/bug report). I think this is a reasonable option since this capability is likely just a temporary measure anyway. Lots of other marionette tests use _send_message, as well.
Flags: needinfo?(mjzffr)
Comment on attachment 8943949 [details]
Bug 1429338 - Marionette has to honor "moz:useNonSpecCompliantPointerOrigin" capability.

https://reviewboard.mozilla.org/r/214284/#review220600


Static analysis found 12 defects in this patch.
 - 12 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/test_session.js:382
(Diff revision 3)
>    ok(caps.has("rotatable"));
>  
>    equal(false, caps.get("moz:accessibilityChecks"));
>    ok(caps.has("moz:processID"));
>    ok(caps.has("moz:profile"));
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:412
(Diff revision 3)
>    equal(caps.get("rotatable"), json.rotatable);
>  
>    equal(caps.get("moz:accessibilityChecks"), json["moz:accessibilityChecks"]);
>    equal(caps.get("moz:processID"), json["moz:processID"]);
>    equal(caps.get("moz:profile"), json["moz:profile"]);
> +  equal(caps.get("moz:useNonSpecCompliantPointerOrigin"),

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:413
(Diff revision 3)
>  
>    equal(caps.get("moz:accessibilityChecks"), json["moz:accessibilityChecks"]);
>    equal(caps.get("moz:processID"), json["moz:processID"]);
>    equal(caps.get("moz:profile"), json["moz:profile"]);
> +  equal(caps.get("moz:useNonSpecCompliantPointerOrigin"),
> +        json["moz:useNonSpecCompliantPointerOrigin"]);

Error: Expected indentation of 6 spaces but found 8. [eslint: indent-legacy]

::: testing/marionette/test_session.js:458
(Diff revision 3)
>    caps = fromJSON({"moz:accessibilityChecks": true});
>    equal(true, caps.get("moz:accessibilityChecks"));
>    caps = fromJSON({"moz:accessibilityChecks": false});
>    equal(false, caps.get("moz:accessibilityChecks"));
>    Assert.throws(() => fromJSON({"moz:accessibilityChecks": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:accessibilityChecks": 1}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:461
(Diff revision 3)
>    equal(false, caps.get("moz:accessibilityChecks"));
>    Assert.throws(() => fromJSON({"moz:accessibilityChecks": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:accessibilityChecks": 1}));
> +
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": false});
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:463
(Diff revision 3)
> +  Assert.throws(() => fromJSON({"moz:accessibilityChecks": 1}));
> +
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": false});
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": true});
> +  equal(true, caps.get("moz:useNonSpecCompliantPointerOrigin"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:464
(Diff revision 3)
> +
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": false});
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": true});
> +  equal(true, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": "foo"}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:465
(Diff revision 3)
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": false});
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": true});
> +  equal(true, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": 1}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:468
(Diff revision 3)
> +  equal(true, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": 1}));
> +
> +  caps = fromJSON({"moz:webdriverClick": true});
> +  equal(true, caps.get("moz:webdriverClick"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:470
(Diff revision 3)
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": 1}));
> +
> +  caps = fromJSON({"moz:webdriverClick": true});
> +  equal(true, caps.get("moz:webdriverClick"));
> +  caps = fromJSON({"moz:webdriverClick": false});
> +  equal(false, caps.get("moz:webdriverClick"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:471
(Diff revision 3)
> +
> +  caps = fromJSON({"moz:webdriverClick": true});
> +  equal(true, caps.get("moz:webdriverClick"));
> +  caps = fromJSON({"moz:webdriverClick": false});
> +  equal(false, caps.get("moz:webdriverClick"));
> +  Assert.throws(() => fromJSON({"moz:webdriverClick": "foo"}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:472
(Diff revision 3)
> +  caps = fromJSON({"moz:webdriverClick": true});
> +  equal(true, caps.get("moz:webdriverClick"));
> +  caps = fromJSON({"moz:webdriverClick": false});
> +  equal(false, caps.get("moz:webdriverClick"));
> +  Assert.throws(() => fromJSON({"moz:webdriverClick": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:webdriverClick": 1}));

Error: 'Assert' is not defined. [eslint: no-undef]
janx: Why is the mozreview bot complaining about eslint errors in
a set of files (testing/marionette/test_*.js) that is part of the
eslint ignore list in .eslintignore?
Flags: needinfo?(janx)
Comment on attachment 8943949 [details]
Bug 1429338 - Marionette has to honor "moz:useNonSpecCompliantPointerOrigin" capability.

https://reviewboard.mozilla.org/r/214284/#review220656


Static analysis found 12 defects in this patch.
 - 12 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/test_session.js:382
(Diff revision 4)
>    ok(caps.has("rotatable"));
>  
>    equal(false, caps.get("moz:accessibilityChecks"));
>    ok(caps.has("moz:processID"));
>    ok(caps.has("moz:profile"));
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:412
(Diff revision 4)
>    equal(caps.get("rotatable"), json.rotatable);
>  
>    equal(caps.get("moz:accessibilityChecks"), json["moz:accessibilityChecks"]);
>    equal(caps.get("moz:processID"), json["moz:processID"]);
>    equal(caps.get("moz:profile"), json["moz:profile"]);
> +  equal(caps.get("moz:useNonSpecCompliantPointerOrigin"),

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:413
(Diff revision 4)
>  
>    equal(caps.get("moz:accessibilityChecks"), json["moz:accessibilityChecks"]);
>    equal(caps.get("moz:processID"), json["moz:processID"]);
>    equal(caps.get("moz:profile"), json["moz:profile"]);
> +  equal(caps.get("moz:useNonSpecCompliantPointerOrigin"),
> +        json["moz:useNonSpecCompliantPointerOrigin"]);

Error: Expected indentation of 6 spaces but found 8. [eslint: indent-legacy]

::: testing/marionette/test_session.js:458
(Diff revision 4)
>    caps = fromJSON({"moz:accessibilityChecks": true});
>    equal(true, caps.get("moz:accessibilityChecks"));
>    caps = fromJSON({"moz:accessibilityChecks": false});
>    equal(false, caps.get("moz:accessibilityChecks"));
>    Assert.throws(() => fromJSON({"moz:accessibilityChecks": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:accessibilityChecks": 1}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:461
(Diff revision 4)
>    equal(false, caps.get("moz:accessibilityChecks"));
>    Assert.throws(() => fromJSON({"moz:accessibilityChecks": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:accessibilityChecks": 1}));
> +
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": false});
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:463
(Diff revision 4)
> +  Assert.throws(() => fromJSON({"moz:accessibilityChecks": 1}));
> +
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": false});
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": true});
> +  equal(true, caps.get("moz:useNonSpecCompliantPointerOrigin"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:464
(Diff revision 4)
> +
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": false});
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": true});
> +  equal(true, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": "foo"}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:465
(Diff revision 4)
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": false});
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": true});
> +  equal(true, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": 1}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:468
(Diff revision 4)
> +  equal(true, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": 1}));
> +
> +  caps = fromJSON({"moz:webdriverClick": true});
> +  equal(true, caps.get("moz:webdriverClick"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:470
(Diff revision 4)
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": 1}));
> +
> +  caps = fromJSON({"moz:webdriverClick": true});
> +  equal(true, caps.get("moz:webdriverClick"));
> +  caps = fromJSON({"moz:webdriverClick": false});
> +  equal(false, caps.get("moz:webdriverClick"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:471
(Diff revision 4)
> +
> +  caps = fromJSON({"moz:webdriverClick": true});
> +  equal(true, caps.get("moz:webdriverClick"));
> +  caps = fromJSON({"moz:webdriverClick": false});
> +  equal(false, caps.get("moz:webdriverClick"));
> +  Assert.throws(() => fromJSON({"moz:webdriverClick": "foo"}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:472
(Diff revision 4)
> +  caps = fromJSON({"moz:webdriverClick": true});
> +  equal(true, caps.get("moz:webdriverClick"));
> +  caps = fromJSON({"moz:webdriverClick": false});
> +  equal(false, caps.get("moz:webdriverClick"));
> +  Assert.throws(() => fromJSON({"moz:webdriverClick": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:webdriverClick": 1}));

Error: 'Assert' is not defined. [eslint: no-undef]
Comment on attachment 8943949 [details]
Bug 1429338 - Marionette has to honor "moz:useNonSpecCompliantPointerOrigin" capability.

https://reviewboard.mozilla.org/r/214284/#review220986

Looks good, requesting small additions.

::: testing/marionette/driver.js:605
(Diff revision 4)
>   *
>   *  <dt><code>moz:accessibilityChecks</code> (boolean)
>   *  <dd>Run a11y checks when clicking elements.
>   *
> + *  <dt><code>moz:useNonSpecCompliantPointerOrigin</code> (boolean)
> + *  <dd>Use the not WebDriver conforming calculation of the pointer origin.

Explain this only has an impact when the origin is an element and the element center point is used.

::: testing/marionette/harness/marionette_harness/tests/unit/test_mouse_action.py:137
(Diff revision 4)
> +        click_position = Wait(self.marionette).until(lambda _: self.click_position,
> +                                                     message="No click event has been detected")
> +        self.assertAlmostEqual(click_position["x"], elem_center_point["x"], delta=1)
> +        self.assertAlmostEqual(click_position["y"], elem_center_point["y"], delta=1)
> +
> +    def test_click_element_larger_than_viewport_with_center_point_outside(self):

Could you add an equivalent wdspec test to [1] to verify that no error is thrown in the spec-compliant case?

[1] https://searchfox.org/mozilla-central/rev/e3cba77cee3ff1be38313abe9c804d13c51bd95b/testing/web-platform/tests/webdriver/tests/actions/mouse.py#68

::: testing/marionette/harness/marionette_harness/tests/unit/test_mouse_action.py:139
(Diff revision 4)
> +        self.assertAlmostEqual(click_position["x"], elem_center_point["x"], delta=1)
> +        self.assertAlmostEqual(click_position["y"], elem_center_point["y"], delta=1)
> +
> +    def test_click_element_larger_than_viewport_with_center_point_outside(self):
> +        self.marionette.navigate(inline("""
> +          <div id="div" style="width: 200vw; height: 200vh; background: green;"

Make the el a little bit bigger (e.g. 205, 205) to be definitively out of bounds, so the center coords aren't exactly on the corner of the viewport -- we might get intermittents otherwise.

::: testing/marionette/listener.js:791
(Diff revision 4)
>   *      each of which represents an action sequence.
>   */
>  async function performActions(msg) {
>    let chain = action.Chain.fromJSON(msg.actions);
> -  await action.dispatch(chain, curContainer.frame);
> +  await action.dispatch(chain, curContainer.frame,
> +      !capabilities.get("moz:useNonSpecCompliantPointerOrigin"),

Capabilities are only set when the session starts, right? In that case, I think you could safely set a global on `action` in `listener.registerSelf` instead of passing this flag down through function calls.
Attachment #8943949 - Flags: review?(mjzffr) → review-
(In reply to Andreas Tolfsen ‹:ato› from comment #24)
> janx: Why is the mozreview bot complaining about eslint errors in
> a set of files (testing/marionette/test_*.js) that is part of the
> eslint ignore list in .eslintignore?

That's a bug. Ahal reported it as [0] and fixed it in [1]. Thanks Ahal!

[0] https://github.com/mozilla-releng/services/issues/788
[1] https://github.com/mozilla-releng/services/pull/791
Flags: needinfo?(janx)
Comment on attachment 8943949 [details]
Bug 1429338 - Marionette has to honor "moz:useNonSpecCompliantPointerOrigin" capability.

https://reviewboard.mozilla.org/r/214284/#review220986

> Could you add an equivalent wdspec test to [1] to verify that no error is thrown in the spec-compliant case?
> 
> [1] https://searchfox.org/mozilla-central/rev/e3cba77cee3ff1be38313abe9c804d13c51bd95b/testing/web-platform/tests/webdriver/tests/actions/mouse.py#68

I just added a test, but I feel that we should add a new test file specifically for pointer move and in-view center point checks. I would propose to do that on a different bug.

> Capabilities are only set when the session starts, right? In that case, I think you could safely set a global on `action` in `listener.registerSelf` instead of passing this flag down through function calls.

I have made this change.
Comment on attachment 8943949 [details]
Bug 1429338 - Marionette has to honor "moz:useNonSpecCompliantPointerOrigin" capability.

https://reviewboard.mozilla.org/r/214284/#review221272


Static analysis found 13 defects in this patch.
 - 13 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/test_session.js:382
(Diff revision 5)
>    ok(caps.has("rotatable"));
>  
>    equal(false, caps.get("moz:accessibilityChecks"));
>    ok(caps.has("moz:processID"));
>    ok(caps.has("moz:profile"));
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:412
(Diff revision 5)
>    equal(caps.get("rotatable"), json.rotatable);
>  
>    equal(caps.get("moz:accessibilityChecks"), json["moz:accessibilityChecks"]);
>    equal(caps.get("moz:processID"), json["moz:processID"]);
>    equal(caps.get("moz:profile"), json["moz:profile"]);
> +  equal(caps.get("moz:useNonSpecCompliantPointerOrigin"),

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:413
(Diff revision 5)
>  
>    equal(caps.get("moz:accessibilityChecks"), json["moz:accessibilityChecks"]);
>    equal(caps.get("moz:processID"), json["moz:processID"]);
>    equal(caps.get("moz:profile"), json["moz:profile"]);
> +  equal(caps.get("moz:useNonSpecCompliantPointerOrigin"),
> +        json["moz:useNonSpecCompliantPointerOrigin"]);

Error: Expected indentation of 6 spaces but found 8. [eslint: indent-legacy]

::: testing/marionette/test_session.js:458
(Diff revision 5)
>    caps = fromJSON({"moz:accessibilityChecks": true});
>    equal(true, caps.get("moz:accessibilityChecks"));
>    caps = fromJSON({"moz:accessibilityChecks": false});
>    equal(false, caps.get("moz:accessibilityChecks"));
>    Assert.throws(() => fromJSON({"moz:accessibilityChecks": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:accessibilityChecks": 1}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:461
(Diff revision 5)
>    equal(false, caps.get("moz:accessibilityChecks"));
>    Assert.throws(() => fromJSON({"moz:accessibilityChecks": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:accessibilityChecks": 1}));
> +
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": false});
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:463
(Diff revision 5)
> +  Assert.throws(() => fromJSON({"moz:accessibilityChecks": 1}));
> +
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": false});
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": true});
> +  equal(true, caps.get("moz:useNonSpecCompliantPointerOrigin"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:464
(Diff revision 5)
> +
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": false});
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": true});
> +  equal(true, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": "foo"}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:465
(Diff revision 5)
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": false});
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": true});
> +  equal(true, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": 1}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:468
(Diff revision 5)
> +  equal(true, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": 1}));
> +
> +  caps = fromJSON({"moz:webdriverClick": true});
> +  equal(true, caps.get("moz:webdriverClick"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:470
(Diff revision 5)
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": 1}));
> +
> +  caps = fromJSON({"moz:webdriverClick": true});
> +  equal(true, caps.get("moz:webdriverClick"));
> +  caps = fromJSON({"moz:webdriverClick": false});
> +  equal(false, caps.get("moz:webdriverClick"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:471
(Diff revision 5)
> +
> +  caps = fromJSON({"moz:webdriverClick": true});
> +  equal(true, caps.get("moz:webdriverClick"));
> +  caps = fromJSON({"moz:webdriverClick": false});
> +  equal(false, caps.get("moz:webdriverClick"));
> +  Assert.throws(() => fromJSON({"moz:webdriverClick": "foo"}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:472
(Diff revision 5)
> +  caps = fromJSON({"moz:webdriverClick": true});
> +  equal(true, caps.get("moz:webdriverClick"));
> +  caps = fromJSON({"moz:webdriverClick": false});
> +  equal(false, caps.get("moz:webdriverClick"));
> +  Assert.throws(() => fromJSON({"moz:webdriverClick": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:webdriverClick": 1}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/web-platform/tests/webdriver/tests/actions/support/test_actions_wdspec.html:165
(Diff revision 5)
> -          outer.addEventListener("dblclick", recordPointerEvent);
> -          outer.addEventListener("mousedown", recordPointerEvent);
> -          outer.addEventListener("mouseup", recordPointerEvent);
> -          outer.addEventListener("contextmenu", recordPointerEvent);
> +          box.addEventListener("dblclick", recordPointerEvent);
> +          box.addEventListener("mousedown", recordPointerEvent);
> +          box.addEventListener("mouseup", recordPointerEvent);
> +          box.addEventListener("contextmenu", recordPointerEvent);
> +
> +          var box = document.getElementById("box-partial-outside");

Error: 'box' is already defined. [eslint: no-redeclare]
Comment on attachment 8943949 [details]
Bug 1429338 - Marionette has to honor "moz:useNonSpecCompliantPointerOrigin" capability.

https://reviewboard.mozilla.org/r/214284/#review221448

Everything looks good except for some CSS changes. (Thanks for adding a wdspec test.)

::: testing/web-platform/tests/webdriver/tests/actions/support/test_actions_wdspec.html:10
(Diff revision 5)
>      <title>Test Actions</title>
>      <style>
>        div { padding:0px; margin: 0px; }
>        #trackPointer { position: fixed; }
>        #resultContainer { width: 600px; height: 60px; }
> -      .area { width: 100px; height: 50px; background-color: #ccc; }
> +      .area { margin: 5px; width: 100px; height: 50px; outline: solid 1px; background: #ccc; }

I believe the changes to the `area` class have negatively affected the behaviour of `#dragTarget` during drag-and-drop interactions. See screenshot at: https://bugzilla.mozilla.org/attachment.cgi?id=8945600

The JS implementing the drag and drop behaviour is likely strongly tied to the dragTarget dimensions, so changes to border and/or margin might affect it.

Since the CSS changes appear to be purely cosmetic, I suggest that you revert them so we can save ourselves from investigating surprising test behavior in the future. :)
Attachment #8943949 - Flags: review?(mjzffr) → review-
Comment on attachment 8943949 [details]
Bug 1429338 - Marionette has to honor "moz:useNonSpecCompliantPointerOrigin" capability.

https://reviewboard.mozilla.org/r/214284/#review221448

> I believe the changes to the `area` class have negatively affected the behaviour of `#dragTarget` during drag-and-drop interactions. See screenshot at: https://bugzilla.mozilla.org/attachment.cgi?id=8945600
> 
> The JS implementing the drag and drop behaviour is likely strongly tied to the dragTarget dimensions, so changes to border and/or margin might affect it.
> 
> Since the CSS changes appear to be purely cosmetic, I suggest that you revert them so we can save ourselves from investigating surprising test behavior in the future. :)

The outline is not related here because it doesn't add any additional space to the element, so I would propose we keep it. But I can remove the margin for sure.

Please note that the updated patch has a `duration=1000` for now. I will update it once bug 1432773 landed on central.
Comment on attachment 8943949 [details]
Bug 1429338 - Marionette has to honor "moz:useNonSpecCompliantPointerOrigin" capability.

https://reviewboard.mozilla.org/r/214284/#review221588


Static analysis found 12 defects in this patch.
 - 12 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/test_session.js:382
(Diff revision 6)
>    ok(caps.has("rotatable"));
>  
>    equal(false, caps.get("moz:accessibilityChecks"));
>    ok(caps.has("moz:processID"));
>    ok(caps.has("moz:profile"));
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:412
(Diff revision 6)
>    equal(caps.get("rotatable"), json.rotatable);
>  
>    equal(caps.get("moz:accessibilityChecks"), json["moz:accessibilityChecks"]);
>    equal(caps.get("moz:processID"), json["moz:processID"]);
>    equal(caps.get("moz:profile"), json["moz:profile"]);
> +  equal(caps.get("moz:useNonSpecCompliantPointerOrigin"),

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:413
(Diff revision 6)
>  
>    equal(caps.get("moz:accessibilityChecks"), json["moz:accessibilityChecks"]);
>    equal(caps.get("moz:processID"), json["moz:processID"]);
>    equal(caps.get("moz:profile"), json["moz:profile"]);
> +  equal(caps.get("moz:useNonSpecCompliantPointerOrigin"),
> +        json["moz:useNonSpecCompliantPointerOrigin"]);

Error: Expected indentation of 6 spaces but found 8. [eslint: indent-legacy]

::: testing/marionette/test_session.js:458
(Diff revision 6)
>    caps = fromJSON({"moz:accessibilityChecks": true});
>    equal(true, caps.get("moz:accessibilityChecks"));
>    caps = fromJSON({"moz:accessibilityChecks": false});
>    equal(false, caps.get("moz:accessibilityChecks"));
>    Assert.throws(() => fromJSON({"moz:accessibilityChecks": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:accessibilityChecks": 1}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:461
(Diff revision 6)
>    equal(false, caps.get("moz:accessibilityChecks"));
>    Assert.throws(() => fromJSON({"moz:accessibilityChecks": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:accessibilityChecks": 1}));
> +
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": false});
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:463
(Diff revision 6)
> +  Assert.throws(() => fromJSON({"moz:accessibilityChecks": 1}));
> +
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": false});
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": true});
> +  equal(true, caps.get("moz:useNonSpecCompliantPointerOrigin"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:464
(Diff revision 6)
> +
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": false});
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": true});
> +  equal(true, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": "foo"}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:465
(Diff revision 6)
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": false});
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": true});
> +  equal(true, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": 1}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:468
(Diff revision 6)
> +  equal(true, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": 1}));
> +
> +  caps = fromJSON({"moz:webdriverClick": true});
> +  equal(true, caps.get("moz:webdriverClick"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:470
(Diff revision 6)
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": 1}));
> +
> +  caps = fromJSON({"moz:webdriverClick": true});
> +  equal(true, caps.get("moz:webdriverClick"));
> +  caps = fromJSON({"moz:webdriverClick": false});
> +  equal(false, caps.get("moz:webdriverClick"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:471
(Diff revision 6)
> +
> +  caps = fromJSON({"moz:webdriverClick": true});
> +  equal(true, caps.get("moz:webdriverClick"));
> +  caps = fromJSON({"moz:webdriverClick": false});
> +  equal(false, caps.get("moz:webdriverClick"));
> +  Assert.throws(() => fromJSON({"moz:webdriverClick": "foo"}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:472
(Diff revision 6)
> +  caps = fromJSON({"moz:webdriverClick": true});
> +  equal(true, caps.get("moz:webdriverClick"));
> +  caps = fromJSON({"moz:webdriverClick": false});
> +  equal(false, caps.get("moz:webdriverClick"));
> +  Assert.throws(() => fromJSON({"moz:webdriverClick": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:webdriverClick": 1}));

Error: 'Assert' is not defined. [eslint: no-undef]
Latest try build shows failures, but I don't know what that is because there is no assertion introspection done. Lets wait for bug 1433390 being landed.
Depends on: 1433390, 1432773
Comment on attachment 8943949 [details]
Bug 1429338 - Marionette has to honor "moz:useNonSpecCompliantPointerOrigin" capability.

https://reviewboard.mozilla.org/r/214284/#review221624


Static analysis found 12 defects in this patch.
 - 12 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/test_session.js:382
(Diff revision 7)
>    ok(caps.has("rotatable"));
>  
>    equal(false, caps.get("moz:accessibilityChecks"));
>    ok(caps.has("moz:processID"));
>    ok(caps.has("moz:profile"));
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:412
(Diff revision 7)
>    equal(caps.get("rotatable"), json.rotatable);
>  
>    equal(caps.get("moz:accessibilityChecks"), json["moz:accessibilityChecks"]);
>    equal(caps.get("moz:processID"), json["moz:processID"]);
>    equal(caps.get("moz:profile"), json["moz:profile"]);
> +  equal(caps.get("moz:useNonSpecCompliantPointerOrigin"),

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:413
(Diff revision 7)
>  
>    equal(caps.get("moz:accessibilityChecks"), json["moz:accessibilityChecks"]);
>    equal(caps.get("moz:processID"), json["moz:processID"]);
>    equal(caps.get("moz:profile"), json["moz:profile"]);
> +  equal(caps.get("moz:useNonSpecCompliantPointerOrigin"),
> +        json["moz:useNonSpecCompliantPointerOrigin"]);

Error: Expected indentation of 6 spaces but found 8. [eslint: indent-legacy]

::: testing/marionette/test_session.js:458
(Diff revision 7)
>    caps = fromJSON({"moz:accessibilityChecks": true});
>    equal(true, caps.get("moz:accessibilityChecks"));
>    caps = fromJSON({"moz:accessibilityChecks": false});
>    equal(false, caps.get("moz:accessibilityChecks"));
>    Assert.throws(() => fromJSON({"moz:accessibilityChecks": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:accessibilityChecks": 1}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:461
(Diff revision 7)
>    equal(false, caps.get("moz:accessibilityChecks"));
>    Assert.throws(() => fromJSON({"moz:accessibilityChecks": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:accessibilityChecks": 1}));
> +
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": false});
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:463
(Diff revision 7)
> +  Assert.throws(() => fromJSON({"moz:accessibilityChecks": 1}));
> +
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": false});
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": true});
> +  equal(true, caps.get("moz:useNonSpecCompliantPointerOrigin"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:464
(Diff revision 7)
> +
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": false});
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": true});
> +  equal(true, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": "foo"}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:465
(Diff revision 7)
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": false});
> +  equal(false, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  caps = fromJSON({"moz:useNonSpecCompliantPointerOrigin": true});
> +  equal(true, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": 1}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:468
(Diff revision 7)
> +  equal(true, caps.get("moz:useNonSpecCompliantPointerOrigin"));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": 1}));
> +
> +  caps = fromJSON({"moz:webdriverClick": true});
> +  equal(true, caps.get("moz:webdriverClick"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:470
(Diff revision 7)
> +  Assert.throws(() => fromJSON({"moz:useNonSpecCompliantPointerOrigin": 1}));
> +
> +  caps = fromJSON({"moz:webdriverClick": true});
> +  equal(true, caps.get("moz:webdriverClick"));
> +  caps = fromJSON({"moz:webdriverClick": false});
> +  equal(false, caps.get("moz:webdriverClick"));

Error: 'equal' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:471
(Diff revision 7)
> +
> +  caps = fromJSON({"moz:webdriverClick": true});
> +  equal(true, caps.get("moz:webdriverClick"));
> +  caps = fromJSON({"moz:webdriverClick": false});
> +  equal(false, caps.get("moz:webdriverClick"));
> +  Assert.throws(() => fromJSON({"moz:webdriverClick": "foo"}));

Error: 'Assert' is not defined. [eslint: no-undef]

::: testing/marionette/test_session.js:472
(Diff revision 7)
> +  caps = fromJSON({"moz:webdriverClick": true});
> +  equal(true, caps.get("moz:webdriverClick"));
> +  caps = fromJSON({"moz:webdriverClick": false});
> +  equal(false, caps.get("moz:webdriverClick"));
> +  Assert.throws(() => fromJSON({"moz:webdriverClick": "foo"}));
> +  Assert.throws(() => fromJSON({"moz:webdriverClick": 1}));

Error: 'Assert' is not defined. [eslint: no-undef]
The remaining problem here are really different y coordinates of the box:

Linux:
DOMRect { x: -52, y: 214.6666717529297, width: 100, height: 50, top: 214.6666717529297, right: 48, bottom: 264.6666717529297, left: -52 }

Mac:
DOMRect { x: -52, y: 209.26666259765625, width: 100, height: 50, top: 209.26666259765625, right: 48, bottom: 259.26666259765625, left: -52 }

There is clearly a difference in how the elements are placed on that page. I will have to check what's causing it. But I would also say that we may not hard-code values.
The keyreporter input field is actually causing this problem. Its inner rect is 3px higher, and also the top and bottom margin have 1px more, which adds it up to 5px, as it can be seen above.

All in all this is due to native widgets, which have a different styling, and as such dimensions. In the above case we might want to remove all of those for now.
That idea failed completely because using a different styling for the border is not enough. The inner height of the element (without margin and padding) is still higher on Linux than on MacOS.

I tried to replace the input with a contentEditable element, but that doesn't work because all the other tests expect an input. So I don't think it is a good idea to make that change. 

Instead we should create a new HTML testcase or even better use inline content for all the pointer origin subtests.
Comment on attachment 8943949 [details]
Bug 1429338 - Marionette has to honor "moz:useNonSpecCompliantPointerOrigin" capability.

https://reviewboard.mozilla.org/r/214284/#review224356

Thanks for writing so many nice tests. :)

::: testing/web-platform/tests/webdriver/tests/actions/mouse.py
(Diff revision 10)
> -# TODO use pytest.approx once we upgrade to pytest > 3.0
> -def approx(n, m, tolerance=1):
> -    return abs(n - m) <= tolerance
> -
> -
> -def test_click_at_coordinates(session, test_actions_page, mouse_chain):

Let's not delete this test, please.

::: testing/web-platform/tests/webdriver/tests/actions/pointer_origin.py:11
(Diff revision 10)
> +from tests.support.inline import inline
> +
> +
> +def origin_doc(content):
> +    return inline("""
> +      <div onmousemove="window.coords = {{x: event.clientX, y: event.clientY}}">

Why the double-`{`? I get a syntax error when I run this.

::: testing/web-platform/tests/webdriver/tests/actions/pointer_origin.py:25
(Diff revision 10)
> +
> +def test_viewport_inside(session, mouse_chain):
> +    point = {"x": 50, "y": 50}
> +
> +    session.url = origin_doc("""
> +      <div id="div" style="width: 100px; height: 50px; background: green;"/>

Needs to be `<div></div>` instead of `<div/>` to be compliant. Same for other tests.

Since you're reusing this particular string a few times, perhaps you should assign it to a global.

::: testing/web-platform/tests/webdriver/tests/actions/pointer_origin.py:63
(Diff revision 10)
> +
> +
> +def test_pointer_outside(session, mouse_chain):
> +    with pytest.raises(MoveTargetOutOfBoundsException):
> +        mouse_chain \
> +            .pointer_move(-50, -50, origin="pointer") \

Might be safer to first move the pointer to a valid start point?
Attachment #8943949 - Flags: review?(mjzffr) → review-
Comment on attachment 8943949 [details]
Bug 1429338 - Marionette has to honor "moz:useNonSpecCompliantPointerOrigin" capability.

https://reviewboard.mozilla.org/r/214284/#review224356

> Let's not delete this test, please.

Sorry, that was not expected. I reverted this change.

> Why the double-`{`? I get a syntax error when I run this.

This is to escape the required braches from `format()`. For me it works pretty fine, so not sure which kind of syntax error you get.

> Needs to be `<div></div>` instead of `<div/>` to be compliant. Same for other tests.
> 
> Since you're reusing this particular string a few times, perhaps you should assign it to a global.

Yeah we can do that. I will also propose an outer_style so we can have the wrapping div styled with overflow.

> Might be safer to first move the pointer to a valid start point?

Those are absolute coordinates we move the pointer to. It shouldn't matter where in the viewport the pointer currently is.
Comment on attachment 8943949 [details]
Bug 1429338 - Marionette has to honor "moz:useNonSpecCompliantPointerOrigin" capability.

https://reviewboard.mozilla.org/r/214284/#review224578
Attachment #8943949 - Flags: review?(mjzffr) → review+
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e3c84097d0d
Refactor legacy mouse action tests. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/75f63c4d1ebc
Marionette has to honor "moz:useNonSpecCompliantPointerOrigin" capability. r=maja_zf
https://hg.mozilla.org/mozilla-central/rev/4e3c84097d0d
https://hg.mozilla.org/mozilla-central/rev/75f63c4d1ebc
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Please uplift the Marionette patch to beta which will offer a fallback for users of Selenium to not have to use the new WebDriver conforming pointer origin calculation. Thanks.
Whiteboard: [checkin-needed-beta]
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/9456 for changes under testing/web-platform/tests
Can't merge web-platform-tests PR due to failing upstream tests
Can't merge web-platform-tests PR due to failing upstream tests
Can't merge web-platform-tests PR due to failing upstream tests
Whiteboard: [checkin-needed-beta]
You need to log in before you can comment on or make changes to this bug.