Closed
Bug 1429338
Opened 7 years ago
Closed 7 years ago
Honor "moz:useNonSpecCompliantPointerOrigin" capability to allow usage of non-spec behavior
Categories
(Remote Protocol :: Marionette, defect, P2)
Tracking
(firefox-esr52 unaffected, firefox58 unaffected, firefox59 fixed, firefox60 fixed)
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox58 | --- | unaffected |
firefox59 | --- | fixed |
firefox60 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
(Blocks 1 open bug, )
Details
(Keywords: regression)
Attachments
(5 files)
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)
Assignee | ||
Comment 1•7 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.
Assignee | ||
Comment 2•7 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.
Assignee | ||
Comment 3•7 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.
Flags: needinfo?(mjzffr)
Comment 5•7 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.
The testcase include `<img style="width: 2000px; height: 2000px;" ...` so I'm guessing that's what the -1000,-1000 is based on.
Assignee | ||
Comment 8•7 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.
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
Comment 9•7 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.
Flags: needinfo?(ato)
Assignee | ||
Comment 10•7 years ago
|
||
Well, we should. And I will use the minified HTML testcase from this bug for testing that.
Flags: needinfo?(dburns)
Assignee | ||
Comment 11•7 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.
Assignee | ||
Comment 12•7 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.
Flags: needinfo?(mjzffr)
Flags: needinfo?(ato)
Comment 13•7 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.
Flags: needinfo?(ato)
Assignee | ||
Comment 14•7 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.
Depends on: 1354578
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 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?
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 22•7 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]
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8944412 -
Flags: review?(mjzffr)
Attachment #8943949 -
Flags: review?(mjzffr)
Comment 24•7 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?
Flags: needinfo?(janx)
Comment 25•7 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]
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8944412 [details] Bug 1429338 - Refactor legacy mouse action tests. https://reviewboard.mozilla.org/r/214570/#review220980
Attachment #8944412 -
Flags: review?(mjzffr) → review+
Comment 27•7 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.
Attachment #8943949 -
Flags: review?(mjzffr) → review-
Comment 28•7 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
Flags: needinfo?(janx)
Assignee | ||
Comment 29•7 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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 32•7 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]
Comment 34•7 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. :)
Attachment #8943949 -
Flags: review?(mjzffr) → review-
Assignee | ||
Comment 35•7 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.
Comment hidden (mozreview-request) |
Comment 37•7 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]
Assignee | ||
Comment 38•7 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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 41•7 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]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 44•7 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.
Assignee | ||
Comment 45•7 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.
Assignee | ||
Comment 46•7 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.
+1 to inline content
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8943949 -
Flags: review?(mjzffr)
Comment 51•7 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?
Attachment #8943949 -
Flags: review?(mjzffr) → review-
Assignee | ||
Comment 52•7 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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 56•7 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
Attachment #8943949 -
Flags: review?(mjzffr) → review+
Comment 57•7 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
Comment 58•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4e3c84097d0d https://hg.mozilla.org/mozilla-central/rev/75f63c4d1ebc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Assignee | ||
Comment 59•7 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.
Whiteboard: [checkin-needed-beta]
Comment 60•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/1870904d60d3 https://hg.mozilla.org/releases/mozilla-beta/rev/b581cd2edcfb
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
Updated•7 years ago
|
Whiteboard: [checkin-needed-beta]
![]() |
||
Updated•7 years ago
|
Flags: in-testsuite+
Updated•7 years ago
|
status-firefox-esr52:
--- → unaffected
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•