Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Description • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 1 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 2 • 6 years ago

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.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 4 • 6 years ago

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.

	Andreas Tolfsen ❲:ato❳
	Comment 5 • 6 years ago

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.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 6 • 6 years ago

The testcase include `<img style="width: 2000px; height: 2000px;"  ...` so I'm guessing that's what the -1000,-1000 is based on.

	David Burns :automatedtester
	Comment 7 • 6 years ago

[Mass Change 2018-01-15] Moving bugs to backlog

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 8 • 6 years ago

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.

	Andreas Tolfsen ❲:ato❳
	Comment 9 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 10 • 6 years ago

Well, we should. And I will use the minified HTML testcase from this bug for testing that.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 11 • 6 years ago

(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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 12 • 6 years ago

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.

	Andreas Tolfsen ❲:ato❳
	Comment 13 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 14 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 16 • 6 years ago
mozreview-review

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?

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 17 • 6 years ago

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.

	Code Review Bot [:reviewbot]
	Comment 22 • 6 years ago
mozreview-review

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]

	Andreas Tolfsen ❲:ato❳
	Comment 24 • 6 years ago

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?

	Code Review Bot [:reviewbot]
	Comment 25 • 6 years ago
mozreview-review

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]

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 26 • 6 years ago
mozreview-review

Comment on attachment 8944412 [details]
Bug 1429338 - Refactor legacy mouse action tests.

https://reviewboard.mozilla.org/r/214570/#review220980

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 27 • 6 years ago
mozreview-review

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.

	Jan Keromnes [:janx]
	Comment 28 • 6 years ago

(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

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 29 • 6 years ago
mozreview-review-reply

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.

	Code Review Bot [:reviewbot]
	Comment 32 • 6 years ago
mozreview-review

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]

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 34 • 6 years ago
mozreview-review

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. :)

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 35 • 6 years ago
mozreview-review-reply

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.

	Code Review Bot [:reviewbot]
	Comment 37 • 6 years ago
mozreview-review

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]

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 38 • 6 years ago

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.

	Code Review Bot [:reviewbot]
	Comment 41 • 6 years ago
mozreview-review

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]

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 44 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 45 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 46 • 6 years ago

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.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 47 • 6 years ago

+1 to inline content

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 51 • 6 years ago
mozreview-review

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?

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 52 • 6 years ago
mozreview-review-reply

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.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 56 • 6 years ago
mozreview-review

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

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

	Pulsebot
	Comment 57 • 6 years ago

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

	Cosmin Sabou [:CosminS]
	Comment 58 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4e3c84097d0d
https://hg.mozilla.org/mozilla-central/rev/75f63c4d1ebc

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 59 • 6 years ago

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.

	Natalia Csoregi [:nataliaCs]
	Comment 60 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/1870904d60d3
https://hg.mozilla.org/releases/mozilla-beta/rev/b581cd2edcfb

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 61 • 6 years ago

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/9456 for changes under testing/web-platform/tests

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 62 • 6 years ago

Can't merge web-platform-tests PR due to failing upstream tests

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 63 • 6 years ago

Can't merge web-platform-tests PR due to failing upstream tests

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 64 • 6 years ago

Can't merge web-platform-tests PR due to failing upstream tests