David Burns :automatedtester Assignee
	Description • 7 years ago

The spec has changed the expected value from `value` to `text`.

We need to update this accordingly.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 2 • 7 years ago
mozreview-review

Comment on attachment 8849299 [details]
Bug 1348782: Updated expected key parameter for sendKeysToElement.

https://reviewboard.mozilla.org/r/122114/#review124306

::: testing/marionette/client/marionette_driver/marionette.py:110
(Diff revision 1)
> +            if isinstance(string, int):
> +                string = str(string)
> +            text.append(string)
> +        keys = "".join(text)
> +
> +        body = {"id": self.id, "text": keys}

We definitely need a fallback mechanism here, so it will still work with older releases of Firefox. If we can uplift to esr52, a removal could happen with Firefox 56.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 3 • 7 years ago
mozreview-review-reply

Comment on attachment 8849299 [details]
Bug 1348782: Updated expected key parameter for sendKeysToElement.

https://reviewboard.mozilla.org/r/122114/#review124306

> We definitely need a fallback mechanism here, so it will still work with older releases of Firefox. If we can uplift to esr52, a removal could happen with Firefox 56.

I falsely added this comment. Backward compatible code is necessary for those places our Firefox-ui update tests are utilizing. But this is not the case for `send_keys()`. Update tests are opening the about window via the main menu.

	Andreas Tolfsen ❲:ato❳
	Comment 4 • 7 years ago
mozreview-review-reply

Comment on attachment 8849299 [details]
Bug 1348782: Updated expected key parameter for sendKeysToElement.

https://reviewboard.mozilla.org/r/122114/#review124306

> I falsely added this comment. Backward compatible code is necessary for those places our Firefox-ui update tests are utilizing. But this is not the case for `send_keys()`. Update tests are opening the about window via the main menu.

> Update tests are opening the about window via the main menu.

What does the about window have to do with this?

	Andreas Tolfsen ❲:ato❳
	Comment 5 • 7 years ago
mozreview-review

Comment on attachment 8849299 [details]
Bug 1348782: Updated expected key parameter for sendKeysToElement.

https://reviewboard.mozilla.org/r/122114/#review125390

::: commit-message-b5a22:3
(Diff revision 1)
> +The expected command parameter has been updated from `value`
> +to `text`.

Explain motivation for change.

::: testing/marionette/client/marionette_driver/marionette.py:102
(Diff revision 1)
>          """Returns the visible text of the element, and its child elements."""
>          body = {"id": self.id}
>          return self.marionette._send_message("getElementText", body, key="value")
>  
> -    def send_keys(self, *string):
> +    def send_keys(self, *strings):
>          """Sends the string via synthesized keypresses to the element."""

Point out which types are coerced to strings.

::: testing/marionette/client/marionette_driver/marionette.py:103
(Diff revision 1)
> -        keys = Marionette.convert_keys(*string)
> -        body = {"id": self.id, "value": keys}
> +        text = []
> +        for string in strings:
> +            if isinstance(string, int):
> +                string = str(string)
> +            text.append(string)
> +        keys = "".join(text)

Should not all entries be coerced to strings?

Also, you could simply replace this with

> "".join(str(entry) for entry in strings)

or

> "".join(map(str, strings))

if it is the intention to coerce everything to strings.

::: testing/marionette/driver.js:2021
(Diff revision 1)
>   * @param {string} value
>   *     Value to send to the element.
>   */
>  GeckoDriver.prototype.sendKeysToElement = function*(cmd, resp) {
> -  let {id, value} = cmd.parameters;
> -  assert.defined(value, `Expected character sequence: ${value}`);
> +  let {id, text} = cmd.parameters;
> +  assert.string(text, `Expected string: ${text}`);

Make use of error.pprint here:

> assert.string(text, error.pprint`Expected string, got: ${text}`);

	Andreas Tolfsen ❲:ato❳
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8849299 [details]
Bug 1348782: Updated expected key parameter for sendKeysToElement.

https://reviewboard.mozilla.org/r/122114/#review125398

::: testing/marionette/event.js:1286
(Diff revision 1)
>    }
>    element.focus();
>  }
>  
>  /**
>   * @param {Array.<string>} keySequence

This is now incorrect.  You are passing a string.

::: testing/marionette/event.js:1292
(Diff revision 1)
>   * @param {Element} element
>   * @param {Object.<string, boolean>=} opts
>   * @param {Window=} window
>   */
>  event.sendKeysToElement = function (
>      keySequence, el, opts = {}, window = undefined) {

Rename keySequence to reflect that it’s a string.

::: testing/marionette/event.js:1303
(Diff revision 1)
> -    let value = keySequence.join("");
> -    for (let i = 0; i < value.length; i++) {
> +    for (let i = 0; i < keySequence.length; i++) {
> +      let c = keySequence.charAt(i);
> -      let c = value.charAt(i);
>        event.sendSingleKey(c, modifiers, window);
>      }

There are some try failures related to this change.

	Andreas Tolfsen ❲:ato❳
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8849299 [details]
Bug 1348782: Updated expected key parameter for sendKeysToElement.

https://reviewboard.mozilla.org/r/122114/#review125400

You also need to update the code relating to sending a file path for <input type=file> elements.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 8 • 7 years ago
mozreview-review-reply

Comment on attachment 8849299 [details]
Bug 1348782: Updated expected key parameter for sendKeysToElement.

https://reviewboard.mozilla.org/r/122114/#review124306

> > Update tests are opening the about window via the main menu.
> 
> What does the about window have to do with this?

Our tests are using different methods to open other windows. Those are menu, shortcut, or a custom trigger callback. `send_keys()` would affect us if any code for the update tests would use shortcuts. But as I have corrected myself above, this is not the case.

	David Burns :automatedtester Assignee
	Comment 9 • 7 years ago
mozreview-review-reply

Comment on attachment 8849299 [details]
Bug 1348782: Updated expected key parameter for sendKeysToElement.

https://reviewboard.mozilla.org/r/122114/#review125390

> Should not all entries be coerced to strings?
> 
> Also, you could simply replace this with
> 
> > "".join(str(entry) for entry in strings)
> 
> or
> 
> > "".join(map(str, strings))
> 
> if it is the intention to coerce everything to strings.

Not everything in Python is a string. E.g. unicode is not a string so if we pass in one representations for special keys doing `str(u'\\ue009')` we get unhelpful decoding errors. From experience it is better to assume people will pass in strings(ascii or unicode) or integers only and translate those that aren't "strings"

	David Burns :automatedtester Assignee
	Comment 10 • 7 years ago
mozreview-review-reply

Comment on attachment 8849299 [details]
Bug 1348782: Updated expected key parameter for sendKeysToElement.

https://reviewboard.mozilla.org/r/122114/#review125398

> There are some try failures related to this change.

Not sure why mozreview thinks I removed it... its not like that locally...

	Mozilla Autoland
	Comment 13 • 7 years ago

hg error in cmd: hg identify upstream -r tip:

	Andreas Tolfsen ❲:ato❳
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8849299 [details]
Bug 1348782: Updated expected key parameter for sendKeysToElement.

https://reviewboard.mozilla.org/r/122114/#review126246

::: testing/marionette/client/marionette_driver/marionette.py:109
(Diff revision 3)
> -        body = {"id": self.id, "value": keys}
> +           will be joined into a string.
> +           If an integer is passed in like `marionette.send_keys(1234)` it will be
> +           coerced into a string.
> +        """
> +        keys = Marionette.convert_keys(*strings)
> +        body = {"id": self.id, "text": keys}

This is a backwards incompatible change, but per https://bugzilla.mozilla.org/show_bug.cgi?id=1348782#c8 it should not break the update tests.

If it does cause a problem, all we have to do here is also send a `value' field.

	Andreas Tolfsen ❲:ato❳
	Comment 15 • 7 years ago

To be fully sure this doesn’t break the update tests, an example update should be run.

	Pulsebot
	Comment 17 • 7 years ago

Pushed by dburns@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/04503f8fae7e
Updated expected key parameter for sendKeysToElement. r=ato

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 18 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/04503f8fae7e

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/e118d67ad815

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 7 years ago

Also doesn't apply to Beta/ESR52.

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/3f1eb1220015