1396823 - Use unicode-segmentation to process grapheme clusters in key actions

Status: RESOLVED FIXED

(Testing :: geckodriver, enhancement, P2)

Description

[Andreas Tolfsen ❲:ato❳]

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 1

[Greg Fraley]

Attachment: Use unicode-segmentation to iterate graphemes instead of chars

Comment 2

[Andreas Tolfsen ❲:ato❳]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ffc0dda79b9de5e3ad63a2b3383ffc0ead729aa

Comment 3

[Andreas Tolfsen ❲:ato❳]

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 (-:

Comment 4

[Greg Fraley]

Sounds good.  I'll prep some tests and submit them, hopefully through MozReview this time.

Comment 5

[Greg Fraley]

Attachment: Bug 1396823 - Added tests for special keys that use multiple codepoints

Review commit: https://reviewboard.mozilla.org/r/225126/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225126/

Comment 6

[Andreas Tolfsen ❲:ato❳]

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)
> [e-mail]("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.

Comment 7

[Greg Fraley]

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 8

[Greg Fraley]

Attachment: Bug 1396823 - Use unicode-segmentation to iterate graphemes instead of chars

Review commit: https://reviewboard.mozilla.org/r/225532/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225532/

Comment 9

[Greg Fraley]

Attachment: Bug 1396823 - Split multiple codepoint key test into two tests

Review commit: https://reviewboard.mozilla.org/r/225546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225546/

Comment 10

[Greg Fraley]

Attachment: Bug 1396823 - Use unicode-segmentation to iterate graphemes instead of chars

Review commit: https://reviewboard.mozilla.org/r/225636/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225636/

Comment 11

[Andreas Tolfsen ❲:ato❳]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cfc6b3263a5f835813b7f7c7a9b784d5f5c0f79

Comment 12

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8956686 [details]
Bug 1396823 - Use unicode-segmentation to iterate graphemes instead of chars

https://reviewboard.mozilla.org/r/225636/#review231630

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b506268413c
Use unicode-segmentation to iterate graphemes instead of chars r=ato

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/9b506268413c

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10220 for changes under testing/web-platform/tests

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

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)

Comment 17

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged