update sendkeys command parameters to match Webdriver spec

RESOLVED FIXED in Firefox 53

Status

Testing
Marionette
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: automatedtester, Assigned: automatedtester)

Tracking

(Depends on: 1 bug)

Version 3
mozilla55
Points:
---

Firefox Tracking Flags

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

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

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

We need to update this accordingly.
Comment hidden (mozreview-request)

Comment 2

7 months 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.

Comment 3

7 months 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.

Comment 4

7 months 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?

Comment 5

7 months 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}`);
Attachment #8849299 - Flags: review?(ato) → review-

Comment 6

7 months 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.

Comment 7

7 months 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.

Comment 8

7 months 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.
(Assignee)

Comment 9

7 months 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"
(Assignee)

Comment 10

7 months 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...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

7 months ago
hg error in cmd: hg identify upstream -r tip:

Comment 14

7 months 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.
Attachment #8849299 - Flags: review?(ato) → review+
To be fully sure this doesn’t break the update tests, an example update should be run.
Comment hidden (mozreview-request)

Comment 17

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

Comment 18

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/04503f8fae7e
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

7 months ago
Whiteboard: [checkin-needed-beta][checkin-needed-aurora][checkin-needed-esr52]

Comment 19

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/e118d67ad815
status-firefox54: --- → fixed
Whiteboard: [checkin-needed-beta][checkin-needed-aurora][checkin-needed-esr52] → [checkin-needed-beta][checkin-needed-esr52]
(Assignee)

Updated

7 months ago
Depends on: 721859
Also doesn't apply to Beta/ESR52.
status-firefox53: --- → affected
status-firefox-esr52: --- → affected
Flags: needinfo?(dburns)
Whiteboard: [checkin-needed-beta][checkin-needed-esr52]
(Assignee)

Updated

7 months ago
Flags: needinfo?(dburns)
Whiteboard: [checkin-needed-beta][checkin-needed-esr52]

Comment 21

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/3f1eb1220015
status-firefox53: affected → fixed
Whiteboard: [checkin-needed-beta][checkin-needed-esr52] → [checkin-needed-esr52]
status-firefox-esr52: affected → wontfix
Whiteboard: [checkin-needed-esr52]
You need to log in before you can comment on or make changes to this bug.