1119609 - event.key from nsIDOMWindowUtils.sendKeyEvent() is 'undefined'

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[C.M.Chang[:chunmin]]

shell.js
---------------------------
var utils = window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
utils.sendKeyEvent("keyup", 36/*home key*/, 0, null);


hardware_buttons.js
---------------------------
HardwareButtons.prototype.handleEvent = function hb_handleEvent(evt) {
...
dump(evt.key); ==> Get 'undefined' here
...
}

Comment 1

[C.M.Chang[:chunmin]]

the variable mKeyNameIndex at https://dxr.mozilla.org/mozilla-central/source/widget/TextEvents.h#172 assigned by https://dxr.mozilla.org/mozilla-central/source/widget/TextEvents.h#190 would be enum KeyNameIndex(0) here if the event is triggered by nsIDOMWindowUtils.sendKeyEvent().

Comment 2

[Gina Yeh [:gyeh] [:ginayeh]]

Is there any way to synthesize a key event with correct key?

Two possible ways to do so:
1) Implement nsIDOMWindowUtils::SynthesizeNativeKeyEvent() for Gonk. This function should be used to synthesize a native key event (as its name shows). However, it's only implemented on partial platforms (cocoa and windows). So, we may want to implement the function on Gonk.

2) Modify the implementation nsIDOMWindowUtils::SendKeyEvent(). I think we can just map keycode to keyNameIndex.

Masayuki, can you give us some suggestions?

Comment 3

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

I think that the #2 is better.

I believe that new API should take KeyboardEvent for its argument. Then, you can write the code as:

var event = { keyCode: 123, charCode: 0, key: "Foo", code: "Bar", shiftKey: true, ...};
var keydown = new KeyboardEvent("keydown", event);
if (!isModifierKey && domWindowUtils.sendKeyboardEvent(keydown)) {
  var keypress = new KeyboardEvent("keypress", event);
  domWindowUtils.sendKeyboardEvent(keypress);
}
var keyup = new KeyboardEvent("keyup", event);
domWindowUtils.sendKeyboardEvent(keyup);

See also:
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/KeyboardEvent.webidl?rev=8335aff95389#36
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js?rev=4783e19c5feb#594

Comment 4

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

And also we should support keyModifierState* before that. Although, the spec may not be so stable.
https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#h-shared-mouse-and-keyboard-initializers

Comment 5

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

FYI: I'm working on new IME API for Gonk (bug 917322). I want to fix it in early 38. If you are not so hurry for this issue, I'll work on this after that. One of the reason is that I'd like to merge Keyboard API with the new IME API.

Comment 6

[Gina Yeh [:gyeh] [:ginayeh]]

Thanks for your feedback. It's a great idea to take KeyboardEvent for its argument.

Comment 7

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

I like the idea taking a keyboard event as a param.

Comment 8

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

Smaug:

I have a question. I think that nsIInputMethodEditor should have key event dispatching methods too because it manages the rights to use TextEventDispatcher. So, I'm think that we should add following methods into nsIInputMethodEditor:

/**
 * keydown() maybe dispatches a keydown event and some keypress events
 * if preceding keydown event isn't consumed and they are necessary.
 * Note that even if this is called during composition, key events may not
 * be dispatched.  In this case, this returns false.
 *
 * @param aKeyboardEvent  Must be a keydown event which should be dispatched.
 *                        Note that you must set charCode value non-zero if the
 *                        key is a printable key.  Then, this method computes charCode
 *                        values from KeyboardEvent.key value.
 *                        If you call preventDefault() before calling this method, this
 *                        method will dispatch a keydown event whose defaultPrevented is
 *                        true.
 * @return                True if neither the keydown event or following keypress events
 *                        is *not* consumed.
 *                        Otherwise, i.e., preventDefault() is called, false.
 */
bool keydown(KeyboardEvent* aKeyboardEvent);

/**
 * Similar to keydown(), but this dispatches only a keyup event.
 */
bool keyup(KeyboardEvent* aKeyboardEvent);

How do you think this idea?

Comment 9

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

That is fine, but should we perhaps then call it something else than nsIInputMethodEditor?
I don't have any good suggestions for the name though.

Comment 10

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

(In reply to Olli Pettay [:smaug] from comment #9)
> That is fine, but should we perhaps then call it something else than
> nsIInputMethodEditor?
> I don't have any good suggestions for the name though.

Sure. There is an issue, that is key input may not input text.

Random thoughts:

nsITextInput (Sounds text edit control?)
nsIKeyTextInput
nsITextInputProcessor (TIP is term of IME on TSF)
nsIKeyTextInputProcessor (clearest? but long?)
nsITextInputSource (Text Input Source is term of keyboard or IME on Mac)
nsIInputProcessor
nsIInputSource
nsIKeyboardEmulator (sound different interface?)

Do you have a favorite in them? (Anyway, I'll rename it even after your review of bug 917322)

Comment 11

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

nsITextInputManager?

of your suggestions I like
nsITextInputProcessor and nsITextInputSource

Comment 12

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

(In reply to Olli Pettay [:smaug] from comment #11)
> nsITextInputManager?
> 
> of your suggestions I like
> nsITextInputProcessor and nsITextInputSource

Um, in those 3, I like nsITextInputProcessor. I'll update patches of bug 917322.

Comment 13

[Marco Chen [:mchen]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #3)
> I think that the #2 is better.
> 
> I believe that new API should take KeyboardEvent for its argument. Then, you
> can write the code as:

Hi,

I think the new API taking KeyboardEvent is good but I have a question about "#2 is better".

In order to get keyNameIndex, you need to have the native keycode [1] which is platform-dependent.
But the keycode passed by nsIDOMWindowUtils::SendKeyEvent() is for nsIDOMKeyEvent (which is platform-independent) not the native key code.

Therefore it seems that you can not do keyNameIndex inside the nsIDOMWindowUtils::SendKeyEvent() because this place should not know the platform-dependent mapping. It still not make sense to do "nsIDOMKeyEvent -> native key code -> keyNameIndex" inside the nsWindow::DispatchEvent() [2].

Therefore may I know your outcome in this patch is to follow "#2 is better" or new API using KeyboardEvent. Thanks.

[1] https://dxr.mozilla.org/mozilla-central/source/widget/gonk/nsAppShell.cpp#257
[2] https://dxr.mozilla.org/mozilla-central/source/widget/gonk/nsWindow.cpp#502

Comment 14

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

I'm thinking that the callers, i.e., JS-IME or JS-Keyboard should specify keyCode, key, code values by themselves. In ctor of KeyboardEvent, specified key value and code value should be converted to internal index if it's possible.

I'm not thinking that specifying native hardware keycode (scancode) is useful for B2G because mapping from hardware keycode (scancode) needs keyboard layout data in C++ level but it's impossible because JS-Keyboard can create non-PC like keyboard layout.

Comment 15

[Marco Chen [:mchen]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #14)

Got it. So you will not take #2 from comment 2 and the work here is to create a new function for accepting "keycode, key and code" from caller. right?

Comment 16

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

(In reply to Marco Chen [:mchen] from comment #15)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #14)
> 
> Got it. So you will not take #2 from comment 2 and the work here is to
> create a new function for accepting "keycode, key and code" from caller.
> right?

Yes. And note that if the API is called from JS, key events are dispatched via PuppetWidget, not Gonk widget.

Comment 17

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

Attachment: part.1 Implement key event dispatcher in TextEventDispatcher

I created 10 patches which only implement and test new API. I'll do other works later.

This is the first patch. TextEventDispatcher should have methods to dispatch WidgetKeyboardEvent. Their argument should be a WidgetKeyboardEvent instance and dispatches the event indirectly. The method should modify some values for each event type to dispatch. E.g., .keyCode of keypress should be 0 if .charCode isn't 0.

Comment 18

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

Attachment: part.2 Don't dispatch keyboard events from TextEventDispatcher if there is a composition

During composition, we don't dispatch key events. This should be managed by TextEventDispatcher.

This adds a pref to control this behavior for bug 354358.

Comment 19

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

Attachment: part.3 Implement converting methods from key/code value to key/code name index

New API shouldn't require .charCode value since if a key event causes two or more characters, it's impossible to dispatch them with a call.

For solving this issue, .charCode should be computed from .key value. Therefore, non-printable key should store its key names as key name index. In the future, this is necessary when our internal event handlers use .key value instead of .keyCode value.

In other words, TextEventDispatcher will treat key events whose key name isn't registered as a printable key's.

# If this is bad, we can improve the behavior with additional flag argument of new API. See later patches.

Comment 20

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

Attachment: part.4 Add keydown() and keyup() to nsITextInputProcessor

Let's add nsITextInputProcessor.keydown() and nsITextInputProcessor.keyup().

keypress event is automatically dispatched by keydown() if it's necessary.

These methods ignore modifier flags of its argument's KeyboardEvent because:

#1 We're supporting a lot of modifier states. It's not simple to set each modifier state every time to dispatch a key event.
#2 When modifier key is down or up, the modifier state should be stored by TextInputProcessor. And when dispatching key events, the stored modifier state should overwrite the modifier state of dispatching key events.
#3 It's natural behavior to dispatch modifier key events when modifier state is changed. This design guarantees this best behavior.

For conforming to D3E, TextInputProcessor shouldn't allow to use non-registered code value. Therefore, in such case, these methods throw an exception.

For easy to debug, JS-IME/Keyboard can specify KEY_NON_PRINTABLE_KEY flag to the additional argument of keydown() and keyup(). If this is specified, keydown() and keyup() throw an exception if the .key value isn't a registered key name.

Unfortunately, D3E KeyboardEvent cannot distinguish if a key event will cause inputting text or not. For example, if a key causes inputting "Enter", it shouldn't be converted to key name index because it means that the key is treated as a non-printable key. KEY_FORCE_PRINTABLE_KEY solves this issue. Although, this is very edge case.

Comment 21

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

Attachment: part.5 Compute KeyboardEvent.location and .keyCode if they are 0

For easier to use the new API, we should allow simpler KeyboardEvent instance for their argument as far as possible.

.location and .keyCode for non-printable can be guessed from .code value and .key value. Therefore, it's good to overwrite the values if they are not initialized or initialized with 0 (we cannot distinguish the different, unfortunately).

If JS-IME/Keyboard wants to set 0 to them forcibly, they can use KEY_KEEP_KEY_LOCATION_STANDARD and KEY_KEEP_KEYCODE_ZERO. However, if they are specified when the values are not 0, it must be a bug of the caller. Therefore, both keydown() and keyup() throw an exception in such cases.

Comment 22

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

Attachment: part.6 TextInputProcessor should manage modifier key state and set it to dispatching key events automatically

This patch makes TextInputProcessor manages modifier state in every instance. The reason why the modifier state isn't shared with all instances is that another instance might be created by different addon or something. If so, a JS-IME/Keyboard may cause unexpected key events. For preventing it, the modifier state should be manged by each instance.

Modifier state is activated by a modifier keydown. At that time, it's code value is also stored. This allows to emulate pressing left shift key, pressing right shift key, releasing right shift key and releasing left shift key. In this case, shift key state should be inactivated when both shift keys are released.

Comment 23

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

Attachment: part.7 Make TextInputProcerros possible to share modifier state

On the other hand, modifier state should be shared in all windows. This is the typical behavior of desktop platforms.

If JS-IME/Keyboard framework creates TextInputProcessor for each top level window and allows to compose in every window at same time (this is Mac's native IME behavior), it's better to share modifier state between two or more TextInputProcessor instances.

This patch makes ModifierKeyDataArray as a refcountable class. If nsITextInputProcessor.shareModifierStateOf() is called with another instance of TextInputProcessor, the called instance shares the argument's modifier state.

If it's called with null, it detached from other instances.

Comment 24

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

Attachment: part.8 nsITextInputProcessor.keydown() and nsITextInputProcessor.keyup() should be able to only modify its modifier state without dispatching key events

This patch allows JS-IME/Keyboard to modify modifier state without dispatching modifier key events. This is useful for tests and/or keeping compatibility with old API. Although, using this isn't recommended.

Comment 25

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

Attachment: part.9 Make a check in runUnloadTests2() of window_nsITextInputProcessor disabled due to unknown random failure

I don't know the reason of this.

Adding a lot of tests by the previous patches causes random orange at here.

When it fails, the <input> element doesn't get focus. If using setTimeout() from the load event handler and set focus to the input and starting composition causes MOZ_ASSERT() which tells us NS_COMPOSITION_START isn't dispatched.

So, this failure detects actual bug in the case of runUnloadTests2(). However, it should be fixed in another bug. This bug needs a lot of additional patches.

Comment 26

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

Attachment: part.10 Support all key and code values which are enough stable in DOM Level 3 KeyboardEvent key/code Values

Currently, we don't register some key and code values which are not used by native key event handlers. However, JS-IME/Keyboard may need them for emulating other devices which we don't support. Therefore, we should register them.

Note that this doesn't register some key and code values (e.g., Super and Hyper) which are not stable (if spec bugs are filed).

Comment 27

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

I'll keep working on this bug even if part.1 - 10 are r+'ed. If you think that I shouldn't land them until we create all remaining patches (e.g., updating EventUtils, dropping old API), I'll wait to land them. However, I don't think it causes serious problem to land them separately.

Comment 28

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

Gina Yeh:

When you apply the patches, you need to apply a patch for bug 1126673. It conflicts with the patches in all.js. (The patch is already landed into mozilla-inbound, therefore, tomorrow's m-c must include the patch too)

Comment 29

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #28)
> Gina Yeh:
> 
> When you apply the patches, you need to apply a patch for bug 1126673. It
> conflicts with the patches in all.js. (The patch is already landed into
> mozilla-inbound, therefore, tomorrow's m-c must include the patch too)

Thanks for your reminding. :) Jimmy [:chunmin] will try these patches tomorrow.

Comment 30

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

Jimmy:

Oops, forgot to say, when you applied all patches, runKeyTests() of window_nsITextInputProcessor.xul is also a good reference. In most cases, you need to create KeyboardEvent with:

printable key like "a", key, code, and keyCode.
non-printable key like "Enter", key and code.

.charCode and modifier flags are ignored. modifier flags are manged by each instance of nsITextInputProcessor. When you call keydown with modifier key, it activates proper modifier state automatically. Similarly, keyup with modifier key inactivates the modifier state.

If you don't emulate physical keyboard or other device, you should set proper code value. Otherwise, i.e., if the key event is caused by virtual keyboard like 10 key, you don't need to set it.

If you need to implement auto-repeat with the API, you should create KeyboardEvent with |repeat: true| when calling keydown() at second or later time. In that case, keydown() -> keydown() -> ... -> keyup() should be called. (i.e., keyup() shouldn't be called during auto-repeat)

Comment 31

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

Comment on attachment 8559698 [details] [diff] [review]
part.6 TextInputProcessor should manage modifier key state and set it to dispatching key events automatically

Oops, this isn't good behavior for *Lock modifiers. Locked modifiers should not be unlocked by first keyup event.

Comment 32

[C.M.Chang[:chunmin]]

Hi Masayuki,
Thx for your helpful information. After a simple test, your patches exactly fit our needs.

Comment 33

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

(In reply to Chun-Min Chang[:chunmin] from comment #32)
> Hi Masayuki,
> Thx for your helpful information. After a simple test, your patches exactly
> fit our needs.

You're welcome ;-)

You can ignore the bug which I'm fixing if you don't use lockable modifiers such as CapsLock.

Comment 34

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

Comment on attachment 8559692 [details] [diff] [review]
part.4 Add keydown() and keyup() to nsITextInputProcessor

>+ *
>+ * Example #8 JS-IME or JS-Keyboard should dispatch key events even if during
>+ *            composition (non-printable key case):
I think you can drop 'if'


>+ *
>+ *   if (!TIP.init(window, callback)) {
>+ *     return; // You failed to get the rights to dispatch key events
>+ *   }
>+ *
>+ *   // You don't need to specify .keyCode value if it's non-printable key
>+ *   // because it can be computed from .key value.
>+ *   // If you specify non-zero value to .keyCode, it'll be used.
>+ *   var keyEvent = new KeyboardEvent("", { code: "Enter", key: "Enter" });
>+ *   if (TIP.keydown(keyEvent)) {
>+ *     // Handle its default action
>+ *   }
>+ *
>+ *   if (!TIP.init(window, callback)) {
>+ *     return; // You failed to get the rights to dispatch key events
>+ *   }
Do you really need to call init again? That feels odd.

>+ * Example #9 JS-IME or JS-Keyboard should dispatch key events even if during
>+ *            composition (printable key case):
s/if//
>+ *   //   https://developer.mozilla.org/docs/Web/API/KeyboardEvent.keyCode
>+ *   //   #1 If the key can input [0-9], the value should be DOM_VK_[0-9].
>+ *   //   #2 If the key inputs an ASCII alphabet or numeric with no modifier
>+ *   //      states, use a proper value for the character.
>+ *   //   #3 If the key inputs an ASCII alphabet or numeric with shift key
>+ *   //      state, use a proper value for the shifted character.
what does this mean? if I want A (shift+a), what should be passed?

>+ *   //   #4 If the key inputs another ASCII character with no modifier states,
>+ *   //      use a proper value for the character.
>+ *   //   #5 If the key inputs another ASCII character with shift key state,
>+ *   //      use a proper value for the character.
>+ *   //   See above document for the other cases.

Might be good to document here still cases like AltGr+key.
(For example AltGr+e produces € in most European keyboard layouts)

>+ *   if (TIP.keydown(keyEvent)) {
>+ *     // Handle its default action
>+ *   }
>+ *
>+ *   if (!TIP.init(window, callback)) {
>+ *     return; // You failed to get the rights to dispatch key events
>+ *   }
Again, does one really need to call init() all the time? If so, init() should be renamed to something like
beginInputTransaction() to hint it needs to be called all the time

>+  const unsigned long KEY_DEFAULT_PREVENTED                        = 0x00000001;
>+  // If KEY_NON_PRINTABLE_KEY is specified and the .key value isn't valid
>+  // key name, the methods will throws an exception.  In other words, this
will throw
>+   * keydown() maybe dispatches a keydown event and some keypress events
may dispatch

Comment 35

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

Attachment: part.6 TextInputProcessor should manage modifier key state and set it to dispatching key events automatically

*Lock modifier key events only modify the modifier state at keydown.

Comment 36

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

Attachment: part.7 Make TextInputProcerros possible to share modifier state

Updating for the new part.6.

Comment 37

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

Attachment: part.8 nsITextInputProcessor.keydown() and nsITextInputProcessor.keyup() should be able to only modify its modifier state without dispatching key events

Comment 38

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

Comment on attachment 8560427 [details] [diff] [review]
part.7 Make TextInputProcerros possible to share modifier state

The reason for shareModifierStateOf isn't clear.
Is there any use of that outside tests? And even in tests, why not
just copy that state?

Maybe I'm missing something here.

Comment 39

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

Comment on attachment 8560428 [details] [diff] [review]
part.8 nsITextInputProcessor.keydown() and nsITextInputProcessor.keyup() should be able to only modify its modifier state without dispatching key events

Maybe KEY_DONT_DISPATCH_MODIFIER_KEY_EVENT

Btw, why we need KEY_ prefix for those consts.

Comment 40

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

Comment on attachment 8559679 [details] [diff] [review]
part.1 Implement key event dispatcher in TextEventDispatcher

>+    } else {
>+      // Otherwise, its charCode should be the char at the index.
>+      MOZ_ASSERT(aIndexOfKeypressEvent < keyEvent.mKeyValue.Length(),
>+                 "aIndexOfKeypressEvent must be less than mKeyValue.Length()");
>+      keyEvent.keyCode = 0;
>+      // XXX .charCode should be a code point of UTF-16 or UCS-4??
>+      keyEvent.charCode =
>+        static_cast<uint32_t>(keyEvent.mKeyValue[aIndexOfKeypressEvent]);
>+      if (aIndexOfKeypressEvent > 0) {
>+        // If this is second or later keypress event, let's clear mKeyValue
>+        // because DOM key event handler cannot distinguish if coming keypress
>+        // event should cause input the character again or not.
>+        keyEvent.mKeyValue.Truncate();
>+      }
I tried and failed to understand this

>+  // Otherwise, dispatch keypress events for second or later character.
>+  bool consumed = (aStatus == nsEventStatus_eConsumeNoDefault);
>+  for (size_t i = 1; i < aKeyboardEvent.mKeyValue.Length(); i++) {
>+    aStatus = nsEventStatus_eIgnore;
>+    if (!DispatchKeyboardEventInternal(NS_KEY_PRESS, aKeyboardEvent,
>+                                       aStatus, i)) {
>+      // The widget must have been gone.
>+      break;
>+    }
..and this. Where are we collecting data to mKeyValue?

Comment 41

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

Comment on attachment 8559683 [details] [diff] [review]
part.2 Don't dispatch keyboard events from TextEventDispatcher if there is a composition

>+  // Basically, key events shouldn't be dispatched during composition.
>+  if (IsComposing()) {
>+    // However, if we need to behave like other browsers, we need to keydown
s/to/the/


>+    // XXX If there is mOnlyContentDispatch for this case,
If there was ...

Comment 42

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

Comment on attachment 8559706 [details] [diff] [review]
part.9 Make a check in runUnloadTests2() of window_nsITextInputProcessor disabled due to unknown random failure

Ok, a bit annoying.

Are we perhaps missing waitForFocus in the test or
relying on GC to kill TIP or something?

Comment 43

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

Comment on attachment 8559688 [details] [diff] [review]
part.3 Implement converting methods from key/code value to key/code name index


>diff --git a/dom/events/KeyboardEvent.cpp b/dom/events/KeyboardEvent.cpp
>--- a/dom/events/KeyboardEvent.cpp
>+++ b/dom/events/KeyboardEvent.cpp
>@@ -262,20 +262,26 @@ KeyboardEvent::InitWithKeyboardEventInit
>   mDetail = aParam.mDetail;
>   mInitializedByCtor = true;
>   mInitializedWhichValue = aParam.mWhich;
> 
>   WidgetKeyboardEvent* internalEvent = mEvent->AsKeyboardEvent();
>   internalEvent->location = aParam.mLocation;
>   internalEvent->mIsRepeat = aParam.mRepeat;
>   internalEvent->mIsComposing = aParam.mIsComposing;
>-  internalEvent->mKeyNameIndex = KEY_NAME_INDEX_USE_STRING;
>-  internalEvent->mCodeNameIndex = CODE_NAME_INDEX_USE_STRING;
>-  internalEvent->mKeyValue = aParam.mKey;
>-  internalEvent->mCodeValue = aParam.mCode;
>+  internalEvent->mKeyNameIndex =
>+    WidgetKeyboardEvent::GetKeyNameIndex(aParam.mKey);
>+  if (internalEvent->mKeyNameIndex == KEY_NAME_INDEX_USE_STRING) {
>+    internalEvent->mKeyValue = aParam.mKey;
>+  }
>+  internalEvent->mCodeNameIndex =
>+    WidgetKeyboardEvent::GetCodeNameIndex(aParam.mCode);
>+  if (internalEvent->mCodeNameIndex == CODE_NAME_INDEX_USE_STRING) {
>+    internalEvent->mCodeValue = aParam.mCode;
>+  }
> }
It might be good to land this change in a separate bug, since this is visible to the web pages.

Comment 44

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

Comment on attachment 8559710 [details] [diff] [review]
part.10 Support all key and code values which are enough stable in DOM Level 3 KeyboardEvent key/code Values

rs+

Comment 45

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

Comment on attachment 8559696 [details] [diff] [review]
part.5 Compute KeyboardEvent.location and .keyCode if they are 0

I wish the contents of ComputeKeyCodeFromKeyNameIndex could be generated.
Would it be possible to utilize the data we have in
NativeToDOMKeyName.h or something? like extend the macros there a bit.

Comment 46

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

Once all the patches have got r+, I'd like to still see a patch containing all the changes.

Comment 47

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

Thank you for the review very quickly!!

(In reply to Olli Pettay [:smaug] from comment #34)
> Comment on attachment 8559692 [details] [diff] [review]
> part.4 Add keydown() and keyup() to nsITextInputProcessor
> >+ *   if (!TIP.init(window, callback)) {
> >+ *     return; // You failed to get the rights to dispatch key events
> >+ *   }
> >+ *
> >+ *   // You don't need to specify .keyCode value if it's non-printable key
> >+ *   // because it can be computed from .key value.
> >+ *   // If you specify non-zero value to .keyCode, it'll be used.
> >+ *   var keyEvent = new KeyboardEvent("", { code: "Enter", key: "Enter" });
> >+ *   if (TIP.keydown(keyEvent)) {
> >+ *     // Handle its default action
> >+ *   }
> >+ *
> >+ *   if (!TIP.init(window, callback)) {
> >+ *     return; // You failed to get the rights to dispatch key events
> >+ *   }
> Do you really need to call init again? That feels odd.

Yes. For example, another TIP which is created by an addon *might* stole the rights to use TextEventDispatcher from keydown event handler or something. Although, this is really rare case.

> >+ *   //   https://developer.mozilla.org/docs/Web/API/KeyboardEvent.keyCode
> >+ *   //   #1 If the key can input [0-9], the value should be DOM_VK_[0-9].
> >+ *   //   #2 If the key inputs an ASCII alphabet or numeric with no modifier
> >+ *   //      states, use a proper value for the character.
> >+ *   //   #3 If the key inputs an ASCII alphabet or numeric with shift key
> >+ *   //      state, use a proper value for the shifted character.
> what does this mean? if I want A (shift+a), what should be passed?

Our .keyCode value isn't changed by modifier state for printable keys. I'll update the comment for explaining deeper.

> >+ *   //   #4 If the key inputs another ASCII character with no modifier states,
> >+ *   //      use a proper value for the character.
> >+ *   //   #5 If the key inputs another ASCII character with shift key state,
> >+ *   //      use a proper value for the character.
> >+ *   //   See above document for the other cases.
> 
> Might be good to document here still cases like AltGr+key.
> (For example AltGr+e produces € in most European keyboard layouts)

So, AltGr isn't related to compute .keyCode.

> >+ *   if (TIP.keydown(keyEvent)) {
> >+ *     // Handle its default action
> >+ *   }
> >+ *
> >+ *   if (!TIP.init(window, callback)) {
> >+ *     return; // You failed to get the rights to dispatch key events
> >+ *   }
> Again, does one really need to call init() all the time? If so, init()
> should be renamed to something like
> beginInputTransaction() to hint it needs to be called all the time

Okay, I'll add part.0 patch for this.

(In reply to Olli Pettay [:smaug] from comment #38)
> Comment on attachment 8560427 [details] [diff] [review]
> part.7 Make TextInputProcerros possible to share modifier state
> 
> The reason for shareModifierStateOf isn't clear.
> Is there any use of that outside tests? And even in tests, why not
> just copy that state?
> 
> Maybe I'm missing something here.

Yeah, I was thinking about just copying the modifier state. However, if JS-IME addon wants to create TIP instance for every DOM window, sharing the modifier states between multiple instances is very helpful. This must avoid a bug and complicated code from the API users.

I'll request review again.

(In reply to Olli Pettay [:smaug] from comment #39)
> Btw, why we need KEY_ prefix for those consts.

If we will add other constants for other methods, such prefix is clearer for the API users. It gives hints to API users which method takes them.

(In reply to Olli Pettay [:smaug] from comment #40)
> Comment on attachment 8559679 [details] [diff] [review]
> part.1 Implement key event dispatcher in TextEventDispatcher
> 
> >+    } else {
> >+      // Otherwise, its charCode should be the char at the index.
> >+      MOZ_ASSERT(aIndexOfKeypressEvent < keyEvent.mKeyValue.Length(),
> >+                 "aIndexOfKeypressEvent must be less than mKeyValue.Length()");
> >+      keyEvent.keyCode = 0;
> >+      // XXX .charCode should be a code point of UTF-16 or UCS-4??
> >+      keyEvent.charCode =
> >+        static_cast<uint32_t>(keyEvent.mKeyValue[aIndexOfKeypressEvent]);
> >+      if (aIndexOfKeypressEvent > 0) {
> >+        // If this is second or later keypress event, let's clear mKeyValue
> >+        // because DOM key event handler cannot distinguish if coming keypress
> >+        // event should cause input the character again or not.
> >+        keyEvent.mKeyValue.Truncate();
> >+      }
> I tried and failed to understand this

If second or later keypress event's .key value also has all string, it cannot be used for handling text input since same string is inputted repeatedly if such event handler doesn't ignore the text. Note that this is out scope of D3E spec since they don't define keypress event not so deeply.

I'll add this explanation into next patch.

> >+  // Otherwise, dispatch keypress events for second or later character.
> >+  bool consumed = (aStatus == nsEventStatus_eConsumeNoDefault);
> >+  for (size_t i = 1; i < aKeyboardEvent.mKeyValue.Length(); i++) {
> >+    aStatus = nsEventStatus_eIgnore;
> >+    if (!DispatchKeyboardEventInternal(NS_KEY_PRESS, aKeyboardEvent,
> >+                                       aStatus, i)) {
> >+      // The widget must have been gone.
> >+      break;
> >+    }
> ..and this. Where are we collecting data to mKeyValue?

mKeyValue must be string to be inputted if the key is printable key. Therefore, keypress events need to be dispatched for every character in the string since .charCode can represent only one character.

I'll add this explanation into the next patch too.

(In reply to Olli Pettay [:smaug] from comment #43)
> Comment on attachment 8559688 [details] [diff] [review]
> part.3 Implement converting methods from key/code value to key/code name
> index
> >-  internalEvent->mKeyNameIndex = KEY_NAME_INDEX_USE_STRING;
> >-  internalEvent->mCodeNameIndex = CODE_NAME_INDEX_USE_STRING;
> >-  internalEvent->mKeyValue = aParam.mKey;
> >-  internalEvent->mCodeValue = aParam.mCode;
> >+  internalEvent->mKeyNameIndex =
> >+    WidgetKeyboardEvent::GetKeyNameIndex(aParam.mKey);
> >+  if (internalEvent->mKeyNameIndex == KEY_NAME_INDEX_USE_STRING) {
> >+    internalEvent->mKeyValue = aParam.mKey;
> >+  }
> >+  internalEvent->mCodeNameIndex =
> >+    WidgetKeyboardEvent::GetCodeNameIndex(aParam.mCode);
> >+  if (internalEvent->mCodeNameIndex == CODE_NAME_INDEX_USE_STRING) {
> >+    internalEvent->mCodeValue = aParam.mCode;
> >+  }
> > }
> It might be good to land this change in a separate bug, since this is
> visible to the web pages.

I don't think so. mKeyNameIndex and mCodeNameIndex are useful for internal event handling (e.g., can use switch statement, comparing with lower runtime cost and can distinguish if inputting text).

So, seeing from web apps, the result isn't changed. These attribute values are always DOMString.

(In reply to Olli Pettay [:smaug] from comment #45)
> Comment on attachment 8559696 [details] [diff] [review]
> part.5 Compute KeyboardEvent.location and .keyCode if they are 0
> 
> I wish the contents of ComputeKeyCodeFromKeyNameIndex could be generated.
> Would it be possible to utilize the data we have in
> NativeToDOMKeyName.h or something? like extend the macros there a bit.

Perhaps, could be. However, I want to do it later since changing nsITextInputProcessor before branching 38 is the best for compatibility with future version. So, I'd like to land them ASAP. Sounds Okay to you? (Anyway, I'll write the patch before quitting from this bug)

(In reply to Olli Pettay [:smaug] from comment #46)
> Once all the patches have got r+, I'd like to still see a patch containing
> all the changes.

Yeah, I'll post it with new patches.

Comment 48

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

Random thoughts while I'm writing new EventUtils.synthesizeKey():

* While keydown() and keyup() are called, the rights to use the TIP shouldn't be able to be stolen.
* Even if a TIP instance doesn't have the rights to dispatch events, it should allow to modify its modifier state when KEY_DONT_DISPATCH_MODIFIER_KEY_EVENT is specified.

Comment 49

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #48)
> * Even if a TIP instance doesn't have the rights to dispatch events, it
> should allow to modify its modifier state when
> KEY_DONT_DISPATCH_MODIFIER_KEY_EVENT is specified.

Oops, I already implemented as this.

Comment 50

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #47)
> > >+ *   if (!TIP.init(window, callback)) {
> > >+ *     return; // You failed to get the rights to dispatch key events
> > >+ *   }
> > Do you really need to call init again? That feels odd.
> 
> Yes. For example, another TIP which is created by an addon *might* stole the
> rights to use TextEventDispatcher from keydown event handler or something.
How can an addon steal the rights? Doesn't the TIP in the test have the rights.

> 
> Yeah, I was thinking about just copying the modifier state. However, if
> JS-IME addon wants to create TIP instance for every DOM window,
Why would it want to do that? Couldn't it just initialize TIP for different windows when needed

> > 
> > >+    } else {
> > >+      // Otherwise, its charCode should be the char at the index.
> > >+      MOZ_ASSERT(aIndexOfKeypressEvent < keyEvent.mKeyValue.Length(),
> > >+                 "aIndexOfKeypressEvent must be less than mKeyValue.Length()");
> > >+      keyEvent.keyCode = 0;
> > >+      // XXX .charCode should be a code point of UTF-16 or UCS-4??
> > >+      keyEvent.charCode =
> > >+        static_cast<uint32_t>(keyEvent.mKeyValue[aIndexOfKeypressEvent]);
> > >+      if (aIndexOfKeypressEvent > 0) {
> > >+        // If this is second or later keypress event, let's clear mKeyValue
> > >+        // because DOM key event handler cannot distinguish if coming keypress
> > >+        // event should cause input the character again or not.
> > >+        keyEvent.mKeyValue.Truncate();
> > >+      }
> > I tried and failed to understand this
> 
> If second or later keypress event's .key value also has all string, it
> cannot be used for handling text input since same string is inputted
> repeatedly if such event handler doesn't ignore the text. Note that this is
> out scope of D3E spec since they don't define keypress event not so deeply.



> 
> I'll add this explanation into next patch.
> 
> > >+  // Otherwise, dispatch keypress events for second or later character.
> > >+  bool consumed = (aStatus == nsEventStatus_eConsumeNoDefault);
> > >+  for (size_t i = 1; i < aKeyboardEvent.mKeyValue.Length(); i++) {
> > >+    aStatus = nsEventStatus_eIgnore;
> > >+    if (!DispatchKeyboardEventInternal(NS_KEY_PRESS, aKeyboardEvent,
> > >+                                       aStatus, i)) {
> > >+      // The widget must have been gone.
> > >+      break;
> > >+    }
> > ..and this. Where are we collecting data to mKeyValue?
> 
> mKeyValue must be string to be inputted if the key is printable key.
> Therefore, keypress events need to be dispatched for every character in the
> string since .charCode can represent only one character.

So the unclear part is that where we append data to mKeyValue. When does it contain more than just one
key?

> > >-  internalEvent->mKeyNameIndex = KEY_NAME_INDEX_USE_STRING;
> > >-  internalEvent->mCodeNameIndex = CODE_NAME_INDEX_USE_STRING;
> > >-  internalEvent->mKeyValue = aParam.mKey;
> > >-  internalEvent->mCodeValue = aParam.mCode;
> > >+  internalEvent->mKeyNameIndex =
> > >+    WidgetKeyboardEvent::GetKeyNameIndex(aParam.mKey);
> > >+  if (internalEvent->mKeyNameIndex == KEY_NAME_INDEX_USE_STRING) {
> > >+    internalEvent->mKeyValue = aParam.mKey;
> > >+  }
> > >+  internalEvent->mCodeNameIndex =
> > >+    WidgetKeyboardEvent::GetCodeNameIndex(aParam.mCode);
> > >+  if (internalEvent->mCodeNameIndex == CODE_NAME_INDEX_USE_STRING) {
> > >+    internalEvent->mCodeValue = aParam.mCode;
> > >+  }
> > > }
> > It might be good to land this change in a separate bug, since this is
> > visible to the web pages.
> 
> I don't think so. mKeyNameIndex and mCodeNameIndex are useful for internal
> event handling
That is not my point. The point is that in the old code event.key will always point to aParam.mKey, but
in the new code it depends on GetKeyNameIndex.
In fact, why do we want to change this code.
If the event is initialized with key "foo", event.key should be "foo"

Comment 51

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

(In reply to Olli Pettay [:smaug] from comment #50)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #47)
> > > >+ *   if (!TIP.init(window, callback)) {
> > > >+ *     return; // You failed to get the rights to dispatch key events
> > > >+ *   }
> > > Do you really need to call init again? That feels odd.
> > 
> > Yes. For example, another TIP which is created by an addon *might* stole the
> > rights to use TextEventDispatcher from keydown event handler or something.
> How can an addon steal the rights? Doesn't the TIP in the test have the
> rights.

For example:

foo.addEventListener("keydown", function (aEvent) {
  TIP2.init(window, callback);
}, false);

TIP1.init(window, callback);
TIP1.keydown(new KeyboardEvent("", {}));

// Then, TIP2 now has the rights here.

However, as I mentioned in comment 48, while a TIP instance is dispatching event, another instance shouldn't be able to steal the rights. I'll write a patch for this problem at part.11.

So, I'll remove the redundant init() calls in the examples.

> > Yeah, I was thinking about just copying the modifier state. However, if
> > JS-IME addon wants to create TIP instance for every DOM window,
> Why would it want to do that? Couldn't it just initialize TIP for different
> windows when needed

Hmm, I feel that copying manually may be error-prone. So, I'm thinking that sharing is better.

Even if we should take your approach, it should be copyModifierStateOf(in nsITextInputProcessor) because I don't like to redefine modifiers as constants of nsITextInputProcessor. So, I don't like following style in this case:

TIP1.modifierStates = TIP2.modifierStates;

> > > >+  // Otherwise, dispatch keypress events for second or later character.
> > > >+  bool consumed = (aStatus == nsEventStatus_eConsumeNoDefault);
> > > >+  for (size_t i = 1; i < aKeyboardEvent.mKeyValue.Length(); i++) {
> > > >+    aStatus = nsEventStatus_eIgnore;
> > > >+    if (!DispatchKeyboardEventInternal(NS_KEY_PRESS, aKeyboardEvent,
> > > >+                                       aStatus, i)) {
> > > >+      // The widget must have been gone.
> > > >+      break;
> > > >+    }
> > > ..and this. Where are we collecting data to mKeyValue?
> > 
> > mKeyValue must be string to be inputted if the key is printable key.
> > Therefore, keypress events need to be dispatched for every character in the
> > string since .charCode can represent only one character.
> 
> So the unclear part is that where we append data to mKeyValue. When does it
> contain more than just one
> key?

For example, new KeyboardEvent("", { key: "abc" }); causes mKeyValue is "abc" and mKeyNameIndex is KEY_NAME_INDEX_USE_STRING. Possible scenario of this case is JS-Keyboard tries to input a grapheme cluster which contains combining character like accent marks (e.g., "'", "~").

> > > >-  internalEvent->mKeyNameIndex = KEY_NAME_INDEX_USE_STRING;
> > > >-  internalEvent->mCodeNameIndex = CODE_NAME_INDEX_USE_STRING;
> > > >-  internalEvent->mKeyValue = aParam.mKey;
> > > >-  internalEvent->mCodeValue = aParam.mCode;
> > > >+  internalEvent->mKeyNameIndex =
> > > >+    WidgetKeyboardEvent::GetKeyNameIndex(aParam.mKey);
> > > >+  if (internalEvent->mKeyNameIndex == KEY_NAME_INDEX_USE_STRING) {
> > > >+    internalEvent->mKeyValue = aParam.mKey;
> > > >+  }
> > > >+  internalEvent->mCodeNameIndex =
> > > >+    WidgetKeyboardEvent::GetCodeNameIndex(aParam.mCode);
> > > >+  if (internalEvent->mCodeNameIndex == CODE_NAME_INDEX_USE_STRING) {
> > > >+    internalEvent->mCodeValue = aParam.mCode;
> > > >+  }
> > > > }
> > > It might be good to land this change in a separate bug, since this is
> > > visible to the web pages.
> > 
> > I don't think so. mKeyNameIndex and mCodeNameIndex are useful for internal
> > event handling
> That is not my point. The point is that in the old code event.key will
> always point to aParam.mKey, but
> in the new code it depends on GetKeyNameIndex.

Yes, but it's only when aParam.mKey is a registered key name.

> In fact, why do we want to change this code.

Currently, neither WidgetKeyboardEvent nor D3E KeyboardEvent has a flag if the key event should cause inputting text. Therefore, only the way of distinguishing if a key event is printable key or non-printable key is, deciding if mKeyNameIndex is KEY_NAME_INDEX_USE_STRING or not. If it's KEY_NAME_INDEX_USE_STRING, it must be a printable key event. Otherwise, non-printable key event.

> If the event is initialized with key "foo", event.key should be "foo"

Therefore, in this case, if "foo" is a registered key name,
mKeyNameIndex = KEY_NAME_INDEX_foo, mKeyValue = EmptyString().

On the other hand, if "foo" is not a registered key name,
mKeyNameIndex = KEY_NAME_INDEX_USE_STRING, mKeyValue = "foo".

See also spec bug:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=23907

Comment 52

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #51)
> Hmm, I feel that copying manually may be error-prone. So, I'm thinking that
> sharing is better.
But I don't understand why we need any copying? One should just use one TIP, and be able to
re-initialize it with some other window if needed



 
> Therefore, in this case, if "foo" is a registered key name,
> mKeyNameIndex = KEY_NAME_INDEX_foo, mKeyValue = EmptyString().
> 
> On the other hand, if "foo" is not a registered key name,
> mKeyNameIndex = KEY_NAME_INDEX_USE_STRING, mKeyValue = "foo".
> 
Hmm, I guess I must have missed something in the code there.
So, ok, we don't change the web phasing behavior at all, fine.

Comment 53

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

(In reply to Olli Pettay [:smaug] from comment #52)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #51)
> > Hmm, I feel that copying manually may be error-prone. So, I'm thinking that
> > sharing is better.
> But I don't understand why we need any copying? One should just use one TIP,
> and be able to
> re-initialize it with some other window if needed

It depends on the design of addons or forms.js (on B2G). If composition should be managed by each top level window like Mac, i.e., there may be some compositions on Gecko, TIP needs to be created two or more.

Currently, this scenario occurs only on Mac, but I want to solve this inconvenience. (I want to add a way to specify IMEUpdatePreference from nsITextInputProcessor)

Comment 54

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

Hmm, right, I can see some addon js component to be initialized several times, and each of them
having separate TIP... but then, if they can't access each others, they couldn't share
the state anyway, and if they can access each others, the same TIP could be used.

Sharing just makes the API a bit odd that TIP1 can affect to the state of TIP2.

Comment 55

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

(In reply to Olli Pettay [:smaug] from comment #54)
> Hmm, right, I can see some addon js component to be initialized several
> times, and each of them
> having separate TIP... but then, if they can't access each others, they
> couldn't share
> the state anyway, and if they can access each others, the same TIP could be
> used.
> 
> Sharing just makes the API a bit odd that TIP1 can affect to the state of
> TIP2.

Yes, it's my intention. Any modifier states shouldn't be shared with TIPs which are created by different addon or something. For example, if sharing modifiers between TIP1 and TIP2 which are created by different addons:

TIP1.beginInputTransaction(win1, callback1);
TIP1.keydown(new KeyboardEvent("", { key: "Shift", code: "ShiftLeft" }));

TIP2.beginInputTransaction(win2, callback2);
TIP2.keydown(new KeyboardEvent("", { key: "Control", code: "ControlLeft" }));
TIP2.keydown(new KeyboardEvent("", { key: "c", code: "KeyC" }));

then, the last key combination becomes Ctrl + Shift + C. This is not expected by TIP2 creator.

However, I mentioned above, if an addon want to create two or more addons, sharing modifier state must be useful. For example, if there are 3 or more TIP instances, one of them may be difficult to retrieve the latest modifier state because the one may not be able to know which TIP instance is the last one.

Therefore, I believe that sharing approach is the best.

Comment 56

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

But if you have access to one TIP1, why you want to create another TIP2 and share the state?
Why not just use TIP1 all the time?

Comment 57

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

(In reply to Olli Pettay [:smaug] from comment #56)
> But if you have access to one TIP1, why you want to create another TIP2 and
> share the state?
> Why not just use TIP1 all the time?

As I mentioned above, addons might want to create composition in each top level window separately. FIY: Currently, Gecko supports composition per top level window at least on Mac. (might be so on GTK, but not sure)

Comment 58

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

Yes, addon may want to create TIP for each top level window separately, but then the place 
where a TIP runs may not have access to another TIP, in which case sharing wouldn't be possible.

Sharing is possibly only if you have access to both TIP1 and TIP2, in which case you could just
use one TIP.

Comment 59

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

Yeah, but one TIP cannot have two or more compositions even if there are two or more top level windows...

Comment 60

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

but how are the modifier states then relevant for both TIP?
Or, if we want to share modifier state, why not make modifier state a global thing?
(though, I think not sharing in any way would keep the API cleaner)

I feel like I'm missing something here.

Comment 61

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

(In reply to Olli Pettay [:smaug] from comment #60)
> but how are the modifier states then relevant for both TIP?

On any platforms, modifier state are shared in global level. So, independent modifier state per instance sounds odd.

> Or, if we want to share modifier state, why not make modifier state a global
> thing?

As I said, basically, I think that the most natural behavior is sharing the modifier state between all instances, however, that may cause odd behavior of addons. Additionally, it's probably impossible to share the modifier states with native API.

> (though, I think not sharing in any way would keep the API cleaner)
> 
> I feel like I'm missing something here.

So, my idea is based on:

* modifier states should be shared as far as possible
* if addons want to create composition per top level window, they need to create multiple instances
* sharing modifier states between different TIP creators may cause odd behavior (i.e., it may cause trouble between addons)

Therefore, I believe that providing an API to share modifier states between all instances created by same addon.

Comment 62

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

weird API, but fine. Doesn't make things too complicated.

Comment 63

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

Attachment: part.1 Implement key event dispatcher in TextEventDispatcher

Some comments added where it's difficult to understand the reason to initialize WidgetKeyboardEvent members.

Comment 64

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

Attachment: part.2 Don't dispatch keyboard events from TextEventDispatcher if there is a composition (r=smaug)

Comment 65

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

Attachment: part.3 Implement converting methods from key/code value to key/code name index (r=smaug)

Comment 66

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

Attachment: part.4 Add keydown() and keyup() to nsITextInputProcessor (r=smaug, sr=smaug)

Comment 67

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

Attachment: part.5 Compute KeyboardEvent.location and .keyCode if they are 0 (r=smaug, sr=smaug)

Comment 68

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

Attachment: part.6 TextInputProcessor should manage modifier key state and set it to dispatching key events automatically (r=smaug, sr=smaug)

Comment 69

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

Attachment: part.7 Make TextInputProcerros possible to share modifier state

I ask r? and sr? for sharing modifier states between multiple TIP instances created and managed by same addon.

Comment 70

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

Attachment: part.8 nsITextInputProcessor.keydown() and nsITextInputProcessor.keyup() should be able to only modify its modifier state without dispatching key events (r=smaug, sr=smaug)

Comment 71

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

Attachment: part.9 Make a check in runUnloadTests2() of window_nsITextInputProcessor disabled due to unknown random failure (r=smaug)

Comment 72

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

Attachment: part.10 Support all key and code values which are enough stable in DOM Level 3 KeyboardEvent key/code Values (r=smaug)

Comment 73

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

Attachment: part.11 TextEventDispatcher shouldn't allow to begin input transaction during dispatching a event

While TextEventDispatcher is dispatching an event, an addon may steal the rights to dispatch events. This is too bad, especially, TIP users need to call beginInputTransaction() before every call of keydown() and keyup().

This patch makes TextEventDispatcher doesn't allow to begin another input transaction while it's dispatching an event.

Comment 74

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

Attachment: part.12 nsITextInputProcessor should take KeyboardEvent as an argument of composition releated methods for dispatching key events around composition events

When I re-implement EventUtils.synthesizeKey(), I realized that new API cannot dispatch only keydown event immediately before starting composition.

Strictly speaking, all automated tests should dispatch following events for simple composition:

keydown (causes starting composition)
compositionstart
compositionupdate * n
text * n
compositionend
keyup (causes commiting the composition)

The first keydown event cannot emulate with new API. This fact makes some tests broken. Additionally, Firefox OS's JS-IME should dispatch keydown event before every composition change and keyup event after every composition change. But I cannot believe that every IME implements this complicated behavior. Especially, these behavior doesn't affect simple usage of IME but web apps may be confused.

My idea to solve this issue is that every composition change method should take a key event and KEY_* flags. Then, JS-IME can dispatch key events which cause composition changes naturally.

For example,

> TIP.setPendingCompositionString("a");
> TIP.appendClauseToPendingComposition(1, TIP.ATTR_RAW_CLAUSE);
> TIP.flushPendingComposition(
>   new KeyboardEvent("", { key: "a", code: "KeyA", keyCode: KeyboardEvent.DOM_VK_A }));
> 
> TIP.setPendingCompositionString("ab");
> TIP.appendClauseToPendingComposition(2, TIP.ATTR_RAW_CLAUSE);
> TIP.flushPendingComposition(
>   new KeyboardEvent("", { key: "b", code: "KeyB", keyCode: KeyboardEvent.DOM_VK_B }));
> 
> TIP.setPendingCompositionString("AB");
> TIP.appendClauseToPendingComposition(2, TIP.ATTR_SELECTED_CLAUSE);
> TIP.flushPendingComposition(
>   new KeyboardEvent("", { key: "Convert", code: "Convert", keyCode: KeyboardEvent.DOM_VK_CONVERT }));
> 
> TIP.commitString(new KeyboardEvent("", { key: "Enter", code: "Enter" });

This looks very simple to me. JS-IME should just set a key event to the composition API's first argument and its flags to the second argument.

Of course, for compatibility and simpler API, KeyboardEvent and KEY_* flags are optional arguments.

I'd like to land this before next merge because .commitComposition() will lose its compatibility when it's called with commit string. The commit string should be able to be omitted since it should be able to commit composition with the last composition string. However, neither specifying null nor undefined works same as called without specifying it. XPConnect converts them to empty string. Therefore, .commitComposition() should be:

commitComposition([optional] in nsIDOMKeyEvent aDOMKeyEvent,
                  [optional] in unsigned long aKeyFlags,
                  [optional] in DOMString aCommitString);

Then, when JS-IME calls this with commit string but without key event, this looks ugly like this:

> TIP.commitComposition(null, 0, "FooBarBuzz");

But I don't have better idea for this issue.


FYI: According to D3E spec, when keydown event is canceled immediately before starting composition, composition should be canceled. This patch implements this behavior:
https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#event-type-keydown

> If the key is associated with a text composition system, the default action MUST be to launch that system

Comment 75

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

Attachment: part.13 EventUtils.synthesizeComposition() and synthesizeCompositionChange() should take KeyboardEvent for emulating composition state change caused by a key operation

This patch re-implement EventUtils.synthesizeComposition*(). aEvent can take information of key event as |key| property.

And this patch fixes some tests which needs this new API before re-implementing synthesizeKey() with new API.

Note that EventUtils.js is being added some complicated functions which creates KeyboardEvent from given minimum information.

An ugly point of this patch is, _emulateToActiveateModifiers() and _emulateToInactivateModifiers(). Currently, most automated tests don't emulate modifier key operation correctly. Instead, they just specify modifier state to dispatching key events directly. For emulating this unusual behavior, synthesizeComposition*() need to call keydown() and keyup() for setting modifiers.

This may be replaced with an API to set modifier state. But I don't like that for JS-IME and JS-Keyboard developers. They shouldn't use that for emulating strict behavior. But if you think that I should add that and make EventUtils simpler, I'll do it in additional patch. So, even if you don't agree with this point, please give r+ if the others don't have problems.

Comment 76

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

Attachment: part.14 Reimplement/redesign EventUtils.synthesizeKey() with nsITextInputProcessor

This re-implements EventUtils.syntehsizeKey() with new API. The most part of necessary changes are included in part.13.

This patch fixes (or just remove) some odd tests which dispatches only keypress event. And this patch modifies the argument style with new style. We need to rewrite all callers with new style, though, the count of callers over 1000. So, we should do it another bug, later.

Comment 77

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

Attachment: part.15 Fix new orange of test_menulist_keynav.xul because it's caused by timeout of incremental search in menu

The reimplemented synthesizeKey() is a little bit slower than the old implementation. Probably the reason is hack for modifier state.

The slowness causes test_menulist_keynav.xul failure which was filed as intermittent orange (bug 700146). It tests incremental search in menu items. However, it doesn't control the timeout duration. Therefore, we should fix bug 700146 perfectly in this bug.

I'll ask review after I tested this on tryserver (tryserver is now closed :-( )

Comment 78

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

Oh, during posting the patches, tryserver reopened:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e734bfc7d8df

Comment 79

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

Comment on attachment 8564231 [details] [diff] [review]
part.12 nsITextInputProcessor should take KeyboardEvent as an argument of composition releated methods for dispatching key events around composition events


>  * Example #1 JS-IME can start composition like this:
>  *
>  *   var TIP = Components.classes["@mozilla.org/text-input-processor;1"].
>  *               createInstance(Components.interfaces.nsITextInputProcessor);
>  *   if (!TIP.beginInputTransaction(window, callback)) {
>  *     return; // You failed to get the rights to make composition
>  *   }
>+ *   // Create a keyboard event if it's caused by a key event.
What does the "it" refer to here? "create foo if foo is caused by foo" is odd.

>  * Example #2 JS-IME can separate composition string to two or more clauses:
>  *
>+ *   // Create a keyboard event if it's caused by a key event.
Again a bit unclear comment, and elsewhere


>  * Example #7 JS-IME can start composition explicitly:
>  *
>  *   if (!TIP.beginInputTransaction(window, callback)) {
>  *     return; // You failed to get the rights to make composition
>  *   }
>+ *   // Create a keyboard event if the starting is caused by a key event.
Hmm, not sure what this tries to say.. we create a keyboard event, and then
start composition. Could it be
"Create the keyboard event which starts the composition."?

Comment 80

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

(In reply to Olli Pettay [:smaug] from comment #79)
> Comment on attachment 8564231 [details] [diff] [review]
> part.12 nsITextInputProcessor should take KeyboardEvent as an argument of
> composition releated methods for dispatching key events around composition
> events
> 
> 
> >  * Example #1 JS-IME can start composition like this:
> >  *
> >  *   var TIP = Components.classes["@mozilla.org/text-input-processor;1"].
> >  *               createInstance(Components.interfaces.nsITextInputProcessor);
> >  *   if (!TIP.beginInputTransaction(window, callback)) {
> >  *     return; // You failed to get the rights to make composition
> >  *   }
> >+ *   // Create a keyboard event if it's caused by a key event.
> What does the "it" refer to here? "create foo if foo is caused by foo" is
> odd.

Oh, I meant that "the following composition change is caused by a key event".

> >  * Example #7 JS-IME can start composition explicitly:
> >  *
> >  *   if (!TIP.beginInputTransaction(window, callback)) {
> >  *     return; // You failed to get the rights to make composition
> >  *   }
> >+ *   // Create a keyboard event if the starting is caused by a key event.
> Hmm, not sure what this tries to say.. we create a keyboard event, and then
> start composition. Could it be
> "Create the keyboard event which starts the composition."?

yes!

Comment 81

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

Attachment: part.12 nsITextInputProcessor should take KeyboardEvent as an argument of composition releated methods for dispatching key events around composition events (sr=smaug)

The comments are fixed.

Comment 82

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

Comment on attachment 8564248 [details] [diff] [review]
part.15 Fix new orange of test_menulist_keynav.xul because it's caused by timeout of incremental search in menu

The reason of this bug is that the synthesizeKey() with new API is slower than the old implementation.

The test tests incremental search in a menu popup. That timeouts if the gap of two keypress events are > 1 sec. It may be timed out on debug build. Although, we should think about this performance issue, we should fix the possibility of bug 700146 because this is really a bug of the test.

For making automated tests stable, we should add a pref which allows to specify the timeout duration. And when it is changed, processing incremental search should be reset immediately. Then, bug 700146 should never be reproduced.

Comment 83

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

Comment on attachment 8564220 [details] [diff] [review]
part.11 TextEventDispatcher shouldn't allow to begin input transaction during dispatching a event

Actually, how is this setup supposed to happen in case of nested event dispatch.
Say, some event listener calls alert(). How to close the alert using keyboard
if nested event dispatching is prevented?
(I assume we'll need to clear "isComposing" flag when focus moves)

Comment 84

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

Comment on attachment 8564220 [details] [diff] [review]
part.11 TextEventDispatcher shouldn't allow to begin input transaction during dispatching a event

Oh, that's good point. I've never realized the scenario.

However, I don't think that it's an issue of this patch because this patch does NOT allow to dispatch events during another event is being dispatched. This doesn't allow another TIP steals the rights of using TextEventDispatcher.

Comment 85

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

> this patch does NOT allow to dispatch events during another event is being dispatched.

I meant that "this patch does NOT _restrict_ dispatching events while another event is being dispatched."

Comment 86

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

Then I don't understand what the dispatcher->IsDispatchingEvent() check is for.

Comment 87

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

(In reply to Olli Pettay [:smaug] from comment #86)
> Then I don't understand what the dispatcher->IsDispatchingEvent() check is
> for.

It makes TIP.beginInputTrnasaction() return false during dispatching an event. I.e., another TIP instance cannot steal the rights to dispatch events on the window. See the automated test.

Comment 88

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

Yes, and why we want that?

Say there is just one TIP in the system and event is dispatched. Then event listener calls alert().
That TIP can't dispatch another event to hide the alert() (since beginInputTransaction() is supposed to be called when a new, possibly nested transaction starts.)

Comment 89

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

Attachment: part.16 Fix new orange of browser_tilt_controllers.js since they were passed because keypress events were not dispatched

browser_tilt_controller.js doesn't emulate key events correctly. Therefore, this becomes orange with synthesizeKey() with new API.

The test only dispatches keydown events. Therefore, most key handles in chrome and Gecko are skipped since they handles keypress. New synthesizeKey() cannot dispatch only keydown events since such situation shouldn't occur except when the preceding keydown event is consumed.

First, in testEventCancel(), it needs to dispatch keydown + keypress and keyup separately for making sure to work rotation and transformation. Although, I'm not sure the exact reasons but it's okay since it's more natural behavior.

Next, Alt + foo causes activates a menu item if the character is registered in the menubar. Therefore, foo should be a character which is not registered in the menubar currently. I choose "A" for that.

Next, Ctrl + foo and Meta + foo may kick a shortcut key. Therefore, I also specify non-alphabet character for that since such characters shouldn't be used for shortcut key for internationalize the shortcut key.

Finally, Shift + T causes inputting text. Therefore, when auto-start of typeaheadfind is enabled, it causes moving focus to the findbar. And it causes failing following tests. Therefore, I avoid to run the test when the auto-start is enabled.

Comment 90

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

(In reply to Olli Pettay [:smaug] from comment #88)
> Yes, and why we want that?

Because you said that it looks like odd to call .beginInputTransaction() every time before a call of keydown() or keyup(). If event handlers cannot steal the rights, we can guarantee the caller doesn't need to call it every time.

> Say there is just one TIP in the system and event is dispatched. Then event
> listener calls alert().
> That TIP can't dispatch another event to hide the alert() (since
> beginInputTransaction() is supposed to be called when a new, possibly nested
> transaction starts.)

No. In such case, the only one instance of TIP doesn't need to call beginInputTransaction() because it still has the rights to use the TextEventDispatcher. Even if it calls beginInputTransaction() again, it returns true.
http://mxr.mozilla.org/mozilla-central/source/dom/base/TextInputProcessor.cpp#144

Comment 91

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

Oh, but I realized that, if TIP initialize with another TextEventDispatcher, nobody cannot get the rights of the old TextEventDispatcher whose window has alert(). So, the previous if statement should also check mDispatcher->IsDispatchingEvent(). I'll add new patch for it after you agree with the patch.

Comment 92

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

Also if there are two TIPs, one per top level window or so and event is first dispatched to window A
and listener in it opens alert() is window B so B's TIP should there handle the events, what should happen?

Comment 93

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

(In reply to Olli Pettay [:smaug] from comment #92)
> Also if there are two TIPs, one per top level window or so and event is
> first dispatched to window A
> and listener in it opens alert() is window B so B's TIP should there handle
> the events, what should happen?

Not clear to me what case you assumed. If the alert() is opened on another top level window, the another top level window's TextEventDispatcher is free. It must be used for dispatching event on the dialog opened by alert().

If you said window B is the modal dialog's native window opened by alert(), I'm not sure the result, though.

In my understanding, TextEventDispatcher of window A can dispatch events to the dialog opened by alert(), isn't it? If so, TextEventDispatcher for A is still free only for the TIP.

Even otherwise, i.e., another TIP needs to be initialized for the dialog's DOM window (I'm not sure it it's existing), its root widget's TextEventDispatcher must be free because I guess that PresShell::GetRootWidget() for the dialog's DOM window must return the dialog's widget.
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.cpp#1289
# The result of this method for a DOM window is the owner of TextEventDispatcher and associated with it.

Comment 94

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

In XUL's <panel> case, the opener's TextEventDispatcher is the proper dispatcher for opened <panel>.

Comment 95

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

But when dispatching event for the alert(), beginInputTransaction can't be called, since that just fails (returns false). So it is not clear when one should call beginInputTransaction and when not.

Comment 96

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

(In reply to Olli Pettay [:smaug] from comment #95)
> But when dispatching event for the alert(), beginInputTransaction can't be
> called, since that just fails (returns false).

I still don't understand the scenario yet. If the TIP is already associated to the DOM window's root widget, beginInputTransaction() returns true because it does not modify the association.

> So it is not clear when one
> should call beginInputTransaction and when not.

Basically, it should be called always immediately before first call of keydown() or something in the stack.


FYI: Currently, TIP is associated with a widget which is the result of PresContext::GetRootWidget(), the presConetxt is gotten from given DOM window via its docShell:
http://mxr.mozilla.org/mozilla-central/source/dom/base/TextInputProcessor.cpp#120

If the result widget is not always a proper event dispatcher in some cases, it's too bad. Only in such cases, your worry is right. E.g., the result is dialog owner's widget but events can be dispatched only from the dialog's widget.

Comment 97

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #96)
> (In reply to Olli Pettay [:smaug] from comment #95)
> > But when dispatching event for the alert(), beginInputTransaction can't be
> > called, since that just fails (returns false).
> 
> I still don't understand the scenario yet. If the TIP is already associated
> to the DOM window's root widget, beginInputTransaction() returns true
> because it does not modify the association.
hmm, oh, the patch didn't have enough context to show that.
Ok.

Comment 98

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #51)
> For example, new KeyboardEvent("", { key: "abc" }); causes mKeyValue is
> "abc" and mKeyNameIndex is KEY_NAME_INDEX_USE_STRING. Possible scenario of
> this case is JS-Keyboard tries to input a grapheme cluster which contains
> combining character like accent marks (e.g., "'", "~").

Hmm, sounds like KeyboardEvent may just cause more confusion here than helping implementation.
Normally KeyboardEvents don't have that way long string values in .key.

Comment 99

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

Comment on attachment 8564205 [details] [diff] [review]
part.1 Implement key event dispatcher in TextEventDispatcher

># HG changeset patch
># User Masayuki Nakano <[e-mail]>
># Parent  17a79c832f8752b70080628c394fd88158142c1d
>Bug 1119609 part.1 Implement key event dispatcher in TextEventDispatcher r=
>
>diff --git a/widget/TextEventDispatcher.cpp b/widget/TextEventDispatcher.cpp
>--- a/widget/TextEventDispatcher.cpp
>+++ b/widget/TextEventDispatcher.cpp
>@@ -88,19 +88,20 @@ TextEventDispatcher::GetState() const
>   }
>   if (!mWidget || mWidget->Destroyed()) {
>     return NS_ERROR_NOT_AVAILABLE;
>   }
>   return NS_OK;
> }
> 
> void
>-TextEventDispatcher::InitEvent(WidgetCompositionEvent& aEvent) const
>+TextEventDispatcher::InitEvent(WidgetGUIEvent& aEvent) const
> {
>   aEvent.time = PR_IntervalNow();
>+  aEvent.refPoint = LayoutDeviceIntPoint(0, 0);
>   aEvent.mFlags.mIsSynthesizedForTests = mForTests;
> }
> 
> nsresult
> TextEventDispatcher::StartComposition(nsEventStatus& aStatus)
> {
>   aStatus = nsEventStatus_eIgnore;
> 
>@@ -219,16 +220,162 @@ TextEventDispatcher::NotifyIME(const IME
>   // notification or request for now.  In this case, we should return
>   // NS_ERROR_NOT_IMPLEMENTED because it's not implemented at such moment.
>   if (rv == NS_ERROR_NOT_AVAILABLE) {
>     return NS_ERROR_NOT_IMPLEMENTED;
>   }
>   return rv;
> }
> 
>+bool
>+TextEventDispatcher::DispatchKeyboardEvent(
>+                       uint32_t aMessage,
>+                       const WidgetKeyboardEvent& aKeyboardEvent,
>+                       nsEventStatus& aStatus)
>+{
>+  return DispatchKeyboardEventInternal(aMessage, aKeyboardEvent, aStatus);
>+}
>+
>+bool
>+TextEventDispatcher::DispatchKeyboardEventInternal(
>+                       uint32_t aMessage,
>+                       const WidgetKeyboardEvent& aKeyboardEvent,
>+                       nsEventStatus& aStatus,
>+                       uint32_t aIndexOfKeypressEvent)
>+{
>+  MOZ_ASSERT(aMessage == NS_KEY_DOWN || aMessage == NS_KEY_UP ||
>+             aMessage == NS_KEY_PRESS, "Invalid aMessage value");
>+
>+  nsresult rv = GetState();
>+  if (NS_WARN_IF(NS_FAILED(rv))) {
>+    return false;
>+  }
>+
>+  // If the key shouldn't cause keypress events, don't this patch them.
>+  if (aMessage == NS_KEY_PRESS && !aKeyboardEvent.ShouldCauseKeypressEvents()) {
>+    return false;
>+  }
>+
>+  nsCOMPtr<nsIWidget> widget(mWidget);
>+
>+  WidgetKeyboardEvent keyEvent(true, aMessage, widget);
>+  InitEvent(keyEvent);
>+  keyEvent.AssignKeyEventData(aKeyboardEvent, false);
>+
>+  if (aStatus == nsEventStatus_eConsumeNoDefault) {
>+    // If the key event should be dispatched as consumed event, marking it here.
>+    // This is useful to prevent double action.  E.g., when the key was already
>+    // handled by system, our chrome shouldn't handle it.
>+    keyEvent.mFlags.mDefaultPrevented = true;
>+  }
>+
>+  // Corrects each member for the specific key event type.
>+  if (aMessage == NS_KEY_DOWN || aMessage == NS_KEY_UP) {
>+    // charCode of keydown and keyup should be 0.
>+    keyEvent.charCode = 0;
>+    MOZ_ASSERT(aIndexOfKeypressEvent == 0,
>+               "aIndexOfKeypressEvent must be 0 when aMessage is NS_KEY_DOWN "
>+               "or NS_KEY_UP");
>+  } else {
>+    if (keyEvent.mKeyNameIndex != KEY_NAME_INDEX_USE_STRING) {
>+      // If keypress event isn't caused by printable key, its charCode should be
>+      // 0.
>+      MOZ_ASSERT(aIndexOfKeypressEvent == 0,
>+                 "aIndexOfKeypressEvent must be 0 when mKeyNameIndex is not "
>+                 "KEY_NAME_IDNEX_USE_STRING");
>+      keyEvent.charCode = 0;
>+    } else if (keyEvent.mKeyValue.IsEmpty()) {
>+      // If mKeyValue is empty, both keyCode and charCode should be 0.
>+      MOZ_ASSERT(aIndexOfKeypressEvent == 0,
>+                 "aIndexOfKeypressEvent must be 0 when mKeyValue is empty");
>+      keyEvent.keyCode = keyEvent.charCode = 0;
>+    } else {
>+      // Otherwise, its charCode should be the char at the index.
>+      MOZ_ASSERT(aIndexOfKeypressEvent < keyEvent.mKeyValue.Length(),
>+                 "aIndexOfKeypressEvent must be less than mKeyValue.Length()");
>+      keyEvent.keyCode = 0;
>+      // XXX .charCode should be a code point of UTF-16 or UCS-4??
>+      keyEvent.charCode =
>+        static_cast<uint32_t>(keyEvent.mKeyValue[aIndexOfKeypressEvent]);
>+      if (aIndexOfKeypressEvent > 0) {
>+        // If this is second or later keypress event, let's clear mKeyValue
>+        // because DOM key event handler cannot distinguish if coming keypress
>+        // event should cause input the character again or not.
>+        // If a keypress event handler handles to input text with .key value
>+        // and if each keypress event has same .key value, same characters
>+        // must be inputted repeatedly.
>+        keyEvent.mKeyValue.Truncate();
Hmm

>+  // Otherwise, if the key causes two or more characters, we need to dispatch
>+  // key events for every character because .charCode can store only one
>+  // character per a keypress event.
>+  bool consumed = (aStatus == nsEventStatus_eConsumeNoDefault);
>+  for (size_t i = 1; i < aKeyboardEvent.mKeyValue.Length(); i++) {
>+    aStatus = nsEventStatus_eIgnore;
>+    if (!DispatchKeyboardEventInternal(NS_KEY_PRESS, aKeyboardEvent,
>+                                       aStatus, i)) {
>+      // The widget must have been gone.
>+      break;
>+    }
So DispatchKeyboardEventInternal creates a copy of the event and then truncates.

Why is keyValue always empty for other than the first keypress?


>+  /**
>+   * DispatchKeyboardEvent() maybe dispatches aKeyboardEvent.
>+   *
>+   * @param aMessage        Must be NS_KEY_DOWN or NS_KEY_UP.
>+   *                        Use MaybeDispatchKeypressEvents() for dispatching
>+   *                        NS_KEY_PRESS.
>+   * @param aKeybaordEvent  A keyboard event.
s/aKeybaordEvent/aKeyboard/ 
Same also elsewhere.

Comment 100

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

Comment on attachment 8564241 [details] [diff] [review]
part.13 EventUtils.synthesizeComposition() and synthesizeCompositionChange() should take KeyboardEvent for emulating composition state change caused by a key operation

mostly rs+

Comment 101

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

Comment on attachment 8564244 [details] [diff] [review]
part.14 Reimplement/redesign EventUtils.synthesizeKey() with nsITextInputProcessor

Shouldn't synthesizeKey store the original modifier state and then
restore the modifier state to that after all the events have been dispatched.
That way if one activates a modifier twice, the inner most de-activation doesn't clear it.
If there is good reason to not do that, feel free to reask review for this patch.

Comment 102

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #77)
> The reimplemented synthesizeKey() is a little bit slower than the old
> implementation. Probably the reason is hack for modifier state.
How is that slower? Or why? Just because it may dispatch more events or something?
Event dispatch is really quite fast and shouldn't affect to performance in case of 
key or composition events.

Comment 103

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

Comment on attachment 8564248 [details] [diff] [review]
part.15 Fix new orange of test_menulist_keynav.xul because it's caused by timeout of incremental search in menu

I need to understand why this all is needed before reviewing this.
And why static sIncrementalString ?


If we need a pref for the time, why not just use a Preferences::IntVarCache or some such? Why all the other changes?

Comment 104

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

Comment on attachment 8564992 [details] [diff] [review]
part.12 nsITextInputProcessor should take KeyboardEvent as an argument of composition releated methods for dispatching key events around composition events (sr=smaug)

>+TextInputProcessor::StartComposition(nsIDOMKeyEvent* aDOMKeyEvent,
>+                                     uint32_t aKeyFlags,
>+                                     uint8_t aOptionalArgc,
>+                                     bool* aSucceeded)

So I think this method should actually verify that aDOMKeyEvent has sane type, not
any random string value.

>+TextInputProcessor::FlushPendingComposition(nsIDOMKeyEvent* aDOMKeyEvent,
>+                                            uint32_t aKeyFlags,
>+                                            uint8_t aOptionalArgc,
>+                                            bool* aSucceeded)
And same here.
So in case aDOMKeyEvent isn't null, it should have a sane value for .type.
And if not, return NS_ERROR_FAILURE or something.

>-TextInputProcessor::CommitComposition(const nsAString& aCommitString,
>+TextInputProcessor::CommitComposition(nsIDOMKeyEvent* aDOMKeyEvent,
>+                                      uint32_t aKeyFlags,
>+                                      const nsAString& aCommitString,
>                                       uint8_t aOptionalArgc,
>                                       bool* aSucceeded)
ditto

>+TextInputProcessor::CommitCompositionInternal(nsIDOMKeyEvent* aDOMKeyEvent,
>+                                              uint32_t aKeyFlags,
>+                                              const nsAString* aCommitString,
>                                               bool* aSucceeded)

Ok... will continue reviewing from there.

Comment 105

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

Comment on attachment 8564992 [details] [diff] [review]
part.12 nsITextInputProcessor should take KeyboardEvent as an argument of composition releated methods for dispatching key events around composition events (sr=smaug)

>+  bool doDefault = true;
>+  bool wasComposing = IsComposing();
>+  WidgetKeyboardEvent* originalKeyEvent =
>+    aDOMKeyEvent ?
>+      aDOMKeyEvent->GetInternalNSEvent()->AsKeyboardEvent() : nullptr;
>+  if (originalKeyEvent) {
>+    // Modifier keys are not allowed because managing modifier state in this
>+    // method makes this messy.
>+    if (NS_WARN_IF(originalKeyEvent->IsModifierKeyEvent())) {
>+      return NS_ERROR_INVALID_ARG;
>+    }
>+    rv = KeydownInternal(*originalKeyEvent, aKeyFlags, false, doDefault);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      return rv;
>+    }
>+    // If keydown event causes destroying the widget, shouldn't continue.
>+    if (NS_FAILED(IsValidStateForComposition())) {
>+      return NS_OK;
>+    }
>+  }
So there is this kind of code several times.
It should be factored out to some helper method.

Possibly also the relevant KeyUp handling too

Comment 106

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

(In reply to Olli Pettay [:smaug] from comment #98)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #51)
> > For example, new KeyboardEvent("", { key: "abc" }); causes mKeyValue is
> > "abc" and mKeyNameIndex is KEY_NAME_INDEX_USE_STRING. Possible scenario of
> > this case is JS-Keyboard tries to input a grapheme cluster which contains
> > combining character like accent marks (e.g., "'", "~").
> 
> Hmm, sounds like KeyboardEvent may just cause more confusion here than
> helping implementation.
> Normally KeyboardEvents don't have that way long string values in .key.

Yeah, if the value is just some characters, it's a edge case. However, as I said, if the value includes combining character (alphabet with accent mark, some south Asian languages) or non-BMP character, the value may be two or more UTF-16 code points.

(In reply to Olli Pettay [:smaug] from comment #99)
> Comment on attachment 8564205 [details] [diff] [review]
> part.1 Implement key event dispatcher in TextEventDispatcher

> >+      if (aIndexOfKeypressEvent > 0) {
> >+        // If this is second or later keypress event, let's clear mKeyValue
> >+        // because DOM key event handler cannot distinguish if coming keypress
> >+        // event should cause input the character again or not.
> >+        // If a keypress event handler handles to input text with .key value
> >+        // and if each keypress event has same .key value, same characters
> >+        // must be inputted repeatedly.
> >+        keyEvent.mKeyValue.Truncate();
> Hmm
> 
> >+  // Otherwise, if the key causes two or more characters, we need to dispatch
> >+  // key events for every character because .charCode can store only one
> >+  // character per a keypress event.
> >+  bool consumed = (aStatus == nsEventStatus_eConsumeNoDefault);
> >+  for (size_t i = 1; i < aKeyboardEvent.mKeyValue.Length(); i++) {
> >+    aStatus = nsEventStatus_eIgnore;
> >+    if (!DispatchKeyboardEventInternal(NS_KEY_PRESS, aKeyboardEvent,
> >+                                       aStatus, i)) {
> >+      // The widget must have been gone.
> >+      break;
> >+    }
> So DispatchKeyboardEventInternal creates a copy of the event and then
> truncates.
> 
> Why is keyValue always empty for other than the first keypress?

Did you mean that before calling DispatchKeyboardEventInternal() from the for loop, mKeyValue should be truncated? If so, we need to create copy of the aKeyboardEvent twice. Is that OK?

(In reply to Olli Pettay [:smaug] from comment #101)
> Comment on attachment 8564244 [details] [diff] [review]
> part.14 Reimplement/redesign EventUtils.synthesizeKey() with
> nsITextInputProcessor
> 
> Shouldn't synthesizeKey store the original modifier state and then
> restore the modifier state to that after all the events have been dispatched.
> That way if one activates a modifier twice, the inner most de-activation
> doesn't clear it.

I don't understand what case you worry. If a modifier state was already activated in the TIP, synthesizeKey() doesn't re-activate the state with keydown(). Similarly, such skipped modifier state isn't inactivated at restoring the original modifier state.

> If there is good reason to not do that, feel free to reask review for this
> patch.

I'll ask review again after I understand what you find from the patch.

(In reply to Olli Pettay [:smaug] from comment #102)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #77)
> > The reimplemented synthesizeKey() is a little bit slower than the old
> > implementation. Probably the reason is hack for modifier state.
> How is that slower? Or why? Just because it may dispatch more events or
> something?

I guess that two loops for setting and restoring modifier states are the slower. Especially, it may cause a lot of virtual calls if a lot of modifier states of aEvet are true. I don't think dispatching event is slower than the current implementation because number of dispatching events isn't increased by this patch. (changing modifier state isn't dispatch modifier key event actually)

> Event dispatch is really quite fast and shouldn't affect to performance in
> case of 
> key or composition events.

Yeah, but the 1 sec timeout is too fast. In fact, there was an intermittent orange bug (bug 700146).

(In reply to Olli Pettay [:smaug] from comment #103)
> Comment on attachment 8564248 [details] [diff] [review]
> part.15 Fix new orange of test_menulist_keynav.xul because it's caused by
> timeout of incremental search in menu
> 
> I need to understand why this all is needed before reviewing this.

So, working timeout mechanism doesn't make sense for usual automated tests. It should work only when the test tries to check if the timeout occurs as expected.

> And why static sIncrementalString ?

Both text input and active menu popup are single staff, i.e., I think that they must not be handled by two or more instances at once. (FYI: the value is cleared when active popup is closed.) Therefore, static variable is OK and it's useful when we clear the value at the pref changed.

> If we need a pref for the time, why not just use a Preferences::IntVarCache
> or some such? Why all the other changes?

Using IntVarCache() cannot make the old search timed out forcibly. Therefore, this patch uses a callback.

For example, if automated tests want to test incremental search twice or more times, they may want to write as:

SpecialPowers.setIntPref("ui.menu.incremental_search.timeout", 0); // makes timeout disable
synthesizeKey("a", new KeyboardEvent("", {})); // type first character
synthesizeKey("b", new KeyboardEvent("", {})); // type second character
ok(items[n].selected, "unexpected item is selected");
// the test may assume that the tested incremental search is timed out here.
SpecialPowers.clearUserPref("ui.menu.incremental_search.timeout");

...

SpecialPowers.setIntPref("ui.menu.incremental_search.timeout", 0); // makes timeout disable
// and test incremental search again.

If nsMenuPopupFrame doesn't abort the transaction at changing the pref, second test is always a part of the first transaction because timeout is checked when keypress event is being handled. In the example, all keypress events are fired while the timeout is disabled. Therefore, if it's not aborted at changing the pref, the transaction may be kept even in following tests.

(In reply to Olli Pettay [:smaug] from comment #104)
> Comment on attachment 8564992 [details] [diff] [review]
> part.12 nsITextInputProcessor should take KeyboardEvent as an argument of
> composition releated methods for dispatching key events around composition
> events (sr=smaug)
> 
> >+TextInputProcessor::StartComposition(nsIDOMKeyEvent* aDOMKeyEvent,
> >+                                     uint32_t aKeyFlags,
> >+                                     uint8_t aOptionalArgc,
> >+                                     bool* aSucceeded)
> 
> So I think this method should actually verify that aDOMKeyEvent has sane
> type, not
> any random string value.

So, the value must be checked ifthe value is one of NS_KEY_DOWN, NS_KEY_UP or 0 (for "")?

Comment 107

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #106)
> > Why is keyValue always empty for other than the first keypress?
> 
> Did you mean that before calling DispatchKeyboardEventInternal() from the
> for loop, mKeyValue should be truncated? If so, we need to create copy of
> the aKeyboardEvent twice. Is that OK?
Why we report the whole mKeyValue in the first keypress and nothing in the subsequent events.
Yet charCode is set on all the events.


> 
> (In reply to Olli Pettay [:smaug] from comment #101)
> > Comment on attachment 8564244 [details] [diff] [review]
> > part.14 Reimplement/redesign EventUtils.synthesizeKey() with
> > nsITextInputProcessor
> > 
> > Shouldn't synthesizeKey store the original modifier state and then
> > restore the modifier state to that after all the events have been dispatched.
> > That way if one activates a modifier twice, the inner most de-activation
> > doesn't clear it.
> 
> I don't understand what case you worry. If a modifier state was already
> activated in the TIP, synthesizeKey() doesn't re-activate the state with
> keydown().
Sure, reactivating doesn't do anything.

> Similarly, such skipped modifier state isn't inactivated at
> restoring the original modifier state
How so? Could you point to the code.



> I guess that two loops for setting and restoring modifier states are the
> slower.
Loop in JS or C++? both of those should be really fast.

> Especially, it may cause a lot of virtual calls if a lot of modifier
> states of aEvet are true.
Still fast. virtual calls are sure slow in _hot_ code paths, like event dispatch, but even there they show up
when profiling microbenchmarks which dispatch thousands of events in a row.


> Yeah, but the 1 sec timeout is too fast. In fact, there was an intermittent
> orange bug (bug 700146).

Still don't understand how anything performance related could have affected that.
Are we possibly dispatching events in different order or what and that would cause the test to fail?


> 
> So, the value must be checked ifthe value is one of NS_KEY_DOWN, NS_KEY_UP
> or 0 (for "")?
yeah

Comment 108

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

(In reply to Olli Pettay [:smaug] from comment #107)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #106)
> > > Why is keyValue always empty for other than the first keypress?
> > 
> > Did you mean that before calling DispatchKeyboardEventInternal() from the
> > for loop, mKeyValue should be truncated? If so, we need to create copy of
> > the aKeyboardEvent twice. Is that OK?
> Why we report the whole mKeyValue in the first keypress and nothing in the
> subsequent events.
> Yet charCode is set on all the events.

As I said a couple of times, following handler won't work:

foo.addEventListener("keypress", function (aEvent) {
    this.text += aEvent.key;
  }, false);

If "abc" sets all 3 keypress events, the result may be: "abcabcabc". The other possible solution is, we set only one character to the key value only when dispatching keypress. Note that keypress isn't supported by D3E due to a legacy event. So, either behavior should be OK.

> > (In reply to Olli Pettay [:smaug] from comment #101)
> > > Comment on attachment 8564244 [details] [diff] [review]
> > > part.14 Reimplement/redesign EventUtils.synthesizeKey() with
> > > nsITextInputProcessor
> > > 
> > > Shouldn't synthesizeKey store the original modifier state and then
> > > restore the modifier state to that after all the events have been dispatched.
> > > That way if one activates a modifier twice, the inner most de-activation
> > > doesn't clear it.
> > 
> > I don't understand what case you worry. If a modifier state was already
> > activated in the TIP, synthesizeKey() doesn't re-activate the state with
> > keydown().
> Sure, reactivating doesn't do anything.
> 
> > Similarly, such skipped modifier state isn't inactivated at
> > restoring the original modifier state
> How so? Could you point to the code.

https://bugzilla.mozilla.org/attachment.cgi?id=8564241&action=diff

In _emulateToActivateModifiers(), activated modifier's |.activated| is set true. In _emulateToInactivateModifiers(), if the value is not true, the loop call continue, first.

> > I guess that two loops for setting and restoring modifier states are the
> > slower.
> Loop in JS or C++? both of those should be really fast.

I think the JS's in _emulateToActivateModifiers() and _emulateToInactivateModifiers(). They may call virtual methods via XPConnect.

> > Especially, it may cause a lot of virtual calls if a lot of modifier
> > states of aEvet are true.
> Still fast. virtual calls are sure slow in _hot_ code paths, like event
> dispatch, but even there they show up
> when profiling microbenchmarks which dispatch thousands of events in a row.

Hmm, anyway, when I use debugger of VS, the incremental search timed-out. (I think that 1 sec is too short especially for debug builds)

> > Yeah, but the 1 sec timeout is too fast. In fact, there was an intermittent
> > orange bug (bug 700146).
> 
> Still don't understand how anything performance related could have affected
> that.
> Are we possibly dispatching events in different order or what and that would
> cause the test to fail?

I have no idea. If there are such bugs, I guess that a lot of tests become orange.

Comment 109

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

Attachment: part.1 Implement key event dispatcher in TextEventDispatcher

Specifying charCode value directly (via its argument) and truncate mKeyValue before calling DispatchKeyEventInternal().

Comment 110

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

Attachment: part.12 nsITextInputProcessor should take KeyboardEvent as an argument of composition releated methods for dispatching key events around composition events (sr=smaug)

Adding to check the event type. And also created DispatchKeydownEventForComposition() and DispatchKeyupEventForComposition().

Comment 111

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

Attachment: part.17 TextInputProcessor shouldn't allow to begin input transaction with different TextEventDispatcher during dispatching an event

This patch makes TIP cannot begin new input transaction with another window while the TIP is dispatching an event.

Comment 112

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #108)

> As I said a couple of times, following handler won't work:
> 
> foo.addEventListener("keypress", function (aEvent) {
>     this.text += aEvent.key;
>   }, false);
> 
> If "abc" sets all 3 keypress events, the result may be: "abcabcabc".

I don't think I ever hinted it should always be "abc"

 The
> other possible solution is, we set only one character to the key value only
> when dispatching keypress.
This one.
So the events would have .key = "a", .key = "b", .key = "c"


> Note that keypress isn't supported by D3E due to
> a legacy event.
(btw, it isn't clear to me we can in any way deprecate keypress)


> 
> https://bugzilla.mozilla.org/attachment.cgi?id=8564241&action=diff
> 
> In _emulateToActivateModifiers(), activated modifier's |.activated| is set
> true. In _emulateToInactivateModifiers(), if the value is not true, the loop
> call continue, first.
ah there. thanks


> I think the JS's in _emulateToActivateModifiers() and
> _emulateToInactivateModifiers(). They may call virtual methods via XPConnect.
Still should be fast

> 
> I have no idea. If there are such bugs, I guess that a lot of tests become
> orange.
Could you possibly log all dispatched events in the test without the patches and with
(by adding some debug code to EventDispatcher::Dispatch).
Just randomly increasing some timeout value feels hacky way to fix anything.

Comment 113

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

(In reply to Olli Pettay [:smaug] from comment #112)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #108)
>  The
> > other possible solution is, we set only one character to the key value only
> > when dispatching keypress.
> This one.
> So the events would have .key = "a", .key = "b", .key = "c"

Okay, I'll rewrite the part.1 with this approach.

> > I think the JS's in _emulateToActivateModifiers() and
> > _emulateToInactivateModifiers(). They may call virtual methods via XPConnect.
> Still should be fast
> 
> > 
> > I have no idea. If there are such bugs, I guess that a lot of tests become
> > orange.
> Could you possibly log all dispatched events in the test without the patches
> and with
> (by adding some debug code to EventDispatcher::Dispatch).
> Just randomly increasing some timeout value feels hacky way to fix anything.

Although, I'll do it, but please don't forget there was bug 700146. I was marked as WFM, but it's probably fixed by improving performance of some part. So, I think that 1 sec wasn't enough since the test was shipped.

I believe that disabling timeout mechanism is the only way to guarantee to prevent intermittent failure of the test.

Comment 114

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

Attachment: event log without the patches

Comment 115

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

Attachment: event log with the patches

Comment 116

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

Attachment: diff of event logs

Looks like there is no change which causes the orange of test_menulist_keynav.xul.

Comment 117

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

Comment on attachment 8565460 [details] [diff] [review]
diff of event logs

ok, thanks.

Comment 118

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

Comment on attachment 8565384 [details] [diff] [review]
part.1 Implement key event dispatcher in TextEventDispatcher

I don't see where events get their .key value?
There is otherKeypressEvents.mKeyValue.Truncate(); and then 
aKeyboardEvent.mKeyValue[i] is passed, but then in
TextEventDispatcher::DispatchKeyboardEventInternal
aChar is assigned to .charCode.

I thought you we're going to assign mKeyValue[i] to mKeyValue of each event.
"Okay, I'll rewrite the part.1 with this approach." in comment 113

And looks like the first keypress has the original mKeyValue
(but aKeyboardEvent.mKeyValue[0] as its charCode)

Comment 119

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

Comment on attachment 8565386 [details] [diff] [review]
part.12 nsITextInputProcessor should take KeyboardEvent as an argument of composition releated methods for dispatching key events around composition events (sr=smaug)

Couldn't you merge IsValidStateForComposition call into DispatchKeydownForComposition? And perhaps also the null check.

So methods would be

::Foo()
{
  // do something
  bool compositionIsValid;
  nsresult rv = MaybeDispatchKeydownForComposition(originalKeyEvent, aKeyFlags, doDefault, &compositionIsValid);
  NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && compositionIsValid, rv);

  ...do something

  MaybeDispatchKeyupForComposition(originalKeyEvent, aKeyFlags);
}

I think that would make the code a bit easier to read.

Comment 120

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

Attachment: part.1 Implement key event dispatcher in TextEventDispatcher

This is the rewritten patch. I've tested this on the tryserver.

Comment 121

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

Comment on attachment 8564244 [details] [diff] [review]
part.14 Reimplement/redesign EventUtils.synthesizeKey() with nsITextInputProcessor

I'm asking review again since you misunderstood the behavior of _emulateToActivateModifiers() and _emulateToInactivateModifiers().

Comment 122

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

Comment on attachment 8564248 [details] [diff] [review]
part.15 Fix new orange of test_menulist_keynav.xul because it's caused by timeout of incremental search in menu

And also this again since according to the log difference, I still believe this is caused by performance issue. The timeout duration (1 sec) hasn't been enough since the test was shipped. Therefore, there was an intermittent failure. I guess that the reason why the intermittent failure was gone is that some other performance improvement hides timeout. However, the new synthesizeKey() makes the tests cross 1 sec boundary again.

For solving this perfectly, we should disable the unnecessary timeout for tests.

Comment 123

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

Attachment: part.4 Add keydown() and keyup() to nsITextInputProcessor (r=smaug, sr=smaug)

The test in this patch includes a fix for part.1's behavior change.

Comment 124

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

I'd like to suggest that if we cannot fix only part.15 by this Friday, I'd like to land the other patches with disabling the test temporarily. I think that enabling the test on Aurora is easy, but it's impossible to land other patches on Aurora.

Additionally, if we cannot land them by this weekend, we should back out nsITextInputProcessor from Aurora. The first release of nsITextInputProcessor should include all changes in this bug for forward compatibility.

However, I still don't give up to land them this week. Thanks in advance.

Comment 125

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

Attachment: part.12 nsITextInputProcessor should take KeyboardEvent as an argument of composition releated methods for dispatching key events around composition events (sr=smaug)

Comment 126

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

I wonder, TIP.commitComposition(null, 0, "commit-string"); is ugly.

It might be better to add commitCompositionWith(aCommitString, nsIDOMKeyEvent, unit32_t) and make commitComposition as (nsIDOMKeyEvent, uint32_t).

I'll post the patch as part.18. If smaug agree with this change, we should take this.

Comment 127

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

Attachment: part.12 nsITextInputProcessor should take KeyboardEvent as an argument of composition releated methods for dispatching key events around composition events (sr=smaug)

Fix some nits of the comment in nsITextInputProcessor.idl

Comment 128

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

Attachment: part.18 Add nsITextInputProcessor.commitCompositionWith() and drop aCommitString from nsITextInputProcessor.commitComposition()

Comment 129

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

Attachment: part.15 Disable test_menulist_keynav.xul temporarily due to permanent orange with new synthesizeKey()

last resort for landing the patches without fix the new orange of test_menulist_keynav.xul.

Comment 130

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

Comment on attachment 8565875 [details] [diff] [review]
part.18 Add nsITextInputProcessor.commitCompositionWith() and drop aCommitString from nsITextInputProcessor.commitComposition()

This is a proposal patch for avoiding the ugly style like commitComposition(null, 0, "commitString"); This makes:

commitComposition(keyEvent, keyFlags, "commitString") ->
  commitCompositionWith("commitString", keyEvent, keyFlags).

I.e., if key event isn't specified, it can be:

commitCompositionWith("commitString")

Comment 131

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #124)
> I'd like to suggest that if we cannot fix only part.15 by this Friday, I'd
> like to land the other patches with disabling the test temporarily. I think
> that enabling the test on Aurora is easy, but it's impossible to land other
> patches on Aurora.
> 
> Additionally, if we cannot land them by this weekend, we should back out
> nsITextInputProcessor from Aurora. The first release of
> nsITextInputProcessor should include all changes in this bug for forward
> compatibility.
> 
> However, I still don't give up to land them this week. Thanks in advance.

I'll keep reviewing. I think we can get this in before the next merge.

Comment 132

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

Comment on attachment 8564248 [details] [diff] [review]
part.15 Fix new orange of test_menulist_keynav.xul because it's caused by timeout of incremental search in menu

>+nsString nsMenuPopupFrame::sIncrementalString;
Does this not create a static constructor?
You may need to use 
nsString* and manually delete or some such if tests complain about increased static ctors

>+nsMenuPopupFrame::IncrementalSearchPrefCallback(const char* aPref,
>+                                                void* aClosure)
>+{
>+  MOZ_ASSERT(nsDependentCString(aPref).Equals(kPrefIncrementalSearchTimeout));
>+  sTimeoutOfIncrementalSearch =
>+    Preferences::GetUint(kPrefIncrementalSearchTimeout, 1000);
>+  sLastKeyTime = 0;
>+  sIncrementalString.Truncate();
This is a bit ugly that setting the pref has a side effect to clear sIncrementalString.


But, I still don't understand why you don't just 
have a IntVarCache and set the pref at the beginning of the test, then run the test, and
then wait for the amount of the timeout and then continue to the next test or clear the pref and calls Simpletest.finish()
(or whatever the relevant test is using.).

IMO we should try to avoid code for testing if we can live without those by changing tests.

Comment 133

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

(In reply to Olli Pettay [:smaug] from comment #132)
> >+nsMenuPopupFrame::IncrementalSearchPrefCallback(const char* aPref,
> >+                                                void* aClosure)
> >+{
> >+  MOZ_ASSERT(nsDependentCString(aPref).Equals(kPrefIncrementalSearchTimeout));
> >+  sTimeoutOfIncrementalSearch =
> >+    Preferences::GetUint(kPrefIncrementalSearchTimeout, 1000);
> >+  sLastKeyTime = 0;
> >+  sIncrementalString.Truncate();
> This is a bit ugly that setting the pref has a side effect to clear
> sIncrementalString.
> 
> 
> But, I still don't understand why you don't just 
> have a IntVarCache and set the pref at the beginning of the test, then run
> the test, and
> then wait for the amount of the timeout and then continue to the next test
> or clear the pref and calls Simpletest.finish()
> (or whatever the relevant test is using.).

Hmm, so, do you like set enough long time for/from tests and shouldn't make it never timed our with 0? But then, for example, if we set 5 sec for that, each timeout needs over 5 sec. It may make the test slower. Is it OK?

Comment 134

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

Comment on attachment 8565875 [details] [diff] [review]
part.18 Add nsITextInputProcessor.commitCompositionWith() and drop aCommitString from nsITextInputProcessor.commitComposition()

This indeed makes the API nicer to use.

"If there is no composition, this will just insert the specified string." is a bit unclear. What does 'insert' mean there? Will a new composition be started and then the string committed?
Please clarify when landing the patch.

Comment 135

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #133)
> (In reply to Olli Pettay [:smaug] from comment #132)
> > >+nsMenuPopupFrame::IncrementalSearchPrefCallback(const char* aPref,
> > >+                                                void* aClosure)
> > >+{
> > >+  MOZ_ASSERT(nsDependentCString(aPref).Equals(kPrefIncrementalSearchTimeout));
> > >+  sTimeoutOfIncrementalSearch =
> > >+    Preferences::GetUint(kPrefIncrementalSearchTimeout, 1000);
> > >+  sLastKeyTime = 0;
> > >+  sIncrementalString.Truncate();
> > This is a bit ugly that setting the pref has a side effect to clear
> > sIncrementalString.
> > 
> > 
> > But, I still don't understand why you don't just 
> > have a IntVarCache and set the pref at the beginning of the test, then run
> > the test, and
> > then wait for the amount of the timeout and then continue to the next test
> > or clear the pref and calls Simpletest.finish()
> > (or whatever the relevant test is using.).
> 
> Hmm, so, do you like set enough long time for/from tests and shouldn't make
> it never timed our with 0? But then, for example, if we set 5 sec for that,
> each timeout needs over 5 sec. It may make the test slower. Is it OK?

Ah, I'm idiot. This can make the resolution easier:

>   keyCheck(list, "A", 2, "letter pressed");
>   SpecialPowers.setIntPref("ui.menu.incremental_search.timeout", 0); // prevent to timeout
>   keyCheck(list, "B", 2, "letter pressed");
>   SpecialPowers.clearUserPref("ui.menu.incremental_search.timeout");
>   keyCheck(list, "A", 2, "letter pressed");
>   SpecialPowers.setIntPref("ui.menu.incremental_search.timeout", 0); // prevent to timeout
>   keyCheck(list, "B", 2, "letter pressed");
>   SpecialPowers.clearUserPref("ui.menu.incremental_search.timeout");

Then, the "A" key press is timed out if it spends 1 sec. But "B" key press is never timed out.

Comment 136

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

Comment on attachment 8565862 [details] [diff] [review]
part.12 nsITextInputProcessor should take KeyboardEvent as an argument of composition releated methods for dispatching key events around composition events (sr=smaug)

Why can't you move IsValidStateForComposition() to be checked also
at the end of MaybeDispatchKeydownForComposition() ?
It is a bit ugly that everyone who calls MaybeDispatchKeydownForComposition
needs to also check IsValidStateForComposition()

Comment 137

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

(In reply to Olli Pettay [:smaug] from comment #136)
> Comment on attachment 8565862 [details] [diff] [review]
> part.12 nsITextInputProcessor should take KeyboardEvent as an argument of
> composition releated methods for dispatching key events around composition
> events (sr=smaug)
> 
> Why can't you move IsValidStateForComposition() to be checked also
> at the end of MaybeDispatchKeydownForComposition() ?

Because it make the result complicated. If the widget is destroyed during dispatching keydown event, the original method should return NS_OK but returning NS_OK from MaybeDispatchKeydownForComposition() cannot distinguish if the caller should quit it before changing the composition.

Comment 138

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

Comment on attachment 8565761 [details] [diff] [review]
part.1 Implement key event dispatcher in TextEventDispatcher

>+TextEventDispatcher::MaybeDispatchKeypressEvents(
>+                       const WidgetKeyboardEvent& aKeyboardEvent,
>+                       nsEventStatus& aStatus)
>+{
>+  // If the key event was consumed, keypress event shouldn't be fired.
>+  if (aStatus == nsEventStatus_eConsumeNoDefault) {
>+    return false;
>+  }
>+
>+  // Dispatch first keypress event.
>+  aStatus = nsEventStatus_eIgnore;
>+  if (!DispatchKeyboardEventInternal(NS_KEY_PRESS, aKeyboardEvent, aStatus)) {
>+    return false;
>+  }
>+
>+  // If the key isn't a printable key or just inputting one character or
>+  // no character, we don't need to dispatch keypress events anymore.
>+  if (aKeyboardEvent.mKeyNameIndex != KEY_NAME_INDEX_USE_STRING ||
>+      aKeyboardEvent.mKeyValue.Length() < 2) {
Technically you don't need aKeyboardEvent.mKeyValue.Length() < 2 here, since the for loop
just doesn't do anything if Length() < 2.

But couldn't you merge the first keypress dispatch to the for loop.
Basically
size_t len =
  aKeyboardEvent.mKeyNameIndex != KEY_NAME_INDEX_USE_STRING ?
  1 : aKeyboardEvent.mKeyValue.Length();

and then just use that len in the 'for' 

With that, r+

Comment 139

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

Attachment: part.15 Fix new orange of test_menulist_keynav.xul because it's caused by timeout of incremental search in menu

Okay, this is simpler and guarantees the test won't become orange due to timeout.

Comment 140

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #137)
> Because it make the result complicated. If the widget is destroyed during
> dispatching keydown event, the original method should return NS_OK but
> returning NS_OK from MaybeDispatchKeydownForComposition() cannot distinguish
> if the caller should quit it before changing the composition.


bool compositionIsValid;
nsresult rv = MaybeDispatchKeydownForComposition(originalKeyEvent, aKeyFlags, doDefault, &compositionIsValid);
NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && compositionIsValid, rv);
doesn't look too bad to me.

Comment 141

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

Attachment: part.18 Add nsITextInputProcessor.commitCompositionWith() and drop aCommitString from nsITextInputProcessor.commitComposition() (r=smaug, sr=smaug)

Improving the comments:

> +   * commitCompositionWith() will commit composition with the specific string.
> +   * If there is no composition, this will start composition and commit it
> +   * with the specified string.

Comment 142

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

(In reply to Olli Pettay [:smaug] from comment #140)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #137)
> > Because it make the result complicated. If the widget is destroyed during
> > dispatching keydown event, the original method should return NS_OK but
> > returning NS_OK from MaybeDispatchKeydownForComposition() cannot distinguish
> > if the caller should quit it before changing the composition.
> 
> 
> bool compositionIsValid;
> nsresult rv = MaybeDispatchKeydownForComposition(originalKeyEvent,
> aKeyFlags, doDefault, &compositionIsValid);
> NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && compositionIsValid, rv);
> doesn't look too bad to me.

Hmm...

Comment 143

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

Comment on attachment 8565938 [details] [diff] [review]
part.15 Fix new orange of test_menulist_keynav.xul because it's caused by timeout of incremental search in menu

Add the var cache only once. So you need a static bool variable around
Preferences::AddUintVarCache.

static void Shutdown(); is not needed anymore.

Comment 144

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

(In reply to Olli Pettay [:smaug] from comment #143)
> Comment on attachment 8565938 [details] [diff] [review]
> part.15 Fix new orange of test_menulist_keynav.xul because it's caused by
> timeout of incremental search in menu
> 
> Add the var cache only once. So you need a static bool variable around
> Preferences::AddUintVarCache.

Yeah,

>    if (sDefaultLevelIsTop >= 0)
>      return;

should work. Do you think this isn't enough? (I don't like setting -1 to the sDefaultLevelIsTop at initializing the variable, though).

> static void Shutdown(); is not needed anymore.

Oops!

Comment 145

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

Yeah, I was wondering if (sDefaultLevelIsTop >= 0) would be enough. 
And apparently it is. bool is then assigned to it. Odd code, but fine, that is enough for your case too.

Comment 146

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

Attachment: part.12 nsITextInputProcessor should take KeyboardEvent as an argument of composition releated methods for dispatching key events around composition events (sr=smaug)

I don't like to add out arguments anymore. I like using struct for the result better. How about you?

Comment 147

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

Attachment: part.15 Fix new orange of test_menulist_keynav.xul because it's caused by timeout of incremental search in menu (r=smaug)

Thanks!

Enn, could you review this? These patches should be landed by this weekend. So, I'd like you to review this ASAP.

Comment 148

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

Attachment: part.1 Implement key event dispatcher in TextEventDispatcher (r=smaug)

Comment 149

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

Attachment: All changes

I think that you'd like to check all changes with a large patch.

Comment 150

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

Comment on attachment 8565960 [details] [diff] [review]
part.12 nsITextInputProcessor should take KeyboardEvent as an argument of composition releated methods for dispatching key events around composition events (sr=smaug)

>+
>+  EventDispatcherResult dispatcherResult =
>+    MaybeDispatchKeydownForComposition(keyboardEvent, aKeyFlags);
>+  if (NS_WARN_IF(NS_FAILED(dispatcherResult.mResult))) {
>+    return dispatcherResult.mResult;
>+  }
>+
>+  if (!dispatcherResult.mCanContinue) {
>+    return NS_OK;
>+  }
This is rather verbose.
Why not just
  EventDispatcherResult dispatcherResult =
    MaybeDispatchKeydownForComposition(keyboardEvent, aKeyFlags);
  if (NS_WARN_IF(NS_FAILED(dispatcherResult.mResult)) || !dispatcherResult.mCanContinue) {
    return dispatcherResult.mResult;
  }


>+  struct EventDispatcherResult
>+  {
>+    nsresult mResult;
>+    bool     mDoDefault;
>+    bool     mCanContinue;
>+
>+    EventDispatcherResult()
>+      : mResult(NS_OK)
>+      , mDoDefault(true)
>+      , mCanContinue(false)
I would initialize mCanContinue to true and then set it to false when needed.

Comment 151

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

Looking at the all-patches...

EventUtils.synthesizeKey("a", { type: "keydown", code: "KeyA", keyCode: KeyboardEvent.DOM_VK_A });
has quite a bit redundant data. Do we really need 'a' 3 times there?

Comment 152

[Neil Deakin]

Comment on attachment 8565963 [details] [diff] [review]
part.15 Fix new orange of test_menulist_keynav.xul because it's caused by timeout of incremental search in menu (r=smaug)

>+
>+  // If 0, never timed out.  Otherwise, the value is millisecond.

'is in milliseconds'


This seems ok, but if its taking longer than one second to fire a key event, then that would affect the tests that depend on it taking longer as well.

Do you have a pointer to a log showing this failing with the other patches in this bug?

Comment 153

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

(In reply to Olli Pettay [:smaug] (high review load) from comment #151)
> Looking at the all-patches...
> 
> EventUtils.synthesizeKey("a", { type: "keydown", code: "KeyA", keyCode:
> KeyboardEvent.DOM_VK_A });
> has quite a bit redundant data. Do we really need 'a' 3 times there?

When a test emulates physical keyboard's key press perfectly, all of them are necessary. However, we can create a utility method which emulates specific layout only from .key value. I think that it's useful at rewriting all callers. So, I'll do it in follow up bug.

(In reply to Neil Deakin from comment #152)
> Do you have a pointer to a log showing this failing with the other patches
> in this bug?

I don't understand well what you said, sorry. If you meant that which patch causes the orange is, I can say part.14 which rewrites synthesizeKey() with new API. If you meant that the actual cause of the patch earlier part.14, I have no idea because part.14 really depends on all of them.

I find this from this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cc23045531c

e.g.,
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6cc23045531c/try-linux-debug/try_ubuntu32_vm-debug_test-mochitest-other-bm01-tests1-linux32-build84.txt.gz

Comment 154

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3ab2f41fa63
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb629fa95d06
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7ef5a56b661
https://hg.mozilla.org/integration/mozilla-inbound/rev/4af5d66b1c0b
https://hg.mozilla.org/integration/mozilla-inbound/rev/79a7a9fb13a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/d97c2fcf2bf8
https://hg.mozilla.org/integration/mozilla-inbound/rev/87ed00d8ae5c
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd6067b851b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f20df12d0ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/38827922cd9b
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ea202182017
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcf39cf75424
https://hg.mozilla.org/integration/mozilla-inbound/rev/faa6c7dc1dd1
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1e9fee45538
https://hg.mozilla.org/integration/mozilla-inbound/rev/07f7bc36519c
https://hg.mozilla.org/integration/mozilla-inbound/rev/8aa90e1304a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a1b3c42f287
https://hg.mozilla.org/integration/mozilla-inbound/rev/26de49fecaae

Thank you for your cooperation!

Comment 155

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e3ab2f41fa63
https://hg.mozilla.org/mozilla-central/rev/eb629fa95d06
https://hg.mozilla.org/mozilla-central/rev/d7ef5a56b661
https://hg.mozilla.org/mozilla-central/rev/4af5d66b1c0b
https://hg.mozilla.org/mozilla-central/rev/79a7a9fb13a9
https://hg.mozilla.org/mozilla-central/rev/d97c2fcf2bf8
https://hg.mozilla.org/mozilla-central/rev/87ed00d8ae5c
https://hg.mozilla.org/mozilla-central/rev/cd6067b851b4
https://hg.mozilla.org/mozilla-central/rev/6f20df12d0ff
https://hg.mozilla.org/mozilla-central/rev/38827922cd9b
https://hg.mozilla.org/mozilla-central/rev/0ea202182017
https://hg.mozilla.org/mozilla-central/rev/bcf39cf75424
https://hg.mozilla.org/mozilla-central/rev/faa6c7dc1dd1
https://hg.mozilla.org/mozilla-central/rev/d1e9fee45538
https://hg.mozilla.org/mozilla-central/rev/07f7bc36519c
https://hg.mozilla.org/mozilla-central/rev/8aa90e1304a3
https://hg.mozilla.org/mozilla-central/rev/4a1b3c42f287
https://hg.mozilla.org/mozilla-central/rev/26de49fecaae

Comment 156

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

https://developer.mozilla.org/en-US/Firefox/Releases/38
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsITextInputProcessor
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIDOMWindowUtils