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)

enhancement

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.
Priority: -- → P3
Blocks: webdriver
Priority: P3 → P2
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!
Attachment #8956216 - Attachment is obsolete: true
Attachment #8956604 - Attachment is obsolete: true
Attachment #8956604 - Attachment is obsolete: false
Attachment #8956216 - Attachment is obsolete: false
Attachment #8955847 - Attachment is obsolete: true
Attachment #8956612 - Attachment is obsolete: true
Attachment #8956604 - Attachment is obsolete: true
Attachment #8956216 - Attachment is obsolete: true
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 atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9b506268413c
Use unicode-segmentation to iterate graphemes instead of chars r=ato
https://hg.mozilla.org/mozilla-central/rev/9b506268413c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
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
You need to log in before you can comment on or make changes to this bug.