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

ASSIGNED
Assigned to

Status

()

ASSIGNED
4 years ago
8 months ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks: 2 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(20 attachments)

38.65 KB, patch
smaug
: review+
Details | Diff | Splinter Review
109.50 KB, patch
smaug
: review+
Details | Diff | Splinter Review
61.43 KB, patch
smaug
: review-
Details | Diff | Splinter Review
33.84 KB, patch
Details | Diff | Splinter Review
2.74 KB, patch
smaug
: review+
Details | Diff | Splinter Review
12.88 KB, patch
smaug
: review+
Details | Diff | Splinter Review
13.50 KB, patch
smaug
: review-
Details | Diff | Splinter Review
86.37 KB, patch
smaug
: review+
Details | Diff | Splinter Review
214.54 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.44 KB, patch
smaug
: review+
Details | Diff | Splinter Review
61.43 KB, patch
smaug
: review+
Details | Diff | Splinter Review
916.22 KB, patch
Details | Diff | Splinter Review
7.14 KB, patch
smaug
: review+
Details | Diff | Splinter Review
18.39 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.80 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.99 KB, patch
smaug
: review+
Details | Diff | Splinter Review
13.42 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.04 KB, patch
smaug
: review-
Details | Diff | Splinter Review
1.83 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.53 KB, patch
smaug
: review-
Details | Diff | Splinter Review
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]);
Blocks: 1136539
Created attachment 8584336 [details] [diff] [review]
part.0 Replace CRLF with LF of some tests

Some tests are written with CRLF :-(
Attachment #8584336 - Flags: review?(bugs)
Status: NEW → ASSIGNED
Created attachment 8584338 [details] [diff] [review]
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.
Attachment #8584338 - Flags: review?(bugs)
Created attachment 8584340 [details] [diff] [review]
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.
Attachment #8584340 - Flags: review?(bugs)
Created attachment 8584341 [details] [diff] [review]
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.
Attachment #8584341 - Flags: review?(bugs)
Created attachment 8584344 [details] [diff] [review]
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).
Attachment #8584344 - Flags: review?(bugs)
Created attachment 8584345 [details] [diff] [review]
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.
Attachment #8584345 - Flags: review?(bugs)
Created attachment 8584348 [details] [diff] [review]
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.
Attachment #8584348 - Flags: review?(bugs)
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.
Created attachment 8584349 [details] [diff] [review]
part.2 Replace sendChar() and sendString() with inputTextFromKeyboard()
Attachment #8584349 - Flags: review?(bugs)
Created attachment 8584350 [details] [diff] [review]
part.3 Use inputTextFromKeyboard() if synthesizeKey() attempts to type one or more characters
Attachment #8584350 - Flags: review?(bugs)
Created attachment 8584351 [details] [diff] [review]
part.4 Use inputTextFromKeyboardExpectEvent() if synthesizeKeyExpectEvent() attempts to type a character
Attachment #8584351 - Flags: review?(bugs)
Created attachment 8584352 [details] [diff] [review]
part.6 Rewrite sendKey() in all tests
Attachment #8584352 - Flags: review?(bugs)
Created attachment 8584353 [details] [diff] [review]
part.7 Rewrite synthesizeKey() with sendKey()
Attachment #8584353 - Flags: review?(bugs)
Created attachment 8584354 [details] [diff] [review]
part.10 Rewrite synthesizeKey() with sendNumpadKey()
Attachment #8584354 - Flags: review?(bugs)
Created attachment 8584355 [details] [diff] [review]
part.11 Rewrite accessible/tests/mochitest/events.js with sendKey()
Attachment #8584355 - Flags: review?(bugs)
Created attachment 8584356 [details] [diff] [review]
part.12 Rewrite browser/base/content/test/general/searchSuggestionUI.js with sendKey()
Attachment #8584356 - Flags: review?(bugs)
Created attachment 8584357 [details] [diff] [review]
part.13 Rewrite layout/base/tests/selection-utils.js with sendKey()
Attachment #8584357 - Flags: review?(bugs)
Created attachment 8584358 [details] [diff] [review]
part.15 Rewrite tests of devtools with EventUtils.sendKeyDefinedByXULKey()
Attachment #8584358 - Flags: review?(bugs)
Created attachment 8584359 [details] [diff] [review]
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.
Attachment #8584359 - Flags: review?(bugs)
Created attachment 8584360 [details] [diff] [review]
part.17 Rewrite browser/devtools/performance/test/head.js with EventUtils.sendKey()
Attachment #8584360 - Flags: review?(bugs)
Created attachment 8584362 [details] [diff] [review]
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().
Attachment #8584362 - Flags: review?(bugs)
I hope that you only hate some function names even if you will mark r- :-)
(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.
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.
(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 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
Attachment #8584336 - Flags: review?(bugs) → review+
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.
Attachment #8584338 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #8584349 - Flags: review?(bugs) → review+
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.
Attachment #8584350 - Flags: review?(bugs) → review-
part 19 explains...but this setup is getting very complicated.
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
Attachment #8584345 - Flags: review?(bugs) → review-

Updated

4 years ago
Attachment #8584358 - Flags: review?(bugs) → review+
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.
Attachment #8584345 - Flags: review- → review+
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, ...); ?

Updated

4 years ago
Attachment #8584357 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #8584354 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #8584355 - Flags: review?(bugs) → review+
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.
Attachment #8584359 - Flags: review?(bugs) → review-

Updated

4 years ago
Attachment #8584360 - Flags: review?(bugs) → review+
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.
Attachment #8584362 - Flags: review?(bugs) → review-

Updated

4 years ago
Attachment #8584356 - Flags: review?(bugs) → review+
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.
Attachment #8584344 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #8584351 - Flags: review?(bugs) → review+
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
Attachment #8584352 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #8584348 - Flags: review?(bugs) → review+
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.
Attachment #8584350 - Flags: review- → review+
Comment on attachment 8584348 [details] [diff] [review]
part.19 Rename synthesizeKey() to synthesizeKeyRaw()

Do we ever need synthesizeKeyRaw? And why? We shouldn't.
Attachment #8584348 - Flags: review+ → review-
Hmm, other events use synthesizeFoo. Why this new stuff uses sendFoo.
Some consistency would be good.
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.
Attachment #8584340 - Flags: review?(bugs) → review-

Updated

4 years ago
Attachment #8584341 - Flags: review?(bugs)

Updated

4 years ago
Attachment #8584353 - Flags: review?(bugs)
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)
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.
Flags: needinfo?(bugs)
> 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...
(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.
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.
Oh, I'll try to get my review queue empty still today.
Flags: needinfo?(bugs)
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()...
Blocks: 711906

Comment 49

4 years ago
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.
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.
(Assignee)

Updated

8 months ago
Depends on: 1436926
You need to log in before you can comment on or make changes to this bug.