1134539 - Create higher level synthesizeKey() API which emulate key events with minimum arguments

Status: RESOLVED DUPLICATE of bug 1119609

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Currently, if tests attempt to emulate physical key events strictly, they need to specify, .key value, .code value and .keyCode value at least.

If keyboard layout is specified, .keyCode value can be computed from .key value or .code value even if it's printable key.

When synthesizing a printable key event, the minimum information is inputting character. Therefore, I'm thinking the new API for printable key is:

> synthesizePrintableKey(aChar, aAdditionalData[, aKeyboardLayout, aWindow]);

On the other hand, when synthesizeing a non-printable key event, the minimum information is .key value. Although, some modifier keys need .code value too.

> synthesizeNonPrintableKey(aKey, aAdditionalData[, aKeyboardLayout, aWindow]);

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.0 Replace CRLF with LF of some tests

Some tests are written with CRLF :-(

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Implement inputTextFromKeyboard() in EventUtils.js

Instead of sendChar() and sendString(), this patch creates inputTextFromKeyboard(). It can take keyboard layout name which is defined in bottom of EventUtils.js. Using this function makes all tests:

* dispatch "Shift" key event automatically when it should be dispatched. This makes all tests can dispatch events naturally.
* dispatch key events whose all attribute values conform to D3E spec and are same as actual keyboard's behavior.

The function doesn't work when one or modifier state is active. But it's not problem in most cases (actually, I've not met any trouble rewriting all tests with this).

For checking all desktop keyboard behavior, when the keyboard isn't specified explicitly, the default keyboard layout is US keyboard layout on each platform.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.5 Implement sendKey(), sendKeydown(), sendKeyup(), sendKeyExpectEvent(), sendKeydownExpectEvent() and sendKeyupExpectEvent()

sendKey(), sendKeydown() and sendKeyup() will replace most synthesizeKey() calls in all tests with simpler arguments.

These methods takes one or more key values (when it's a registered key name, "KEY_" prefix is needed). When the caller wants to emulate a key press, the first argument should be string. When the caller wants to emulate two or more key presses, it should be array of string.

If two or more registered key names are specified continuously, this function assume second or later keys are generated by auto repeat (i.e., keydown -> keypress -> keydown -> keypress -> ... -> keyup).

The second argument must be array of modifier keys. For example, [ "Shift", "Control", "Alt" ] is specified, the modifier keydown events will be dispatched first (only when the modifier state is not active). Finally, the modifier keyup events will be dispatched opposite order.

If specified key name is printable key's but it doesn't match Shift, CapsLock and AltGraph state, the key value is adjusted automatically.

E.g., sendKey("a", [ "Shift" ]); without "CapsLock" state, the key value will be "A" rather than "a".

This method makes all tests dispatch key events whose attribute values conform to D3E spec and actual keyboard behavior.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.9 Implement sendNumpadKey(), sendNumpadKeydown(), sendNumpadKeyup(), sendNumpadKeyExpectEvent(), sendNumpadKeydownExpectEvent() and sendNumpadKeyupExpectEvent()

This patch adds sendNumpadKey*(). This can dispatch key events for numpad keys. If "NumLock" state is necessary, the test needs to call sendKey("KEY_NumLock", []); before calling of these functions.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.8 Add shareModifierState() in EventUtils.js

For emulating actual modifier key behavior between multiple windows, this is necessary (only for window_menubar.xul for now).

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.14 Implement sendKeyDefinedByXULKey() and sendKeyDefinedByXULKeyExpectEvent()

A lot of tests of devtools try to dispatch key events whose data are retrieved from a XUL key element. For making these tests simpler and conform to D3E and actual behavior, we should create these functions.

sendKeyDefinedByXULKey() computes key value and modifier value from given XUL key element and use sendKey() with specified keyboard layout.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.19 Rename synthesizeKey() to synthesizeKeyRaw()

For conforming D3E and actual behavior, all tests shouldn't use synthesizeKey() directly because each test author investigate all D3E attribute values on actual environment and need to modify the argument when D3E spec is updated.

So, for preventing developers to use synthesizeKey() directly, rename it to synthesizeKeyRaw(). It might be better to append "_" prefix, though.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Following patches rewrites all tests which use key event synthesizer in EventUtils.js. So, some of these patches are rejected almost everyday. Therefore, I think that I should land all patches of this bug on Sunday for preventing bustage as far as possible.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Replace sendChar() and sendString() with inputTextFromKeyboard()

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 Use inputTextFromKeyboard() if synthesizeKey() attempts to type one or more characters

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.4 Use inputTextFromKeyboardExpectEvent() if synthesizeKeyExpectEvent() attempts to type a character

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.6 Rewrite sendKey() in all tests

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.7 Rewrite synthesizeKey() with sendKey()

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.10 Rewrite synthesizeKey() with sendNumpadKey()

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.11 Rewrite accessible/tests/mochitest/events.js with sendKey()

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.12 Rewrite browser/base/content/test/general/searchSuggestionUI.js with sendKey()

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.13 Rewrite layout/base/tests/selection-utils.js with sendKey()

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.15 Rewrite tests of devtools with EventUtils.sendKeyDefinedByXULKey()

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.16 Rewrite browser/devtools/framework/test/browser_toolbox_window_shortcuts.js

gDevTools._tools stores XUL key element as an simple object like { key: "x", modifiers: "shift" } or { keycode: "VK_X", modifiers: "shift" }. Therefore, sendKeyDefinedByXULKey() should be able to take such object too.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.17 Rewrite browser/devtools/performance/test/head.js with EventUtils.sendKey()

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.18 Rewrite browser_editor_autocomplete_js.js and browser_editor_autocomplete_events.js with EventUtils.sendKeyDefinedByXULKey()

I think that it doesn't make sense to add another API into EventUtils.js only for Editor.keyFor(). sendKeyDefinedByXULKey() is enough to handle it because the test isn't so many.

With it, the keycode value could be "VK_SPACE" or something which is not supported by _guessKeyNameFromKeyCode(). Therefore, this patch touches sendKeyDefinedByXULKey().

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I hope that you only hate some function names even if you will mark r- :-)

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #8)
> Following patches rewrites all tests which use key event synthesizer in
> EventUtils.js. So, some of these patches are rejected almost everyday.
> Therefore, I think that I should land all patches of this bug on Sunday for
> preventing bustage as far as possible.

I meant 5th, April for landing them.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Ok, so reviewing during next week should be fine, right?
Though, I'm planning to take perhaps couple of days of around Easter (and Easter Friday and Monday are holidays anyway in Finland), so I'll try to get through the patches still during this month.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #25)
> Ok, so reviewing during next week should be fine, right?

Right.

> Though, I'm planning to take perhaps couple of days of around Easter (and
> Easter Friday and Monday are holidays anyway in Finland), so I'll try to get
> through the patches still during this month.

Yep, it's no problem to land the patches on even later than 4/5. I think that the changes of EventUtils.js is the important part. Other huge changes are just simply rewriting all tests with new API.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8584336 [details] [diff] [review]
part.0 Replace CRLF with LF of some tests

there seems to be some change to 
browser_autocomplete_oldschool_wrap.js
since the patch didn't apply there.
(I just wanted -w so, applied the patch locally.)
But this is trivial fix, just update whatever is needed for browser_autocomplete_oldschool_wrap.js

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8584338 [details] [diff] [review]
part.1 Implement inputTextFromKeyboard() in EventUtils.js

>+  var TIP = _getTIP(aWindow);
>+  // Following non-lockable modifiers shouldn't be true.
I think "The following..."

>+  var activeModifiers = _activeModifiers(aWindow);
>+  if (activeModifiers.length) {
>+    throw activeModifiers[i] + " should be deactivated when " +
>+      "inputTextFromKeyboard() is called";
>+  }
'i' isn't define here. Please fix it.


>+  for (var i = 0; i < aText.length; i++) {
>+    var keyData = null;
>+    // A key may input multiple characters. So, we need to search with short
>+    // text when there is no key to input the character.
could you say in the comment what text you're trying to find.

>+function _getKeyData(aKeyValue, aKeyboardLayout)
>+{
>+  var keyData = null;
>+  for (var layout = aKeyboardLayout; layout !== null;
>+       layout = kKeyboardLayouts[layout].printableKeys.base) {

This needs some comment. It is not clear to me what we're iterating here, or how.



>+/**
>+ * You can inherits another key layouts with "base". This must be specified.
...ah, that is explained here. Perhaps just add a comment to _getKeyData: @see kKeyboardLayouts
>+        // "\u02DC": { code: "KeyN", keyCode: KeyboardEvent.DOM_VK_N, shiftKey: false, altGraphKey: true, capsLockKey: false, deadKey: true },
This could use some comment, why it is //'ed
Same with other similar cases.


I didn't review different key layouts carefully. Please re-ask review if you think one should go through them key by key.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8584350 [details] [diff] [review]
part.3 Use inputTextFromKeyboard() if synthesizeKey() attempts to type one or more characters

>-      EventUtils.synthesizeKey("e", { shiftKey: 1 }, content);
>+      EventUtils.inputTextFromKeyboard("e", content);
so shiftKey: 1 just isn't used here?

>       EventUtils.synthesizeKey("l", { accelKey: true });
>-      EventUtils.synthesizeKey("1", {});
>-      EventUtils.synthesizeKey("5", {});
>+      EventUtils.inputTextFromKeyboard("15");
Looks odd to use both synthesizeKey and inputTextFromKeyboard.
Not sure I now understand the setup. When is one supposed to use 
synthesizeKey and when inputTextFromKeyboard - why does the caller need to know if text would be inputted.

In browser_inspector_search-01.js you should update the comment which explains what KEY_STATES contains.
Similar in browser_inspector_search-navigation.js and
browser_markupview_css_completion_style_attribute.js


But I don't know why we need this patch at all.
r- for now, but could be r+ if some latter patch explains the setup.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

part 19 explains...but this setup is getting very complicated.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8584345 [details] [diff] [review]
part.14 Implement sendKeyDefinedByXULKey() and sendKeyDefinedByXULKeyExpectEvent()

I don't think we should add sendKeyDefinedByXULKey.
Odd to have native key event emulation to depend on some xul feature.
And adding yet another way to send key events makes this all just more complicated, IMO

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8584345 [details] [diff] [review]
part.14 Implement sendKeyDefinedByXULKey() and sendKeyDefinedByXULKeyExpectEvent()

I changed my mind after reading part 15. This kind of helper method could be nice after all. Weird, but useful.

Could you please add a check to sendKeyDefinedByXULKey that it really is a
xul key element.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

But something in the setup is still odd.
When to use sendFoo() and when inputTextFromKeyboard()? Why the functions have so different names.

Could we rename inputTextFromKeyboard() to sendKeyEventsFor(text, ...); ?

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8584359 [details] [diff] [review]
part.16 Rewrite browser/devtools/framework/test/browser_toolbox_window_shortcuts.js

Oh, here sendKeyDefinedByXULKey is used for something else.
sendKeyDefinedByXULKey should definitely be renamed to something else.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8584362 [details] [diff] [review]
part.18 Rewrite browser_editor_autocomplete_js.js and browser_editor_autocomplete_events.js with EventUtils.sendKeyDefinedByXULKey()

So this is using sendKeyDefinedByXULKey in a way which has nothing to do with
XULKey, so the other patch needs to be updated first so that the function name is something sane.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8584344 [details] [diff] [review]
part.8 Add shareModifierState() in EventUtils.js

TIP doesn't say anything to most of the readers of this code. So please expand TIP.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8584352 [details] [diff] [review]
part.6 Rewrite sendKey() in all tests

>+ * sendKey(), sendKeydown() and sendKeyup() synthesizes key events with modifier
>+ * key events specified by aModifiers.
synthesize

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8584350 [details] [diff] [review]
part.3 Use inputTextFromKeyboard() if synthesizeKey() attempts to type one or more characters

So I guess I can give r+ to this, but 
I don't like the name inputTextFromKeyboard() since it is so different to
other key event functions. So that should be fixed.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8584348 [details] [diff] [review]
part.19 Rename synthesizeKey() to synthesizeKeyRaw()

Do we ever need synthesizeKeyRaw? And why? We shouldn't.

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Hmm, other events use synthesizeFoo. Why this new stuff uses sendFoo.
Some consistency would be good.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8584340 [details] [diff] [review]
part.5 Implement sendKey(), sendKeydown(), sendKeyup(), sendKeyExpectEvent(), sendKeydownExpectEvent() and sendKeyupExpectEvent()

So, why send*, why not synthesize.

I think the function naming needs to be sorted out before I continue reviewing.

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

So I think there should be consistently named small set of functions for various use cases.
And naming should make it clear how different methods differ from each others.

(in general this all looks great of course, just trying to make sure the API is a simple and clear as it can be for the users and for whoever ends up reading the code using the API)

Comment 43

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you for the review!

(In reply to Olli Pettay [:smaug] (review queue is getting too long. New requests after Easter, please) from comment #27)
> Comment on attachment 8584336 [details] [diff] [review]
> part.0 Replace CRLF with LF of some tests
> 
> there seems to be some change to 
> browser_autocomplete_oldschool_wrap.js
> since the patch didn't apply there.
> (I just wanted -w so, applied the patch locally.)
> But this is trivial fix, just update whatever is needed for
> browser_autocomplete_oldschool_wrap.js

Yeah, I'm updating the patches everyday locally but I just don't update the patches here because there is no change logically (i.e., just rejected with non-related line's changes).

(In reply to Olli Pettay [:smaug] (review queue is getting too long. New requests after Easter, please) from comment #28)
> Comment on attachment 8584338 [details] [diff] [review]
> part.1 Implement inputTextFromKeyboard() in EventUtils.js
> >+        // "\u02DC": { code: "KeyN", keyCode: KeyboardEvent.DOM_VK_N, shiftKey: false, altGraphKey: true, capsLockKey: false, deadKey: true },
> This could use some comment, why it is //'ed
> Same with other similar cases.

In some keyboard layout, some printable key inputs same character(s) (which is inputted with other keys) or not input any characters (i.e., inputs empty string). In such case, the object cannot have same key in an object. Therefore, I commented out rather than just removed. If I removed them, it would make other developers hard to understand why some keys are not defined in the object.

> I didn't review different key layouts carefully. Please re-ask review if you
> think one should go through them key by key.

I don't think that it needs to be reviewed deeply. When somebody meets odd result due to my mistake, they can fix that with changing the layout database.

(In reply to Olli Pettay [:smaug] (review queue is getting too long. New requests after Easter, please) from comment #29)
> Comment on attachment 8584350 [details] [diff] [review]
> part.3 Use inputTextFromKeyboard() if synthesizeKey() attempts to type one
> or more characters
> 
> >-      EventUtils.synthesizeKey("e", { shiftKey: 1 }, content);
> >+      EventUtils.inputTextFromKeyboard("e", content);
> so shiftKey: 1 just isn't used here?

Oh, could be "E", but I was creating the patch, I met a case which don't work with actual key sequence. In that case, I changed the modifier state for preferring the specified key value.

> >       EventUtils.synthesizeKey("l", { accelKey: true });
> >-      EventUtils.synthesizeKey("1", {});
> >-      EventUtils.synthesizeKey("5", {});
> >+      EventUtils.inputTextFromKeyboard("15");
> Looks odd to use both synthesizeKey and inputTextFromKeyboard.
> Not sure I now understand the setup. When is one supposed to use 
> synthesizeKey and when inputTextFromKeyboard - why does the caller need to
> know if text would be inputted.

inputTextFromKeyboard() is higher level API for synthesizing key events which cause text input. They can be rewritten as |sendKey([ "1", "5" ], []);|, However, I found some cases that a physical key causes two or more character input. Even in such case, |inputTextFromKeyboard()| can find expected keys smoothly. Therefore, developers can write tests simpler.

Should I rewrite it with sendKey() (perhaps, will be renamed)?

> In browser_inspector_search-01.js you should update the comment which
> explains what KEY_STATES contains.
> Similar in browser_inspector_search-navigation.js and
> browser_markupview_css_completion_style_attribute.js

Thanks, I'll check them.

> But I don't know why we need this patch at all.
> r- for now, but could be r+ if some latter patch explains the setup.

Okay, please consider if all printable keys should be emulated with sendKey().

(In reply to Olli Pettay [:smaug] (review queue is getting too long. New requests after Easter, please) from comment #32)
> Comment on attachment 8584345 [details] [diff] [review]
> part.14 Implement sendKeyDefinedByXULKey() and
> sendKeyDefinedByXULKeyExpectEvent()
> 
> I changed my mind after reading part 15. This kind of helper method could be
> nice after all. Weird, but useful.

Yeah, I didn't want to create it when I'm writing previous patches, but the tests are not rewritable simply.

> Could you please add a check to sendKeyDefinedByXULKey that it really is a
> xul key element.

Hmm, at the later patch, it becomes more hacky... Sigh...

(In reply to Olli Pettay [:smaug] (review queue is getting too long. New requests after Easter, please) from comment #34)
> Comment on attachment 8584359 [details] [diff] [review]
> part.16 Rewrite
> browser/devtools/framework/test/browser_toolbox_window_shortcuts.js
> 
> Oh, here sendKeyDefinedByXULKey is used for something else.
> sendKeyDefinedByXULKey should definitely be renamed to something else.

Hmm, do you have better name idea? Anyway, the object is collected XUL key element's information...

(In reply to Olli Pettay [:smaug] (review queue is getting too long. New requests after Easter, please) from comment #35)
> Comment on attachment 8584362 [details] [diff] [review]
> part.18 Rewrite browser_editor_autocomplete_js.js and
> browser_editor_autocomplete_events.js with
> EventUtils.sendKeyDefinedByXULKey()
> 
> So this is using sendKeyDefinedByXULKey in a way which has nothing to do with
> XULKey, so the other patch needs to be updated first so that the function
> name is something sane.

Hmm...

(In reply to Olli Pettay [:smaug] (review queue is getting too long. New requests after Easter, please) from comment #39)
> Comment on attachment 8584348 [details] [diff] [review]
> part.19 Rename synthesizeKey() to synthesizeKeyRaw()
> 
> Do we ever need synthesizeKeyRaw? And why? We shouldn't.

If developers want to test keys which are NOT in PC keyboard. E.g., emulating virtual keys on mobile device. In such case, lower level API may be useful. For example, |synthesizeKeyRaw("KEY_VolumeDown", { code: "", keyCode: KeyboardEvent.DOM_VK_VOLUME_DOWN });| or |synthesizeKeyRaw("a", { code: "", keyCode: KeyboardEvent.DOM_VK_A });|

(In reply to Olli Pettay [:smaug] (review queue is getting too long. New requests after Easter, please) from comment #41)
> Comment on attachment 8584340 [details] [diff] [review]
> part.5 Implement sendKey(), sendKeydown(), sendKeyup(),
> sendKeyExpectEvent(), sendKeydownExpectEvent() and sendKeyupExpectEvent()
> 
> So, why send*, why not synthesize.
> 
> I think the function naming needs to be sorted out before I continue
> reviewing.

Because the higher level API have used send*(). If you like, we can rename them to:

synthesizeKey(), synthesizeKeydown(), synthesizeKeyup()...

And also

inputTextFromKeyboard() -> synthesizeTextFromKeyboard()

sendNumpadKey() -> synthesizeNumpadKey()

sendKeyDefinedByXULKey() -> synthesizeKeyDefinedXULKey()

?

# I need to go out today, therefore, probably, I cannot land them this Sunday. We should put off the schedule to next Sunday.

Comment 44

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> Because the higher level API have used send*(). If you like, we can rename them to:
> 
> synthesizeKey(), synthesizeKeydown(), synthesizeKeyup()...

Ah, but synthesizeKey() makes some developers confused due to same name with current API.

So, I still like "send" for higher API. The shorter name is more useful, "synthesize" is too long if we append additional name for the various functions...

Comment 45

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] (New review requests after Easter, please, Friday and Monday are holidays in Finland) from comment #29)
> Comment on attachment 8584350 [details] [diff] [review]
> part.3 Use inputTextFromKeyboard() if synthesizeKey() attempts to type one
> or more characters
> 
> >-      EventUtils.synthesizeKey("e", { shiftKey: 1 }, content);
> >+      EventUtils.inputTextFromKeyboard("e", content);
> so shiftKey: 1 just isn't used here?

Okay, I confirmed that the original code is buggy. Shift + "e" should be "E". However, if sending "E" causes test failure. So, it's been testing impossible case.

> >       EventUtils.synthesizeKey("l", { accelKey: true });
> >-      EventUtils.synthesizeKey("1", {});
> >-      EventUtils.synthesizeKey("5", {});
> >+      EventUtils.inputTextFromKeyboard("15");
> Looks odd to use both synthesizeKey and inputTextFromKeyboard.
> Not sure I now understand the setup. When is one supposed to use 
> synthesizeKey and when inputTextFromKeyboard - why does the caller need to
> know if text would be inputted.
> 
> But I don't know why we need this patch at all.
> r- for now, but could be r+ if some latter patch explains the setup.

So, there is no strong rule to use inputTextFromKeyboard() because sendKey() can replace it. However, inputTextFromKeyboard() is more optimized than sendKey() for inputting text with printable keys. I can say, it's just a syntax sugar.

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Unfortunately, I'm not available on 4/18 - 4/20, however, probably, remaining patches won't be r+'ed only tomorrow. So, the next chance to land them is 4/26.

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

Oh, I'll try to get my review queue empty still today.

Comment 48

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm happy if you would decide the new API names first (even if you cannot review them actually). Then, I'll rewrite all patches (simply, using replace) and request remaining reviews again.

I think that synthesize* could be very long name. So, I still like send* or something similar name.

And also I still don't have better idea of alternative name of sendKeyDefinedByXULKey()...

Comment 49

[Neil Deakin]

I would prefer to use synthesizeKey to match synthesizeMouse and other similar functions Otherwise it looks like it mirrors EventUtils.sendMouseEvent which doesn't really synthesize a mouse event.

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, Enn. My concern of using synthesizeKey() is same as current API. Although, even if we'll use it, the arguments are changed and developers will see exception if the arguments are same as old one.

If we will use synthesize*Key*(), I'll replace the sendKey in the patches with synthesizeKeyNew temporarily and adding new patch to the last for replacing it with synthesizeKey().

Anyway, I'd like smaug to decide the new names as soon as possible.

Comment 51

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Well, we've already have better API than when I filed this bug.