Closed Bug 1320389 Opened 6 years ago Closed 6 years ago

Implement dispatch of key actions in content context

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox52 fixed, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: impossibus, Assigned: impossibus)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

Debug output and test data
24.77 KB, text/plain
Details
0001-Bug-1320389-Rename-legacyactions-r-ato.patch
7.37 KB, patch
Details | Diff | Splinter Review
0002-Bug-1320389-Implement-dispatch-of-key-actions-rebase.patch
50.28 KB, patch
Details | Diff | Splinter Review 
Attaching my test data and debug output in case it helps to visualize what the code is doing with key events and such.
Assignee: nobody → mjzffr
Comment on attachment 8814498 [details]
Bug 1320389 - Rename legacyactions;

https://reviewboard.mozilla.org/r/95706/#review95934

::: testing/marionette/legacyaction.js:20
(Diff revision 1)
>  
> -this.EXPORTED_SYMBOLS = ["action"];
> +this.EXPORTED_SYMBOLS = ["legacyaction"];
>  
>  const logger = Log.repository.getLogger("Marionette");
>  
> -this.action = {};
> +this.legacyaction = this.action = {};

I guess it’s fine to just alias it like this since it will hopefully soon be removed.
Attachment #8814498 - Flags: review?(ato) → review+
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review95936

This really looks very good!  I see you also unrooted some bugs in testing/marionette/event.js, which I kind of expected.

The one thing I really miss here are functional tests, but I guess this is hard because we haven’t landed support for the action commands in geckodriver yet.  We should get that sorted as soon as possible so that we can write meaningful tests.  Undecided if we need to do that before landing this, but it’s a point of fact that the WebDriver algorithm has not been proven in practice yet, and the sooner we discover bugs with it, the less likely we are to make fundamental design errors in our implementation.

::: testing/marionette/action.js:353
(Diff revision 1)
>   * Input state associated with current session. This is a map between input ID and
>   * the device state for that input source, with one entry for each active input source.
> + *
> + * Initialized in listener.js
>   */
> -action.inputStateMap = new Map();
> +action.inputStateMap = undefined;

This is undefined by default.

::: testing/marionette/action.js:362
(Diff revision 1)
> + * events when resetting the state of the input sources. Reset operations are assumed
> + * to be idempotent.
> + *
> + * Initialized in listener.js
> + */
> +action.inputsToCancel = undefined;

Same.

::: testing/marionette/action.js:437
(Diff revision 1)
> +    if (["Alt", "Shift", "Control", "Meta"].includes(key)) {
> +      let k = key == "Control" ? "ctrl" : key.toLowerCase();

I’d prefer a lookup table from UI Events modifier key names to the WebDriver names, although I must say I really don’t understand why WebDriver just doesn’t call them the same things as UI Events does.

::: testing/marionette/action.js:545
(Diff revision 1)
> +          throw new InvalidArgumentError(
> +              "Expected 'value' to be a string that represents single code point, got: " + key);

Technically we are supposed to accept grapheme clusters, which could consist of multiple code points, but this is fine for now.

::: testing/marionette/action.js:761
(Diff revision 1)
> +    this.altKey = inputState.alt;
> +    this.shiftKey = inputState.shift;
> +    this.ctrlKey = inputState.ctrl;
> +    this.metaKey = inputState.meta;

These properties need to be defined in the constructor as well.  We want them to be “discoverable” when the object is constructed but this function has not yet been called.

Since modifier state can be combined, I wonder if a bitmap would be a better choice of data type here?

::: testing/marionette/action.js:769
(Diff revision 1)
> +    this.metaKey = inputState.meta;
> +  }
> +};
> +
> +/**
> + * Dispatch actions by tick

I think this documentation can be improved.  We need to point out that this dispatches an _action chain that consists of sequences of ticks_.

Can you also elaborate a bit on how this is different to `action.dispatchTickActions`?

::: testing/marionette/action.js:782
(Diff revision 1)
> +      // TODO (?) Wait until a bunch of conditions are met
> +      // setTimeout tick duration??

We will probably do the wait on pointerMove duration when pointerMove is dispatch.  Since this hasn’t been implemented yet it’s fine to leave a TODO here.

For the second condition, about waiting for the event loop to have spun enough times to generate the right events, it would probably be fine to wait for `requestAnimationFrame`.

::: testing/marionette/action.js:814
(Diff revision 1)
> +      case action.PointerDown:
> +        throw new UnsupportedOperationError();
> +        break;
> +
> +      case action.PointerUp:
> +        throw new UnsupportedOperationError();
> +        break;
> +
> +      case action.PointerMove:
> +        throw new UnsupportedOperationError();
> +        break;
> +
> +      case action.PointerCancel:
> +        throw new UnsupportedOperationError();
> +        break;

You can pull these together, and the `break` is unnecessary:

```js
case action.PointerDown:
case action.PointerUp:
case action.PointerMove:
case action.PointerCancel:
  throw new UnsupportedOperationError();
```

::: testing/marionette/action.js:871
(Diff revision 1)
> +  if (!inputState.pressed.has(keyEvent.key)) {
> +    return;
> +  }

I wonder if we should hide this implementation detail inside `action.InputState.Key` by defining a new method `isPressed` that calls the `pressed` map internal property.

::: testing/marionette/action.js:875
(Diff revision 1)
> +  let keyEvent = new action.Key(a.value);
> +  if (!inputState.pressed.has(keyEvent.key)) {
> +    return;
> +  }
> +  inputState.setModState(keyEvent.key, false);
> +  inputState.pressed.delete(keyEvent.key);

Same as above.  I think we ought to hide this.  Perhaps `inputState.depress(keyEvent.key)` is a good API?

::: testing/marionette/action.js:891
(Diff revision 1)
> + * @return {number}
> + *     Longest action duration in |tickActions| if any, or 0.
> + */
> +action.computeTickDuration = function(tickActions) {
> +  let max = 0;
> +  let duration = undefined;

A variable’s implicit value is undefined, so the `= undefined` assignment can be left out.

I also think this should be defined inside the for-loop, quite possibly as late as inside the if-condition.  We don’t really need this variable unless.

::: testing/marionette/action.js:893
(Diff revision 1)
> + */
> +action.computeTickDuration = function(tickActions) {
> +  let max = 0;
> +  let duration = undefined;
> +  for (let a of tickActions) {
> +    if (a.subtype == action.Pause || (a.type == "pointer" && a.subtype == action.PointerMove)) {

I just noticed that we [don’t appear to check the action sequence’s `type` property](https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/action.js#L216) when it is passed in.  I wonder if we should possibly make this an enum like you have done for subtypes (`action.PointerMove`).

Feel free to defer this to later.

::: testing/marionette/action.js:893
(Diff revision 1)
> +    if (a.subtype == action.Pause || (a.type == "pointer" && a.subtype == action.PointerMove)) {
> +      duration = a['duration'];
> +    }
> +    if (duration != undefined) {
> +      max = Math.max(duration, max);
> +    }

This seems a bit tideous.  I am aware this is exactly how the specification is written, but there’s room to make this more ideomatic.

I would suggest:

```js
let affectsWallClockTime = a.subtype == action.Pause ||
    (a.type == "pointer" && a.subtype == action.PointerMove));
if (affectsWallClockTime && a.duration) {
  max = Math.max(a.duration, max);
}
```

You could even filter out the actions that it doesn’t apply to first:

```js
let affectsWallClockTime = a => a.subtype == action.Pause ||
    (a.type == "pointer" && a.subtype == action.PointerMove));

let max = 0;
for (let a of tickActions.filter(affectsWallClockTime)) {
  if (a.duration) {
    max = Math.max(a.duration, max);
  }
}

return max;
```

::: testing/marionette/driver.js:1610
(Diff revision 1)
>  };
>  
> +
> +
> +/**
> + *  Perform a series of grouped actions at the specified points in time.

Here too: a few more blank lines that needed.

::: testing/marionette/driver.js:1614
(Diff revision 1)
> +/**
> + *  Perform a series of grouped actions at the specified points in time.
> + *
> + *  @param {Array<?>} actions
> + *      Array of objects that each represent an action sequence.
> + */

One space after the `*` is sufficient.

::: testing/marionette/driver.js:1622
(Diff revision 1)
> +    case Context.CHROME:
> +        throw new UnsupportedOperationError(
> +            "Command 'performActions' is not available in chrome context");
> +      break;

Indentation of the `throw` statement is wrong.

::: testing/marionette/driver.js:1622
(Diff revision 1)
> +    case Context.CHROME:
> +        throw new UnsupportedOperationError(
> +            "Command 'performActions' is not available in chrome context");
> +      break;

The `break` statement here is not needed.

::: testing/marionette/driver.js:1626
(Diff revision 1)
> +    case Context.CONTENT:
> +      yield this.listener.performActions({'actions': cmd.parameters.actions});
> +      break;

You can actually return the promise and it will be resolved automatically, which would make the `break` statement superfluous.

::: testing/marionette/driver.js:1627
(Diff revision 1)
> +    case Context.CHROME:
> +        throw new UnsupportedOperationError(
> +            "Command 'performActions' is not available in chrome context");
> +      break;
> +    case Context.CONTENT:
> +      yield this.listener.performActions({'actions': cmd.parameters.actions});

Please use double quotes.

::: testing/marionette/harness/marionette/tests/unit/test_click.py:65
(Diff revision 1)
>          test_html = self.marionette.absolute_url("test.html")
>          self.marionette.navigate(test_html)
>          link = self.marionette.find_element(By.ID, "mozLink")
>          link.click()
>          self.assertEqual("Clicked", self.marionette.execute_script("return document.getElementById('mozLink').innerHTML;"))
> +        import pdb; pdb.set_trace();

This causes a number of tests to fail on try.  I’m guessing it’s leftover from debugging.

Please re-run tests when you have a chance.

::: testing/marionette/listener.js:680
(Diff revision 1)
>  /**
> + * Perform a series of grouped actions at the specified points in time.
> + *
> + *
> + * @param {obj} msg
> + *      Object with an |actions| attribute that is an Array of objects
> + *      each of which represents an action sequence.
> + */

Superfluous blank line.

::: testing/marionette/test_action.js:13
(Diff revision 1)
>  
>  Cu.import("chrome://marionette/content/action.js");
>  Cu.import("chrome://marionette/content/element.js");
>  Cu.import("chrome://marionette/content/error.js");
>  
> +action.inputStateMap = new Map();

Should `actions.inputsToCancel` also be initialised here?

::: testing/marionette/test_action.js:37
(Diff revision 1)
>  });
>  
>  add_test(function test_processPointerParameters() {
>    let check = (regex, message, arg) => checkErrors(
>        regex, action.PointerParameters.fromJson, [arg], message);
> -  let parametersData = {pointerType: "foo"};
> +  let parametersData = {pointerType: "foo", primary: undefined};

Missing properties in the data input are undefined by default.

::: testing/marionette/test_action.js:474
(Diff revision 1)
> +    // invalid action, should be ignored
> +    {type: "key", subtype: "keyDown", duration: 100},

Is the action itself invalid, or the `duration` property?  I don’t think the key down action itself is ignored in this case?

::: testing/marionette/test_action.js:490
(Diff revision 1)
> +    // invalid action, should be ignored
> +    {type: "key", subtype: "keyDown", duration: 100},

Same as above.
Attachment #8814499 - Flags: review?(ato) → review-
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review95936

Agreed. This week, my main goal will be to tackle the functional tests.

> We will probably do the wait on pointerMove duration when pointerMove is dispatch.  Since this hasn’t been implemented yet it’s fine to leave a TODO here.
> 
> For the second condition, about waiting for the event loop to have spun enough times to generate the right events, it would probably be fine to wait for `requestAnimationFrame`.

There's also duration for pause, which is available in all action types. If you can suggest how best to address that, I'm happy to take care of it now.

> I just noticed that we [don’t appear to check the action sequence’s `type` property](https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/action.js#L216) when it is passed in.  I wonder if we should possibly make this an enum like you have done for subtypes (`action.PointerMove`).
> 
> Feel free to defer this to later.

We do check the type: https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/action.js#L353-L354
Either way, I'll look into the enum idea.

> Missing properties in the data input are undefined by default.

I added it because I was getting ugly warnings in the xpcshell test results.

> Is the action itself invalid, or the `duration` property?  I don’t think the key down action itself is ignored in this case?

I'll clarify the comments. I mean the action (and therefore its duration) should be ignored _just_ when computing the tick duration.
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review95936

> I added it because I was getting ugly warnings in the xpcshell test results.

Ah yes.  I wonder if we can suppress them somehow?  Anyway, I also hate the xpcshell warnings, so dropping this.
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review96486

::: testing/marionette/action.js:353
(Diff revision 1)
>   * Input state associated with current session. This is a map between input ID and
>   * the device state for that input source, with one entry for each active input source.
> + *
> + * Initialized in listener.js
>   */
> -action.inputStateMap = new Map();
> +action.inputStateMap = undefined;

Yeah, I thought it might be clearer to have explicit "placeholders" here, thus keeping all the action-related docs in one place. No?

_I suppose idiomatic js aims for the perfect mix of elegance, surprise and mystery?_ ;)

::: testing/marionette/test_action.js:13
(Diff revision 1)
>  
>  Cu.import("chrome://marionette/content/action.js");
>  Cu.import("chrome://marionette/content/element.js");
>  Cu.import("chrome://marionette/content/error.js");
>  
> +action.inputStateMap = new Map();

It's not used in any of the tests for now. 

I'd like to write a test that checks the content of `inputsToCancel` before/after `dispatchKeyDownAction`, etc, but I need a way to patch calls into event.js, which I don't see right now.

Or I could refactor the dispatch code a bit for testability so that only dispatchTickActions calls into event.js. Something like:

```
case action.KeyDown:
  // setupKeyDownAction is easy to test
  keyEvent = action.setupKeyDownAction(a, inputState);
  event.sendKeyDown(keyEvent.key, keyEvent, container.frame);
  break;
```

Any thoughts?
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review96486

> Yeah, I thought it might be clearer to have explicit "placeholders" here, thus keeping all the action-related docs in one place. No?
> 
> _I suppose idiomatic js aims for the perfect mix of elegance, surprise and mystery?_ ;)

No, I agree.  That’s absolutely fine.  I’ve dropped the relevant issues.

For what it’s worth, I didn’t raise this issue because it wasn’t idiomatic.  That comment was related to something else.
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review95936

> These properties need to be defined in the constructor as well.  We want them to be “discoverable” when the object is constructed but this function has not yet been called.
> 
> Since modifier state can be combined, I wonder if a bitmap would be a better choice of data type here?

"Since modifier state can be combined," -- can you elaborate?

I'm not sure that we can play with the data type, since these are the fields that `sendKeyDown`, etc. expect.
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review95936

> "Since modifier state can be combined," -- can you elaborate?
> 
> I'm not sure that we can play with the data type, since these are the fields that `sendKeyDown`, etc. expect.

Code speaks better than prose.  I was imagining a better data type to use throughout might be something along the lines of

```js
key.modifiers = Modifier.Alt | Modifier.Control;
```

But if `sendKeyDown` expects this, then it’s fine.

> There's also duration for pause, which is available in all action types. If you can suggest how best to address that, I'm happy to take care of it now.

Maybe we have to make the event functions generated from the [tick actions](https://w3c.github.io/webdriver/webdriver-spec.html#dfn-dispatch-tick-actions) (`dispatchKeyUp` et al.) into promises.  That  would let us write a clean implementation of a pause action, or generally of anything that requires us to suspend until a wall-clock timeout is reached.  They could be enqueued and resolved in sequence here using [`Promise.all`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/all), which would ensure that the “[t]here are no pending asynchronous waits arising from the last invocation of the dispatch tick actions steps” part of the spec could be fulfilled.

Unfortunately this means we have to change certain things in this patch, but not too drastically I think.  For example, a hypothetical implementation of the pause action might be:

```js
action.pause = function (a) {
  const timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
  return new Promise(resolve => timer.initWithCallback(
      resolve, a.duration, Ci.nsITimer.TYPE_ONE_SHOT));
};
```

Similarily converting `action.dispatchKeyUp` to a promise:

```js
action.keyUp = function (a, inputState, win) {
  return new Promise(resolve => {
    let keyEvent = new action.Key(a.value);
    if (!inputState.isPressed(keyEvent.key)) {
      return;
    }
    inputState.setModState(keyEvent.key, false);
    inputState.release(keyEvent.key);
    keyEvent.update(inputState);

    // event.sendKeyUp is synchronous from our point of view
    event.sendKeyUp(keyEvent.key, keyEvent, win);

    resolve();
  });
};
```

For each sequence of tick actions in the action chain, we could imagine mapping each `action.Action` to their respective event functions (`action.pause`, `action.KeyUp` from above et al.).  Since each  tick has to share a certain amount of state (input state, overall tick duration, seen element store, window frame container), we can create a closure that holds it for us.  But certainly other solutions are also imaginable.

```js
action.dispatch = function(chain, seenEls, container) {
  let tickDuration = action.computeTickDuration(tickActions);

  for (let tickActions of chain) {
    if (!action.inputStateMap.has(a.id)) {
      action.inputStateMap.set(a.id, InputState.fromJson(a));
    }
    let inputState = action.inputStateMap.get(a.id);

    // enqueue the events created from their |action.Action| definition
    let pendingEvents = tickActions.map(
        toEvents(inputState, tickDuration, seenEls, container));

    // perform and resolve all pending events, in sequence
    Promise.all(pendingEvents);

    // force any pending DOM events to fire
    container.frame.requestAnimationFrame();
  }
};

// Returns a closure that maps actions to promise events
// sharing the same input state, tick duration, seen element store, and window.
function toEvents(inputState, tickDuration, seenEls, container) {
  return function (action) {
    switch (a.subtype) {
      case action.KeyUp:
        return action.keyUp(a, inputState, container.frame);

      case action.Pause:
        return action.pause(a);

      …
    }
  };
}
```

I’m not 100% convinced this is the correct overall solution, and I certainly made some perhaps unnecessary optimisations above, but I have a strong feeling that the general approach of encoding events as promises is the right one.  Because although testing/marionette/event.js and the synthesised input library calls appear synchronous to JavaScript, coding functions that block for _N_ amount of time using traditional callbacks is extremely tedious, because you will end up having to pass the `message.Response.send` (from testing/marionette/message.js) through all the way to the bottom of the stack.

Again, very sorry that I didn’t catch this earlier in the review. )-:
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review97120

::: testing/marionette/action.js:435
(Diff revisions 1 - 2)
>    /**
>     * Update modifier state according to |key|
>     *
>     * @param {string} key
>     *     Normalized key value
>     *
>     * @param {boolean} value
> -   *     Value to set the mod attribute to
> +   *     Value to set the modifier attribute to
>     */
>    setModState(key, value) {
> -    if (["Alt", "Shift", "Control", "Meta"].includes(key)) {
> -      let k = key == "Control" ? "ctrl" : key.toLowerCase();
> -      this[k] = value;
> +    if (key in MODIFIER_NAME_LOOKUP) {
> +      this[MODIFIER_NAME_LOOKUP[key]] = value;
> +    }
> +  }

Is this ever called with an incorrect value?  Should that be possible?

Should consider that it might be less surprising behaviour for this to throw in case of invalid input.

::: testing/marionette/action.js:462
(Diff revisions 1 - 2)
> +  /**
> +   * Add |key| to list of pressed keys.
> +   *
> +   * @param {string} key
> +   *     Normalized key value.
> +   */
> +  press(key){
> +    this.pressed.add(key);
> -    }
> +  }

Return the return value of `Set.add`.

::: testing/marionette/action.js:472
(Diff revisions 1 - 2)
> +  /**
> +   * Remove |key| from list of pressed keys.
> +   *
> +   * @param {string} key
> +   *     Normalized key value.
> +   */
> +  release(key){
> +    this.pressed.delete(key);
>    }

Same here, return `Set.delete`’s return value.
Attachment #8814499 - Flags: review?(ato) → review-
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review95936

> Should `actions.inputsToCancel` also be initialised here?

[Pasting this comment from yesterday as reply to the correct issue. No need to reply right away; can wait until next post-Promise round of review.]

It's not used in any of the tests for now. 

I'd like to write a test that checks the content of inputsToCancel before/after dispatchKeyDownAction, etc, but I need a way to patch calls into event.js, which I don't see right now.

Or I could refactor the dispatch code a bit for testability so that only dispatchTickActions calls into event.js. Something like:

```
case action.KeyDown:
  // setupKeyDownAction is easy to test
  keyEvent = action.setupKeyDownAction(a, inputState);
  event.sendKeyDown(keyEvent.key, keyEvent, container.frame);
  break;
```

Any thoughts?
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review98770

::: testing/marionette/action.js:1000
(Diff revisions 2 - 3)
> -    let affectsWallClockTime = a.subtype == action.Pause ||
> -        (a.type == "pointer" && a.subtype == action.PointerMove);
> +  return new Promise(resolve => timer.initWithCallback(
> +      resolve, duration * 1000, Ci.nsITimer.TYPE_ONE_SHOT));

I can't get this part with initWithCallback to work: 

* when I test this with a chain like "keydown c", "pause 5", "keydown b", I expect to see a pause in between c and b, but I don't;
* however, when I test in the js console, a Promise with initWithCallback works as expected.

Throwing this up for review anyway, but I will return to this particular issue tomorrow.
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review98942

::: testing/marionette/action.js:444
(Diff revisions 2 - 3)
> -   *     Normalized key value
> +   *     Normalized key value of a modifier key
>     *
>     * @param {boolean} value
>     *     Value to set the modifier attribute to
> +   *
> +   * @throws Error

Wrap in ‘type brackets’: `{Error}`.

::: testing/marionette/action.js:451
(Diff revisions 2 - 3)
> +      throw new Error("Expected 'key' to be one of " +
> +          `${Object.keys(MODIFIER_NAME_LOOKUP)}; got: ${key}`);

`InvalidArgumentError` would be more applicable because plain `Error`’s get serialised to `WebDriverError`.

::: testing/marionette/action.js:474
(Diff revisions 2 - 3)
> -   * Add |key| to list of pressed keys.
> +   * Add |key| to the set of pressed keys.
>     *
>     * @param {string} key
>     *     Normalized key value.
> +   *
> +   * @return {Set}

s/Set/boolean/

::: testing/marionette/action.js:968
(Diff revisions 2 - 3)
>   *     Current window
> + *
> + * @return {Promise}
> + *     Promise to dispatch a keyup event
>   */
> -action.dispatchKeyUpAction = function(a, inputState, win) {
> +function dispatchKeyUp(a, inputState, win) {

Did you consider exporting these so that they can be tested individually?

```js
action.keyUp = function (a, inputState, win) { … }
```

::: testing/marionette/action.js:998
(Diff revisions 2 - 3)
>   *
> - * @return {number}
> - *     Longest action duration in |tickActions| if any, or 0.
> + * @return {Promise}
> + *     Promise that is resolved after the specified time interval.
>   */
> -action.computeTickDuration = function(tickActions) {
> -  let max = 0;
> +function dispatchPause(a, tickDuration) {
> +  let duration = typeof a.duration == "undefined" ? tickDuration : a.duration;

This, I think, is a rare occasion where I find the ternary operator acceptable.  It almost feels aesthetically pleasing.

::: testing/marionette/action.js:474
(Diff revision 3)
> +   * Add |key| to the set of pressed keys.
> +   *
> +   * @param {string} key
> +   *     Normalized key value.
> +   *
> +   * @return {Set}

s/Set/boolean

::: testing/marionette/action.js:832
(Diff revision 3)
> + * @param {action.Chain} chain
> + *     Actions grouped by tick; each element in |chain| is a sequence of
> + *     actions for one tick.
> + * @param {element.Store} seenEls
> + *     Element store
> + * @param {?} container

This isn’t an issue, but we should make a concrete type of the container object at some point.  They provide very exact typing information for `frame` and `shadowRoot`.

::: testing/marionette/action.js:836
(Diff revision 3)
> +    for (let tickActions of chain) {
> +      action.dispatchTickActions(
> +          tickActions, action.computeTickDuration(tickActions), seenEls, container);
> +    }

Two space indent.

FWIW I have a patch-in-progress to enable eslint for testing/marionette.

::: testing/marionette/action.js:845
(Diff revision 3)
> + * This is done by creating a Promise that resolves when each Promise for each action
> + * in the tick is resolved, which takes at least |tickDuration| seconds. The Promise
> + * for each action concludes with firing of pending DOM events.

Good explanation.

::: testing/marionette/action.js:849
(Diff revision 3)
> + *
> + * This is done by creating a Promise that resolves when each Promise for each action
> + * in the tick is resolved, which takes at least |tickDuration| seconds. The Promise
> + * for each action concludes with firing of pending DOM events.
> + *
> + * @param {Array<action.Action>} tickActions

`Array.<action.Action>`

::: testing/marionette/action.js:850
(Diff revision 3)
> + * This is done by creating a Promise that resolves when each Promise for each action
> + * in the tick is resolved, which takes at least |tickDuration| seconds. The Promise
> + * for each action concludes with firing of pending DOM events.
> + *
> + * @param {Array<action.Action>} tickActions
> + *     List of actions for one tick,

.

::: testing/marionette/action.js:854
(Diff revision 3)
> + * @param {Array<action.Action>} tickActions
> + *     List of actions for one tick,
> + * @param {number} tickDuration
> + *     Duration in seconds of this tick.
> + * @param {element.Store} seenEls
> + *     Element store

.

::: testing/marionette/action.js:860
(Diff revision 3)
> +  // force any pending DOM events to fire
> +  pendingEvents.push(new Promise(resolve => container.frame.requestAnimationFrame(resolve)));

Very clever.

::: testing/marionette/action.js:868
(Diff revision 3)
> +};
> +
> +/**
> + * Compute tick duration in seconds for a collection of actions.
> + *
> + * @param {Array<action.Action>} tickActions

`Array.<action.Action>`

::: testing/marionette/action.js:998
(Diff revision 3)
> + *
> + * @return {Promise}
> + *     Promise that is resolved after the specified time interval.
> + */
> +function dispatchPause(a, tickDuration) {
> +  let duration = typeof a.duration == "undefined" ? tickDuration : a.duration;

This, I think, is a rare occasion I find ternary operators acceptable.  It’s even aesthetically pleasing.

::: testing/marionette/action.js:1001
(Diff revision 3)
> + */
> +function dispatchPause(a, tickDuration) {
> +  let duration = typeof a.duration == "undefined" ? tickDuration : a.duration;
> +  const timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +  return new Promise(resolve => timer.initWithCallback(
> +      resolve, duration * 1000, Ci.nsITimer.TYPE_ONE_SHOT));

The duration input is given in milliseconds.

::: testing/marionette/driver.js:1642
(Diff revision 3)
>  };
>  
>  /**
> + * Perform a series of grouped actions at the specified points in time.
> + *
> + * @param {Array<?>} actions

`Array.<?>`

::: testing/marionette/driver.js:1649
(Diff revision 3)
> + */
> +GeckoDriver.prototype.performActions = function(cmd, resp) {
> +  switch (this.context) {
> +    case Context.CHROME:
> +      throw new UnsupportedOperationError(
> +            "Command 'performActions' is not available in chrome context");

Four space indent.

::: testing/marionette/test_action.js:471
(Diff revision 3)
> +    {type: "none", subtype: "pause", duration: 5},
> +    {type: "key", subtype: "pause", duration: 1},
> +    {type: "pointer", subtype: "pointerMove", duration: 6},
> +    // invalid because keyDown should not have duration, so duration should be ignored.
> +    {type: "key", subtype: "keyDown", duration: 100},
> +    {type: "pointer", subtype: "pause", duration: 8},

These should all be in milliseconds.
Attachment #8814499 - Flags: review?(ato) → review-
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review98942

> Did you consider exporting these so that they can be tested individually?
> 
> ```js
> action.keyUp = function (a, inputState, win) { … }
> ```

I don't see how I can write reasonable xpcshell unit tests for these functions because they trigger events. If I only write functional tests for these, then exporting them is not necessary.

Re unit test - I wrote a comment describing how I might refactor the dispatch\* functions to be more testable (see other open issue) but I'm not sure it's worthwhile.
Not sure if it helps to see if events are triggered or not in tests, there is a way to check that. I was using that formerly with Mozmill and EventUtils.js. But the latter has been replaced, so the necessary code could be found here:

https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/testing/marionette/event.js#865

Not sure if that code is reachable or not, because I haven't checked that yet.
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review99796

::: testing/marionette/action.js:46
(Diff revision 8)
> +/** For dispatching actions with a duration */
> +const TIMER = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);

Even if events are not technically despatched concurrently, it might be better to create a fresh timer instance for each event.

::: testing/marionette/action.js:405
(Diff revision 8)
> -   * @param {?} actionSequence
> -   *     Object representing an action sequence.
> +   * @param {?} obj
> +   *     Object with property |type|, representing an action sequence or an action item.

This line is too long.

::: testing/marionette/action.js:411
(Diff revision 8)
>     * @throws InvalidArgumentError
>     *     If |actionSequence.type| is not valid.

Add “type braces”.

::: testing/marionette/action.js:440
(Diff revision 8)
> +  /**
> +   * Update modifier state according to |key|
> +   *
> +   * @param {string} key
> +   *     Normalized key value of a modifier key
> +   *
> +   * @param {boolean} value
> +   *     Value to set the modifier attribute to
> +   *
> +   * @throws {InvalidArgumentError}
> +   *     If |key| is not a modifier
> +   */

Add some final stops.

::: testing/marionette/action.js:443
(Diff revision 8)
> +   * @param {string} key
> +   *     Normalized key value of a modifier key
> +   *
> +   * @param {boolean} value
> +   *     Value to set the modifier attribute to

Drop empty line between parameter documentation.

::: testing/marionette/action.js:464
(Diff revision 8)
> +   * @param {string} key
> +   *     Normalized key value.
> +   * @return {boolean}
> +   *     True if |key| is in set of pressed keys.

Add blank line between parameter- and return documentation.

::: testing/marionette/action.js:570
(Diff revision 8)
>     * @throws InvalidArgumentError
>     *     If any |actionSequence| or |actionItem| attributes are invalid.
>     * @throws UnsupportedOperationError
>     *     If |actionItem.type| is |pointerCancel|.

Add “type braces”.

::: testing/marionette/action.js:672
(Diff revision 8)
>     * @throws InvalidArgumentError
>     *     If |actions| is not an Array.

Add “type braces”.

::: testing/marionette/action.js:676
(Diff revision 8)
>      if (!Array.isArray(actions)) {
>        throw new InvalidArgumentError(`Expected 'actions' to be an Array, got: ${actions}`);
>      }

Perhaps we should add this check to testing/marionette/assert.js?

::: testing/marionette/action.js:695
(Diff revision 8)
>  /**
> - * Represents one input source action sequence; this is essentially an |Array<action.Action>|.
> + * Represents one input source action sequence; this is essentially an |Array.<action.Action>|.
>   */

This line is too long.

::: testing/marionette/action.js:710
(Diff revision 8)
>     * @throws InvalidArgumentError
>     *     If |actionSequence.id| is not a string or it's aleady mapped
>     *     to an |action.InputState} incompatible with |actionSequence.type|.
>     *     If |actionSequence.actions| is not an Array.

Add “type braces”.

::: testing/marionette/action.js:748
(Diff revision 8)
>   * @param {boolean=} primary
>   *     Whether the input source is the primary pointing device.
>   *     If the parameter is underfined, true is used.

s/underfined/undefined/

::: testing/marionette/action.js:790
(Diff revision 8)
>   * @throws InvalidArgumentError
>   *     If |id| is already mapped to an |action.InputState| that is
>   *     not compatible with |act.subtype|.

Add “type braces”.

::: testing/marionette/action.js:882
(Diff revision 8)
> +      // force any pending DOM events to fire
> +      new Promise(resolve => container.frame.requestAnimationFrame(resolve)));

Split out the chained promise as a separate function. I think that will read better:

```js
action.dispatchTickActions = function (…) {
  let pendingEvents = tickActions.map(toEvents(tickDuration, seenEls, container));
  let flushDOMEvents = action.flushEvents(container);
  return Promsie.all(pendingEvents).then(flushDOMEvents);
};

action.flushEvents = function (container) {
  return new Promise(resolve => container.frame.requestAnimationFrame(resolve));
};
```

::: testing/marionette/action.js:889
(Diff revision 8)
> + * @param {Array<action.Action>} tickActions
> + *     List of actions for one tick.

`Array<action.Action>` → `Array.<action.Action>`

::: testing/marionette/action.js:1021
(Diff revision 8)
> +  return new Promise(resolve => {
> +    TIMER.initWithCallback(resolve, duration, Ci.nsITimer.TYPE_ONE_SHOT);
> +  });

The curly braces are strictly unnecessary here, but I don’t mind.

::: testing/marionette/driver.js:1647
(Diff revision 8)
>  /**
> + * Perform a series of grouped actions at the specified points in time.
> + *
> + * @param {Array<?>} actions
> + *     Array of objects that each represent an action sequence.
> + */

Document that it will throw `UnsupportedOperationError` in chrome space.

::: testing/marionette/driver.js:1650
(Diff revision 8)
> + * @param {Array<?>} actions
> + *     Array of objects that each represent an action sequence.

`Array<?>` → `Array.<?>`

::: testing/marionette/event.js:1230
(Diff revision 8)
> +/**
> + *  Synthesize a key event for a single key.
> + *

Superfluous space character before “Synthesize”.

::: testing/marionette/event.js:1241
(Diff revision 8)
> + *     Object with properties used in KeyboardEvent (shiftkey, repeat, ...)
> + *     as well as, the event |type| such as keydown. All properties are optional.
> + * @param {Window=} window
> + *     Window object.  If |window| is undefined, the event is synthesized in
> + *     current window.
> + *

Drop blank line.

::: testing/marionette/event.js:1246
(Diff revision 8)
> + *
> + */
>  event.sendSingleKey = function (keyToSend, modifiers, window = undefined) {
>    let keyCode = getKeyCode(keyToSend);
>    if (keyCode in KEYCODES_LOOKUP) {
> +    // We assume that if |keytoSend| is a raw code point (like "\uE009") then

s/keytoSend/keyToSend/

::: testing/marionette/listener.js:420
(Diff revision 8)
> +  if (action.inputStateMap !== undefined) {
> +    action.inputStateMap.clear();
> +  }
> +  if (action.inputsToCancel !== undefined) {
> +    action.inputsToCancel.length = 0;
> +  }

What are the undefined tests for?

::: testing/marionette/test_action.js:469
(Diff revision 8)
> +add_test(function test_computeTickDuration() {
> +  let expected = 8000;
> +  let tickActions = [
> +    {type: "none", subtype: "pause", duration: 5000},
> +    {type: "key", subtype: "pause", duration: 1000},
> +    {type: "pointer", subtype: "pointerMove", duration: 6000},
> +    // invalid because keyDown should not have duration, so duration should be ignored.
> +    {type: "key", subtype: "keyDown", duration: 100000},
> +    {type: "pointer", subtype: "pause", duration: expected},
> +    {type: "pointer", subtype: "pointerUp"},
> +  ];
> +  equal(expected, action.computeTickDuration(tickActions));
> +  run_next_test();
> +});

How do you arrive at 8000 here?
Attachment #8814499 - Flags: review?(ato) → review+
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review98770

> I can't get this part with initWithCallback to work: 
> 
> * when I test this with a chain like "keydown c", "pause 5", "keydown b", I expect to see a pause in between c and b, but I don't;
> * however, when I test in the js console, a Promise with initWithCallback works as expected.
> 
> Throwing this up for review anyway, but I will return to this particular issue tomorrow.

Did you fix this?
(In reply to Henrik Skupin (:whimboo) from comment #25)
> Not sure if it helps to see if events are triggered or not in tests, there
> is a way to check that. I was using that formerly with Mozmill and
> EventUtils.js. But the latter has been replaced, so the necessary code could
> be found here:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/testing/marionette/event.js#865
> 
> Not sure if that code is reachable or not, because I haven't checked that
> yet.

It looks like those are reachable through the […]ExpectEvent functions in testing/marionette/event.js, e.g. event.synthesizeMouseExpectEvent.  But it’s not entirely clear to me if we want to use these?
(In reply to Maja Frydrychowicz (:maja_zf) from comment #20)
> Comment on attachment 8814499 [details]
> Bug 1320389 - Implement dispatch of key actions in content context;
> 
> https://reviewboard.mozilla.org/r/95708/#review98942
> 
> > Did you consider exporting these so that they can be tested individually?
> > 
> > ```js
> > action.keyUp = function (a, inputState, win) { … }
> > ```
> 
> I don't see how I can write reasonable xpcshell unit tests for these
> functions because they trigger events. If I only write functional tests for
> these, then exporting them is not necessary.
> 
> Re unit test - I wrote a comment describing how I might refactor the
> dispatch\* functions to be more testable (see other open issue) but I'm not
> sure it's worthwhile.

That’s fair.
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review98770

> Did you fix this?

Yes.
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review99796

> What are the undefined tests for?

I added them to fix errors encountered in a recent try push. https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e1477664e191fb471894dc7538c4ffbb407b4f0

> How do you arrive at 8000 here?

It's just the longest duration among valid durations in `tickActions`.
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review100210

::: testing/marionette/action.js:674
(Diff revision 10)
> -    if (!Array.isArray(actions)) {
> -      throw new InvalidArgumentError(`Expected 'actions' to be an Array, got: ${actions}`);
> +    assert.array(actions,
> +              error.pprint`Expected 'actions' to be an Array, got: ${actions}`);

Second line indentation should be four spaces.

::: testing/marionette/action.js:1015
(Diff revision 10)
> +  const TIMER = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +  let duration = typeof a.duration == "undefined" ? tickDuration : a.duration;
> +  return new Promise(resolve =>
> +      TIMER.initWithCallback(resolve, duration, Ci.nsITimer.TYPE_ONE_SHOT)

s/TIMER/timer/ when the constant isn’t global.

::: testing/marionette/action.js:1023
(Diff revision 10)
> +/**
> + * Force any pending DOM events to fire.
> + *
> + * @param {?} container
> + *     Object with |frame| attribute of type |nsIDOMWindow|.
> + *
> + * @return {Promise}
> + *     Promise to flush DOM events.
> + */

BTW there’s no need to document helper functions.

::: testing/marionette/assert.js:211
(Diff revision 10)
> + * @throws {InvalidArgumentError}
> + *     If |obj| is not an Array.
> + */
> +assert.array = function (obj, msg = "") {
> +  msg = msg || error.pprint`Expected ${obj} to be an Array`;
> +  return assert.that(o => Array.isArray(o), msg)(obj);

The arrow function is not needed here, you can just do:

```js
return assert.that(Array.isArray, msg)(obj);
```
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review99796

> I added them to fix errors encountered in a recent try push. https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e1477664e191fb471894dc7538c4ffbb407b4f0

It’s bizarre that these are just happening for non-e10s.  Anyway, if try is green all is good.
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review99796

> It's just the longest duration among valid durations in `tickActions`.

Sorry, I completely misread this code.
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review100210

> Second line indentation should be four spaces.

Gah, sorry. I should really fix that in my editor settings or something.
Comment on attachment 8814499 [details]
Bug 1320389 - Implement dispatch of key actions in content context;

https://reviewboard.mozilla.org/r/95708/#review100210

> Gah, sorry. I should really fix that in my editor settings or something.

I need to fix my eslint patch so I don’t have to be such an annoying bastard in code reviews (-;
Pushed by mjzffr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1f6800191787
Rename legacyactions; r=ato
https://hg.mozilla.org/integration/autoland/rev/14c79da88344
Implement dispatch of key actions in content context; r=ato
https://hg.mozilla.org/mozilla-central/rev/1f6800191787
https://hg.mozilla.org/mozilla-central/rev/14c79da88344
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Sheriffs: Test-only uplift.  Let me know if it does not apply and I will rebase it.
Whiteboard: [checkin-needed-aurora]
Second patch needs rebasing around bug 1316975.
status-firefox52: --- → affected
Flags: needinfo?(ato)
Whiteboard: [checkin-needed-aurora]
(In reply to Ryan VanderMeulen [:RyanVM] from comment #46)
> Second patch needs rebasing around bug 1316975.

Rebased.
Flags: needinfo?(ato)
Whiteboard: [checkin-needed-aurora]
2nd part has problems to apply:

applying https://bugzilla.mozilla.org/attachment.cgi?id=8823252
patching file testing/marionette/action.js
Hunk #3 succeeded at 344 with fuzz 1 (offset 0 lines).
patching file testing/marionette/event.js
Hunk #3 succeeded at 1213 with fuzz 1 (offset 0 lines).
Hunk #4 FAILED at 1226
1 out of 4 hunks FAILED -- saving rejects to file testing/marionette/event.js.rej
abort: patch failed to apply
Flags: needinfo?(mjzffr)
Whiteboard: [checkin-needed-aurora]
Please note that any merge conflict in js files might be due to bug 1316975. I already requested uplift for this patch.
Fixed again.
Whiteboard: [checkin-needed-aurora]
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.