Closed
Bug 1396823
Opened 7 years ago
Closed 6 years ago
Use unicode-segmentation to process grapheme clusters in key actions
Categories
(Testing :: geckodriver, enhancement, P2)
Testing
geckodriver
Tracking
(firefox60 fixed)
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: ato, Assigned: gsfraley)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 4 obsolete files)
mjzffr in https://github.com/mozilla/webdriver-rust/issues/59: > Right now only single characters for key "value" are accepted when > parsing message body for key actions, which means we error out > when a valid grapheme cluster is sent in. (Example: tamil nadu > char "\u0BA8\u0BBF"). > > Apparently this unicode-segmentation library can be used to > determine whether a string is a single grapheme cluster.
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•6 years ago
|
||
Reporter | ||
Comment 2•6 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ffc0dda79b9de5e3ad63a2b3383ffc0ead729aa
Reporter | ||
Comment 3•6 years ago
|
||
Comment on attachment 8955847 [details] [diff] [review] Use unicode-segmentation to iterate graphemes instead of chars Review of attachment 8955847 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for this patch! All the tests appear to be passing, but I can’t accept this patch without proper tests. What is missing here are WPT WebDriver tests. The existing body of action tests can be found under testing/web-platform/tests/webdriver/tests/actions and are invoked with "./mach wpt FILE". You can also pass --webdriver-arg=-vv to pass -vv on to geckodriver to get more logging. Let me know if you have any questions (-:
Attachment #8955847 -
Flags: review-
Reporter | ||
Updated•6 years ago
|
Assignee: nobody → gsfraley
Status: NEW → ASSIGNED
OS: Unspecified → All
Hardware: Unspecified → All
Version: Version 3 → Trunk
Assignee | ||
Comment 4•6 years ago
|
||
Sounds good. I'll prep some tests and submit them, hopefully through MozReview this time.
Comment hidden (mozreview-request) |
Reporter | ||
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8956216 [details] Bug 1396823 - Added tests for special keys that use multiple codepoints https://reviewboard.mozilla.org/r/225126/#review231252 Can you also put your first commit in this mozreview? ::: commit-message-827a7:1 (Diff revision 1) > +Bug 1396823 - Added tests for special keys that use multiple codepoints Small thing, but it is better practice to use normative language in a commit message. E.g. “Add tests for special keys […]”. ::: testing/web-platform/tests/webdriver/tests/actions/special_keys.py:3 (Diff revision 1) > import pytest > import time > +from webdriver import InvalidArgumentException > from tests.actions.support.keys import ALL_EVENTS, Keys > from tests.actions.support.refine import filter_dict, get_keys, get_events Sort these lexicographically. ::: testing/web-platform/tests/webdriver/tests/actions/special_keys.py:48 (Diff revision 1) > +@pytest.mark.parametrize("value,valid", [ > + (u"f", True), > + (u"fa", False), > + (u"\u0BA8\u0BBF", True), > + (u"\u0BA8\u0BBFb", False), > + (u"\u0BA8\u0BBF\u0BA8", False), > + (u"\u1100\u1161\u11A8", True), > + (u"\u1100\u1161\u11A8c", False) > +]) > +def test_multiple_codepoint_keys_behave_correctly(session, > + key_reporter, > + key_chain, > + value, > + valid): I would split this test in two: one that tests the valid grapheme clusters and one for the invalid inputs. I don’t like having to parametrise the expected outcome of the test. This necessarily means a bit more code, but will make it more readable. ::: testing/web-platform/tests/webdriver/tests/actions/special_keys.py:67 (Diff revision 1) > + try: > + key_chain \ > + .key_down(value) \ > + .key_up(value) \ > + .perform() > + Whitespace.
Attachment #8956216 -
Flags: review-
Assignee | ||
Comment 7•6 years ago
|
||
All sounds good! I'll keep the points in mind for further commits, and get a patch to address these issues pushed. Thanks for the help!
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8956216 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8956604 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8956604 -
Attachment is obsolete: false
Assignee | ||
Updated•6 years ago
|
Attachment #8956216 -
Attachment is obsolete: false
Assignee | ||
Updated•6 years ago
|
Attachment #8955847 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8956612 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8956604 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8956216 -
Attachment is obsolete: true
Reporter | ||
Comment 11•6 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cfc6b3263a5f835813b7f7c7a9b784d5f5c0f79
Reporter | ||
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8956686 [details] Bug 1396823 - Use unicode-segmentation to iterate graphemes instead of chars https://reviewboard.mozilla.org/r/225636/#review231630
Attachment #8956686 -
Flags: review?(ato) → review+
Comment 13•6 years ago
|
||
Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9b506268413c Use unicode-segmentation to iterate graphemes instead of chars r=ato
Comment 14•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9b506268413c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10220 for changes under testing/web-platform/tests
Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/w3c/web-platform-tests/pull/10220 * continuous-integration/travis-ci/pr (https://travis-ci.org/w3c/web-platform-tests/builds/359524253?utm_source=github_status&utm_medium=notification)
Upstream PR merged
You need to log in
before you can comment on or make changes to this bug.
Description
•