Closed
Bug 1348782
Opened 7 years ago
Closed 7 years ago
update sendkeys command parameters to match Webdriver spec
Categories
(Remote Protocol :: Marionette, enhancement)
Tracking
(firefox-esr52 wontfix, firefox53 fixed, firefox54 fixed, firefox55 fixed)
RESOLVED
FIXED
mozilla55
People
(Reporter: automatedtester, Assigned: automatedtester)
References
(Depends on 1 open bug)
Details
Attachments
(1 file)
The spec has changed the expected value from `value` to `text`. We need to update this accordingly.
Comment hidden (mozreview-request) |
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.
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.
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?
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}`);
Attachment #8849299 -
Flags: review?(ato) → review-
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.
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.
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.
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"
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...
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
hg error in cmd: hg identify upstream -r tip:
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.
Attachment #8849299 -
Flags: review?(ato) → review+
Comment 15•7 years ago
|
||
To be fully sure this doesn’t break the update tests, an example update should be run.
Comment hidden (mozreview-request) |
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
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/04503f8fae7e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Updated•7 years ago
|
Whiteboard: [checkin-needed-beta][checkin-needed-aurora][checkin-needed-esr52]
Comment 19•7 years ago
|
||
bugherder uplift |
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]
Comment 20•7 years ago
|
||
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 years ago
|
Flags: needinfo?(dburns)
Whiteboard: [checkin-needed-beta][checkin-needed-esr52]
Comment 21•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/3f1eb1220015
Whiteboard: [checkin-needed-beta][checkin-needed-esr52] → [checkin-needed-esr52]
Updated•7 years ago
|
Whiteboard: [checkin-needed-esr52]
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
•