update sendkeys command parameters to match Webdriver spec

RESOLVED FIXED in Firefox 53

Status

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: automatedtester, Assigned: automatedtester)

Tracking

(Depends on 1 bug)

Version 3
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

Attachments

(1 attachment)

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

We need to update this accordingly.
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.
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.
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?
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}`);
Attachment #8849299 - Flags: review?(ato) → 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.
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.
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.
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"
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...
hg error in cmd: hg identify upstream -r tip:
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.
Attachment #8849299 - Flags: review?(ato) → review+
To be fully sure this doesn’t break the update tests, an example update should be run.
Pushed by dburns@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/04503f8fae7e
Updated expected key parameter for sendKeysToElement. r=ato
https://hg.mozilla.org/mozilla-central/rev/04503f8fae7e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Whiteboard: [checkin-needed-beta][checkin-needed-aurora][checkin-needed-esr52]
https://hg.mozilla.org/releases/mozilla-aurora/rev/e118d67ad815
Whiteboard: [checkin-needed-beta][checkin-needed-aurora][checkin-needed-esr52] → [checkin-needed-beta][checkin-needed-esr52]
Depends on: webdriver
Also doesn't apply to Beta/ESR52.
Flags: needinfo?(dburns)
Whiteboard: [checkin-needed-beta][checkin-needed-esr52]
Flags: needinfo?(dburns)
Whiteboard: [checkin-needed-beta][checkin-needed-esr52]
https://hg.mozilla.org/releases/mozilla-beta/rev/3f1eb1220015
Whiteboard: [checkin-needed-beta][checkin-needed-esr52] → [checkin-needed-esr52]
Whiteboard: [checkin-needed-esr52]
You need to log in before you can comment on or make changes to this bug.