Closed
Bug 1337133
Opened 8 years ago
Closed 8 years ago
Dispatch pointer actions for mouse pointer type
Categories
(Remote Protocol :: Marionette, defect)
Tracking
(firefox52 fixed, firefox-esr52 fixed, firefox53 fixed, firefox54 fixed)
RESOLVED
FIXED
mozilla54
People
(Reporter: impossibus, Assigned: impossibus)
References
(Blocks 1 open bug)
Details
(Keywords: pi-marionette-server)
Attachments
(16 files, 1 obsolete file)
59 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jgraham
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jgraham
:
review+
|
Details |
119.94 KB,
patch
|
Details | Diff | Splinter Review | |
149.79 KB,
patch
|
Details | Diff | Splinter Review |
https://w3c.github.io/webdriver/webdriver-spec.html#h-pointer-actions
Dispatch pointerDown, pointerUp, pointerMove for pointer type "mouse".
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•8 years ago
|
||
Early feedback welcome on the WIP patches if you feel like it, but they're not intended for review yet. I'll be back on Friday.
Comment 12•8 years ago
|
||
mozreview-review |
Comment on attachment 8835244 [details]
Bug 1337133 - Add number check to assert.js;
https://reviewboard.mozilla.org/r/110938/#review112392
::: testing/marionette/assert.js:153
(Diff revision 1)
> +assert.number = function (obj, msg = "") {
> + msg = msg || error.pprint`Expected ${obj} to be number`;
> + return assert.that(n => typeof n == "number", msg)(obj);
> +};
This does not handle NaN or Infinity.
::: testing/marionette/test_assert.js:43
(Diff revision 1)
> +add_test(function test_number() {
> + assert.number(1);
> + assert.number(0);
> + assert.number(-1);
> + assert.number(1.2);
> + Assert.throws(() => assert.number("foo"), InvalidArgumentError);
> + run_next_test();
> +});
Add a test for NaN and Infinity here.
::: testing/marionette/test_assert.js:57
(Diff revision 1)
> add_test(function test_integer() {
> assert.integer(1);
> assert.integer(0);
> assert.integer(-1);
> Assert.throws(() => assert.integer("foo"), InvalidArgumentError);
> + Assert.throws(() => assert.integer(1.2), InvalidArgumentError);
Well spotted.
Attachment #8835244 -
Flags: review?(ato) → review+
Comment 13•8 years ago
|
||
mozreview-review |
Comment on attachment 8835245 [details]
Bug 1337133 - Remove primary pointer attribute;
https://reviewboard.mozilla.org/r/110940/#review112396
Attachment #8835245 -
Flags: review?(ato) → review+
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8835246 [details]
Bug 1337133 - Replace element with origin for pointerMove actions;
https://reviewboard.mozilla.org/r/110942/#review112402
::: testing/marionette/action.js:355
(Diff revision 1)
> + if (!(name in this)) {
> + throw new InvalidArgumentError(`Unknown pointer-move origin: ${obj}`);
> + }
I’m curious, will this allow "get"?
::: testing/marionette/test_action.js:105
(Diff revision 1)
> +});
> +
> +add_test(function test_processPointerMoveActionOriginStringValidation() {
> + let actionSequence = {type: "pointer", id: "some_id"};
> + let actionItem = {duration: 5000, type: "pointerMove"};
> + for (let d of ["a", "", "undefined"]) {
Should "undefined" not be allowed?
Attachment #8835246 -
Flags: review?(ato) → review+
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8835247 [details]
Bug 1337133 - Replace some checks in actions with calls to assert.js;
https://reviewboard.mozilla.org/r/110944/#review112404
Attachment #8835247 -
Flags: review?(ato) → review+
Comment 16•8 years ago
|
||
mozreview-review |
Comment on attachment 8835248 [details]
Bug 1337133 - Add MoveTargetOutOfBoundsError as a new Webdriver error type;
https://reviewboard.mozilla.org/r/110946/#review112406
Attachment #8835248 -
Flags: review?(ato) → review+
Comment 17•8 years ago
|
||
mozreview-review |
Comment on attachment 8835249 [details]
Bug 1337133 - Split test kinds, common fixtures, utilities in wdspec tests;
https://reviewboard.mozilla.org/r/110948/#review112412
::: testing/web-platform/tests/webdriver/actions/support/util.py:1
(Diff revision 1)
> +def get_keys(input_el):
Generally speaking, avoid naming stuff “util”. There’s a danger turning these things into kitchen sinks.
Comment 18•8 years ago
|
||
mozreview-review |
Comment on attachment 8835251 [details]
Bug 1337133 - Set up fixtures for mouse actions in wdspec tests;
https://reviewboard.mozilla.org/r/110952/#review112416
::: testing/web-platform/tests/test_actions_wdspec.html:40
(Diff revision 1)
> }
> }
> return key;
> }
>
> - function recordEvent(event) {
> + function recordKeyBoardEvent(event) {
Keyboard is normally one word, so the Board capitalisation seems a bit unnecessary.
Comment 19•8 years ago
|
||
(In reply to Maja Frydrychowicz (:maja_zf) from comment #11)
> Early feedback welcome on the WIP patches if you feel like it, but they're
> not intended for review yet. I'll be back on Friday.
I didn’t set r+, but generally the unreviewed draft patches look good.
Assignee | ||
Comment 20•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8835246 [details]
Bug 1337133 - Replace element with origin for pointerMove actions;
https://reviewboard.mozilla.org/r/110942/#review112402
> I’m curious, will this allow "get"?
Ha, yeah, that aspect is kind of ugly.
It doesn't allow "get" and I'll add tests to reflect that. The same happens with action.PointerType.
The only reason it doesn't accept "get" is because `obj` gets capitalized, and there's no "Get" property.
> Should "undefined" not be allowed?
Whoops. `undefined` is allowed, "undefined" is silly. Will remove.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 33•8 years ago
|
||
mozreview-review |
Comment on attachment 8835244 [details]
Bug 1337133 - Add number check to assert.js;
https://reviewboard.mozilla.org/r/110938/#review115252
::: testing/marionette/assert.js:153
(Diff revision 2)
> +assert.number = function (obj, msg = "") {
> + msg = msg || error.pprint`Expected ${obj} to be number`;
> + return assert.that(n => (typeof n == "number" && Number.isFinite(n)), msg)(obj);
> +};
I guess that `Number.isFinite` also checks the type, so this could be replaced with this:
```js
return assert.that(Number.isFinite, msg)(obj)
```
Comment 34•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8835246 [details]
Bug 1337133 - Replace element with origin for pointerMove actions;
https://reviewboard.mozilla.org/r/110942/#review112402
> Ha, yeah, that aspect is kind of ugly.
>
> It doesn't allow "get" and I'll add tests to reflect that. The same happens with action.PointerType.
>
> The only reason it doesn't accept "get" is because `obj` gets capitalized, and there's no "Get" property.
Ouch (-:
Comment 35•8 years ago
|
||
mozreview-review |
Comment on attachment 8835248 [details]
Bug 1337133 - Add MoveTargetOutOfBoundsError as a new Webdriver error type;
https://reviewboard.mozilla.org/r/110946/#review115256
This patch’s commit message, “Add MoveTargetOutOfBoundsError to set of Webdriver errors”, is misleading now since it adds a new WebDriver error type.
Comment 36•8 years ago
|
||
mozreview-review |
Comment on attachment 8835249 [details]
Bug 1337133 - Split test kinds, common fixtures, utilities in wdspec tests;
https://reviewboard.mozilla.org/r/110948/#review115258
::: testing/web-platform/meta/MANIFEST.json
(Diff revision 2)
> - "./test_keys_wdspec.html": [
> - [
> - {}
> - ]
> - ],
Assuming the changes to testing/web-platform/meta/MANIFEST.json are fine.
::: testing/web-platform/tests/webdriver/actions/actions.py:1
(Diff revision 2)
> +from support.refine import get_keys, filter_dict
testing/web-platform/tests/webdriver/actions/actions.py (webdriver.actions.actions) is not a great name for a Python module. I guess this file could be renamed chain.py or sequence.py or something better?
::: testing/web-platform/tests/webdriver/actions/mouse_actions.py:1
(Diff revision 2)
> +import pytest
I would argue we could shorten the filename to testing/web-platform/tests/webdriver/actions/mouse.py, since it already lives inside an “actions” namespace.
Attachment #8835249 -
Flags: review?(ato) → review+
Comment 37•8 years ago
|
||
mozreview-review |
Comment on attachment 8835250 [details]
Bug 1337133 - Add pointer actions endpoints to wdclient;
https://reviewboard.mozilla.org/r/110950/#review115260
::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:132
(Diff revision 2)
> + # TODO change to pointerMove once https://github.com/mozilla/webdriver-rust/issues/68 resolved
> + action = {
> + "type": "move",
> + "x": x,
> + "y": y
> + }
This appears to have been fixed now, but I guess the fix is not yet available in the geckodriver on central?
In the interim I’ve started work on https://bugzilla.mozilla.org/show_bug.cgi?id=1340637 to compile geckodriver master on the build slaves. But I should point that it needs a follow-up patch to have wptrunner pick up the in-tree-built geckodriver.
I don’t know if that/the followup will land before this, but just making you aware of the ongoing work in this area.
::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:138
(Diff revision 2)
> + if (duration is not None):
> + action["duration"] = duration
> + if (origin is not None):
> + action["origin"] = origin
Drop the paranthesis.
Attachment #8835250 -
Flags: review?(ato) → review+
Comment 38•8 years ago
|
||
mozreview-review |
Comment on attachment 8835250 [details]
Bug 1337133 - Add pointer actions endpoints to wdclient;
https://reviewboard.mozilla.org/r/110950/#review115262
Can you change the commit message to not contain “(wd client)” first? I suggest “Add pointer action endpoints to wdclient”.
Comment 39•8 years ago
|
||
mozreview-review |
Comment on attachment 8835251 [details]
Bug 1337133 - Set up fixtures for mouse actions in wdspec tests;
https://reviewboard.mozilla.org/r/110952/#review115264
::: testing/web-platform/tests/webdriver/actions/actions.py:1
(Diff revision 2)
> -from support.refine import get_keys, filter_dict
> +from support.refine import get_keys, filter_dict, get_events
> -
> -
> -def get_events(session):
> - """Return list of key events recorded in the test_keys_page fixture."""
> - events = session.execute_script("return allEvents.events;") or []
> - # `key` values in `allEvents` may be escaped (see `escapeSurrogateHalf` in
> - # test_keys_wdspec.html), so this converts them back into unicode literals.
> - for e in events:
> - # example: turn "U+d83d" (6 chars) into u"\ud83d" (1 char)
> - if e["key"].startswith(u"U+"):
> - key = e["key"]
> - hex_suffix = key[key.index("+") + 1:]
> - e["key"] = unichr(int(hex_suffix, 16))
> - return events
Strictly speaking, this function could have been moved in your earlier patch. But don’t consider that a blocker.
::: testing/web-platform/tests/webdriver/actions/key_actions.py:4
(Diff revision 2)
> -from support.refine import get_keys, filter_dict
> +from support.refine import get_keys, filter_dict, get_events
> -
> -
> -def get_events(session):
> - """Return list of key events recorded in the test_keys_page fixture."""
> - events = session.execute_script("return allEvents.events;") or []
> - # `key` values in `allEvents` may be escaped (see `escapeSurrogateHalf` in
> - # test_keys_wdspec.html), so this converts them back into unicode literals.
> - for e in events:
> - # example: turn "U+d83d" (6 chars) into u"\ud83d" (1 char)
> - if e["key"].startswith(u"U+"):
> - key = e["key"]
> - hex_suffix = key[key.index("+") + 1:]
> - e["key"] = unichr(int(hex_suffix, 16))
> - return events
Same applies for this.
::: testing/web-platform/tests/webdriver/actions/support/test_actions_wdspec.html:71
(Diff revision 2)
> + appendMessage(`${event.type}(button:${event.button}, pageX:${event.pageX}, `
> + + `pageY:${event.pageY}, button: ${event.button}, buttons: ${event.buttons}, `
> + + `target id: ${event.target.id}`);
Sorry for this style nit, but could you break this on the commas to make each line more syntactically relevant?
It’s a bit hard to read as it is now, and changing it will not make a great diff. If each distinct thing is on its own line, a future diff to this will be easier to read.
::: testing/web-platform/tests/webdriver/actions/support/test_actions_wdspec.html:86
(Diff revision 2)
> function resetEvents() {
> allEvents.events.length = 0;
> displayMessage("");
> }
> +
> + document.addEventListener("DOMContentLoaded", function() {
Space after `function` so it doesn’t look like you call a function named "function".
::: testing/web-platform/tests/webdriver/actions/support/test_actions_wdspec.html:88
(Diff revision 2)
> displayMessage("");
> }
> +
> + document.addEventListener("DOMContentLoaded", function() {
> + var keyReporter = document.getElementById("keys");
> + ["keyup", "keypress", "keydown"].forEach(function(e) {
Same here with regards to space after `function`.
::: testing/web-platform/tests/webdriver/actions/support/test_actions_wdspec.html:100
(Diff revision 2)
> + });
> + window.addEventListener("mousemove", recordFirstPointerMove);
> + //visual cue for mousemove
> + var pointer = document.getElementById("trackPointer");
> + window.addEventListener("mousemove", function (e) {
> + setTimeout(function() {
And here.
FWIW I think it would be fine to use arrow functions here as they are supported by all browsers we test on.
Attachment #8835251 -
Flags: review?(ato)
Comment 40•8 years ago
|
||
mozreview-review |
Comment on attachment 8838771 [details]
Bug 1337133 - Add more MouseEvent event properties to synthesizeMouseAtPoint;
https://reviewboard.mozilla.org/r/113576/#review115266
::: testing/marionette/event.js:251
(Diff revision 1)
> + let inputSource = ("inputSource" in opts) ? opts.inputSource :
> + Ci.nsIDOMMouseEvent.MOZ_SOURCE_MOUSE;
So just for clarity, this is 1: https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/dist/include/nsIDOMMouseEvent.h#77
Do you mind adding a postfix comment `MOZ_SOURCE_MOUSE /* 1 */;`?
::: testing/marionette/event.js:259
(Diff revision 1)
> - if (("type" in event) && opts.type) {
> + if (("type" in opts) && opts.type) {
> domutils.sendMouseEvent(
> - opts.type, left, top, button, clickCount, modifiers);
> + opts.type, left, top, button, clickCount, modifiers, false, pressure, inputSource,
> + isDOMEventSynthesized, isWidgetEventSynthesized, buttons);
> } else {
> domutils.sendMouseEvent(
> - "mousedown", left, top, button, clickCount, modifiers);
> + "mousedown", left, top, button, clickCount, modifiers, false, pressure, inputSource,
> + isDOMEventSynthesized, isWidgetEventSynthesized, buttons);
> domutils.sendMouseEvent(
> - "mouseup", left, top, button, clickCount, modifiers);
> + "mouseup", left, top, button, clickCount, modifiers, false, pressure, inputSource,
> + isDOMEventSynthesized, isWidgetEventSynthesized, buttons);
I didn’t check these changes in detail or in practice, but trusting you they are correct.
Attachment #8838771 -
Flags: review?(ato) → review+
Comment 41•8 years ago
|
||
mozreview-review |
Comment on attachment 8835252 [details]
Bug 1337133 - Dispatch pointerDown and pointerUp actions for mouse;
https://reviewboard.mozilla.org/r/110954/#review115268
::: testing/marionette/action.js:553
(Diff revision 2)
> + /**
> + * Check whether |button| is pressed.
> + *
> + * @param {number} button
> + * Positive integer that refers to a mouse button.
> + *
> + * @return {boolean}
> + * True if |button| is in set of pressed buttons.
> + */
> + isPressed(button) {
> + return this.pressed.has(button);
> + }
If we are supposed to only accept positive integers, I would suggest adding an `assert.positiveNumber` check here for added security.
::: testing/marionette/action.js:576
(Diff revision 2)
> + *
> + * @return {Set}
> + * Set of pressed buttons.
> + */
> + press(button) {
> + return this.pressed.add(button);
Same with more security here.
::: testing/marionette/action.js:589
(Diff revision 2)
> + *
> + * @return {boolean}
> + * True if |button| was present before removals, false otherwise.
> + */
> + release(button) {
> + return this.pressed.delete(button);
And here.
::: testing/marionette/action.js:876
(Diff revision 2)
> + constructor(type, button = 0) {
> + this.type = type;
> + this.button = button;
> + this.buttons = 0;
> + }
Maybe we can add more security here too?
::: testing/marionette/action.js:882
(Diff revision 2)
> + update(inputState) {
> + let allButtons = Array.from(inputState.pressed);
> + this.buttons = allButtons.reduce((a, i) => a + Math.pow(2, i), 0);
> + }
Nice. Joy to read.
::: testing/marionette/action.js:1069
(Diff revision 2)
> resolve();
> });
> }
>
> /**
> + * Dispatch a pointerDown action equivalent to pressing a button on the pointer device.
Nit: This line is a bit long.
::: testing/marionette/action.js:1091
(Diff revision 2)
> + case action.PointerType.mouse:
> + let mouseEvent = new action.Mouse("mousedown", a.button);
> + mouseEvent.update(inputState);
> + event.synthesizeMouseAtPoint(inputState.x, inputState.y, mouseEvent, win);
> + break;
> + case action.PointerType.pen:
> + case action.PointerType.touch:
> + throw new UnsupportedOperationError("Only 'mouse' pointer type is supported.");
You appear to have the wrong casing for `mouse`, `pen`, and `touch` here.
They should be:
- `action.PointerType.Mouse`
- `action.PointerType.Pen`
- `action.PointerType.Touch`
It also would make sense to add a default case wherein you throw `TypeError` if `inputState.subtype` isn’t recognised.
::: testing/marionette/action.js:1105
(Diff revision 2)
> + resolve();
> + });
> +}
> +
> +/**
> + * Dispatch a pointerUp action equivalent to releasing a button on the pointer device
Break at ~75 character (or run the line through fmt(1) (-:).
Also add a final stop.
::: testing/marionette/action.js:1124
(Diff revision 2)
> + switch (inputState.subtype) {
> + case action.PointerType.mouse:
> + let mouseEvent = new action.Mouse("mouseup", a.button);
> + mouseEvent.update(inputState);
> + event.synthesizeMouseAtPoint(inputState.x, inputState.y,
> + mouseEvent, win);
> + break;
> + case action.PointerType.pen:
> + case action.PointerType.touch:
> + throw new UnsupportedOperationError("Only 'mouse' pointer type is supported.");
> + }
I am tempted to ask you to add a default case here too.
Also add a new line break betwen the two branches.
::: testing/web-platform/tests/webdriver/actions/mouse_actions.py:6
(Diff revision 2)
> from support.refine import get_events
>
>
> def test_nothing(session, test_actions_page, mouse_chain):
> mouse_chain \
> - .pointer_down(1) \
> + .pointer_down(0) \
Maybe consider introducing a constant in the future so that we don’t have magic numbers floating around.
Attachment #8835252 -
Flags: review?(ato) → review+
Comment 42•8 years ago
|
||
mozreview-review |
Comment on attachment 8835253 [details]
Bug 1337133 - Dispatch pointerMove action for mouse;
https://reviewboard.mozilla.org/r/110956/#review115272
I made a cursory pass on this patch today regarding its technical aspects. I want to go over it in more detail next week to compare it to the specification prose.
::: testing/marionette/action.js:971
(Diff revision 2)
> + * @param {obj=} center
> + * Object representing x and y coordinates of an element center-point.
> + * This is only used if |a.origin| is a web element reference.
I would use `Map.<string, number>` as type information.
::: testing/marionette/action.js:978
(Diff revision 2)
> + * This is only used if |a.origin| is a web element reference.
> + *
> + * @return {Array.<number>}
> + * x and y coordinates of pointer destination.
> + */
> +action.computePointerDestination = function(a, inputState, center) {
Mark `center` as optional:
```js
center = undefined
```
::: testing/marionette/action.js:979
(Diff revision 2)
> + let x = a.x;
> + let y = a.y;
This is of course absolutely fine, but you could shorten it to
```js
const {x, y} = a;
```
::: testing/marionette/action.js:996
(Diff revision 2)
> + assert.in("x", center);
> + assert.in("y", center);
> + x += center.x;
> + y += center.y;
> + }
> + return [x, y];
Unless you have a specific reason to return a two-element array, I would suggest changing this to return a dictionary with `x` and `y` fields.
This would have parity with the input to this function, be in sync with what we do elsewhere in Marionette when we operate on coordinates, and have a clearer syntactical meaning since an Array can have an arbitrary number of elements.
::: testing/marionette/action.js:1200
(Diff revision 2)
> + * as appropriate.
> + */
> +function dispatchPointerMove(a, inputState, tickDuration, seenEls, container) {
> + const timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> + // interval between pointermove increments in ms, based on common vsync: 60fps
> + const refresh = 17;
I would rename this `fps60`.
::: testing/marionette/action.js:1208
(Diff revision 2)
> + let uuid = a.origin[element.Key] || a.origin[element.LegacyKey];
> + let rects = seenEls.get(uuid, container).getClientRects();
> + elementCenter = element.getInViewCentrePoint(rects[0], container.frame);
Note that `rects[0]` may be empty if the element is outside the viewport. You can either check if `rects` has a length of over 0, or call `element.isInView` first.
::: testing/marionette/action.js:1214
(Diff revision 2)
> + throw new MoveTargetOutOfBoundsError(
> + `(${targetX}, ${targetY}) is out of bounds of viewport ` +
> + `width (${container.frame.innerWidth}) and height (${container.frame.innerHeight}).`);
Drop full stop in error message.
::: testing/marionette/action.js:1229
(Diff revision 2)
> + return;
> + }
> +
> + const distanceX = targetX - startX;
> + const distanceY = targetY - startY;
> + let intermediatePointerEvents = Task.spawn(function*() {
Space after `function*`.
::: testing/marionette/action.js:1230
(Diff revision 2)
> + }
> +
> + const distanceX = targetX - startX;
> + const distanceY = targetY - startY;
> + let intermediatePointerEvents = Task.spawn(function*() {
> + // wait |refresh| ms before performing first incremental pointer move
s/refresh/60fps/
::: testing/marionette/action.js:1231
(Diff revision 2)
> + yield new Promise(resolveTimer =>
> + timer.initWithCallback(resolveTimer, refresh, Ci.nsITimer.TYPE_ONE_SHOT)
> + );
Move the `);` up to :1232.
::: testing/marionette/action.js:1232
(Diff revision 2)
> + const distanceX = targetX - startX;
> + const distanceY = targetY - startY;
> + let intermediatePointerEvents = Task.spawn(function*() {
> + // wait |refresh| ms before performing first incremental pointer move
> + yield new Promise(resolveTimer =>
> + timer.initWithCallback(resolveTimer, refresh, Ci.nsITimer.TYPE_ONE_SHOT)
To shorten these lines, because you call it multiple times, I would suggest assigining `Ci.nsITimer.TYPE_ONE_SHOT` to a constant at the beginning of the function:
```js
const ONE_SHOT = Ci.nsITimer.TYPE_ONE_SHOT;
```
::: testing/marionette/action.js:1234
(Diff revision 2)
> + let durationRatio = Math.floor(Date.now() - start) / duration;
> + const epsilon = refresh / 1000;
> + while ((1 - durationRatio) > epsilon) {
> + let x = Math.floor(durationRatio * distanceX + startX);
> + let y = Math.floor(durationRatio * distanceY + startY);
> + performOnePointerMove(inputState, x, y, container.frame);
> + // wait |refresh| ms before performing next pointer move
> + yield new Promise(resolveTimer =>
> + timer.initWithCallback(resolveTimer, refresh, Ci.nsITimer.TYPE_ONE_SHOT)
> + );
> + durationRatio = Math.floor(Date.now() - start) / duration;
> + }
I would request some feedback from jgraham on this, to check if this is his intention. It looks technically correct to me.
::: testing/marionette/action.js:1249
(Diff revision 2)
> + durationRatio = Math.floor(Date.now() - start) / duration;
> + }
> + });
> + // perform last pointer move after all incremental moves are resolved and
> + // durationRatio is close enough to 1
> + intermediatePointerEvents.then(function() {
Use a pointer function or place a space after `function`.
::: testing/marionette/action.js:1261
(Diff revision 2)
> + switch (inputState.subtype) {
> + case action.PointerType.mouse:
> + let mouseEvent = new action.Mouse("mousemove");
> + mouseEvent.update(inputState);
> + //TODO both pointermove (if available) and mousemove
> + event.synthesizeMouseAtPoint(targetX, targetY, mouseEvent, win);
> + break;
> + case action.PointerType.pen:
> + case action.PointerType.touch:
> + throw new UnsupportedOperationError("Only 'mouse' pointer type is supported.");
> + }
Add a default case, just… in case.
::: testing/marionette/action.js:1270
(Diff revision 2)
> + //TODO both pointermove (if available) and mousemove
> + event.synthesizeMouseAtPoint(targetX, targetY, mouseEvent, win);
> + break;
> + case action.PointerType.pen:
> + case action.PointerType.touch:
> + throw new UnsupportedOperationError("Only 'mouse' pointer type is supported.");
Drop full stop.
::: testing/marionette/action.js:1315
(Diff revision 2)
> +function inViewPort(x, y, win) {
> + assert.number(x);
> + assert.number(y);
> + // Viewport includes scrollbars if rendered.
> + return !(x < 0 || y < 0 || x > win.innerWidth || y > win.innerHeight);
> +}
Please use `element.isInView` instead.
::: testing/marionette/test_action.js:214
(Diff revision 2)
> + run_next_test();
> +});
> +
> +add_test(function test_computePointerDestinationElement() {
> + // origin represents a web element
> + let act = { type: "pointerMove", x: 100, y: 200, origin: {}};
Can an origin be a dict?
Attachment #8835253 -
Flags: review?(ato) → review+
Comment 43•8 years ago
|
||
mozreview-review |
Comment on attachment 8838772 [details]
Bug 1337133 - Add a wdspec test for mouse actions;
https://reviewboard.mozilla.org/r/113578/#review115274
::: testing/web-platform/tests/webdriver/actions/mouse_actions.py:5
(Diff revision 1)
> + x = 82
> + y = 187
Are these randomly chosen coordinates or do they have some significance?
Attachment #8838772 -
Flags: review?(ato) → review+
Assignee | ||
Comment 44•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8835250 [details]
Bug 1337133 - Add pointer actions endpoints to wdclient;
https://reviewboard.mozilla.org/r/110950/#review115260
> This appears to have been fixed now, but I guess the fix is not yet available in the geckodriver on central?
>
> In the interim I’ve started work on https://bugzilla.mozilla.org/show_bug.cgi?id=1340637 to compile geckodriver master on the build slaves. But I should point that it needs a follow-up patch to have wptrunner pick up the in-tree-built geckodriver.
>
> I don’t know if that/the followup will land before this, but just making you aware of the ongoing work in this area.
Yep, waiting for availability on central. Thanks for the summary. :)
Assignee | ||
Comment 45•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8835253 [details]
Bug 1337133 - Dispatch pointerMove action for mouse;
https://reviewboard.mozilla.org/r/110956/#review115272
> Please use `element.isInView` instead.
I'm using this to check destination pointer coordinates, which aren't necessarily associated with an element.
> Can an origin be a dict?
No, it would get rejected by `action.Action.fromJson`. Origin validation is tested in `test_processPointerMoveAction`.
The origin value in this line is a dummy value to test the `default` case in `computePointerDestination`. I'll clarify that.
Assignee | ||
Comment 46•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8838772 [details]
Bug 1337133 - Add a wdspec test for mouse actions;
https://reviewboard.mozilla.org/r/113578/#review115274
> Are these randomly chosen coordinates or do they have some significance?
Random coords that are well inside the `#outer` div that has event listeners attached in the test page. Changed the code a bit to make that clearer.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 60•8 years ago
|
||
Comment on attachment 8835252 [details]
Bug 1337133 - Dispatch pointerDown and pointerUp actions for mouse;
Could you have a quick look to see if this matches your intention in the spec?
Attachment #8835252 -
Flags: feedback?(james)
Assignee | ||
Updated•8 years ago
|
Attachment #8835253 -
Flags: feedback?(james)
Assignee | ||
Comment 61•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8835248 [details]
Bug 1337133 - Add MoveTargetOutOfBoundsError as a new Webdriver error type;
https://reviewboard.mozilla.org/r/110946/#review115256
Fixed.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8835253 -
Flags: review?(james)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8839680 -
Flags: review?(ato)
Comment 76•8 years ago
|
||
mozreview-review |
Comment on attachment 8839273 [details]
Bug 1337133 - Fix creation of pointer InputState;
https://reviewboard.mozilla.org/r/113946/#review115914
Attachment #8839273 -
Flags: review?(ato) → review+
Comment 77•8 years ago
|
||
mozreview-review |
Comment on attachment 8835251 [details]
Bug 1337133 - Set up fixtures for mouse actions in wdspec tests;
https://reviewboard.mozilla.org/r/110952/#review115916
Attachment #8835251 -
Flags: review?(ato) → review+
Comment 78•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8835253 [details]
Bug 1337133 - Dispatch pointerMove action for mouse;
https://reviewboard.mozilla.org/r/110956/#review115272
> I'm using this to check destination pointer coordinates, which aren't necessarily associated with an element.
Right, yes, understood.
Comment 79•8 years ago
|
||
mozreview-review |
Comment on attachment 8839680 [details]
Bug 1337133 - Move update of inputStateMap to action.Sequence.fromJson;
https://reviewboard.mozilla.org/r/114258/#review115926
Attachment #8839680 -
Flags: review?(ato) → review+
Comment 80•8 years ago
|
||
mozreview-review |
Comment on attachment 8835253 [details]
Bug 1337133 - Dispatch pointerMove action for mouse;
https://reviewboard.mozilla.org/r/110956/#review115920
::: testing/marionette/action.js:1228
(Diff revision 4)
> + * as appropriate.
> + */
> +function dispatchPointerMove(a, inputState, tickDuration, seenEls, container) {
> + const timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> + // interval between pointermove increments in ms, based on common vsync
> + const fps60 = 17;
I wonder if the 2/3ms per step drift from 60fps is important here.
::: testing/marionette/action.js:1240
(Diff revision 4)
> + if (element.isWebElementReference(a.origin)) {
> + let uuid = a.origin[element.Key] || a.origin[element.LegacyKey];
> + let el = seenEls.get(uuid, container);
> + let rects = el.getClientRects();
> + if (rects.length == 0) {
> + throw new MoveTargetOutOfBoundsError(error.pprint`Target element ${el}` +
I don't see this in the spec. Am I missing something?
::: testing/marionette/action.js:1270
(Diff revision 4)
> + // wait |fps60| ms before performing first incremental pointer move
> + yield new Promise(resolveTimer =>
> + timer.initWithCallback(resolveTimer, fps60, ONE_SHOT)
> + );
> + let durationRatio = Math.floor(Date.now() - start) / duration;
> + const epsilon = fps60 / 1000;
It's not really clear where this comes from. It seems like what you actully care about is whether `time - start > duration - fps60` i.e. whether we are at a time where we should reach the target in the next interval.
::: testing/marionette/action.js:1304
(Diff revision 4)
> + //TODO both pointermove (if available) and mousemove
> + event.synthesizeMouseAtPoint(targetX, targetY, mouseEvent, win);
> + break;
> + case action.PointerType.Pen:
> + case action.PointerType.Touch:
> + throw new UnsupportedOperationError("Only 'mouse' pointer type is supported");
Hopefully we already checked this earlier when processing the actions? Otherwise one can end up with a partially complete action chain.
::: testing/marionette/test_action.js:186
(Diff revision 4)
>
> }
> run_next_test();
> });
>
> +add_test(function test_computePointerDestinationViewport() {
I would really like to see some wpt tests for this feature. Is there some reason that's hard at the moment?
Attachment #8835253 -
Flags: review?(james) → review-
Assignee | ||
Comment 81•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8835253 [details]
Bug 1337133 - Dispatch pointerMove action for mouse;
https://reviewboard.mozilla.org/r/110956/#review115920
> I wonder if the 2/3ms per step drift from 60fps is important here.
The spec says "model the overall pointer move as a series of small movements occuring at an implementation defined rate (e.g. one movement per vsync)." so my thinking is that an interval that is slightly less frequent than vsync guarantees that we don't dispatch an unnecessarily high number of movements.
> I don't see this in the spec. Am I missing something?
Whoops, brain fail: I was thinking of the element as the target, not the origin, when I added that line.
Since it's actually alright for the origin to be outide the viewport, I can also simplify this section to use `element.coordinates(el)`.
> It's not really clear where this comes from. It seems like what you actully care about is whether `time - start > duration - fps60` i.e. whether we are at a time where we should reach the target in the next interval.
You're right. Since we're looking at the duration ratio, intuitively the epsilon should be based on `fps60 / duration`. If we make it exactly that, the last movement will be up to twice as big as the others in the series. So I'll make the epsilon smaller than `fps60 / duration` to err on the side of more precise movements when we're close to the target.
> Hopefully we already checked this earlier when processing the actions? Otherwise one can end up with a partially complete action chain.
Good point, will do. (There's no earlier check, now.)
> I would really like to see some wpt tests for this feature. Is there some reason that's hard at the moment?
Agreed. I wanted to write more wpt tests as a follow-up, but now that you mention it, I see there's no geckodriver support for pointer-move origin: https://github.com/mozilla/webdriver-rust/issues/71
Assignee | ||
Comment 82•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8835253 [details]
Bug 1337133 - Dispatch pointerMove action for mouse;
https://reviewboard.mozilla.org/r/110956/#review115920
> Agreed. I wanted to write more wpt tests as a follow-up, but now that you mention it, I see there's no geckodriver support for pointer-move origin: https://github.com/mozilla/webdriver-rust/issues/71
Until geckodriver is updated, perhaps I can reassure you with the following test, which passes locally with the changes in my next review request:
```python
def test_click_element_center(session, test_actions_page, mouse_chain):
outer = session.find.css("#outer", all=False)
outer_rect = outer.rect
center_x = outer_rect["width"] / 2 + outer_rect["x"];
center_y = outer_rect["height"] / 2 + outer_rect["y"];
button = 0
mouse_chain \
# using element instead of origin to circumvent out-of-date webdriver-rust
.pointer_move(0, 0, element=outer) \
.pointer_down(button) \
.pointer_up(button) \
.perform()
events = get_events(session)
assert len(events) == 4
for e in events:
if e["type"] == "mousedown":
# TODO use pytest.approx once we upgrade to pytest > 3.0
assert abs(e["pageX"] - center_x) < 1
assert abs(e["pageY"] - center_y) < 1
assert e["target"] == "outer"
```
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 91•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8835253 [details]
Bug 1337133 - Dispatch pointerMove action for mouse;
https://reviewboard.mozilla.org/r/110956/#review115920
> Good point, will do. (There's no earlier check, now.)
Earlier check added in https://reviewboard.mozilla.org/r/110954/diff/4-5/
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 97•8 years ago
|
||
mozreview-review |
Comment on attachment 8840129 [details]
Bug 1337133 - Remove references to pen and touch in test_action.js;
https://reviewboard.mozilla.org/r/114648/#review116384
Attachment #8840129 -
Flags: review?(james) → review+
Comment 98•8 years ago
|
||
mozreview-review |
Comment on attachment 8835253 [details]
Bug 1337133 - Dispatch pointerMove action for mouse;
https://reviewboard.mozilla.org/r/110956/#review116388
Attachment #8835253 -
Flags: review?(james) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8835247 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 123•8 years ago
|
||
I've confirmed that the merge conflicts post-rebase haven't changed anything, and that the wpt tests pass with the harness prior to the wpt update in Bug 1340474 (which broke the wpt harness as in https://bugzilla.mozilla.org/show_bug.cgi?id=1308969#c26).
Comment 124•8 years ago
|
||
Pushed by mjzffr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7902afebdf2f
Add number check to assert.js; r=ato+446296
https://hg.mozilla.org/integration/autoland/rev/397ef772d437
Remove primary pointer attribute; r=ato+446296
https://hg.mozilla.org/integration/autoland/rev/0aa0fd540ef0
Replace element with origin for pointerMove actions; r=ato+446296
https://hg.mozilla.org/integration/autoland/rev/2d7d58a97e46
Add MoveTargetOutOfBoundsError as a new Webdriver error type; r=ato+446296
https://hg.mozilla.org/integration/autoland/rev/be1184fd9911
Split test kinds, common fixtures, utilities in wdspec tests; r=ato+446296
https://hg.mozilla.org/integration/autoland/rev/1e827bb8b11a
Add pointer actions endpoints to wdclient; r=ato+446296
https://hg.mozilla.org/integration/autoland/rev/86ed4af7aed4
Set up fixtures for mouse actions in wdspec tests; r=ato+446296
https://hg.mozilla.org/integration/autoland/rev/e7edf0e6f5f8
Add more MouseEvent event properties to synthesizeMouseAtPoint; r=ato+446296
https://hg.mozilla.org/integration/autoland/rev/6d8950160186
Fix creation of pointer InputState; r=ato+446296
https://hg.mozilla.org/integration/autoland/rev/b2fc3f9520fe
Remove references to pen and touch in test_action.js; r=jgraham
https://hg.mozilla.org/integration/autoland/rev/aba48a32380e
Dispatch pointerDown and pointerUp actions for mouse; r=ato+446296
https://hg.mozilla.org/integration/autoland/rev/356602cac222
Dispatch pointerMove action for mouse; r=ato+446296,jgraham
https://hg.mozilla.org/integration/autoland/rev/47ac31461e9d
Add a wdspec test for mouse actions; r=ato+446296
https://hg.mozilla.org/integration/autoland/rev/fa0e8a07bd1e
Move update of inputStateMap to action.Sequence.fromJson; r=ato+446296
Comment 125•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7902afebdf2f
https://hg.mozilla.org/mozilla-central/rev/397ef772d437
https://hg.mozilla.org/mozilla-central/rev/0aa0fd540ef0
https://hg.mozilla.org/mozilla-central/rev/2d7d58a97e46
https://hg.mozilla.org/mozilla-central/rev/be1184fd9911
https://hg.mozilla.org/mozilla-central/rev/1e827bb8b11a
https://hg.mozilla.org/mozilla-central/rev/86ed4af7aed4
https://hg.mozilla.org/mozilla-central/rev/e7edf0e6f5f8
https://hg.mozilla.org/mozilla-central/rev/6d8950160186
https://hg.mozilla.org/mozilla-central/rev/b2fc3f9520fe
https://hg.mozilla.org/mozilla-central/rev/aba48a32380e
https://hg.mozilla.org/mozilla-central/rev/356602cac222
https://hg.mozilla.org/mozilla-central/rev/47ac31461e9d
https://hg.mozilla.org/mozilla-central/rev/fa0e8a07bd1e
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 126•8 years ago
|
||
Sheriffs: Please uplift to Aurora and Beta under test-only.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
Comment 127•8 years ago
|
||
Beta is unrealistic because the merge from beta -> release happened yesterday. We might be able to try to uplift to esr52 later if allowed.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta] → [checkin-needed-aurora]
Comment 128•8 years ago
|
||
needs rebasing for aurora because failed like
^[[CTomcats-MBP-462:mozilla-central Tomcat$ hg graft -er 397ef772d437
grafting 401816:397ef772d437 "Bug 1337133 - Remove primary pointer attribute; r=ato+446296"
merging testing/marionette/action.js
merging testing/marionette/test_action.js
warning: conflicts while merging testing/marionette/action.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(mjzffr)
Whiteboard: [checkin-needed-aurora]
Assignee | ||
Comment 129•8 years ago
|
||
Test-only uplift to aurora and esr52, please. Please uplift Bug 1335240, first, which will fix the above conflict. Thanks.
Flags: needinfo?(mjzffr)
Whiteboard: [checkin-needed-aurora][checkin-needed-esr52]
Comment 130•8 years ago
|
||
I made it as far as rev 1e827bb8b11a before hitting more conflicts (and that was after adding bug 1336214 into the mix to get past some earlier conflicts). I think you're going to need to provide a branch patch or three here. Please check the remaining commits for conflicts as well before requesting uplift again.
Flags: needinfo?(mjzffr)
Whiteboard: [checkin-needed-aurora][checkin-needed-esr52]
Assignee | ||
Comment 131•8 years ago
|
||
Sorry. Let's try that again. Please uplift to aurora, a=testonly. Do the uplift for Bug 1328726 first.
Flags: needinfo?(mjzffr)
Assignee | ||
Comment 132•8 years ago
|
||
(I'll tackle the esr52 request when I return on Friday.)
Whiteboard: [checkin-needed-aurora]
Comment 133•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/e52897d91ec6
https://hg.mozilla.org/releases/mozilla-aurora/rev/bc5a0dc72fe0
https://hg.mozilla.org/releases/mozilla-aurora/rev/54ce59792538
https://hg.mozilla.org/releases/mozilla-aurora/rev/406823a5be09
https://hg.mozilla.org/releases/mozilla-aurora/rev/8f0667a7bd40
https://hg.mozilla.org/releases/mozilla-aurora/rev/3d937ea641e7
https://hg.mozilla.org/releases/mozilla-aurora/rev/4750fbf396d0
https://hg.mozilla.org/releases/mozilla-aurora/rev/8910e502db34
https://hg.mozilla.org/releases/mozilla-aurora/rev/e67838014e6b
https://hg.mozilla.org/releases/mozilla-aurora/rev/824f8566434b
https://hg.mozilla.org/releases/mozilla-aurora/rev/4260c76cda67
https://hg.mozilla.org/releases/mozilla-aurora/rev/f4170cf2573e
https://hg.mozilla.org/releases/mozilla-aurora/rev/3e5374111d4a
https://hg.mozilla.org/releases/mozilla-aurora/rev/5a0cb2766a77
Assignee | ||
Comment 135•8 years ago
|
||
I didn't get to setting up the other uplift patches today. The next chance I'll get will be on Tuesday.
Assignee | ||
Comment 137•8 years ago
|
||
Please uplift the attached patch to esr52, test-only.
Assignee | ||
Comment 138•8 years ago
|
||
Note: there have been lots of changes to the wpt harness since 52, including session setup and tear-down, so I had to fix conflicts and disable some tests. All the actions tests work as on 54.
Flags: needinfo?(mjzffr)
Whiteboard: [checkin-needed-esr52]
Comment 139•8 years ago
|
||
applying the pointer_actions esr52 patch cause
:
patching file testing/web-platform/meta/MANIFEST.json
Hunk #1 FAILED at 38754
1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/meta/MANIFEST.json.rej
unable to find 'testing/web-platform/meta/webdriver/actions.py.ini' for patching
(use '--prefix' to apply patch relative to the current directory)
1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/meta/webdriver/actions.py.ini.rej
unable to find 'testing/web-platform/tests/test_keys_wdspec.html' for patching
(use '--prefix' to apply patch relative to the current directory)
1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/tests/test_keys_wdspec.html.rej
unable to find 'testing/web-platform/tests/webdriver/actions.py' for patching
(use '--prefix' to apply patch relative to the current directory)
1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/tests/webdriver/actions.py.rej
unable to find 'testing/web-platform/tests/webdriver/support/keys.py' for patching
(use '--prefix' to apply patch relative to the current directory)
1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/tests/webdriver/support/keys.py.rej
patching file testing/web-platform/tests/tools/webdriver/webdriver/client.py
Hunk #1 FAILED at 77
1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/tests/tools/webdriver/webdriver/client.py.rej
testing/web-platform/tests/webdriver/support/__init__.py not tracked!
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh pointer_actions_esr52.patch
from a clean clone the directory of mozilla-esr52/testing/web-platform/tests/webdriver
Tomcats-MacBook-Pro-820:webdriver Tomcat$ ls -all
total 40
drwxr-xr-x 7 Tomcat wheel 238 Mar 13 11:09 .
drwxr-xr-x 114 Tomcat wheel 3876 Mar 13 11:09 ..
-rw-r--r-- 1 Tomcat wheel 44 Mar 13 11:09 OWNERS
-rw-r--r-- 1 Tomcat wheel 2458 Mar 13 11:09 README.md
-rw-r--r-- 1 Tomcat wheel 900 Mar 13 11:09 contexts.py
-rw-r--r-- 1 Tomcat wheel 506 Mar 13 11:09 interface.html
-rw-r--r-- 1 Tomcat wheel 3062 Mar 13 11:09 navigation.py
so that seems to explain some errors from above.
Maja do i miss something here ?
Flags: needinfo?(mjzffr)
Assignee | ||
Comment 140•8 years ago
|
||
Thanks for the info. Could you try again? I'm fairly confident that the patch should apply cleanly. It does on my end, even on a fresh clone.
If you look at https://hg.mozilla.org/releases/mozilla-esr52/file/tip/testing/web-platform/tests/webdriver, the expected files, like actions.py, are there.
Those files got added to esr52 in the following push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr52&revision=a1b92421760f87cc141ea33aa51e7206cf1d17df
Namely, with:
> hg log --removed --stat testing/web-platform/tests/webdriver/actions.py testing/web-platform/meta/webdriver/actions.py.ini testing/web-platform/tests/test_keys_wdspec.html testing/web-platform/tests/webdriver/support/keys.py testing/web-platform/tests/webdriver/support/__init__.py -r "::esr52"
We can see:
changeset: 398788:f98c4db03e53
user: Maja Frydrychowicz
date: Tue Jan 24 16:59:04 2017 -0500
summary: Bug 1328726 - Add web-platform wdspec tests for key actions. r=ato, r=jgraham, a=test-only
testing/web-platform/tests/test_keys_wdspec.html | 55 ++++++++
testing/web-platform/tests/webdriver/actions.py | 231 +++++++++++++++++++++++++++++++++++++
testing/web-platform/tests/webdriver/support/__init__.py | 0
testing/web-platform/tests/webdriver/support/keys.py | 812 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 1098 insertions(+), 0 deletions(-)
changeset: 398789:17913deae97f
user: Maja Frydrychowicz
date: Wed Mar 08 17:22:58 2017 -0500
summary: Bug 1328726 - Add wdspec test for emoji in key actions. r=ato, r=jgraham, a=test-only
testing/web-platform/tests/test_keys_wdspec.html | 31 +++++++++++++++++++++++--------
testing/web-platform/tests/webdriver/actions.py | 48 ++++++++++++++++++++++++++++++++++++++----------
2 files changed, 61 insertions(+), 18 deletions(-)
changeset: 398790:a1b92421760f
user: Maja Frydrychowicz
date: Wed Jan 25 17:16:45 2017 -0500
summary: Bug 1328726 - Disable wdspec tests on linux64 debug. r=jgraham, a=test-only
testing/web-platform/meta/webdriver/actions.py.ini | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
One of the changesets in the supplied patch moves a bunch of these files elsewhere (Bug 1337133 - Split test kinds, common fixtures, utilities in wdspec tests.) Maybe that's where you're running into trouble? But again, the patch import works for me.
Thanks.
Flags: needinfo?(mjzffr)
Comment 141•8 years ago
|
||
Hi Maja, i tried again and it failed again with a super new clone.
From
cannot patch testing/web-platform/tests/webdriver/actions/conftest.py: file is not tracked.
Log from the patching
renaming 1337133 to pointer_actions_esr52.patch
applying pointer_actions_esr52.patch
patching file testing/marionette/assert.js
patching file testing/marionette/test_assert.js
patching file testing/marionette/action.js
patching file testing/marionette/test_action.js
patching file testing/marionette/action.js
patching file testing/marionette/test_action.js
patching file testing/marionette/error.js
patching file testing/web-platform/meta/MANIFEST.json
patching file testing/web-platform/meta/webdriver/actions.py.ini
patching file testing/web-platform/meta/webdriver/actions/key.py.ini
patching file testing/web-platform/meta/webdriver/actions/sequence.py.ini
patching file testing/web-platform/tests/test_keys_wdspec.html
patching file testing/web-platform/tests/webdriver/actions.py
patching file testing/web-platform/tests/webdriver/actions/conftest.py
patching file testing/web-platform/tests/webdriver/actions/key.py
patching file testing/web-platform/tests/webdriver/actions/mouse.py
patching file testing/web-platform/tests/webdriver/actions/sequence.py
patching file testing/web-platform/tests/webdriver/actions/support/keys.py
patching file testing/web-platform/tests/webdriver/actions/support/refine.py
patching file testing/web-platform/tests/webdriver/actions/support/test_actions_wdspec.html
patching file testing/web-platform/tests/webdriver/support/keys.py
patching file testing/web-platform/tests/tools/webdriver/webdriver/client.py
patching file testing/web-platform/tests/webdriver/actions/conftest.py
adding testing/web-platform/meta/webdriver/actions/key.py.ini
adding testing/web-platform/meta/webdriver/actions/sequence.py.ini
adding testing/web-platform/tests/webdriver/actions/__init__.py
adding testing/web-platform/tests/webdriver/actions/conftest.py
adding testing/web-platform/tests/webdriver/actions/key.py
adding testing/web-platform/tests/webdriver/actions/mouse.py
adding testing/web-platform/tests/webdriver/actions/sequence.py
adding testing/web-platform/tests/webdriver/actions/support/__init__.py
adding testing/web-platform/tests/webdriver/actions/support/keys.py
adding testing/web-platform/tests/webdriver/actions/support/refine.py
adding testing/web-platform/tests/webdriver/actions/support/test_actions_wdspec.html
cannot patch testing/web-platform/tests/webdriver/actions/conftest.py: file is not tracked
committing files:
testing/marionette/action.js
testing/marionette/assert.js
testing/marionette/error.js
testing/marionette/test_action.js
testing/marionette/test_assert.js
testing/web-platform/meta/MANIFEST.json
testing/web-platform/meta/webdriver/actions/key.py.ini
testing/web-platform/meta/webdriver/actions/sequence.py.ini
testing/web-platform/tests/tools/webdriver/webdriver/client.py
testing/web-platform/tests/webdriver/actions/__init__.py
testing/web-platform/tests/webdriver/actions/conftest.py
testing/web-platform/tests/webdriver/actions/key.py
testing/web-platform/tests/webdriver/actions/mouse.py
testing/web-platform/tests/webdriver/actions/sequence.py
testing/web-platform/tests/webdriver/actions/support/__init__.py
testing/web-platform/tests/webdriver/actions/support/keys.py
testing/web-platform/tests/webdriver/actions/support/refine.py
testing/web-platform/tests/webdriver/actions/support/test_actions_wdspec.html
committing manifest
committing changelog
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh pointer_actions_esr52.patch
Updated•8 years ago
|
Flags: needinfo?(mjzffr)
Assignee | ||
Comment 142•8 years ago
|
||
Not sure why it's not working for you.
<maja_zf> KWierso: hey, could you check if this uplift patch applies cleanly for you? https://bugzilla.mozilla.org/show_bug.cgi?id=1337133#c137 -- I can't reproduce tomcat's issue. Maybe the difference is that he's using mq and I'm not?
<•KWierso> maja_zf: yeah, applies to esr52 just fine for me
Flags: needinfo?(mjzffr)
Assignee | ||
Comment 143•8 years ago
|
||
The same patch (https://bug1337133.bmoattachments.org/attachment.cgi?id=8846216) should also be uplifted to release. I've tested the patch locally and it applies without issue. (Bug 1328726 needs to be uplifted first.)
Whiteboard: [checkin-needed-esr52] → [checkin-needed-esr52] [checkin-needed-release]
Comment 144•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-release/rev/2cff78b88526
https://hg.mozilla.org/releases/mozilla-release/rev/4a49dbd38732
https://hg.mozilla.org/releases/mozilla-release/rev/a3645d3d9dc9
https://hg.mozilla.org/releases/mozilla-release/rev/35e40eed53c2
https://hg.mozilla.org/releases/mozilla-release/rev/64cb9487b8e1
https://hg.mozilla.org/releases/mozilla-release/rev/03e196a5fefb
https://hg.mozilla.org/releases/mozilla-release/rev/13704f1cb196
https://hg.mozilla.org/releases/mozilla-release/rev/2e80487f86d2
https://hg.mozilla.org/releases/mozilla-release/rev/c17cd42146ea
https://hg.mozilla.org/releases/mozilla-release/rev/074b4e2bfff2
https://hg.mozilla.org/releases/mozilla-release/rev/d95e363adab9
https://hg.mozilla.org/releases/mozilla-release/rev/7244bd8e6337
https://hg.mozilla.org/releases/mozilla-release/rev/8e6d1517386f
https://hg.mozilla.org/releases/mozilla-release/rev/3ea6ba421277
status-firefox52:
--- → fixed
Whiteboard: [checkin-needed-esr52] [checkin-needed-release] → [checkin-needed-esr52]
Comment 145•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/c0a30c1a174a
https://hg.mozilla.org/releases/mozilla-esr52/rev/ce6f4800781e
https://hg.mozilla.org/releases/mozilla-esr52/rev/b23610725e5c
https://hg.mozilla.org/releases/mozilla-esr52/rev/6a3b5e11c098
https://hg.mozilla.org/releases/mozilla-esr52/rev/97ccb7e44e22
https://hg.mozilla.org/releases/mozilla-esr52/rev/9b38c30e945c
https://hg.mozilla.org/releases/mozilla-esr52/rev/8e4a7f111e7c
https://hg.mozilla.org/releases/mozilla-esr52/rev/2886d0ee5adb
https://hg.mozilla.org/releases/mozilla-esr52/rev/b517b9486a6d
https://hg.mozilla.org/releases/mozilla-esr52/rev/b55ac003ed08
https://hg.mozilla.org/releases/mozilla-esr52/rev/3c798cc31016
https://hg.mozilla.org/releases/mozilla-esr52/rev/2ef39668b34a
https://hg.mozilla.org/releases/mozilla-esr52/rev/b9837fddde95
https://hg.mozilla.org/releases/mozilla-esr52/rev/28f25140ed06
status-firefox-esr52:
--- → fixed
Whiteboard: [checkin-needed-esr52]
Comment 146•8 years ago
|
||
Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f257c058033a
Add pointer actions endpoints to wdclient; r=ato+446296
Comment 147•8 years ago
|
||
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd23eea2fb3f
Backed out changeset f257c058033a
Comment 148•8 years ago
|
||
Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/mozilla-inbound/rev/85aec7a95087
Add pointer actions endpoints to wdclient; r=ato+446296
![]() |
||
Comment 149•8 years ago
|
||
bugherder |
Comment 150•8 years ago
|
||
Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4d0ea9dc717
Add pointer actions endpoints to wdclient; r=ato+446296
Comment 151•8 years ago
|
||
bugherder |
Comment 152•8 years ago
|
||
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/7fc722252862
Backed out changeset b4d0ea9dc717
![]() |
||
Comment 153•8 years ago
|
||
backout bugherder |
Backed out for neverending W(7) jobs on Windows 7 VM debug:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f98b750bb85085a61341dc267f5ebf313820f960&filter-searchStr=2e76e93115e24a1caffbba81d961e84fb651fa8c
https://hg.mozilla.org/mozilla-central/rev/7fc722252862
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
![]() |
||
Updated•8 years ago
|
Flags: needinfo?(james)
Updated•8 years ago
|
Flags: needinfo?(james)
Assignee | ||
Comment 154•8 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
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
•