Closed
Bug 1337133
Opened 7 years ago
Closed 7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 years ago
|
Attachment #8835253 -
Flags: feedback?(james)
Assignee | ||
Comment 61•7 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•7 years ago
|
Attachment #8835253 -
Flags: review?(james)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8839680 -
Flags: review?(ato)
Comment 76•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 126•7 years ago
|
||
Sheriffs: Please uplift to Aurora and Beta under test-only.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
Comment 127•7 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•7 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•7 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•7 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•7 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•7 years ago
|
||
(I'll tackle the esr52 request when I return on Friday.)
Whiteboard: [checkin-needed-aurora]
Comment 133•7 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•7 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•7 years ago
|
||
Please uplift the attached patch to esr52, test-only.
Assignee | ||
Comment 138•7 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•7 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•7 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•7 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•7 years ago
|
Flags: needinfo?(mjzffr)
Assignee | ||
Comment 142•7 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•7 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•7 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•7 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•7 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•7 years ago
|
||
Backout by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/mozilla-inbound/rev/cd23eea2fb3f Backed out changeset f257c058033a
Comment 148•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/85aec7a95087
Comment 150•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b4d0ea9dc717
Comment 152•7 years ago
|
||
Backout by archaeopteryx@coole-files.de: https://hg.mozilla.org/mozilla-central/rev/7fc722252862 Backed out changeset b4d0ea9dc717
Comment 153•7 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•7 years ago
|
Flags: needinfo?(james)
Updated•7 years ago
|
Flags: needinfo?(james)
Assignee | ||
Comment 154•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1ee09646e328
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•