Closed Bug 1396823 Opened 3 years ago Closed 3 years ago
Use unicode-segmentation to process grapheme clusters in key actions
59 bytes, text/x-review-board-request
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.
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-
Assignee: nobody → gsfraley
Status: NEW → ASSIGNED
OS: Unspecified → All
Hardware: Unspecified → All
Version: Version 3 → Trunk
Sounds good. I'll prep some tests and submit them, hopefully through MozReview this time.
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-
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 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+
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/9b506268413c Use unicode-segmentation to iterate graphemes instead of chars r=ato
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.