Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Description • 10 years ago

Starting Gecko 38, Gecko provides nsITextInputProcessor interface for providing better way to create composition and dispatch KeyboardEvent. The document is here:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsITextInputProcessor

Now, CompositionManager is already using nsITextInputProcessor at creating composition. However, not so for dispatching key events.

I think that some methods of CompositionManager should be changed or we might need to add some methods for keeping backward compatibility.

I'm thinking we should improve following points:

* Dispatching keyboard events via nsITextInputProcessor, the keyboard information should be sent with object like { key: "Foo", code: "Bar", keyCode: KeyboardEvent.DOM_VK_BUZZ, repeat: (true|false) } or KeyboardEvent same as nsITextInputProcessor.keydown() and keyup().

  - The change makes Firefox OS D3E aware. In the future, all our event handlers should check .key value instead of .keyCode value (bug 1136539) because .keyCode won't support new keys such as multimedia keys nor phone specific keys.

* Send keyboard events when CompositionManager calls startComposition(), flushPendingComposition(), commitComposition(), commitCompositionWith() and cancelComposition() if it's caused by a keypress of VKB.

  - Although, current Gecko doesn't dispatch key events during composition. However, the other browsers do it. Therefore, we might need to dispatch them in some cases (bug 1112212, bug 354358). TextInputProcessor was designed as every key down and up are notified and it dispatches key events when it's necessary. So, passing key events to these method may cause improving mobile site compatibility with other browsers.



Anyway, Gaia's keyboard and IME have to be reimplemented with new API for conforming to nsITextInputProcessor, though. But I believe that it should be performed as soon as possible. In my understanding, 3rd party's keyboard/IME isn't available. So, we should do it before it's supported.

Yuan, Tim:

Can you work on this? Or do you know somebody who can work on this?

	Yuan Xulei [:yxl] (Leave from May 18)
	Comment 1 • 10 years ago

Masayuki, thanks for providing details.
I'm afraid I don't have timeframe to work on this for the following 3 months, but I would like to help.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 2 • 10 years ago

Yuan:

Thank you for your reply. Okay, no problem.



I forgot the people who requested me to implement new KeyboardEvent API.

Chun-Min, Gina:

Can you work on this?

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 3 • 10 years ago

The change is desirable, but I can't figure out when we will be available for this either. Adding flags now so it can't be lost.

It sounds like we are looking at application-facing API changes as well, since the virtual keyboard would need to tell Gecko which KeyboardEvent to emit when the composition changes right? 

The more I think, the more I think we would have to add a D3D key string parameter to many methods in the API, presumably by with function overloading. Do we support function overloading in JavaScript implemented API?

	Gina Yeh [:gyeh] [:ginayeh]
	Comment 4 • 10 years ago

I'd like to, but I'm working on Presentation API and I still need a few months for it.

Do we have any targetted timeline for this one? Maybe 3.0?

	C.M.Chang[:chunmin]
	Comment 5 • 10 years ago

Hi, Masayuki
It sounds interesting to me. However, I am unfamiliar with CompositionManager. I think I need to spend some time studying how it works.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 6 • 9 years ago

Hi, thank you for your quick replies.

I think that this should be fixed before we allow 3rd party's keyboard/IME on Firefox OS. In other words, this does not cause any problems until that.

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #3)
> It sounds like we are looking at application-facing API changes as well,
> since the virtual keyboard would need to tell Gecko which KeyboardEvent to
> emit when the composition changes right? 

No. With the new API, JS-IME and/or JS-Keyboard should send KeyboardEvent always if actually a key in physical or virtual keyboard is pressed. Whether it should be fired or not is decided by the API automatically. If a passed KeyboardEvent shouldn't cause DOM KeyboardEvents actually, the event is just ignored.

> The more I think, the more I think we would have to add a D3D key string
> parameter to many methods in the API, presumably by with function
> overloading. Do we support function overloading in JavaScript implemented
> API?

I think that each method of CompositionManager should have an optional argument as its last argument. Then, just checking if the argument is undefined is okay.

For example,

If a call of setComposition() is caused by a key action, JS-IME should do:

CompositionManager.setComposition(
  element, "foo", "foo".length,
  [ { length: "foo".length, selectionType: Ci.nsITextInputProcessor.ATTR_RAW_CLAUSE } ],
  new KeyboardEvent("", { key: "o", code: "", keyCode: KeyboardEvent.DOM_VK_O }));

If it's not caused by a key action, e.g., tapping by an item in the candidate list:

CompositionManager.setComposition(
  element, "foo", "foo".length,
  [ { length: "foo".length, selectionType: Ci.nsITextInputProcessor.ATTR_RAW_CLAUSE } ]);

Although, I don't like that KeyboardEvent.keyCode is decided by JS-IME or JS-Keyboard because the rules of deciding it are very complicated and this is important for web compatibility.
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode#Printable_keys_in_standard_position

But we have no option in realistic scenarios.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 7 • 9 years ago

I intend to pick up this once I finish the downloadable dictionary feature. Please steal if anyone is available.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 8 • 9 years ago

This patch removes sendKeyEvent from forms.js and attempt to make no API changes. It will at least unblock bug 1134542.

Obviously it does not do all the things described in comment 0. I think we should split things in comment 0 into smaller chunks.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 9 • 9 years ago

Comment on attachment 8592746 [details] [diff] [review]
Untested WIP

>diff --git a/dom/inputmethod/forms.js b/dom/inputmethod/forms.js
>--- a/dom/inputmethod/forms.js
>+++ b/dom/inputmethod/forms.js
>+XPCOMUtils.defineLazyGetter(this, "textInputProcessor", function () {
>+  return Cc["@mozilla.org/text-input-processor;1"].
>+    createInstance(Ci.nsITextInputProcessor);
> });

>@@ -1231,27 +1258,16 @@ let CompositionManager =  {
>           break;
>       }
>     } catch (e) {
>       return false;
>     }
>     return true;
>   },
> 
>-  _prepareTextInputProcessor: function cm_prepareTextInputProcessor(aWindow)
>-  {
>-    if (!this._textInputProcessor) {
>-      this._textInputProcessor =
>-        Cc["@mozilla.org/text-input-processor;1"].
>-          createInstance(Ci.nsITextInputProcessor);
>-    }
>-    return this._textInputProcessor.beginInputTransaction(aWindow,
>-                                                          this._callback);
>-  },
>-
>   setComposition: function cm_setComposition(element, text, cursor, clauses) {
>     // Check parameters.
>     if (!element) {
>       return;
>     }
>     let len = text.length;
>     if (cursor > len) {
>       cursor = len;
>@@ -1280,39 +1296,39 @@ let CompositionManager =  {
>         clauseLens[clauseLens.length - 1] += remainingLength;
>       }
>     } else {
>       clauseLens.push(len);
>       clauseAttrs.push(Ci.nsITextInputProcessor.ATTR_RAW_CLAUSE);
>     }
> 
>     let win = element.ownerDocument.defaultView;
>-    if (!this._prepareTextInputProcessor(win)) {
>+    if (!textInputProcessor.beginInputTransaction(win, this._callback)) {
>       return;
>     }
>     // Update the composing text.
>-    this._textInputProcessor.setPendingCompositionString(text);
>+    textInputProcessor.setPendingCompositionString(text);
>     for (var i = 0; i < clauseLens.length; i++) {
>       if (!clauseLens[i]) {
>         continue;
>       }
>-      this._textInputProcessor.appendClauseToPendingComposition(clauseLens[i],
>-                                                                clauseAttrs[i]);
>+      textInputProcessor.appendClauseToPendingComposition(clauseLens[i],
>+                                                          clauseAttrs[i]);
>     }
>     if (cursor >= 0) {
>-      this._textInputProcessor.setCaretInPendingComposition(cursor);
>+      textInputProcessor.setCaretInPendingComposition(cursor);
>     }
>-    this._isStarted = this._textInputProcessor.flushPendingComposition();
>+    this._isStarted = textInputProcessor.flushPendingComposition();
>   },
> 
>   endComposition: function cm_endComposition(text) {
>     if (!this._isStarted) {
>       return;
>     }
>-    this._textInputProcessor.commitCompositionWith(text ? text : "");
>+    textInputProcessor.commitCompositionWith(text ? text : "");
>     this._isStarted = false;
>   },
> 
>   // Composition ends due to external actions.
>   onCompositionEnd: function cm_onCompositionEnd() {
>     if (!this._isStarted) {
>       return;
>     }

I'm not sure this is right way. If .Keydown() or .Keyup() are called during composition on another window which belongs to another top level widget, .beginInputTransaction() returns false.

So, nsITextInputProcessor shouldn't be shared between applications. Although, I don't know if form.js is loaded by each application.

>+          // For printable keys, the value should be the actual character.
>+          // For non-printable keys, it should be a value in the D3E spec.
>+          // We use "Unidentified" for all non-printable keys because we don't
>+          // have enough information here to figure out the correct value,
>+          // unless we construct a reverse table to map all possible keyCode
>+          // back to D3E key values.
>+          key: printable ? String.fromCharCode(json.charCode) : "Unidentified",

Using "Unidentified" isn't improved anything. I think that json.key should be available. If its type is string, new API should use it. Otherwise, fallback to this logic.

And I think that json should accept "code" for specifying KeyboardEvent.code.

>+        this._editing = true;
>+        textInputProcessor.keydown(keyEvent);
>+        textInputProcessor.keyup(keyEvent);

If the window has been closed, keyup may throw an exception. So, you should wrap it with try-catch.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 10 • 9 years ago

Updated untested patch. This is how I think forms.js should work with nsITextInputProcesser for now, according to the MDN documentation. I will test this next week (unless someone want to steal it from me).

The goal of this patch is to abstract the new interface into the old method(s). This should be part 1 of the series of Gecko patches:

1) Remove usage of domWindowUtils.sendKey and use nsITextInputProcesser to back all current APIs (this patch)
2) Expose necessary methods e.g. keydown(), keyup() etc. to the Gaia Keyboard app through MozInputContext so apps could provide finer control over the editor, including signaling virtual key presses when composition. The exact methods to expose are subject to discussion.
3) Maybe moving the focus management to be backed by nsITextInputProcesser as well, instead of binding the focus/blur events? I don't know.

Masayuki-san, please tell me if this is a good plan and cover everything you said in comment 0. Also I don't know if we should do this in 3 bugs or just on this one.

Particular for (3), I also need you help to understand if that's the intend use of notify-focus/notify-blur notification from nsITextInputProcessorNotification.

Thanks!

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 11 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> >   endComposition: function cm_endComposition(text) {
> >     if (!this._isStarted) {
> >       return;
> >     }
> >-    this._textInputProcessor.commitCompositionWith(text ? text : "");
> >+    textInputProcessor.commitCompositionWith(text ? text : "");
> >     this._isStarted = false;
> >   },
> > 
> >   // Composition ends due to external actions.
> >   onCompositionEnd: function cm_onCompositionEnd() {
> >     if (!this._isStarted) {
> >       return;
> >     }
> 
> I'm not sure this is right way. If .Keydown() or .Keyup() are called during
> composition on another window which belongs to another top level widget,
> .beginInputTransaction() returns false.
> 

I don't know what this code do actually and attempt to make no API changes in this first patch.

> So, nsITextInputProcessor shouldn't be shared between applications.
> Although, I don't know if form.js is loaded by each application.

forms.js is loaded in every <iframe mozbrowser>. What do you mean by "application" here? Are you referring to Gaia keyboard app which sits at the remote end of the mozInputMethod API? Or the content app that hosts the <input>?

> 
> >+          // For printable keys, the value should be the actual character.
> >+          // For non-printable keys, it should be a value in the D3E spec.
> >+          // We use "Unidentified" for all non-printable keys because we don't
> >+          // have enough information here to figure out the correct value,
> >+          // unless we construct a reverse table to map all possible keyCode
> >+          // back to D3E key values.
> >+          key: printable ? String.fromCharCode(json.charCode) : "Unidentified",
> 
> Using "Unidentified" isn't improved anything. I think that json.key should
> be available. If its type is string, new API should use it. Otherwise,
> fallback to this logic.

Certainly the new API should use it, but I can't make up that information from the argument of the old API.

the |json| object is constructed here. It simply contains whatever argument passed into sendKey() from the virtual keyboard app to the mozInputMethod API.

https://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/MozKeyboard.js#682-689

> And I think that json should accept "code" for specifying KeyboardEvent.code.

The new APIs should certainly ask vkb apps to compute and supply these information ( point (2) in comment 10)

> >+        this._editing = true;
> >+        textInputProcessor.keydown(keyEvent);
> >+        textInputProcessor.keyup(keyEvent);
> 
> If the window has been closed, keyup may throw an exception. So, you should
> wrap it with try-catch.

OK.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 12 • 9 years ago

I wonder if it's better if we arrange an IRC or Vidyo meeting, for me to go though how mozInputMethod API works? It would help us come up with the best interface and design between the API and nsITextInputProcesser.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 13 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #10)
> Created attachment 8593392 [details] [diff] [review]
> Untested WIP2
> 
> Updated untested patch. This is how I think forms.js should work with
> nsITextInputProcesser for now, according to the MDN documentation. I will
> test this next week (unless someone want to steal it from me).
> 
> The goal of this patch is to abstract the new interface into the old
> method(s). This should be part 1 of the series of Gecko patches:
> 
> 1) Remove usage of domWindowUtils.sendKey and use nsITextInputProcesser to
> back all current APIs (this patch)
> 2) Expose necessary methods e.g. keydown(), keyup() etc. to the Gaia
> Keyboard app through MozInputContext so apps could provide finer control
> over the editor, including signaling virtual key presses when composition.
> The exact methods to expose are subject to discussion.
> 3) Maybe moving the focus management to be backed by nsITextInputProcesser
> as well, instead of binding the focus/blur events? I don't know.
> 
> Masayuki-san, please tell me if this is a good plan and cover everything you
> said in comment 0. Also I don't know if we should do this in 3 bugs or just
> on this one.

I think that separating bugs should be okay. However, for compatibility reason, we should perform all API changes in a release.

> Particular for (3), I also need you help to understand if that's the intend
> use of notify-focus/notify-blur notification from
> nsITextInputProcessorNotification.

Yeah, okay. Although, I need to update nsITextInputProcessor implementation, we should manage it in C++ level because JS cannot retrieve some information, e.g., if the <input> or something actually IME available.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 14 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #11)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> > >   endComposition: function cm_endComposition(text) {
> > >     if (!this._isStarted) {
> > >       return;
> > >     }
> > >-    this._textInputProcessor.commitCompositionWith(text ? text : "");
> > >+    textInputProcessor.commitCompositionWith(text ? text : "");
> > >     this._isStarted = false;
> > >   },
> > > 
> > >   // Composition ends due to external actions.
> > >   onCompositionEnd: function cm_onCompositionEnd() {
> > >     if (!this._isStarted) {
> > >       return;
> > >     }
> > 
> > I'm not sure this is right way. If .Keydown() or .Keyup() are called during
> > composition on another window which belongs to another top level widget,
> > .beginInputTransaction() returns false.
> > 
> 
> I don't know what this code do actually and attempt to make no API changes
> in this first patch.

First of all, nsIDOMWindowUtils.send*() are not stateful. I.e., you can dispatch any event in any situation.

On the other hand, nsITextInputProcessor is stateful and its implementation manages composition state. E.g., all widgets belonging to same top level widget can have only one composition. And also if dispatching key events while there is composition may be ignored (i.e., keydown, keyup events won't be fired but not throw exception).

If applications are run in a top level widget on B2G, only one nsITextInputProcessor can create composition at same time. This means that if an application is hidden by "Home" button when it has composition and if we fail to commit/cancel the composition at hiding the application, other applications cannot use nsITextInputProcessor because nsITextInputProcessor.beginInputTransaction() returns if it's another instance or throwing exception if it has the owner of another application's composition.

So, we need to ensure that composition is committed or canceled at switching application.

> > So, nsITextInputProcessor shouldn't be shared between applications.
> > Although, I don't know if form.js is loaded by each application.
> 
> forms.js is loaded in every <iframe mozbrowser>. What do you mean by
> "application" here? Are you referring to Gaia keyboard app which sits at the
> remote end of the mozInputMethod API? Or the content app that hosts the
> <input>?

I meant same as users. I.e., open by tab an application's icon, closed (hidden) by "Home" button or "Back" button. Perhaps, the last one is what I think.

> > >+          // For printable keys, the value should be the actual character.
> > >+          // For non-printable keys, it should be a value in the D3E spec.
> > >+          // We use "Unidentified" for all non-printable keys because we don't
> > >+          // have enough information here to figure out the correct value,
> > >+          // unless we construct a reverse table to map all possible keyCode
> > >+          // back to D3E key values.
> > >+          key: printable ? String.fromCharCode(json.charCode) : "Unidentified",
> > 
> > Using "Unidentified" isn't improved anything. I think that json.key should
> > be available. If its type is string, new API should use it. Otherwise,
> > fallback to this logic.
> 
> Certainly the new API should use it, but I can't make up that information
> from the argument of the old API.
> 
> the |json| object is constructed here. It simply contains whatever argument
> passed into sendKey() from the virtual keyboard app to the mozInputMethod
> API.
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/MozKeyboard.
> js#682-689

Oh, I understand.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 15 • 9 years ago

Thanks for the reply. I will dive into this bug next week or late this week.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 16 • 9 years ago

This is the updated and tested Part I of the patch, which replaces domWindowUtils and rely on textInputProcesser for sendKey().

This mostly work except that I can no longer tell if the edit is caused by ourselves from nsIEditor. Before this patch we can tell that by setting the |this._editing| flag and have the nsIEditorObserver callback |EditAction| to check that. The timing of the callback no longer make the trick work.

With this patch, |EditAction| will cause the Gaia keyboard to reset the input status on every sendKey() call.

:masayuki, do you know if there is any alternative way for this?

==

The more I write the more I think forms.js warrants a big requirement analysis and reimplementation -- not sure if it's worth but I am going to translate the behavior of the code into a requirement document first.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 17 • 9 years ago

Comment on attachment 8599697 [details] [diff] [review]
WIP Part 1 v1.0

>+// This object implements nsITextInputProcessorCallback
>+let textInputProcessorCallback = {
>+  onNotify: function(aTextInputProcessor, aNotification) {
>+    try {
>+      switch (aNotification.type) {
>+        case "request-to-commit":
>+          // TODO: Send a notification through asyncMessage to the keyboard here.
>+          aTextInputProcessor.commitComposition();
>+
>+          break;
>+        case "request-to-cancel":
>+          // TODO: Send a notification through asyncMessage to the keyboard here.
>+          aTextInputProcessor.cancelComposition();
>+
>+          break;
>+
>+        case "notify-detached":
>+          // TODO: Send a notification through asyncMessage to the keyboard here.
>+          break;
>+
>+        // TODO: Manage _focusedElement for text input from here instead.
>+        //       (except for <select> which will be need to handled else where)
>+        case "notify-focus":
>+          break;
>+
>+        case "notify-blur":
>+          break;

FYI: When an editor already has focus at beginning a transaction, notify-focus won't be notified until next editor gets focus. This is a bug, I'll file and fix that.

Perhaps, notify-blur should be immediately before detached?

>       case "Forms:Input:SendKey":
>         CompositionManager.endComposition('');
> 
>-        let flags = domWindowUtils.KEY_FLAG_NOT_SYNTHESIZED_FOR_TESTS;
>-        this._editing = true;
>-        let doKeypress = domWindowUtils.sendKeyEvent('keydown', json.keyCode,
>-                                                     json.charCode, json.modifiers, flags);
>-        if (doKeypress) {
>-          domWindowUtils.sendKeyEvent('keypress', json.keyCode,
>-                                      json.charCode, json.modifiers, flags);
>+        let win = this._focusedElement.ownerDocument.defaultView;
>+        if (!textInputProcessor.beginInputTransaction(win, textInputProcessorCallback)) {
>+          if (json.requestId) {
>+            sendAsyncMessage("Forms:SendKey:Result:Error", {
>+              requestId: json.requestId,
>+              error: "Unable to start componsition."
>+            });
>+          }
>+
>+          return;
>         }

Could you check following steps work well?

1. Start composition on an application (and don't commit the composition)
2. Switch application to another one (e.g., using "Home" button or something)
3. Start composition or type a key on the new one.
4. Switch back to the old one.
5. Start composition or type a key on the old one.

Then, both #3 and #5 should be able to start composition and/or type keys. I guess that when application is being inactivated, remaining composition should be committed or canceled.

>-        if(!json.repeat) {
>-          domWindowUtils.sendKeyEvent('keyup', json.keyCode,
>-                                      json.charCode, json.modifiers, flags);
>+        // XXX: The inputContext.sendKey() method is deprecated because it is
>+        // not D3E aware.
>+        //
>+        // Any keys sent from this API does will not have the |key| property
>+        // properly set.
>+        win.console.warn("The sendKey() method is deprecated in favor of " +
>+          "keydown() and keyup() methods.");

Do you mean that you'll add new API keydown() and keyup() for the keyboard?

>+        // The naive way to figure out if the key to dispatch is printable.
>+        let printable = !!json.charCode;
>+
>+        let keyboardEvent = new win.KeyboardEvent("", {
>+          // For printable keys, the value should be the actual character.
>+          // For non-printable keys, it should be a value in the D3E spec.
>+          // We use "Unidentified" for all non-printable keys because we don't
>+          // have enough information here to figure out the correct value,
>+          // unless we construct a reverse table to map all possible keyCode
>+          // back to D3E key values.
>+          key: printable ? String.fromCharCode(json.charCode) : "Unidentified",

Almost all of defined keyCode values are simply mappable to non-printable key value. So, it should do it in another patch (or another bug).

This function of EventUtils.js for mochitest is probably enough for now:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js?rev=f06affb13067#978

Although, with new API, .key value should be explicitly specified by keyboard or IME.

>+          // Likewise, we don't have any information to figure out the value of
>+          // code in this method.
>+          code: "",

Yes, and if the VKB doesn't emulate physical keyboard, the code value should be "".

Otherwise, i.e., emulating physical keyboard like QWERTY, AZERTY, Dvorak or 10 key of simple phone, the code value should be properly set. But it's not so important for now because we are the only browser to support KeyboardEvent.code.

>+          // For printable keys, the keyCode should be a value from
>+          // mdn.io/keyCode#Printable_keys_in_standard_position.

Yep, but we don't have any concern about mobile VKB's keyCode.

>+          // For non-printable keys it should be left unset
>+          // for textInputProcessor to compute from the |key| property.
>+          //
>+          // We violate the spec here to keep the original behavior also because
>+          // the fact textInputProcessor can't compute non-printable keys from
>+          // "Unidentified".
>+          keyCode: json.keyCode,

If keyboard or IME specify a specific keyCode value explicitly, we should use it even with new API. But basically, only .key value should be specified and .keyCode should be computed automatically.

So, this is no problem even with new API.

>+          // We do not have information to infer location either (and we would
>+          // need textInputProcessor not to compute it).
>+          location: 0,

0 doesn't cause any problem. If .code value is specified, TIP will be able to compute proper value.

>+          // This indicates the key is triggered for repeats.
>+          repeat: json.repeat
>+        });
>+
>+        let flags = textInputProcessor.KEY_KEEP_KEY_LOCATION_STANDARD;

I don't think that this is necessary because .code is always "". So, .location will be never computed by TIP.

>+        if (printable) {
>+          flags &= textInputProcessor.KEY_NON_PRINTABLE_KEY;
>         }

I think that this should be:

if (!printable) {
  flags |= textInputProcessor.KEY_NON_PRINTABLE_KEY;
}

This prevents to input unsupported key name as text. E.g., "Return" is specified, "R", "e", "t", "u", "r" and "n" will be inputted. If the flag is set, this causes an exception.

>+        this._editing = true;
>+        let notDefaultPrevented;
>+        try {
>+          notDefaultPrevented =
>+            textInputProcessor.keydown(keyboardEvent, flags);
>+          if (!json.repeat) {
>+            textInputProcessor.keyup(keyboardEvent, flags);
>+          }
>+        } catch (e) {
>+          dump("forms.js:" + e.toString() + "\n");
>+
>+          this._editing = false;
>+
>+          if (json.requestId) {
>+            sendAsyncMessage("Forms:SendKey:Result:Error", {
>+              requestId: json.requestId,
>+              error: "Unable to type into destoryed input."
>+            });
>+          }
>+
>+          return;
>+        }
>         this._editing = false;
> 
>-        if (json.requestId && doKeypress) {
>-          sendAsyncMessage("Forms:SendKey:Result:OK", {
>-            requestId: json.requestId,
>-            selectioninfo: this.getSelectionInfo()
>-          });
>+        if (json.requestId) {
>+          if (!notDefaultPrevented) {
>+            sendAsyncMessage("Forms:SendKey:Result:Error", {
>+              requestId: json.requestId,
>+              error: "Key event(s) was cancelled."
>+            });

error? it's usual behavior of web applications...

>+          } else {
>+            sendAsyncMessage("Forms:SendKey:Result:OK", {
>+              requestId: json.requestId,
>+              selectioninfo: this.getSelectionInfo()
>+            });
>+          }
>         }
>-        else if (json.requestId && !doKeypress) {
>-          sendAsyncMessage("Forms:SendKey:Result:Error", {
>-            requestId: json.requestId,
>-            error: "Keydown event got canceled"
>-          });

Oh, but current code claims it's an error...

I'll check your previous comment soon, please wait.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 18 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #16)
> Created attachment 8599697 [details] [diff] [review]
> WIP Part 1 v1.0
> 
> This is the updated and tested Part I of the patch, which replaces
> domWindowUtils and rely on textInputProcesser for sendKey().

Thank you for your great work!

> This mostly work except that I can no longer tell if the edit is caused by
> ourselves from nsIEditor. Before this patch we can tell that by setting the
> |this._editing| flag and have the nsIEditorObserver callback |EditAction| to
> check that. The timing of the callback no longer make the trick work.

I don't understand why that occurs. TextInputProcessor dispatches events synchronously. What's the problem??

> The more I write the more I think forms.js warrants a big requirement
> analysis and reimplementation -- not sure if it's worth but I am going to
> translate the behavior of the code into a requirement document first.

Yeah, we need a document.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 19 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 5/2 - 5/6) from comment #17)
> Comment on attachment 8599697 [details] [diff] [review]
> WIP Part 1 v1.0
> 
> >+// This object implements nsITextInputProcessorCallback
> >+let textInputProcessorCallback = {
> >+  onNotify: function(aTextInputProcessor, aNotification) {
> >+    try {
> >+      switch (aNotification.type) {
> >+        case "request-to-commit":
> >+          // TODO: Send a notification through asyncMessage to the keyboard here.
> >+          aTextInputProcessor.commitComposition();
> >+
> >+          break;
> >+        case "request-to-cancel":
> >+          // TODO: Send a notification through asyncMessage to the keyboard here.
> >+          aTextInputProcessor.cancelComposition();
> >+
> >+          break;
> >+
> >+        case "notify-detached":
> >+          // TODO: Send a notification through asyncMessage to the keyboard here.
> >+          break;
> >+
> >+        // TODO: Manage _focusedElement for text input from here instead.
> >+        //       (except for <select> which will be need to handled else where)
> >+        case "notify-focus":
> >+          break;
> >+
> >+        case "notify-blur":
> >+          break;
> 
> FYI: When an editor already has focus at beginning a transaction,
> notify-focus won't be notified until next editor gets focus. This is a bug,
> I'll file and fix that.
> 
> Perhaps, notify-blur should be immediately before detached?
> 

If we ended up using these notifications for focus management, I would imaging the transaction should begin when the forms.js loads. We should be able to hide this behavior behind that...

... any way, let's plan this in the requirement document.

> >       case "Forms:Input:SendKey":
> >         CompositionManager.endComposition('');
> > 
> >-        let flags = domWindowUtils.KEY_FLAG_NOT_SYNTHESIZED_FOR_TESTS;
> >-        this._editing = true;
> >-        let doKeypress = domWindowUtils.sendKeyEvent('keydown', json.keyCode,
> >-                                                     json.charCode, json.modifiers, flags);
> >-        if (doKeypress) {
> >-          domWindowUtils.sendKeyEvent('keypress', json.keyCode,
> >-                                      json.charCode, json.modifiers, flags);
> >+        let win = this._focusedElement.ownerDocument.defaultView;
> >+        if (!textInputProcessor.beginInputTransaction(win, textInputProcessorCallback)) {
> >+          if (json.requestId) {
> >+            sendAsyncMessage("Forms:SendKey:Result:Error", {
> >+              requestId: json.requestId,
> >+              error: "Unable to start componsition."
> >+            });
> >+          }
> >+
> >+          return;
> >         }
> 
> Could you check following steps work well?
> 
> 1. Start composition on an application (and don't commit the composition)
> 2. Switch application to another one (e.g., using "Home" button or something)
> 3. Start composition or type a key on the new one.
> 4. Switch back to the old one.
> 5. Start composition or type a key on the old one.
> 
> Then, both #3 and #5 should be able to start composition and/or type keys. I
> guess that when application is being inactivated, remaining composition
> should be committed or canceled.

Yes I tried, both #3 and #5 works however when I go back to #5 the original composition is not committed but remain on screen.

> 
> >-        if(!json.repeat) {
> >-          domWindowUtils.sendKeyEvent('keyup', json.keyCode,
> >-                                      json.charCode, json.modifiers, flags);
> >+        // XXX: The inputContext.sendKey() method is deprecated because it is
> >+        // not D3E aware.
> >+        //
> >+        // Any keys sent from this API does will not have the |key| property
> >+        // properly set.
> >+        win.console.warn("The sendKey() method is deprecated in favor of " +
> >+          "keydown() and keyup() methods.");
> 
> Do you mean that you'll add new API keydown() and keyup() for the keyboard?

Yes, that's what I intend to do in the next series of patches.

> 
> >+        // The naive way to figure out if the key to dispatch is printable.
> >+        let printable = !!json.charCode;
> >+
> >+        let keyboardEvent = new win.KeyboardEvent("", {
> >+          // For printable keys, the value should be the actual character.
> >+          // For non-printable keys, it should be a value in the D3E spec.
> >+          // We use "Unidentified" for all non-printable keys because we don't
> >+          // have enough information here to figure out the correct value,
> >+          // unless we construct a reverse table to map all possible keyCode
> >+          // back to D3E key values.
> >+          key: printable ? String.fromCharCode(json.charCode) : "Unidentified",
> 
> Almost all of defined keyCode values are simply mappable to non-printable
> key value. So, it should do it in another patch (or another bug).
> 
> This function of EventUtils.js for mochitest is probably enough for now:
> http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/
> SimpleTest/EventUtils.js?rev=f06affb13067#978
> 

I will steal this function :)

> Although, with new API, .key value should be explicitly specified by
> keyboard or IME.

Certainly.

> 
> >+          // Likewise, we don't have any information to figure out the value of
> >+          // code in this method.
> >+          code: "",
> 
> Yes, and if the VKB doesn't emulate physical keyboard, the code value should
> be "".
> 
> Otherwise, i.e., emulating physical keyboard like QWERTY, AZERTY, Dvorak or
> 10 key of simple phone, the code value should be properly set. But it's not
> so important for now because we are the only browser to support
> KeyboardEvent.code.
> 
> >+          // For printable keys, the keyCode should be a value from
> >+          // mdn.io/keyCode#Printable_keys_in_standard_position.
> 
> Yep, but we don't have any concern about mobile VKB's keyCode.

I will add this into the comment.

> 
> >+          // For non-printable keys it should be left unset
> >+          // for textInputProcessor to compute from the |key| property.
> >+          //
> >+          // We violate the spec here to keep the original behavior also because
> >+          // the fact textInputProcessor can't compute non-printable keys from
> >+          // "Unidentified".
> >+          keyCode: json.keyCode,
> 
> If keyboard or IME specify a specific keyCode value explicitly, we should
> use it even with new API. But basically, only .key value should be specified
> and .keyCode should be computed automatically.
> 
> So, this is no problem even with new API.
> 
> >+          // We do not have information to infer location either (and we would
> >+          // need textInputProcessor not to compute it).
> >+          location: 0,
> 
> 0 doesn't cause any problem. If .code value is specified, TIP will be able
> to compute proper value.
> 
> >+          // This indicates the key is triggered for repeats.
> >+          repeat: json.repeat
> >+        });
> >+
> >+        let flags = textInputProcessor.KEY_KEEP_KEY_LOCATION_STANDARD;
> 
> I don't think that this is necessary because .code is always "". So,
> .location will be never computed by TIP.
> 
> >+        if (printable) {
> >+          flags &= textInputProcessor.KEY_NON_PRINTABLE_KEY;
> >         }
> 
> I think that this should be:
> 
> if (!printable) {
>   flags |= textInputProcessor.KEY_NON_PRINTABLE_KEY;
> }
> 
> This prevents to input unsupported key name as text. E.g., "Return" is
> specified, "R", "e", "t", "u", "r" and "n" will be inputted. If the flag is
> set, this causes an exception.
> 

Thanks for spotting the logic error.

> >+        this._editing = true;
> >+        let notDefaultPrevented;
> >+        try {
> >+          notDefaultPrevented =
> >+            textInputProcessor.keydown(keyboardEvent, flags);
> >+          if (!json.repeat) {
> >+            textInputProcessor.keyup(keyboardEvent, flags);
> >+          }
> >+        } catch (e) {
> >+          dump("forms.js:" + e.toString() + "\n");
> >+
> >+          this._editing = false;
> >+
> >+          if (json.requestId) {
> >+            sendAsyncMessage("Forms:SendKey:Result:Error", {
> >+              requestId: json.requestId,
> >+              error: "Unable to type into destoryed input."
> >+            });
> >+          }
> >+
> >+          return;
> >+        }
> >         this._editing = false;
> > 
> >-        if (json.requestId && doKeypress) {
> >-          sendAsyncMessage("Forms:SendKey:Result:OK", {
> >-            requestId: json.requestId,
> >-            selectioninfo: this.getSelectionInfo()
> >-          });
> >+        if (json.requestId) {
> >+          if (!notDefaultPrevented) {
> >+            sendAsyncMessage("Forms:SendKey:Result:Error", {
> >+              requestId: json.requestId,
> >+              error: "Key event(s) was cancelled."
> >+            });
> 
> error? it's usual behavior of web applications...
> 
> >+          } else {
> >+            sendAsyncMessage("Forms:SendKey:Result:OK", {
> >+              requestId: json.requestId,
> >+              selectioninfo: this.getSelectionInfo()
> >+            });
> >+          }
> >         }
> >-        else if (json.requestId && !doKeypress) {
> >-          sendAsyncMessage("Forms:SendKey:Result:Error", {
> >-            requestId: json.requestId,
> >-            error: "Keydown event got canceled"
> >-          });
> 
> Oh, but current code claims it's an error...
> 
> I'll check your previous comment soon, please wait.


(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 5/2 - 5/6) from comment #18)
> (In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to
> queue) from comment #16)
> > Created attachment 8599697 [details] [diff] [review]
> > WIP Part 1 v1.0
> > 
> > This is the updated and tested Part I of the patch, which replaces
> > domWindowUtils and rely on textInputProcesser for sendKey().
> 
> Thank you for your great work!
> 
> > This mostly work except that I can no longer tell if the edit is caused by
> > ourselves from nsIEditor. Before this patch we can tell that by setting the
> > |this._editing| flag and have the nsIEditorObserver callback |EditAction| to
> > check that. The timing of the callback no longer make the trick work.
> 
> I don't understand why that occurs. TextInputProcessor dispatches events
> synchronously. What's the problem??

This is not the case for forms.js loaded in the content process. I tried to print the stack in the EditAction callback with |dump('forms.js' + (new Error().stack) + '\n');| and I can verify the function is called in the function loop of |keydown()| *only* for an input in the chrome process (i.e. the rocket bar). There must be an implementation bug between TIP and nsIEditor.

Would you be able to debug this? You might need a phone or build the Emulator...

> > The more I write the more I think forms.js warrants a big requirement
> > analysis and reimplementation -- not sure if it's worth but I am going to
> > translate the behavior of the code into a requirement document first.
> 
> Yeah, we need a document.

The WIP is here https://wiki.mozilla.org/User:Timdream/forms.js_requirements

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 20 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #19)
> > > This mostly work except that I can no longer tell if the edit is caused by
> > > ourselves from nsIEditor. Before this patch we can tell that by setting the
> > > |this._editing| flag and have the nsIEditorObserver callback |EditAction| to
> > > check that. The timing of the callback no longer make the trick work.
> > 
> > I don't understand why that occurs. TextInputProcessor dispatches events
> > synchronously. What's the problem??
> 
> This is not the case for forms.js loaded in the content process. I tried to
> print the stack in the EditAction callback with |dump('forms.js' + (new
> Error().stack) + '\n');| and I can verify the function is called in the
> function loop of |keydown()| *only* for an input in the chrome process (i.e.
> the rocket bar). There must be an implementation bug between TIP and
> nsIEditor.
> 
> Would you be able to debug this? You might need a phone or build the
> Emulator...
> 

Sigh, I should probably learn gdb eventually....

Before that, with only looking at dxr, I found this coming from the patch of bug 1146243, which is where  a nsIDOMWindowUtils.sendKeyEvent() call and a nsITextInputProcesser.keydown() call eventually deviates.

https://hg.mozilla.org/mozilla-central/diff/884d44a41ac7/widget/TextEventDispatcher.cpp

Going to printf() to find out this tomorrow.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 21 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #20)
> Going to printf() to find out this tomorrow.

I can now confirm I can workaround this issue by forcing TextEventDispatcher::DispatchEvent() to call widget->DispatchEvent() instead of widget->DispatchInputEvent().

However widget->DispatchEvent() will report the keypress event being preventDefaulted when it's not. Not being able to find where DispatchEvent() is implemented, I ended up overwriting aStatus there.

I will continue work on the next patches for now.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 22 • 9 years ago

I am now working on the second version of the patch uploaded, but still seeing bug 1161904.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 23 • 9 years ago

Hi :masayuki,

I have addressed the feedback on the first patch. This does not change a lot other than creating TIP for each window (and keep them in a WeakMap) and also try to guess the value of |key| property with a function stole from EventUtils.js.


(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #18)
> (In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to
> queue) from comment #16)
> > Created attachment 8599697 [details] [diff] [review]
> > WIP Part 1 v1.0
> > 
> > This is the updated and tested Part I of the patch, which replaces
> > domWindowUtils and rely on textInputProcesser for sendKey().
> 
> Thank you for your great work!
> 
> > This mostly work except that I can no longer tell if the edit is caused by
> > ourselves from nsIEditor. Before this patch we can tell that by setting the
> > |this._editing| flag and have the nsIEditorObserver callback |EditAction| to
> > check that. The timing of the callback no longer make the trick work.
> 
> I don't understand why that occurs. TextInputProcessor dispatches events
> synchronously. What's the problem??
> 

This is the top blocking issue here. I have confirmation that TIP does *not* dispatch events synchronously _if the page lives in a content process_. The event will go through APZ-related code from chrome process and back. Stacktrace attached (breakpoint was set at nsEditor.cpp:1838):

===

#0  nsEditor::NotifyEditorObservers (this=this@entry=0xb226ada0, 
    aNotification=nsEditor::eNotifyEditorObserversOfEnd)
    at /home/timdream/repo/gecko/mozilla-central/editor/libeditor/nsEditor.cpp:1838
#1  0xb5ab1400 in nsEditor::EndPlaceHolderTransaction (this=0xb226ada0)
    at /home/timdream/repo/gecko/mozilla-central/editor/libeditor/nsEditor.cpp:1002
#2  0xb5a8a1e6 in nsAutoPlaceHolderBatch::~nsAutoPlaceHolderBatch (
    this=0xbec30048, __in_chrg=<optimized out>)
    at /home/timdream/repo/gecko/mozilla-central/editor/libeditor/nsEditorUtils.h:40
#3  0xb5a8c886 in nsHTMLEditor::TypedText (this=this@entry=0xb226ada0, 
    aString=..., aAction=aAction@entry=nsPlaintextEditor::eTypedText)
    at /home/timdream/repo/gecko/mozilla-central/editor/libeditor/nsHTMLEditor.cpp:1029
#4  0xb5a9b84a in HandleKeyPressEvent (aKeyEvent=0xaedd57e0, this=0xb226ada0)
    at /home/timdream/repo/gecko/mozilla-central/editor/libeditor/nsHTMLEditor.cpp:691
#5  nsHTMLEditor::HandleKeyPressEvent (this=0xb226ada0, aKeyEvent=0xaedd57e0)
    at /home/timdream/repo/gecko/mozilla-central/editor/libeditor/nsHTMLEditor.cpp:583
#6  0xb5a8f412 in nsEditorEventListener::KeyPress (this=this@entry=0xb03c9fc0, 
    aKeyEvent=0xaedd57e0)
    at /home/timdream/repo/gecko/mozilla-central/editor/libeditor/nsEditorEventListener.cpp:628
#7  0xb5ab122a in nsEditorEventListener::HandleEvent (this=0xb03c9fc0, 
    aEvent=0xaedd5780)
    at /home/timdream/repo/gecko/mozilla-central/editor/libeditor/nsEditorEventListener.cpp:408
#8  0xb57bdd38 in mozilla::EventListenerManager::HandleEventSubType (
    this=this@entry=0xb0507940, aListener=<optimized out>, 
    aListener@entry=0xaea12040, aDOMEvent=0xaedd5780, 
    aCurrentTarget=aCurrentTarget@entry=0xb0243000)
    at /home/timdream/repo/gecko/mozilla-central/dom/events/EventListenerManager.cpp:995
#9  0xb57bdf68 in mozilla::EventListenerManager::HandleEventInternal (
    this=0xb0507940, aPresContext=0xb054c000, aEvent=0xbec30820, 
    aDOMEvent=aDOMEvent@entry=0xbec3051c, 
    aCurrentTarget=aCurrentTarget@entry=0xb0243000, 
    aEventStatus=aEventStatus@entry=0xbec30520)
    at /home/timdream/repo/gecko/mozilla-central/dom/events/EventListenerManager.cpp:1144
#10 0xb57be0fe in HandleEvent (aEventStatus=0xbec30520, 
    aCurrentTarget=0xb0243000, aDOMEvent=0xbec3051c, aEvent=<optimized out>, 
    aPresContext=<optimized out>, this=<optimized out>)
    at ../../dist/include/mozilla/EventListenerManager.h:330
#11 HandleEvent (aVisitor=..., this=<optimized out>, aCd=...)
    at /home/timdream/repo/gecko/mozilla-central/dom/events/EventDispatcher.cpp:209
#12 mozilla::EventTargetChainItem::HandleEvent (this=<optimized out>, 
    aVisitor=..., aCd=...)
    at /home/timdream/repo/gecko/mozilla-central/dom/events/EventDispatcher.cpp:188
#13 0xb57be31e in mozilla::EventTargetChainItem::HandleEventTargetChain (
    aChain=..., aVisitor=..., aCallback=aCallback@entry=0xbec30644, aCd=...)
    at /home/timdream/repo/gecko/mozilla-central/dom/events/EventDispatcher.cpp:320
#14 0xb57be2ca in mozilla::EventTargetChainItem::HandleEventTargetChain (
    aChain=..., aVisitor=..., aCallback=aCallback@entry=0xbec30644, aCd=...)
    at /home/timdream/repo/gecko/mozilla-central/dom/events/EventDispatcher.cpp:351
#15 0xb57be728 in mozilla::EventDispatcher::Dispatch (
    aTarget=aTarget@entry=0xb04bcdd0, aPresContext=<optimized out>, 
    aEvent=aEvent@entry=0xbec30820, aDOMEvent=aDOMEvent@entry=0x0, 
    aEventStatus=aEventStatus@entry=0xbec307b8, 
    aCallback=aCallback@entry=0xbec30644, aTargets=aTargets@entry=0x0)
    at /home/timdream/repo/gecko/mozilla-central/dom/events/EventDispatcher.cpp:636
#16 0xb5b7511c in PresShell::HandleKeyboardEvent (this=this@entry=0xb21d7260, 
    aTarget=aTarget@entry=0xb04bcdd0, aEvent=..., 
    aEmbeddedCancelled=aEmbeddedCancelled@entry=false, 
    aStatus=aStatus@entry=0xbec307b8, aEventCB=aEventCB@entry=0xbec30644)
    at /home/timdream/repo/gecko/mozilla-central/layout/base/nsPresShell.cpp:6899
#17 0xb5b75200 in PresShell::DispatchEventToDOM (this=this@entry=0xb21d7260, 
    aEvent=aEvent@entry=0xbec30820, aStatus=aStatus@entry=0xbec307b8, 
    aEventCB=aEventCB@entry=0xbec30644)
    at /home/timdream/repo/gecko/mozilla-central/layout/base/nsPresShell.cpp:8014
#18 0xb5b76858 in PresShell::HandleEventInternal (this=this@entry=0xb21d7260, 
    aEvent=aEvent@entry=0xbec30820, aStatus=aStatus@entry=0xbec307b8)
    at /home/timdream/repo/gecko/mozilla-central/layout/base/nsPresShell.cpp:7911
#19 0xb5b778e4 in PresShell::HandleEvent (this=0xb21d7260, 
    aFrame=<optimized out>, aEvent=0xbec30820, 
    aDontRetargetEvents=<optimized out>, aEventStatus=0xbec307b8, 
    aTargetContent=0x0)
    at /home/timdream/repo/gecko/mozilla-central/layout/base/nsPresShell.cpp:7635
#20 0xb5a575bc in nsViewManager::DispatchEvent (this=<optimized out>, 
    aEvent=aEvent@entry=0xbec30820, aView=aView@entry=0xb05ac920, 
    aStatus=aStatus@entry=0xbec307b8)
    at ../../../gecko/mozilla-central/view/nsViewManager.cpp:786
#21 0xb5a56b36 in nsView::HandleEvent (this=<optimized out>, aEvent=0xbec30820, 
    aUseAttachedEvents=<optimized out>)
    at ../../../gecko/mozilla-central/view/nsView.cpp:1094
#22 0xb5a5e734 in mozilla::widget::PuppetWidget::DispatchEvent (
    this=0xb2251820, event=0xbec30820, 
    aStatus=@0xbec307fc: nsEventStatus_eIgnore)
    at /home/timdream/repo/gecko/mozilla-central/widget/PuppetWidget.cpp:317
#23 0xb52d9882 in mozilla::layers::APZCCallbackHelper::DispatchWidgetEvent (
    aEvent=...)
    at /home/timdream/repo/gecko/mozilla-central/gfx/layers/apz/util/APZCCallbackHelper.cpp:443
#24 0xb59a6784 in mozilla::dom::TabChild::RecvRealKeyEvent (this=0xb0e4ec00, 
    event=..., aBindings=...)
    at /home/timdream/repo/gecko/mozilla-central/dom/ipc/TabChild.cpp:2562
#25 0xb51395b8 in mozilla::dom::PBrowserChild::OnMessageReceived (
    this=0xb0e4ed74, msg__=...) at PBrowserChild.cpp:3264
#26 0xb5175eb8 in mozilla::dom::PContentChild::OnMessageReceived (
    this=0xb4064c18, msg__=...) at PContentChild.cpp:5435
#27 0xb5082890 in mozilla::ipc::MessageChannel::DispatchAsyncMessage (
    this=0xb4064c50, aMsg=...)
    at /home/timdream/repo/gecko/mozilla-central/ipc/glue/MessageChannel.cpp:1279
#28 0xb5086d8e in mozilla::ipc::MessageChannel::DispatchMessage (
    this=0xb4064c50, aMsg=...)
    at /home/timdream/repo/gecko/mozilla-central/ipc/glue/MessageChannel.cpp:1198
#29 0xb508891e in mozilla::ipc::MessageChannel::OnMaybeDequeueOne (
    this=<optimized out>)
    at /home/timdream/repo/gecko/mozilla-central/ipc/glue/MessageChannel.cpp:1182
#30 0xb4f04724 in DispatchToMethod<FdWatcher, void (FdWatcher::*)()> (
    method=(void (FdWatcher::*)(FdWatcher * const)) 0xb50888a3 <mozilla::ipc::MessageChannel::OnMaybeDequeueOne()>, obj=<optimized out>, arg=...)
    at ../../../../gecko/mozilla-central/ipc/chromium/src/base/tuple.h:387
#31 RunnableMethod<FdWatcher, void (FdWatcher::*)(), Tuple0>::Run (
    this=<optimized out>)
    at ../../../../gecko/mozilla-central/ipc/chromium/src/base/task.h:310
#32 0xb50810c6 in Run (this=<optimized out>)
    at ../../dist/include/mozilla/ipc/MessageChannel.h:446
#33 mozilla::ipc::MessageChannel::DequeueTask::Run (this=<optimized out>)
    at ../../dist/include/mozilla/ipc/MessageChannel.h:463
#34 0xb5074e14 in MessageLoop::RunTask (this=0xbec30e50, task=0xaedace38)
    at /home/timdream/repo/gecko/mozilla-central/ipc/chromium/src/base/message_loop.cc:364
#35 0xb50774ea in MessageLoop::DeferOrRunPendingTask (this=<optimized out>, 
    pending_task=...)
    at /home/timdream/repo/gecko/mozilla-central/ipc/chromium/src/base/message_loop.cc:372
#36 0xb5079130 in DoWork (this=<optimized out>)
    at /home/timdream/repo/gecko/mozilla-central/ipc/chromium/src/base/message_loop.cc:459
#37 MessageLoop::DoWork (this=0xbec30e50)
    at /home/timdream/repo/gecko/mozilla-central/ipc/chromium/src/base/message_loop.cc:438
#38 0xb50817f8 in mozilla::ipc::DoWorkRunnable::Run (this=<optimized out>)
    at /home/timdream/repo/gecko/mozilla-central/ipc/glue/MessagePump.cpp:220
#39 0xb4f2a21c in nsThread::ProcessNextEvent (this=0xb40025c0, 
    aMayWait=<optimized out>, aResult=0xbec30d57)
    at /home/timdream/repo/gecko/mozilla-central/xpcom/threads/nsThread.cpp:848
#40 0xb4f3ac9a in NS_ProcessNextEvent (aThread=<optimized out>, 
    aMayWait=aMayWait@entry=false)
    at /home/timdream/repo/gecko/mozilla-central/xpcom/glue/nsThreadUtils.cpp:265
#41 0xb5083b64 in mozilla::ipc::MessagePump::Run (this=0xb4055580, 
    aDelegate=0xbec30e50)
    at /home/timdream/repo/gecko/mozilla-central/ipc/glue/MessagePump.cpp:95
#42 0xb5074da0 in MessageLoop::RunInternal (this=this@entry=0xbec30e50)
    at /home/timdream/repo/gecko/mozilla-central/ipc/chromium/src/base/message_loop.cc:234
#43 0xb5074e54 in RunHandler (this=0xbec30e50)
    at /home/timdream/repo/gecko/mozilla-central/ipc/chromium/src/base/message_loop.cc:227
#44 MessageLoop::Run (this=0xbec30e50)
    at /home/timdream/repo/gecko/mozilla-central/ipc/chromium/src/base/message_loop.cc:201
#45 0xb5a608a6 in nsBaseAppShell::Run (this=0xb0fa0f40)
    at /home/timdream/repo/gecko/mozilla-central/widget/nsBaseAppShell.cpp:165
#46 0xb5dd97fe in XRE_RunAppShell ()
    at ../../../../gecko/mozilla-central/toolkit/xre/nsEmbedFunctions.cpp:778
#47 0xb5074da0 in MessageLoop::RunInternal (this=this@entry=0xbec30e50)
    at /home/timdream/repo/gecko/mozilla-central/ipc/chromium/src/base/message_loop.cc:234
#48 0xb5074e54 in RunHandler (this=0xbec30e50)
    at /home/timdream/repo/gecko/mozilla-central/ipc/chromium/src/base/message_loop.cc:227
#49 MessageLoop::Run (this=this@entry=0xbec30e50)
    at /home/timdream/repo/gecko/mozilla-central/ipc/chromium/src/base/message_loop.cc:201
#50 0xb5dd9cd2 in XRE_InitChildProcess (aArgc=<optimized out>, 
    aArgv=<optimized out>, aGMPLoader=<optimized out>)
    at ../../../../gecko/mozilla-central/toolkit/xre/nsEmbedFunctions.cpp:614
#51 0xb6f08844 in content_process_main (argc=7, argv=0xbec317e4)
    at ../../../../gecko/mozilla-central/ipc/app/../contentproc/plugin-container.cpp:236
#52 0xb6e434a4 in __libc_init (raw_args=0xbec317e0, onexit=<optimized out>, 
    slingshot=0xb6f08859 <main(int, char**)>, structors=<optimized out>)
    at bionic/libc/bionic/libc_init_dynamic.cpp:112
#53 0xb6f08730 in _start ()


===

By using dump(), I've also verified the JS world will not hear the keydown/keypress/keyup event synchronously in FormAssistant#receiveMessage too. That also means TIP#keydown() doesn't really return a boolean indicating preventDefault() is called or not.

Is this a behavior I should workaround, or this is a bug in the native code? If so I will think of more complex way than a |this._editing| flag to tell if EditAction is called due to our own action or not. It is also concerning because the timing of that callback is different for forms.js in chrome process and content process -- it would be hard to cover both.

For comparison, below is the stacktrace at the same breakpoint in chrome process, in which TextInputProcessor::KeydownInternal synchronously trigger the observer:

===

#0  nsEditor::NotifyEditorObservers (this=this@entry=0xa8d2a480, 
    aNotification=nsEditor::eNotifyEditorObserversOfEnd)
    at /home/timdream/repo/gecko/mozilla-central/editor/libeditor/nsEditor.cpp:1838
#1  0xb574b400 in nsEditor::EndPlaceHolderTransaction (this=0xa8d2a480)
    at /home/timdream/repo/gecko/mozilla-central/editor/libeditor/nsEditor.cpp:1002
#2  0xb57241e6 in nsAutoPlaceHolderBatch::~nsAutoPlaceHolderBatch (
    this=0xbe8a376c, __in_chrg=<optimized out>)
    at /home/timdream/repo/gecko/mozilla-central/editor/libeditor/nsEditorUtils.h:40
#3  0xb574c0c6 in nsPlaintextEditor::TypedText (this=<optimized out>, 
    aString=..., aAction=nsPlaintextEditor::eTypedText)
    at /home/timdream/repo/gecko/mozilla-central/editor/libeditor/nsPlaintextEditor.cpp:435
#4  0xb574be78 in HandleKeyPressEvent (aKeyEvent=0xa785e2a0, this=0xa8d2a480)
    at /home/timdream/repo/gecko/mozilla-central/editor/libeditor/nsPlaintextEditor.cpp:413
#5  nsPlaintextEditor::HandleKeyPressEvent (this=0xa8d2a480, 
    aKeyEvent=0xa785e2a0)
    at /home/timdream/repo/gecko/mozilla-central/editor/libeditor/nsPlaintextEditor.cpp:348
#6  0xb5729412 in nsEditorEventListener::KeyPress (this=this@entry=0xa72ddcc0, 
    aKeyEvent=0xa785e2a0)
    at /home/timdream/repo/gecko/mozilla-central/editor/libeditor/nsEditorEventListener.cpp:628
#7  0xb574b22a in nsEditorEventListener::HandleEvent (this=0xa72ddcc0, 
    aEvent=0xa785e240)
    at /home/timdream/repo/gecko/mozilla-central/editor/libeditor/nsEditorEventListener.cpp:408
#8  0xb5457d38 in mozilla::EventListenerManager::HandleEventSubType (
    this=this@entry=0xb00df580, aListener=<optimized out>, 
    aListener@entry=0xa721e8b0, aDOMEvent=0xa785e240, 
    aCurrentTarget=aCurrentTarget@entry=0xad462740)
    at /home/timdream/repo/gecko/mozilla-central/dom/events/EventListenerManager.cpp:995
#9  0xb5457f68 in mozilla::EventListenerManager::HandleEventInternal (
    this=0xb00df580, aPresContext=0xae68e400, aEvent=0xbe8a4030, 
    aDOMEvent=aDOMEvent@entry=0xbe8a3c0c, 
    aCurrentTarget=aCurrentTarget@entry=0xad462740, 
    aEventStatus=aEventStatus@entry=0xbe8a3c10)
    at /home/timdream/repo/gecko/mozilla-central/dom/events/EventListenerManager.cpp:1144
#10 0xb54580fe in HandleEvent (aEventStatus=0xbe8a3c10, 
    aCurrentTarget=0xad462740, aDOMEvent=0xbe8a3c0c, aEvent=<optimized out>, 
    aPresContext=<optimized out>, this=<optimized out>)
    at ../../dist/include/mozilla/EventListenerManager.h:330
#11 HandleEvent (aVisitor=..., this=<optimized out>, aCd=...)
    at /home/timdream/repo/gecko/mozilla-central/dom/events/EventDispatcher.cpp:209
#12 mozilla::EventTargetChainItem::HandleEvent (this=<optimized out>, 
    aVisitor=..., aCd=...)
    at /home/timdream/repo/gecko/mozilla-central/dom/events/EventDispatcher.cpp:188
#13 0xb5458312 in mozilla::EventTargetChainItem::HandleEventTargetChain (
    aChain=..., aVisitor=..., aCallback=aCallback@entry=0xbe8a3d34, aCd=...)
    at /home/timdream/repo/gecko/mozilla-central/dom/events/EventDispatcher.cpp:299
#14 0xb54582ca in mozilla::EventTargetChainItem::HandleEventTargetChain (
    aChain=..., aVisitor=..., aCallback=aCallback@entry=0xbe8a3d34, aCd=...)
    at /home/timdream/repo/gecko/mozilla-central/dom/events/EventDispatcher.cpp:351
#15 0xb5458728 in mozilla::EventDispatcher::Dispatch (
    aTarget=aTarget@entry=0xad462740, aPresContext=<optimized out>, 
    aEvent=aEvent@entry=0xbe8a4030, aDOMEvent=aDOMEvent@entry=0x0, 
    aEventStatus=aEventStatus@entry=0xbe8a3fc8, 
    aCallback=aCallback@entry=0xbe8a3d34, aTargets=aTargets@entry=0x0)
    at /home/timdream/repo/gecko/mozilla-central/dom/events/EventDispatcher.cpp:636
#16 0xb580f11c in PresShell::HandleKeyboardEvent (this=this@entry=0xabe725c0, 
    aTarget=aTarget@entry=0xad462740, aEvent=..., 
    aEmbeddedCancelled=aEmbeddedCancelled@entry=false, 
    aStatus=aStatus@entry=0xbe8a3fc8, aEventCB=aEventCB@entry=0xbe8a3d34)
    at /home/timdream/repo/gecko/mozilla-central/layout/base/nsPresShell.cpp:6899
#17 0xb580f200 in PresShell::DispatchEventToDOM (this=this@entry=0xabe725c0, 
    aEvent=aEvent@entry=0xbe8a4030, aStatus=aStatus@entry=0xbe8a3fc8, 
    aEventCB=aEventCB@entry=0xbe8a3d34)
    at /home/timdream/repo/gecko/mozilla-central/layout/base/nsPresShell.cpp:8014
#18 0xb5810858 in PresShell::HandleEventInternal (this=this@entry=0xabe725c0, 
    aEvent=aEvent@entry=0xbe8a4030, aStatus=aStatus@entry=0xbe8a3fc8)
    at /home/timdream/repo/gecko/mozilla-central/layout/base/nsPresShell.cpp:7911
#19 0xb58118e4 in PresShell::HandleEvent (this=0xabe725c0, 
    aFrame=<optimized out>, aEvent=0xbe8a4030, 
    aDontRetargetEvents=<optimized out>, aEventStatus=0xbe8a3fc8, 
    aTargetContent=0x0)
    at /home/timdream/repo/gecko/mozilla-central/layout/base/nsPresShell.cpp:7635
#20 0xb5811098 in PresShell::HandleEvent (this=<optimized out>, 
    aFrame=<optimized out>, aEvent=0xbe8a4030, 
    aDontRetargetEvents=<optimized out>, aEventStatus=0xbe8a3fc8, 
    aTargetContent=0x0)
    at /home/timdream/repo/gecko/mozilla-central/layout/base/nsPresShell.cpp:7152
#21 0xb56f15bc in nsViewManager::DispatchEvent (this=<optimized out>, 
    aEvent=aEvent@entry=0xbe8a4030, aView=aView@entry=0xaed83b50, 
    aStatus=aStatus@entry=0xbe8a3fc8)
    at ../../../gecko/mozilla-central/view/nsViewManager.cpp:786
#22 0xb56f0b36 in nsView::HandleEvent (this=<optimized out>, aEvent=0xbe8a4030, 
    aUseAttachedEvents=<optimized out>)
    at ../../../gecko/mozilla-central/view/nsView.cpp:1094
#23 0xb57160ec in nsWindow::DispatchEvent (this=0xaed886a0, 
    aEvent=<optimized out>, aStatus=@0xbe8a3ff4: 3196731440)
    at ../../../../gecko/mozilla-central/widget/gonk/nsWindow.cpp:562
#24 0xb56f1fba in nsBaseWidget::DispatchInputEvent (this=0xaed886a0, 
    aEvent=0xbe8a4030)
    at ../../../gecko/mozilla-central/widget/nsBaseWidget.cpp:1092
#25 0xb56f83fa in mozilla::widget::TextEventDispatcher::DispatchEvent (
    this=this@entry=0xa953fd90, aWidget=<optimized out>, aEvent=..., 
    aStatus=@0xbe8a4110: nsEventStatus_eIgnore)
    at /home/timdream/repo/gecko/mozilla-central/widget/TextEventDispatcher.cpp:152
#26 0xb56f8854 in DispatchKeyboardEventInternal (aIndexOfKeypress=0, 
    aStatus=@0xbe8a4110: nsEventStatus_eIgnore, aKeyboardEvent=..., 
    aMessage=131, this=0xa953fd90)
    at /home/timdream/repo/gecko/mozilla-central/widget/TextEventDispatcher.cpp:377
#27 mozilla::widget::TextEventDispatcher::DispatchKeyboardEventInternal (
    this=0xa953fd90, aMessage=131, aKeyboardEvent=..., 
    aStatus=@0xbe8a4110: nsEventStatus_eIgnore, aIndexOfKeypress=0)
    at /home/timdream/repo/gecko/mozilla-central/widget/TextEventDispatcher.cpp:295
#28 0xb56f88fc in mozilla::widget::TextEventDispatcher::MaybeDispatchKeypressEvents (this=0xa953fd90, aKeyboardEvent=..., 
    aStatus=@0xbe8a4110: nsEventStatus_eIgnore)
    at /home/timdream/repo/gecko/mozilla-central/widget/TextEventDispatcher.cpp:404
#29 0xb505213c in mozilla::TextInputProcessor::KeydownInternal (
    this=0xa7626d80, aKeyboardEvent=..., aKeyFlags=<optimized out>, 
    aAllowToDispatchKeypress=<optimized out>, aDoDefault=@0xbe8a4338: true)
    at /home/timdream/repo/gecko/mozilla-central/dom/base/TextInputProcessor.cpp:826
#30 0xb4bc80da in NS_InvokeByIndex (that=<optimized out>, 
    methodIndex=<optimized out>, paramCount=<optimized out>, 
    params=<optimized out>)
    at ../../../../../../../gecko/mozilla-central/xpcom/reflect/xptcall/md/unix/xptcinvoke_arm.cpp:163
#31 0xb4e64e4e in Invoke (this=0xbe8a42d8)
    at /home/timdream/repo/gecko/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:2080
#32 Call (this=0xbe8a42d8)
    at /home/timdream/repo/gecko/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:1417
#33 XPCWrappedNative::CallMethod (ccx=..., 
    mode=mode@entry=XPCWrappedNative::CALL_METHOD)
    at /home/timdream/repo/gecko/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:1384
#34 0xb4e6528c in XPC_WN_CallMethod (cx=0xb6b311b0, argc=2, vp=0xb2fdf0b0)
    at /home/timdream/repo/gecko/mozilla-central/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1144
#35 0xb5dda78e in CallJSNative (args=..., 
    native=0xb4e65199 <XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)>, cx=0xb6b311b0) at ../../../../gecko/mozilla-central/js/src/jscntxtinlines.h:235
#36 js::Invoke (cx=0xb6b311b0, args=..., construct=js::NO_CONSTRUCT)
    at /home/timdream/repo/gecko/mozilla-central/js/src/vm/Interpreter.cpp:711
#37 0xb5dd78c6 in Interpret (cx=0xb6b311b0, state=...)
    at /home/timdream/repo/gecko/mozilla-central/js/src/vm/Interpreter.cpp:2959
#38 0xb5dda512 in js::RunScript (cx=cx@entry=0xb6b311b0, state=...)
    at /home/timdream/repo/gecko/mozilla-central/js/src/vm/Interpreter.cpp:655
#39 0xb5dda7f4 in js::Invoke (cx=cx@entry=0xb6b311b0, args=..., 
    construct=js::NO_CONSTRUCT)
    at /home/timdream/repo/gecko/mozilla-central/js/src/vm/Interpreter.cpp:731
#40 0xb5ddaeb0 in js::Invoke (cx=cx@entry=0xb6b311b0, thisv=..., fval=..., 
    argc=argc@entry=1, argv=argv@entry=0xbe8a53f0, rval=rval@entry=...)
    at /home/timdream/repo/gecko/mozilla-central/js/src/vm/Interpreter.cpp:768
#41 0xb5f5e438 in JS_CallFunctionValue (cx=cx@entry=0xb6b311b0, obj=..., 
    obj@entry=..., fval=..., fval@entry=..., args=..., rval=rval@entry=...)
    at /home/timdream/repo/gecko/mozilla-central/js/src/jsapi.cpp:4560
#42 0xb501137e in nsFrameMessageManager::ReceiveMessage (this=0xaf644c40, 
    aTarget=aTarget@entry=0xae193c00, aTargetFrameLoader=0xacd45c40, 
    aTargetClosed=aTargetClosed@entry=false, aMessage=..., 
    aIsSync=aIsSync@entry=false, aCloneData=aCloneData@entry=0xbe8a551c, 
    aCpows=aCpows@entry=0xbe8a5528, aPrincipal=aPrincipal@entry=0x0, 
    aRetVal=aRetVal@entry=0x0)
    at ../../../../gecko/mozilla-central/dom/base/nsFrameMessageManager.cpp:1250
#43 0xb501165e in nsFrameMessageManager::ReceiveMessage (this=<optimized out>, 
    aTarget=aTarget@entry=0xae193c00, aTargetFrameLoader=<optimized out>, 
    aMessage=..., aIsSync=aIsSync@entry=false, 
    aCloneData=aCloneData@entry=0xbe8a551c, aCpows=aCpows@entry=0xbe8a5528, 
    aPrincipal=0x0, aRetVal=aRetVal@entry=0x0)
    at ../../../../gecko/mozilla-central/dom/base/nsFrameMessageManager.cpp:1067
#44 0xb5011f90 in nsSameProcessAsyncMessageBase::ReceiveMessage (
    this=this@entry=0xabf2d7e8, aTarget=aTarget@entry=0xae193c00, 
    aTargetFrameLoader=<optimized out>, aManager=0xaf644c40)
    at ../../../../gecko/mozilla-central/dom/base/nsFrameMessageManager.cpp:2217
#45 0xb5080f2e in nsAsyncMessageToChild::Run (this=0xabf2d7e0)
    at /home/timdream/repo/gecko/mozilla-central/dom/base/nsFrameLoader.cpp:2380
#46 0xb4bc421c in nsThread::ProcessNextEvent (this=0xb6b024e0, 
    aMayWait=<optimized out>, aResult=0xbe8a55df)
    at /home/timdream/repo/gecko/mozilla-central/xpcom/threads/nsThread.cpp:848
#47 0xb4bd4c9a in NS_ProcessNextEvent (aThread=<optimized out>, 
    aMayWait=aMayWait@entry=false)
    at /home/timdream/repo/gecko/mozilla-central/xpcom/glue/nsThreadUtils.cpp:265
#48 0xb4d1db64 in mozilla::ipc::MessagePump::Run (this=0xb6b558b0, 
    aDelegate=0xb2ddd1a0)
    at /home/timdream/repo/gecko/mozilla-central/ipc/glue/MessagePump.cpp:95
#49 0xb4d0eda0 in MessageLoop::RunInternal (this=this@entry=0xb2ddd1a0)
    at /home/timdream/repo/gecko/mozilla-central/ipc/chromium/src/base/message_loop.cc:234
#50 0xb4d0ee54 in RunHandler (this=0xb2ddd1a0)
    at /home/timdream/repo/gecko/mozilla-central/ipc/chromium/src/base/message_loop.cc:227
#51 MessageLoop::Run (this=0xb2ddd1a0)
    at /home/timdream/repo/gecko/mozilla-central/ipc/chromium/src/base/message_loop.cc:201
#52 0xb56fa8a6 in nsBaseAppShell::Run (this=0xb0911760)
    at /home/timdream/repo/gecko/mozilla-central/widget/nsBaseAppShell.cpp:165
#53 0xb5a4f302 in nsAppStartup::Run (this=0xb0942040)
    at /home/timdream/repo/gecko/mozilla-central/toolkit/components/startup/nsAppStartup.cpp:280
#54 0xb5a730ea in XREMain::XRE_mainRun (this=this@entry=0xbe8a5754)
    at ../../../../gecko/mozilla-central/toolkit/xre/nsAppRunner.cpp:4275
#55 0xb5a73328 in XREMain::XRE_main (this=this@entry=0xbe8a5754, 
    argc=argc@entry=1, argv=argv@entry=0xb6b2b188, 
    aAppData=aAppData@entry=0xb6fefc70 <_ZL8sAppData>)
    at ../../../../gecko/mozilla-central/toolkit/xre/nsAppRunner.cpp:4359
#56 0xb5a7349a in XRE_main (argc=1, argv=0xb6b2b188, 
    aAppData=0xb6fefc70 <_ZL8sAppData>, aFlags=<optimized out>)
    at ../../../../gecko/mozilla-central/toolkit/xre/nsAppRunner.cpp:4448
#57 0xb6fd183c in do_main (argc=argc@entry=1, argv=argv@entry=0xb6b2b188)
    at ../../../../gecko/mozilla-central/b2g/app/nsBrowserApp.cpp:167
#58 0xb6fd194a in b2g_main (argc=argc@entry=1, argv=argv@entry=0xbe8a6a14)
    at ../../../../gecko/mozilla-central/b2g/app/nsBrowserApp.cpp:299
#59 0xb6fd16cc in RunProcesses (aReservedFds=..., argv=0xbe8a6a14, argc=1)
    at ../../../../gecko/mozilla-central/b2g/app/B2GLoader.cpp:232
#60 main (argc=1, argv=0xbe8a6a14)
    at ../../../../gecko/mozilla-central/b2g/app/B2GLoader.cpp:297

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 24 • 9 years ago

So it looks like while TIP (specifically, TextEventDispatcher) dispatch the event in the same way, the content process dispatcher is getting PuppetWidget, while nsDOMWindowUtils will dispatch on somewhere else (have not yet use gdb to tell what it is). This is what sendKeyEvent() getting the widget to dispatch from window instance:

nsIWidget*
nsDOMWindowUtils::GetWidget(nsPoint* aOffset)
{
  nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
  if (window) {
    nsIDocShell *docShell = window->GetDocShell();
    if (docShell) {
      nsCOMPtr<nsIPresShell> presShell = docShell->GetPresShell();
      return nsContentUtils::GetWidget(presShell, aOffset);
    }
  }

  return nullptr;
}

nsIWidget*
nsContentUtils::GetWidget(nsIPresShell* aPresShell, nsPoint* aOffset) {
  if (aPresShell) {
    nsIFrame* frame = aPresShell->GetRootFrame();
    if (frame)
      return frame->GetView()->GetNearestWidget(aOffset);
  }
  return nullptr;
}

and TIP is determining the widget to dispatch here:

https://dxr.mozilla.org/mozilla-central/source/dom/base/TextInputProcessor.cpp#133-158

I will try to change this bit somehow and see if we could stop dispatch events to PuppetWidget.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 25 • 9 years ago

> This is the top blocking issue here. I have confirmation that TIP does *not* dispatch events
> synchronously _if the page lives in a content process_. The event will go through APZ-related code
> from chrome process and back. Stacktrace attached (breakpoint was set at nsEditor.cpp:1838):

Okay, I understand.

> Is this a behavior I should workaround, or this is a bug in the native code? If so I will think
> of more complex way than a |this._editing| flag to tell if EditAction is called due to our own
> action or not. It is also concerning because the timing of that callback is different for forms.js
> in chrome process and content process -- it would be hard to cover both.

I can say that it's a bug. When I test it with https://dvcs.w3.org/hg/d4e/raw-file/tip/key-event-test.html on desktop-Nightly (with e10s), preventDefault() of e10s works fine to me. I should investigate how we implement that and I guess that TIPCallback should notify IME of that.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 26 • 9 years ago

FYI:

You can paste with Ctrl + middle button of your mouse can paste the text as quoted (make middlemouse.paste true first). Then, you can prevent to redundant linebreaks in the stacktrace and source code.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 27 • 9 years ago

Wow, very hacky...
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=e17e313a3066&mark=2544-2549#2539

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 28 • 9 years ago

> Is this a behavior I should workaround, or this is a bug in the native code?

So, TIP.keydown() cannot return proper result synchronously if the dispatched event is sent to another process. So, we need workaround here.

> If so I will think of more complex way than a |this._editing| flag to tell if EditAction is called
> due to our own action or not.

I think so. But looks like that current forms.js isn't e10s-aware, isn't it? It sets _isEditing true before calls of nsIDOMWindowUtils.sendKey() and sets it false immediately after that. So, if edit action is notified asynchronously, _isEditing is false even if sending key event is caused.

Additionally, this is hacky and doesn't work well if web apps handle key events and sending some edit commands to the focused editor. So, anyway, it seems that around this is buggy...

> It is also concerning because the timing of that callback is different
> for forms.js in chrome process and content process -- it would be hard to cover both.

I think that you should listen callback always. Don't handle anything without callback.


Let's sort out what are the problem here. I completely forget the detail of your patch. Do you need if default of keydown or keypress is prevented with callback? And if it's fired, does your problem will be gone?

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 29 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #28)
> > Is this a behavior I should workaround, or this is a bug in the native code?
> 
> So, TIP.keydown() cannot return proper result synchronously if the
> dispatched event is sent to another process. So, we need workaround here.
> 

Yeah. I am trying to figure out the way nsDOMWindowUtil dispatches the event and keep the event in the content process. It looks like it uses PuppetWidget#dispatchEvent instead of PuppteWidget#dispatchInputEvent.

> > If so I will think of more complex way than a |this._editing| flag to tell if EditAction is called
> > due to our own action or not.
> I think so. But looks like that current forms.js isn't e10s-aware, isn't it?

forms.js itself is not e10s-aware. The whole InputMethod API deals with e10s/oop by having one forms.js loaded in each of the content processes and passing messages to the keyboard app. Changing this assumption will be a big rewrite to the whole API. We could do that but that might be considered altogether with bug 1025516 too.

> It sets _isEditing true before calls of nsIDOMWindowUtils.sendKey() and sets
> it false immediately after that. So, if edit action is notified
> asynchronously, _isEditing is false even if sending key event is caused.

Yes.

> Additionally, this is hacky and doesn't work well if web apps handle key
> events and sending some edit commands to the focused editor. So, anyway, it
> seems that around this is buggy...

In such case, the keyboard app will getting the end result of not only the edits caused by ourselves but also by the application. I agree this might not be considered and handled properly by the app itself...

> > It is also concerning because the timing of that callback is different
> > for forms.js in chrome process and content process -- it would be hard to cover both.
> 
> I think that you should listen callback always. Don't handle anything
> without callback.

The EditAction observer is always installed, I am not thinking about removing it.

> Let's sort out what are the problem here. I completely forget the detail of
> your patch. Do you need if default of keydown or keypress is prevented with
> callback? And if it's fired, does your problem will be gone?

Please look at the patch and check the |notDefaultPrevented| part. That boolean needs to be reflect the actual interaction with the content for keyboard app to receive the proper result of the API call. As of the EditAction callback, it is currently under the assumption that it should return early when _editing is set.

I would say we should just change the impl of PuppteWidget#dispatchInputEvent or always use PuppetWidget#dispatchEvent in TextEventDispatcher; I don't really know why this event has to travel to chrome process. nsDOMWindowUtils#sendKeyEvent dispatches the event in the content process along and it works fine. It would cause some performance regression too if it have to cross the process boundary.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 30 • 9 years ago

Sorry, I have an urgent crash bug. I'll check this tomorrow.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 31 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #29)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #28)
> > > Is this a behavior I should workaround, or this is a bug in the native code?
> > 
> > So, TIP.keydown() cannot return proper result synchronously if the
> > dispatched event is sent to another process. So, we need workaround here.
> > 
> 
> Yeah. I am trying to figure out the way nsDOMWindowUtil dispatches the event
> and keep the event in the content process. It looks like it uses
> PuppetWidget#dispatchEvent instead of PuppteWidget#dispatchInputEvent.

Ah, I misunderstood because current behavior isn't designed by me. It was changed by bug 1146243.

I'm not sure why the synthesized events not for testing should be dispatched with nsIWidget::DispatchInputEvent(), though.

So, I don't understand well, but if it's not necessary to dispatch keyboard events to the remote process in b2g, we should make TextEventDispatcher::DispatchEvent() use nsIWidget::DispatchEvent() always in b2g. However, perhaps, Gonk should use nsIWidget::DispatchInputEvent() path. So, we need to change TIP side to control which path is used by TextEventDispatcher.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 32 • 9 years ago

http://mxr.mozilla.org/mozilla-central/source/widget/TextEventDispatcher.cpp?rev=76a74dc6ccc9&mark=150-152#139

> 139 nsresult
> 140 TextEventDispatcher::DispatchEvent(nsIWidget* aWidget,
> 141                                    WidgetGUIEvent& aEvent,
> 142                                    nsEventStatus& aStatus)
> 143 {
> 144   nsRefPtr<TextEventDispatcher> kungFuDeathGrip(this);
> 145   nsCOMPtr<nsIWidget> widget(aWidget);
> 146   mDispatchingEvent++;
> 147 
> 148   nsresult rv = NS_OK;
> 149   if (aEvent.AsInputEvent() &&
> 150       (!aEvent.mFlags.mIsSynthesizedForTests || gfxPrefs::TestEventsAsyncEnabled()))
> 151   {
> 152     aStatus = widget->DispatchInputEvent(aEvent.AsInputEvent());
> 153   } else {
> 154     rv = widget->DispatchEvent(&aEvent, aStatus);
> 155   }
> 156 
> 157   mDispatchingEvent--;
> 158   return rv;
> 159 }

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 33 • 9 years ago

I guess that this helps you.

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 34 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #31)
> (In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to
> queue) from comment #29)
> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #28)
> > > > Is this a behavior I should workaround, or this is a bug in the native code?
> > > 
> > > So, TIP.keydown() cannot return proper result synchronously if the
> > > dispatched event is sent to another process. So, we need workaround here.
> > > 
> > 
> > Yeah. I am trying to figure out the way nsDOMWindowUtil dispatches the event
> > and keep the event in the content process. It looks like it uses
> > PuppetWidget#dispatchEvent instead of PuppteWidget#dispatchInputEvent.
> 
> Ah, I misunderstood because current behavior isn't designed by me. It was
> changed by bug 1146243.
> 
> I'm not sure why the synthesized events not for testing should be dispatched
> with nsIWidget::DispatchInputEvent(), though.
> 
> So, I don't understand well, but if it's not necessary to dispatch keyboard
> events to the remote process in b2g, we should make
> TextEventDispatcher::DispatchEvent() use nsIWidget::DispatchEvent() always
> in b2g. However, perhaps, Gonk should use nsIWidget::DispatchInputEvent()
> path. So, we need to change TIP side to control which path is used by
> TextEventDispatcher.

For keyboard events, I think the only risk of not sending them through the parent widget is that they wouldn't end an active wheel transaction in APZ.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 35 • 9 years ago

(In reply to David Anderson [:dvander] from comment #34)
> For keyboard events, I think the only risk of not sending them through the
> parent widget is that they wouldn't end an active wheel transaction in APZ.

Thank you for the information!

So, then, APZ will just dispatch the key event again in the focused process? Then, sounds like that we don't need to send KeyboardEvent to APZ. Instead, we need to just send notification of KeyboardEvent via IPC, isn't it?

Anyway, TIP is currently used by only for tests or software keyboard. So, must not be no problem even if we don't send KeyboardEvent to APZ.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 36 • 9 years ago

Comment on attachment 8632105 [details] [diff] [review]
part.0 TextEventDispatcher shouldn't forward keyboard events coming from TextInputProcessor to the parent process

Sorry about the delay, I am still trying to figure out if there is a problem in my patch or if this patch does not do what it supposedly should do.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 37 • 9 years ago

Comment on attachment 8632105 [details] [diff] [review]
part.0 TextEventDispatcher shouldn't forward keyboard events coming from TextInputProcessor to the parent process

Thank you. I can verify the patch ensures the EditAction callback triggers synchronously at the time keydown() is called from froms.js.

However, keydown() always returns false even though no one actually cancel the event. Should be a problem with my own patch. Will keep looking.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 38 • 9 years ago

I've found the keypress event to be cancelled by nsHTMLEditor.cpp

https://hg.mozilla.org/mozilla-central/annotate/e7e69cc8c07b/editor/libeditor/nsHTMLEditor.cpp#l689

So this probably only affects contenteditables... going to try the patch with a normal input.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 39 • 9 years ago

https://hg.mozilla.org/mozilla-central/annotate/e7e69cc8c07b/editor/libeditor/nsPlaintextEditor.cpp#l411

nsPlaintextEditor.cpp cancel the same event. The only difference is that the character is indeed being send on contenteditable but it does not show up on <input>.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 40 • 9 years ago

Whole keys should be consumed by text editor because typing something shouldn't cause "double action".

So, what's the problem? nsIDOMWindowUtils::SendKeyEvent() must return same result:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentUtils.cpp#7736

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 41 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #40)
> Whole keys should be consumed by text editor because typing something
> shouldn't cause "double action".
> 
> So, what's the problem? nsIDOMWindowUtils::SendKeyEvent() must return same
> result:
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentUtils.
> cpp#7736

I must be confused somehow, will be comparing backtrace of event dispatched from domWindowUtils and TIP and see what I can found.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 42 • 9 years ago

Now I see what the problem is.

The current forms.js rely on the bool named |doKeypress| to tell if the page have cancelled the keydown event. If the flag is set to |false|, keypress event will not be sent and the keyboard app will be notified.

https://hg.mozilla.org/mozilla-central/file/e7e69cc8c07b/dom/inputmethod/forms.js#l507
https://hg.mozilla.org/mozilla-central/file/e7e69cc8c07b/dom/inputmethod/forms.js#l527

The keypress event is always default-prevented by what's found in comment 39 and comment 40, so domWindowUtils.sendKeyEvent("keypress") is always false here.

https://hg.mozilla.org/mozilla-central/file/e7e69cc8c07b/dom/inputmethod/forms.js#l510

But the current impl disregard this return value so nothing happen here.

I was confused because TIP.keydown() sends both events and it is always return |false| since keypress is always default-prevented. I have no way to find out if it is really caused by an evt.preventDefault() on the page.

This is really strange because this is not what the test cases of nsITextInputProcesor demonstrates, nor what's being said on MDN. Gonna take a closer look at the test case [1] tomorrow to find out what's wrong here.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/test/chrome/window_nsITextInputProcessor.xul

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 43 • 9 years ago

Yeah, TIP sends both keydown and keypress at a call of keydown() because the keypress event is too unstable due to a lot of incompatibility with other browsers. Therefore, Gecko wants to manage it in XP level.

Well, indeed, TIP.keydown() can return int value instead of bool, e.g.,

nsITextInputProcessor.KEYDOWN_IS_CONSUMED  = 1 << 0;
nsITextInputProcessor.KEYPRESS_IS_CONSUMED = 1 << 1;

Then, |if (TIP.keydown())| behavior won't be changed but IME can retrieve the detail.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 44 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #42)
> This is really strange because this is not what the test cases of
> nsITextInputProcesor demonstrates, nor what's being said on MDN. Gonna take
> a closer look at the test case [1] tomorrow to find out what's wrong here.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/base/test/chrome/
> window_nsITextInputProcessor.xul

And that's right... I overlooked this test case

https://dxr.mozilla.org/mozilla-central/source/dom/base/test/chrome/window_nsITextInputProcessor.xul#2343-2344

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #43)
> Yeah, TIP sends both keydown and keypress at a call of keydown() because the
> keypress event is too unstable due to a lot of incompatibility with other
> browsers. Therefore, Gecko wants to manage it in XP level.
> 
> Well, indeed, TIP.keydown() can return int value instead of bool, e.g.,
> 
> nsITextInputProcessor.KEYDOWN_IS_CONSUMED  = 1 << 0;
> nsITextInputProcessor.KEYPRESS_IS_CONSUMED = 1 << 1;
> 
> Then, |if (TIP.keydown())| behavior won't be changed but IME can retrieve
> the detail.

Thank you for the suggestion, I will try to create a patch for this.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 45 • 9 years ago

Patch # are re-ordered so keydown() modification is now Part I.

This patch do what comment 43 says. Modification to tests are not tested yet so do feedback instead of review first.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 46 • 9 years ago

Comment on attachment 8634597 [details] [diff] [review]
part 1, modification to returned value of keydown()

Actually -- let me make sure this works before asking for feedback.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 47 • 9 years ago

Comment on attachment 8634597 [details] [diff] [review]
part 1, modification to returned value of keydown()

>+  uint32_t consumedFlags;

Please initialize with 0.

>   result.mResult = KeydownInternal(*aKeyboardEvent, aKeyFlags, false,
>-                                   result.mDoDefault);
>+                                   consumedFlags);
>+  result.mDoDefault = (consumedFlags == 0);

!consumedFlags

>@@ -809,30 +813,36 @@ TextInputProcessor::KeydownInternal(cons
>   keyEvent.modifiers = GetActiveModifiers();
> 
>   nsRefPtr<TextEventDispatcher> kungfuDeathGrip(mDispatcher);
>   rv = IsValidStateForComposition();
>   if (NS_WARN_IF(NS_FAILED(rv))) {
>     return rv;
>   }
> 
>-  nsEventStatus status = aDoDefault ? nsEventStatus_eIgnore :
>-                                      nsEventStatus_eConsumeNoDefault;
>-  if (!mDispatcher->DispatchKeyboardEvent(NS_KEY_DOWN, keyEvent, status,
>+  nsEventStatus keydownStatus = aConsumedFlags ? nsEventStatus_eIgnore :
>+                                                nsEventStatus_eConsumeNoDefault;

nsEventStatus status =
  aConsumedFlags & KEYDOWN_IS_CONSUMED ? nsEventStatus_eConsumeNoDefault :
                                         nsEventStatus_eIgnore;

>+  if (!mDispatcher->DispatchKeyboardEvent(NS_KEY_DOWN, keyEvent, keydownStatus,
>                                           false)) {
>     // If keydown event isn't dispatched, we don't need to dispatch keypress
>     // events.
>     return NS_OK;
>   }
> 
>+  nsEventStatus keypressStatus = aConsumedFlags ? nsEventStatus_eIgnore :
>+                                               nsEventStatus_eConsumeNoDefault;
>+

No, this is not necessary. If keydown is consumed already, keypress event shouldn't be fired.

>   if (aAllowToDispatchKeypress) {
>-    mDispatcher->MaybeDispatchKeypressEvents(keyEvent, status, false);
>+    mDispatcher->MaybeDispatchKeypressEvents(keyEvent, keypressStatus, false);
>   }
> 
>-  aDoDefault = (status != nsEventStatus_eConsumeNoDefault);
>+  aConsumedFlags |= 
>+    (keypressStatus != nsEventStatus_eConsumeNoDefault) ? KEYPRESS_IS_CONSUMED :
>+                                                          0;

So, this should be:

aConsumedFlags =
  status == nsEventStatus_eConsumeNoDefault ? EYPRESS_IS_CONSUMED : 0;

>+   *                        0x11 if both events has been consumed.

So, this case isn't possible.

>+  // These values can be used to do bitwise operation with the return value of
>+  // the keydown() method.
>+  const unsigned long KEYDOWN_IS_CONSUMED                          = 0x00000001;
>+  const unsigned long KEYPRESS_IS_CONSUMED                         = 0x00000010;

I don't understand why you define as so... They can be 0x00000001 and 0x00000002...

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 48 • 9 years ago

I found out I messed up these binary conditions all the time :'(. Anyway, this is the correct patch and tests have passed locally.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 49 • 9 years ago

Comment on attachment 8634636 [details] [diff] [review]
part 1, modification to returned value of keydown(), v2

>+  uint32_t consumedFlags;

I know this will be initialized with 0, but please do that here too. It's easier to understand (and prevents a bug).

>+  aConsumedFlags |= 
>+    (status == nsEventStatus_eConsumeNoDefault) ? KEYDOWN_IS_CONSUMED :
>+                                                  0;
>   if (aAllowToDispatchKeypress) {
>     mDispatcher->MaybeDispatchKeypressEvents(keyEvent, status, false);
>   }
> 
>-  aDoDefault = (status != nsEventStatus_eConsumeNoDefault);
>+  aConsumedFlags |= 
>+    (status == nsEventStatus_eConsumeNoDefault) ? KEYPRESS_IS_CONSUMED :
>+                                                  0;

I don't think that this is right. These code should be in the if block of |if (aAllowToDisptchKeypress)|. When it doesn't dispatch keypress event, it's odd the result to include KEYPRESS_IS_CONSUMED. If the caller wants to check either keydown or keypress is consumed, just |if (TIP.keydown())| is enough. So, we don't need to set both flags when keydown is consumed.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 50 • 9 years ago

Comment on attachment 8632105 [details] [diff] [review]
part.0 TextEventDispatcher shouldn't forward keyboard events coming from TextInputProcessor to the parent process

Currently, TIP doesn't need to send key events to APZ since it's used for implementing VKB. So, when key events are dispatched from TextEventDispatcher whose client is TextInputProcessor, the DispatchEvent() should always dispatch key events synchronously.


In the future, I guess that it's enough to just notify parent process of dispatching keyboard event in a child process (i.e., dispatching keyboard events synchronously). But it should be discussed another bug.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 53 • 9 years ago

> That's not enough since |status| is shared on both.

Ah, right. How about just check |aAllowToDispatchKeypress && status != nsEventStatus_eConsumeNoDefault|? According to the standard spec and actual behavior of the other browsers, keypress events shouldn't be fired when preceding keydown event is consumed. So, that's enough.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 54 • 9 years ago

The flag to probe is not |aAllowToDispatchKeypress|, but the |isDispatched| bool returned by TextEventDispatcher->MaybeDispatchKeypressEvents(), since the desired outcome is never turn on the 0x02 bit if the keypress event is not dispatched. |aAllowToDispatchKeypress| can be true even if keypress is not dispatched since |status| can be |nsEventStatus_eConsumeNoDefault| already.

IDL comment and tests have updated accordingly and passed. 0x03 is indeed not possible.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 55 • 9 years ago

This is the final working patch that would do sendKey() with TIP. Tests with dom/inputmethod/mochitests/ have passed locally.

I have been investigating whether or not to actually deprecate this function, but I realized it better if we could continue support it and overload it with one that could actually take the KeyboardEvent as a dict and pass it to TIP in order to support bug 1142893.

An method call takes two async messages, so if I deprecate this method and in favor of inputcontext#keydown() and inputcontext#keyup() that would two round trip time to type every key. So it's better if we keep this method.

If you agree I will go ahead and do that. I assume JS-implemented API can't overload functions since JS does not support that, so I would have to move some type checking to JS from WebIDL from there.

For supporting bug 1025516 I will still implement inputcontext#keydown() and inputcontext#keyup(), just to make sure we can reflect the timing of these hardware keys, but again for virtual keys (especially the ones that comes with a longpress menu) it is not possible to map the timing of touchstart to the keydown() call.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 56 • 9 years ago

User stories updated on my new API proposal.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 57 • 9 years ago

Comment on attachment 8635117 [details] [diff] [review]
part 1, modification to returned value of keydown()

Looks good except:

>+  bool keypressDispatched = false;
>+
>   if (aAllowToDispatchKeypress) {
>-    mDispatcher->MaybeDispatchKeypressEvents(keyEvent, status, false);
>+    keypressDispatched = 
>+      mDispatcher->MaybeDispatchKeypressEvents(keyEvent, status, false);
>   }
> 
>-  aDoDefault = (status != nsEventStatus_eConsumeNoDefault);
>+  if (keypressDispatched) {
>+    aConsumedFlags |=
>+      (status == nsEventStatus_eConsumeNoDefault) ? KEYPRESS_IS_CONSUMED :
>+                                                    0;      
>+  }


This can be:

  if (aAllowToDispatchKeypress &&
      mDispatcher->MaybeDispatchKeypressEvents(keyEvent, status, false)) {
    aConsumedFlags |=
      (status == nsEventStatus_eConsumeNoDefault) ? KEYPRESS_IS_CONSUMED :
                                                    0;
  }

Otherwise, looks good to me. Please ask r+sr to :smaug after fixing that.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 58 • 9 years ago

As previously discussed with :masayuki, this patch is written so that the return value of TIP.keydown() can be used to figured out whether or not keydown event is consumed alone.

(I will update the commit message to the right one later.)

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 59 • 9 years ago

Comment on attachment 8635142 [details] [diff] [review]
part 2, move forms.js away from domWindowUtils and use TIP

>       case "Forms:Input:SendKey":
>         CompositionManager.endComposition('');
> 
>-        let flags = domWindowUtils.KEY_FLAG_NOT_SYNTHESIZED_FOR_TESTS;
>-        this._editing = true;
>-        let doKeypress = domWindowUtils.sendKeyEvent('keydown', json.keyCode,
>-                                                     json.charCode, json.modifiers, flags);
>-        if (doKeypress) {
>-          domWindowUtils.sendKeyEvent('keypress', json.keyCode,
>-                                      json.charCode, json.modifiers, flags);
>+        let win = target.ownerDocument.defaultView;
>+        let tip = WindowMap.getTextInputProcessor(win);
>+        if (!tip) {
>+          if (json.requestId) {
>+            sendAsyncMessage("Forms:SendKey:Result:Error", {
>+              requestId: json.requestId,
>+              error: "Unable to start componsition."

"composition". But I think that "input transaction" is better.

>+            });
>+          }
>+
>+          break;
>         }
> 
>-        if(!json.repeat) {
>-          domWindowUtils.sendKeyEvent('keyup', json.keyCode,
>-                                      json.charCode, json.modifiers, flags);
>+        // The naive way to figure out if the key to dispatch is printable.
>+        let printable = !!json.charCode;
>+
>+        let keyboardEventDict = {
>+          // For printable keys, the value should be the actual character.
>+          // For non-printable keys, it should be a value in the D3E spec.
>+          // Here we make some educated guess for it.
>+          key: printable ?
>+            String.fromCharCode(json.charCode) :
>+            guessKeyNameFromKeyCode(win.KeyboardEvent, json.keyCode),

In another bug, we should make this can be specified by keyboard apps.

>+        } else if (keyboardEventDict.key === "Unidentified") {
>+          // For non-printable keys it should be left unset
>+          // for TextInputProcessor to compute from the |key| property.
>+          // But if the key property can't be guessed, we should set the value
>+          // anyway so TextInputProcessor can have something to figure out what
>+          // to output.
>+          keyboardEventDict.keyCode = json.keyCode;
>         }

Hmm, I think that if the keyCode is specified by keyboard apps, we should use it even if the .key isn't "Unidentified" since it's not guaranteed that the mapping from .key to .keyCode is same as the guessKeyNameFromKeyCode().

>-        this._editing = false;
>+        let keyboardEvent = new win.KeyboardEvent("", keyboardEventDict);
>+        let flags = tip.KEY_KEEP_KEY_LOCATION_STANDARD;
>+        if (!printable) {
>+          flags |= tip.KEY_NON_PRINTABLE_KEY;
>+        }

Add:

if (!keyboardEvent.keyCode) {
  flags |= tip.KEY_KEEP_KEYCODE_ZERO;
}

>     /*
>       * send a character with its key events.
>-      * @param modifiers see http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl#206
>+      * @param modifiers this paramater is no longer honored.

This will break compatibility with current API. Is it okay? If you don't want that, you can synthesize both keydown and keyup of each modifier key with KEY_DONT_DISPATCH_MODIFIER_KEY_EVENT flag. But I don't mind, up to you.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 60 • 9 years ago

Thank you for the quick turnaround. Please keep in mind that this patch is only about wiring the old API to TIP. New API methods that could allow the vkb app to set |key| and |code| will be introduced in the next patch.

After spending some time thinking about this, I move the |keyCode| back to the object construction block and have it always set, since we need that in all conditions (non-printable & printable).

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 61 • 9 years ago

BTW modifiers is not used in the Gaia Keyboard app and I don't know who added it, so I think we could remove it just to simplify the logic.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 62 • 9 years ago

Comment on attachment 8635186 [details] [diff] [review]
part 1, modification to returned value of keydown(), v3

>@@ -524,23 +524,32 @@ interface nsITextInputProcessor : nsISup
>    *                        defined by any standards, use "" (empty string).
>    *                        #4 .keyCode is guessed from .key value if the key
>    *                        name is registered and .keyCode isn't initialized.
>    *                        #5 modifier key states, e.g., .shiftKey, are
>    *                        ignored.  Instead, modifier states are managed by
>    *                        each instance and set automatically.
>    * @param aKeyFlags       Special flags.  The values can be some of KEY_*
>    *                        constants.
>-   * @return                true if neither the keydown event or following
>+   * @return                0x00 if neither the keydown event or following
>    *                        keypress events is *not* consumed.
>-   *                        Otherwise, i.e., preventDefault() is called, false.
>+   *                        0x01 if the keydown event is consumed, and
>+   *                        0x02 if the keypress event has been consumed.
>+   *                        Note that keypress event is always consumed by
>+   *                        native code for the printable keys (indicating the
>+   *                        default action has been taken).
>    */
>   [optional_argc]
>-    boolean keydown(in nsIDOMKeyEvent aKeyboardEvent,
>-                    [optional] in unsigned long aKeyFlags);
>+    unsigned long keydown(in nsIDOMKeyEvent aKeyboardEvent,
>+                          [optional] in unsigned long aKeyFlags);
>+
>+  // These values can be used to do bitwise operation with the return value of
>+  // the keydown() method.
>+  const unsigned long KEYDOWN_IS_CONSUMED                          = 0x00000001;
>+  const unsigned long KEYPRESS_IS_CONSUMED                         = 0x00000002;
Please move these to be above keydown() and use the names in the comment, not the values of the consts.
Also, I'd prefer if there was a separate const for 0x00000000 case. Perhaps
const unsigned long NOT_CONSUMED                         = 0x00000000;

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 63 • 9 years ago

Comment on attachment 8635186 [details] [diff] [review]
part 1, modification to returned value of keydown(), v3

> 
>-  if (aAllowToDispatchKeypress) {
>-    mDispatcher->MaybeDispatchKeypressEvents(keyEvent, status, false);
>+  aConsumedFlags |= 
>+    (status == nsEventStatus_eConsumeNoDefault) ? KEYDOWN_IS_CONSUMED :
>+                                                  0;
>+
>+
>+  if (aAllowToDispatchKeypress &&
>+      mDispatcher->MaybeDispatchKeypressEvents(keyEvent, status, false)) {
>+    aConsumedFlags |=
>+      (status == nsEventStatus_eConsumeNoDefault) ? KEYPRESS_IS_CONSUMED :
>+                                                    0;

so instead of 0, I'd use NOT_CONSUMED or something like that, or should it be called KEYEVENT_NOT_CONSUMED

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 64 • 9 years ago

Comment on attachment 8632105 [details] [diff] [review]
part.0 TextEventDispatcher shouldn't forward keyboard events coming from TextInputProcessor to the parent process

> TextEventDispatcher::DispatchEvent(nsIWidget* aWidget,
>                                    WidgetGUIEvent& aEvent,
>-                                   nsEventStatus& aStatus)
>+                                   nsEventStatus& aStatus,
>+                                   bool aMayForwardKeysToParent)
> {
>   nsRefPtr<TextEventDispatcher> kungFuDeathGrip(this);
>   nsCOMPtr<nsIWidget> widget(aWidget);
>   mDispatchingEvent++;
> 
>   nsresult rv = NS_OK;
>-  if (aEvent.AsInputEvent() &&
>-      (!aEvent.mFlags.mIsSynthesizedForTests || gfxPrefs::TestEventsAsyncEnabled()))
>-  {
>-    aStatus = widget->DispatchInputEvent(aEvent.AsInputEvent());
>+  WidgetInputEvent* inputEvent =
>+    aMayForwardKeysToParent ||
>+    (mForTests && gfxPrefs::TestEventsAsyncEnabled()) ? aEvent.AsInputEvent() :
>+                                                        nullptr;
>+  if (inputEvent) {
>+    aStatus = widget->DispatchInputEvent(inputEvent);
>   } else {
>     rv = widget->DispatchEvent(&aEvent, aStatus);
>   }
hmmm, this clearly needs documentation. What is the difference between DispatchInputEvent vs. DispatchEvent.
(I can see from code that puppet widget forwards to parent in case of DispatchInputEvent, and in parent process we use APZ if available)


Why do we in some cases pass false to DispatchEvent, but in some other cases !mForTests? 


>    * DispatchEvent() dispatches aEvent on aWidget.
>+   *
>+   * @param aMayForwardKeysToParent     If true, the event may be forwarded to
>+   *                                    the parent process.  Otherwise,
>+   *                                    dispatched in the process.
>    */
>   nsresult DispatchEvent(nsIWidget* aWidget,
>                          WidgetGUIEvent& aEvent,
>-                         nsEventStatus& aStatus);
>+                         nsEventStatus& aStatus,
>+                         bool aMayForwardKeysToParent);
I'd prefer using some enum or const values here, not bool.
And what does "may" mean here. I mean, the comment should perhaps have an example when forwarding doesn't happen even if the
flag is set. Also, aMayForwardKeysToParent means that only in child process. Parent process just uses APZ if the flag is set and APZ available.
Same also elsewhere.



Ok, so we're making the currently only for testing code path to be available for other cases too. that should be fine. But I'd like to see some more comments here, especially given the oddity of DispatchInputEvent. (I wonder if we could even rename that method to something more descriptive.)

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 65 • 9 years ago

Thanks, :smaug and :masayuki.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 66 • 9 years ago

Hi masayuki,

This is WIP of the next patch. Since WebIDL does not support overloading functions, I would have to move type checking into the JS code. With this patch sendKey() now accepts a dict and pass it directly (except the |nonPrintable| property) into TIP. You can read the WebIDL change to know the properties the VKB app is allowed to set within the dict.

I have manually verified the new sendKey() can conveniently accept a printable key like this:

navigator.mozInputMethod.inputcontext.sendKey({ key: 'A' }).then(...)

and a non-printable key like this

navigator.mozInputMethod.inputcontext.sendKey({ key: 'Enter', nonPrintable: true }).then(...)

Obviously the caller should also set the |code| and |repeat| property if they applies. I purposely use |nonPrintable| as opposed to |printable| just because of the fact we type printable characters more often than non-printables.

Do you agree with this approach and the API design? If so I will follow-up this with proper tests and also the same modifications to setComposition() and endComposition() methods.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 67 • 9 years ago

Comment on attachment 8635247 [details] [diff] [review]
part 2, move forms.js away from domWindowUtils and use TIP

>@@ -743,23 +743,25 @@ MozInputContext.prototype = {
>   },
> 
>   deleteSurroundingText: function ic_deleteSurrText(offset, length) {
>     return this.replaceSurroundingText(null, offset, length);
>   },
> 
>   sendKey: function ic_sendKey(keyCode, charCode, modifiers, repeat) {
>     let self = this;
>+
>+    // XXX: modifiers are ignored in this API method.
>+
>     return this._sendPromise(function(resolverId) {
>       cpmmSendAsyncMessageWithKbID(self, 'Keyboard:SendKey', {
>         contextId: self._contextId,
>         requestId: resolverId,
>         keyCode: keyCode,
>         charCode: charCode,
>-        modifiers: modifiers,

Okay.

>+  getTextInputProcessor: function(win) {

...

>+    if (!tip.beginInputTransaction(win, textInputProcessorCallback)) {
>+      tip = obj.tip = null;
>+    }

Sorry, I should've reviewed here.

nsITextInputProcessor.beginInputTransaction() may cause throwing an exception. So, you should wrap here with try-catch.

>+          if (keydownDefaultPrevented) {
>+            sendAsyncMessage("Forms:SendKey:Result:Error", {
>+              requestId: json.requestId,
>+              error: "Key event(s) was cancelled."
>+            });
>+          } else {
>+            sendAsyncMessage("Forms:SendKey:Result:OK", {
>+              requestId: json.requestId,
>+              selectioninfo: this.getSelectionInfo()
>+            });
>+          }

I don't understand why this doesn't notify keyboard apps of consuming keypress event. But it's current behavior, so, this is not a bug of this bug's scope even if it actually causes some trouble.


With wrapping .beginInputTransaction() with try-catch, r=me.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 68 • 9 years ago

Comment on attachment 8635929 [details] [diff] [review]
part 3 WIP, modification to sendKey()

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #66)
> I have manually verified the new sendKey() can conveniently accept a
> printable key like this:
> 
> navigator.mozInputMethod.inputcontext.sendKey({ key: 'A' }).then(...)
> 
> and a non-printable key like this
> 
> navigator.mozInputMethod.inputcontext.sendKey({ key: 'Enter', nonPrintable:
> true }).then(...)
> 
> Obviously the caller should also set the |code| and |repeat| property if
> they applies. I purposely use |nonPrintable| as opposed to |printable| just
> because of the fact we type printable characters more often than
> non-printables.

|key| should be specified explicitly. And TextInputProcessor will judge if it's a non-printable key name automatically. Basically, non-printable key name shouldn't be specified as a printable key's key value. However, it's possible according to the UI Events spec. So, it's very rare case to specify if a non-printable key name should be treated as printable keys. So, basically, the flag should be optional (i.e., not necessary to set it explicitly in usual cases). However, for keyboard apps which we cannot imagine, we should provide a flag to make the key name treated as a printable key forcibly. So, I guess |printable| is enough and we need to check if it's undefined or not. How about you?

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 69 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #68)
> Comment on attachment 8635929 [details] [diff] [review]
> part 3 WIP, modification to sendKey()
> 
> (In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to
> queue) from comment #66)
> > I have manually verified the new sendKey() can conveniently accept a
> > printable key like this:
> > 
> > navigator.mozInputMethod.inputcontext.sendKey({ key: 'A' }).then(...)
> > 
> > and a non-printable key like this
> > 
> > navigator.mozInputMethod.inputcontext.sendKey({ key: 'Enter', nonPrintable:
> > true }).then(...)
> > 
> > Obviously the caller should also set the |code| and |repeat| property if
> > they applies. I purposely use |nonPrintable| as opposed to |printable| just
> > because of the fact we type printable characters more often than
> > non-printables.
> 
> |key| should be specified explicitly. And TextInputProcessor will judge if
> it's a non-printable key name automatically. Basically, non-printable key
> name shouldn't be specified as a printable key's key value. However, it's
> possible according to the UI Events spec. So, it's very rare case to specify
> if a non-printable key name should be treated as printable keys. So,
> basically, the flag should be optional (i.e., not necessary to set it
> explicitly in usual cases). However, for keyboard apps which we cannot
> imagine, we should provide a flag to make the key name treated as a
> printable key forcibly. So, I guess |printable| is enough and we need to
> check if it's undefined or not. How about you?

|nonPrintable| is indeed optional, as it currently map to TIP.KEY_NON_PRINTABLE_KEY flag. I can do 

navigator.mozInputMethod.inputcontext.sendKey({ key: 'Enter' })

to generate a non-printable enter key, without specifying |nonPrintable: true|.

Are you saying I should do an optional |printable| property and map it to TIP.KEY_FORCE_PRINTABLE_KEY, just to force TIP to dispatch printable key events that outputs the string, e.g. "Enter", in such rare use case? If so, should I still keep the |nonPrintable| flag and allow VKB app the continue access of the TIP.KEY_NON_PRINTABLE_KEY flag?

I personally think both properties should be implemented since both flags would be useful to the VKB app.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 70 • 9 years ago

Like this:

      * @param dictOrKeyCode A keyboardEventDict with following properties
      *                      - key: String/char to output, or registered name of non-printable key
      *                      - code: String/char indicating the virtual hardware key pressed.
      *                              Must be a value defined in http://www.w3.org/TR/DOM-Level-3-Events-code/#keyboard-chording-virtual
      *                      - repeat: Boolean, indicates whether a key would be sent repeatedly.
      *                      - printable: Boolean, true if |key| property is referring to a printable key.
      *                      - nonPrintable: Boolean, true if |key| property is referring to a nonPrintable key.
      *                                      When set to true, |key| must be a registered value for non-printable key.
      *                      |printable| and |nonPrintable| properties should never be both true.
      *                      When both properties are false (the default), it is automatically decided, but
      *                      it is recommended to be explicit when calling the method.

If you agree I will upload the patch and set for feedback again. I would like to know your thought on type checking in JavaScript as well.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 71 • 9 years ago

> Are you saying I should do an optional |printable| property and map it to TIP.KEY_FORCE_PRINTABLE_KEY, just
> to force TIP to dispatch printable key events that outputs the string, e.g. "Enter", in such rare use case?

Yes.

> If so, should I still keep the |nonPrintable| flag and allow VKB app the continue access of the 
> TIP.KEY_NON_PRINTABLE_KEY flag?

Hmm, I don't think so. KEY_NON_PRINTABLE_KEY doesn't overrides any behavior. The flag makes TIP throw an exception if the key name is not a non-prinable key name. However, if non-printable key name is changed due to spec changed, forms.js or something should map old key name to new key name for keeping the compatibility of keyboard app (I don't like key names depend on keyboard apps when spec is changed). So, I don't think we need to use the flag with new API.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 72 • 9 years ago

I will address comment 71 in the next patch -- the tests are mostly complete. But, I have a new question:

According to

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsITextInputProcessor#How_to_create_KeyboardEvent_instance_for_nsITextInputProcessor

keyCode from "printable" keys should be specified manually, and I wonder the reason of design of TIP here. I thought keyCode can be inferred from |code| value specified, but TIP does not do that.

This leave us with two options:

1) Expose keyCode to VKB app and allow it to set it, which I remember you said we shouldn't be doing.
2) Implement a |code| -> |keyCode| mapping in MozKeyboard.js* and always emit keyCode from US Layout on Windows (the post popular one, arguably).

For (2) effectively that means we would have to implement these tables in JS.
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode#Printable_keys_in_standard_position

What do you think?

* I assume MozKeyboard.js will be loaded once only per keyboard process, but forms.js will be loaded in every process, so put the table in MozKeyboard.js is better than forms.js.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 73 • 9 years ago

Or, impl the rules 3/4/6/8 in this chapter in the spec, in (1) Gaia Keyboard app or (2) MozKeyboard.js, just to simplify the rules:

http://www.w3.org/TR/DOM-Level-3-Events/#legacy-key-models

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 74 • 9 years ago

Patch updated to figure out keyCode in MozKeyboard.js based on |code| and |key| passed into the method. This is a mix approach that I think could fit the use cases. Please read the test case to find out the examples given.

If this look alright I will follow-up and start patching setComposition() and endComposition() method. Since the key event is optional in these method for TIP as well, I think I would append the keyboardEventDict argument at the very end of the list instead of what's done here?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 75 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #72)
> According to
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/
> nsITextInputProcessor#How_to_create_KeyboardEvent_instance_for_nsITextInputPr
> ocessor
> 
> keyCode from "printable" keys should be specified manually, and I wonder the
> reason of design of TIP here. I thought keyCode can be inferred from |code|
> value specified, but TIP does not do that.

Yes, the reason is that keyCode value of Gecko for Desktop does not indicate physical key position. It indicates what _ASCII_ character _can_ be inputted by the key.

However, for VKB, we may be able to guess proper keyCode value from inputting character. We need to know how other mobile browsers work.

> This leave us with two options:
> 
> 1) Expose keyCode to VKB app and allow it to set it, which I remember you
> said we shouldn't be doing.
> 2) Implement a |code| -> |keyCode| mapping in MozKeyboard.js* and always
> emit keyCode from US Layout on Windows (the post popular one, arguably).

Yes, if it's possible, we shouldn't choose #1 because keyCode value shouldn't depend on keyboard apps which have same or similar layout. #2 is better if it's possible. However, not |code|. Perhaps, |key|.

> For (2) effectively that means we would have to implement these tables in JS.
> https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/
> keyCode#Printable_keys_in_standard_position
> 
> What do you think?
> 
> * I assume MozKeyboard.js will be loaded once only per keyboard process, but
> forms.js will be loaded in every process, so put the table in MozKeyboard.js
> is better than forms.js.

Agreed.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 76 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #75)
> > 2) Implement a |code| -> |keyCode| mapping in MozKeyboard.js* and always
> > emit keyCode from US Layout on Windows (the post popular one, arguably).
> 
> Yes, if it's possible, we shouldn't choose #1 because keyCode value
> shouldn't depend on keyboard apps which have same or similar layout. #2 is
> better if it's possible. However, not |code|. Perhaps, |key|.
> 

Hum, If you are reading my function now you would see there is a |key| section first follow by |code|. I figured we should have the |code| -> |keyCode| section because without it we would not be setting any keyCode for non-ASCII characters (e.g. on the Greek layout). I think if the VKB app give us |code| to say given hardware key on the PC keyboard it would like to emulate, we should still generate a keyCode based on that.

I should put this in my test cases. Sorry about that.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 77 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #76)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #75)
> > > 2) Implement a |code| -> |keyCode| mapping in MozKeyboard.js* and always
> > > emit keyCode from US Layout on Windows (the post popular one, arguably).
> > 
> > Yes, if it's possible, we shouldn't choose #1 because keyCode value
> > shouldn't depend on keyboard apps which have same or similar layout. #2 is
> > better if it's possible. However, not |code|. Perhaps, |key|.
> > 
> 
> Hum, If you are reading my function now you would see there is a |key|
> section first follow by |code|. I figured we should have the |code| ->
> |keyCode| section because without it we would not be setting any keyCode for
> non-ASCII characters (e.g. on the Greek layout). I think if the VKB app give
> us |code| to say given hardware key on the PC keyboard it would like to
> emulate, we should still generate a keyCode based on that.

Okay. But if the keyboard app doesn't emulates physical keyboard, e.g., inputting Greek characters with 10key + Flicker, the code value should be empty string because they're not inputted.
https://w3c.github.io/uievents/#code-virtual-keyboards

In such case, code value isn't available... So, either code value or keyCode value is required??

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 78 • 9 years ago

> 10key + Flicker

I meant 10-key + swipe.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 79 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 7/27 - 8/2) from comment #77)
> > Hum, If you are reading my function now you would see there is a |key|
> > section first follow by |code|. I figured we should have the |code| ->
> > |keyCode| section because without it we would not be setting any keyCode for
> > non-ASCII characters (e.g. on the Greek layout). I think if the VKB app give
> > us |code| to say given hardware key on the PC keyboard it would like to
> > emulate, we should still generate a keyCode based on that.
> 
> Okay. But if the keyboard app doesn't emulates physical keyboard, e.g.,
> inputting Greek characters with 10key + Flicker, the code value should be
> empty string because they're not inputted.
> https://w3c.github.io/uievents/#code-virtual-keyboards
> 
> In such case, code value isn't available... So, either code value or keyCode
> value is required??

I don't know, I don't think it's a good idea to allow VKB authors to set keyCode w/o getting a code value set. People will most likely set keyCode and forget about code value all the time, since it's new.

10-key/swipe is an interesting question. I was more leaning toward always emulate PC 101 keyboard regardless (hence, always set a code value and generate keyCode based on that), but since you asked it sounds like you don't want that? In that case, given the possibility of negating setting code values from the VKB author, I tend to think these key should have keyCode = 0 if they are not interested in emulating PC keyboard.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 80 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #79)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 7/27 - 8/2)
> from comment #77)
> > > Hum, If you are reading my function now you would see there is a |key|
> > > section first follow by |code|. I figured we should have the |code| ->
> > > |keyCode| section because without it we would not be setting any keyCode for
> > > non-ASCII characters (e.g. on the Greek layout). I think if the VKB app give
> > > us |code| to say given hardware key on the PC keyboard it would like to
> > > emulate, we should still generate a keyCode based on that.
> > 
> > Okay. But if the keyboard app doesn't emulates physical keyboard, e.g.,
> > inputting Greek characters with 10key + Flicker, the code value should be
> > empty string because they're not inputted.
> > https://w3c.github.io/uievents/#code-virtual-keyboards
> > 
> > In such case, code value isn't available... So, either code value or keyCode
> > value is required??
> 
> I don't know, I don't think it's a good idea to allow VKB authors to set
> keyCode w/o getting a code value set. People will most likely set keyCode
> and forget about code value all the time, since it's new.

Indeed. How about the code value is defined as required even if its value is empty string?

> 10-key/swipe is an interesting question. I was more leaning toward always
> emulate PC 101 keyboard regardless (hence, always set a code value and
> generate keyCode based on that), but since you asked it sounds like you
> don't want that? In that case, given the possibility of negating setting
> code values from the VKB author, I tend to think these key should have
> keyCode = 0 if they are not interested in emulating PC keyboard.

I think that if code value is not empty string, keyCode value should be respected. Otherwise, ignoring keyCode is enough. (i.e., code value isn't empty string means that the keyboard app emulates PC keyboard. In this case, keyCode value should be computed by use for consistency between keyboard apps. Otherwise, i.e., "pure" virtual keyboard apps should specify code value always empty string and keyCode value even if it's 0.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 81 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 7/27 - 8/2) from comment #80)
> I think that if code value is not empty string, keyCode value should be
> respected. Otherwise, ignoring keyCode is enough. (i.e., code value isn't
> empty string means that the keyboard app emulates PC keyboard. In this case,
> keyCode value should be computed by use for consistency between keyboard
> apps. Otherwise, i.e., "pure" virtual keyboard apps should specify code
> value always empty string and keyCode value even if it's 0.

This sounds what's already did in the _getKeyCode function. For consistency between keyboard apps, keyCode will always computed by us based on the specified |key| or |code| in which |key| takes precedence.

Did I misunderstand what you said here?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 82 • 9 years ago

Comment on attachment 8637043 [details] [diff] [review]
part 3 WIP, modification to sendKey()

>+    } else if (typeof dictOrKeyCode === 'object') {
>+      let keyboardEventDict = {
>+        key: dictOrKeyCode.key,
>+        code: dictOrKeyCode.code,

I think that if typeof(dictOrKeyCode.code) must be string. (i.e., keyboard apps should be set "" or valid code value.)

>+        repeat: dictOrKeyCode.repeat,
>+        printable: dictOrKeyCode.printable,
>+        keyCode: this._getKeyCode(dictOrKeyCode)

Then, keyCode should be computed only when code is "".

>+  // From http://www.w3.org/TR/DOM-Level-3-Events/#fixed-virtual-key-codes
>+  // and
>+  // http://www.w3.org/TR/DOM-Level-3-Events/#optionally-fixed-virtual-key-codes

These documents must not be good reference, though...

FYI: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode#Printable_keys_in_standard_position

>+  _USKeyboardKeyCodesFromPrintableChars: {
>+    ' ': 32,
>+    ';': 186,
>+    ':': 186,
>+    '=': 187,
>+    '+': 187,
>+    ',': 188,
>+    '<': 188,
>+    '-': 189,
>+    '_': 189,
>+    '.': 190,
>+    '>': 190,
>+    '/': 191,
>+    '?': 191,
>+    '`': 192,
>+    '~': 192,
>+    '[': 219,
>+    '{': 219,
>+    '\\': 220,
>+    '|': 220,
>+    ']': 221,
>+    '}': 221,
>+    '\'': 222,
>+    '"': 222
>+  },

Cannot you use KeyboardEvent.DOM_VK_* instead of numbers?

>+  // From
>+  // http://www.w3.org/TR/DOM-Level-3-Events-code/#code-value-tables
>+  // and map the |code| value of printable keys to the keyCode the character
>+  // produces.
>+  _USKeyboardKeyCodesFromPrintableCode: {
>+    'Space': 32,
>+    'Semicolon': 186,
>+    'Equal': 187,
>+    'Comma': 188,
>+    'Minus': 189,
>+    'Period': 190,
>+    'Slash': 191,
>+    'Backquote': 192,
>+    'BracketLeft': 219,
>+    'Backslash': 220,
>+    'BracketRight': 221,
>+    'Quote': 222
>+  },

Same.

>+  _getKeyCode: function(dict) {
>+    // Try to resolve the keyCode from |key| first since keyCode
>+    // must always follow the actual character to output first.
>+    if (dict.key.length === 1) {
>+      if (/^[a-z0-9]$/i.test(dict.key)) {
>+        return dict.key.toUpperCase().charCodeAt(0);
>+      }
>+
>+      // XXX: Should this part come *after* checking |code|?
>+      if (dict.key in this._USKeyboardKeyCodesFromPrintableChars) {
>+        return this._USKeyboardKeyCodesFromPrintableChars[dict.key];
>+      }

If Shift state and AltGr state is also emulated by keyboard apps, anyway, the keyCode cannot be computed exactly same as desktop. So, computing from .key value first feels better to me because on desktop, we compute keyCode from inputting character.

>+    // Try to resolve the keyCode from |code|.
>+    // Even if the actual character to output is not in the resolvable range,
>+    // if the content wish to emulate a given key on the standard keyboard
>+    // by specifying the |code| value, we should make keyCode follow that.
>+    if (typeof dict.code === 'string') {
>+      if (dict.code.startsWith('Digit')) {

Why don't you check if the following character is only one and it's [0-9]?

>+        return dict.code.charCodeAt(5);
>+      }
>+      if (dict.code.startsWith('Key')) {

Similar. The next character should be [A-Z].

>+        return dict.code.charCodeAt(3);
>+      }
>+      if (dict.code in this._USKeyboardKeyCodesFromPrintableCode) {
>+        return this._USKeyboardKeyCodesFromPrintableCode[dict.code];
>+      }
>+    }
>+
>+    // The dict is either representing a non-printable key, or we don't
>+    // have any information to get the confirming keyCode we should generate.
>+    // In both cases, we should return 0.
>+    return 0;

As I said above, I think that when dict.code is "", specified .keyCode value should be used.

>+        if (keyboardEventDict) {
>+          flags |= keyboardEventDict.printable ? tip.KEY_FORCE_PRINTABLE_KEY : 0;

I like following style better since that reduce the diff at adding flags:

if (keyboardEventDict.printable) {
  flags |= keyboardEventDict.printable;
}


I just roughly check the code. And I'm very sorry, I'm on vacation next week except when my flight would be canceled due to the Typhoon.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 83 • 9 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 84 • 9 years ago

Sorry for the delay. I renamed DispatchEvent() to DispatchInputEvent() and created new DispatchEvent(). And mForTests is checked by TextInputProcessor side. Probably, this is clearer.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 85 • 9 years ago

Comment on attachment 8638496 [details] [diff] [review]
part.0 TextEventDispatcher shouldn't forward keyboard events coming from TextInputProcessor to the parent process

>+
>+  /**
>+   * DispatchTo indicates whether the event may be dispatched to its parent
>+   * process first (if there is) or not.  If the event is dispatched to the
>+   * parent process, APZ will handle it first.  Otherwise, the event won't be
>+   * handled by APZ if it's in a child process.
>+   */
>+  enum DispatchTo
>+  {
>+    // The event may be dispatched to its parent process if there is. 
"... if there is a parent."

In such
>+    // case, the event will be handled asynchronously.
>+    eDispatchToParentProcess = 0,
So, when the flag is used, is the event dispatched to both parent and child? or only parent?
Could you clarify the comment a bit.



>   /**
>-   * DispatchEvent() dispatches aEvent on aWidget.
>+   * DispatchInputEvent() dispatches aEvent on aWidget.
>    */
>   nsresult DispatchEvent(nsIWidget* aWidget,
>                          WidgetGUIEvent& aEvent,
>                          nsEventStatus& aStatus);
Why the comment change? Method is still DispatchEvent, but comment is about DispatchInputEvent

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 86 • 9 years ago

I have updated the _getKeyCode() part according to the last comment. The method will now consider keyCode from the VKB app is code is unset. Properties are now cast to their right types when copying to our own object.

I've also updated the test case to cover the US keyboard characters, Enter key, and Greek alphabets on Greek layout. It has 16206 assertions. It might be easier to read the test to tell how keyCode are observed I think.

WebIDL is updated accordingly to say what keyCode do.

If you think this is the right approach, I will complete the patch with keydown()/keyup() methods, and optional keyboardEventDict argument in composition methods.

Thanks!

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 87 • 9 years ago

Combining the previous rules, the current rules would be (consider dict to be the object that holds the properties passed into the method):

1. if dict.code is unset or '', keyCode will always be dict.keyCode (cast to 0 if unset)
2. if dict.key is in the range of /^[a-z0-9]$/i, use the keyCode of the key.
3. if dict.key is one of the symbols on US Keyboard (kept in _USKeyboardKeyCodesFromPrintableChars), use the keyCode there.
4. if dict.code is /^Digit[0-9]/, use the keyCode of 0-9.
5. if dict.code is /^Key[A-Z]/, use the keyCode of A-Z.
6. if dict.code is one of the key on US Keyboard (kept in _USKeyboardKeyCodesFromPrintableCode), use the keyCode there.
7. keyCode=0 if above rules don't match.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 88 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #87)
> 7. keyCode=0 if above rules don't match.

And of course, for non-printable keys, even though it's being specified as 0 here, TIP will set it to the correct keyCode.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 89 • 9 years ago

Sorry for the very long delay. I couldn't check this today. I'll check this bug tomorrow.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 90 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #87)
> 1. if dict.code is unset or '', keyCode will always be dict.keyCode (cast to
> 0 if unset)

I agree.

> 2. if dict.key is in the range of /^[a-z0-9]$/i, use the keyCode of the key.

Okay.

> 3. if dict.key is one of the symbols on US Keyboard (kept in
> _USKeyboardKeyCodesFromPrintableChars), use the keyCode there.

Okay, android/nsWindow.cpp uses mapping of US keyboard layout (although, I don't like that):
http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp?rev=d2de18bbfedc&mark=1194-1200#1156

> 4. if dict.code is /^Digit[0-9]/, use the keyCode of 0-9.

I agree.

> 5. if dict.code is /^Key[A-Z]/, use the keyCode of A-Z.

I agree.

> 6. if dict.code is one of the key on US Keyboard (kept in
> _USKeyboardKeyCodesFromPrintableCode), use the keyCode there.

I don't think that _USKeyboardKeyCodesFromPrintableCode() is enough. Although, this patch hasn't been landed (but got r+), this helps you to create the table:
https://bugzilla.mozilla.org/attachment.cgi?id=8584352&action=diff#a/testing/mochitest/tests/SimpleTest/EventUtils.js_sec8

> 7. keyCode=0 if above rules don't match.

Okay.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 91 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 7/27 - 8/2) from comment #90)
> (In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to
> queue) from comment #87)
> > 6. if dict.code is one of the key on US Keyboard (kept in
> > _USKeyboardKeyCodesFromPrintableCode), use the keyCode there.
> 
> I don't think that _USKeyboardKeyCodesFromPrintableCode() is enough.
> Although, this patch hasn't been landed (but got r+), this helps you to
> create the table:
> https://bugzilla.mozilla.org/attachment.cgi?id=8584352&action=diff#a/testing/
> mochitest/tests/SimpleTest/EventUtils.js_sec8

See kKeyboardLayouts["Common-PC"].controlKeys definition. (I don't think we need to support IME specific keys nor Mac specific keys)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 92 • 9 years ago

Comment on attachment 8642234 [details] [diff] [review]
part 3 WIP, modification to sendKey()

>+  // This function computes keyCodes from |key| or |code|,
>+  // for printable keys only and only when the |code| is not set.
>+  // If computed from |key|, it uses keyCode values from a US Keyboard.
>+  // This mirrors the rules in
>+  // https://mdn.io/keyCode#Printable_keys_in_standard_position
>+  // and overwrites keyCode set by the content to ensure consistency across
>+  // different virtual keyboards.
>+  _getKeyCode: function(dict) {
>+    // If the code is not specified, it's the content's responsibility to
>+    // specify a correct legacy keyCode.
>+    if (!dict.code) {
>+      return dict.keyCode;
>+    }

Don't you need to check the type of .keyCode?

>+    // Try to resolve the keyCode from |key| first,
>+    // since keyCode must always follow the actual character to output.
>+    if (dict.key.length === 1) {
>+      if (/^[a-z0-9]$/i.test(dict.key)) {
>+        return dict.key.toUpperCase().charCodeAt(0);
>+      }
>+
>+      // XXX: Should this part come *after* checking |code|?

Ah, this is probably right. If a keyboard app tries to emulate AZERTY keyboard layout, code value should be better information to compute the right keyCode value. However, please note that .key value is more important than .code value for [a-zA-Z0-9] keys. E.g., Dvorak keyboard layout.

>+      if (dict.key in this._USKeyboardKeyCodesFromPrintableChars) {
>+        return this._USKeyboardKeyCodesFromPrintableChars[dict.key];
>+      }
>+    }
>+
>+    // Try to resolve the keyCode from |code|.
>+    // Even if the actual character to output is not in the resolvable range,
>+    // if the content wish to emulate a given key on the standard keyboard
>+    // by specifying the |code| value, we should make keyCode follow that.
>+    if (/^Digit[0-9]/.test(dict.code)) {

Don't you need $ before /?

>+      return dict.code.charCodeAt(5);
>+    }
>+    if (/^Key[A-Z]/.test(dict.code)) {

Same.

>diff --git a/dom/inputmethod/mochitest/test_bug1137557.html b/dom/inputmethod/mochitest/test_bug1137557.html

I'll check this later.


And how do you think about the support of modifier keys? I think that keyboard apps should call modifier keydown and modifier keyup separately when user toggles the state on the keyboard. If so, the new API should throw an error when the key value is a modifier key's.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 93 • 9 years ago

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

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 95 • 9 years ago

part 1 rebased on top of the r+'d part 0

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 96 • 9 years ago

Comment addressed.

keyCode and other properties have their typed checked at the time keyboardEventDict is created.

Let me know what you think about the test and IDL etc. and I will continue. Thanks!

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 97 • 9 years ago

Looks good except _USKeyboardKeyCodesFromPrintableCode. Please complete that. If you'd like me to create that, I'll do that (will post the array as a comment). Feel free to tell me.

I'll check the test part when I'm on train to go back Osaka (I'm in Tokyo office now).

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 98 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #97)
> Looks good except _USKeyboardKeyCodesFromPrintableCode. Please complete
> that. If you'd like me to create that, I'll do that (will post the array as
> a comment). Feel free to tell me.

Could you tell me which part is missing? For keys like Tab/Enter/Backspace I purposely left them out because they are not printable.

> I'll check the test part when I'm on train to go back Osaka (I'm in Tokyo
> office now).

Sure, no problem!

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 99 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #98)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #97)
> > Looks good except _USKeyboardKeyCodesFromPrintableCode. Please complete
> > that. If you'd like me to create that, I'll do that (will post the array as
> > a comment). Feel free to tell me.
> 
> Could you tell me which part is missing? For keys like Tab/Enter/Backspace I
> purposely left them out because they are not printable.

Oh, I understand. You try convert from code to keyCode *only* for printable keys. But I think that non-printable keys should be done that too since there keyCode value is still necessary when Gecko handles such keys internally.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 100 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #99)
> (In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to
> queue) from comment #98)
> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #97)
> > > Looks good except _USKeyboardKeyCodesFromPrintableCode. Please complete
> > > that. If you'd like me to create that, I'll do that (will post the array as
> > > a comment). Feel free to tell me.
> > 
> > Could you tell me which part is missing? For keys like Tab/Enter/Backspace I
> > purposely left them out because they are not printable.
> 
> Oh, I understand. You try convert from code to keyCode *only* for printable
> keys. But I think that non-printable keys should be done that too since
> there keyCode value is still necessary when Gecko handles such keys
> internally.

TIP would fill keyCode for non-printable keys automatically. e.g. 0x8 when key is set to "Enter". This is described in MDN and also verified on the test.

Having them listed there sounds like a duplication to me.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 101 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #92)
> And how do you think about the support of modifier keys? I think that
> keyboard apps should call modifier keydown and modifier keyup separately
> when user toggles the state on the keyboard. If so, the new API should throw
> an error when the key value is a modifier key's.

I've realized I didn't reply this.

I couldn't think of a good enough solution on top of my head to be honest. TIP.shareModifierStateOf() will not be able, and should not be used, to track modifier state across processes and pages. The state of modifiers should be managed by the app and the app alone, but the lifetime of the app(s) might be not last for the duration of input context (if the user switches apps or if the app crashes during a input context session).

I would prefer if we could tackle this in a follow-up bug to avoid scope creep.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 102 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #100)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #99)
> > (In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to
> > queue) from comment #98)
> > > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #97)
> > > > Looks good except _USKeyboardKeyCodesFromPrintableCode. Please complete
> > > > that. If you'd like me to create that, I'll do that (will post the array as
> > > > a comment). Feel free to tell me.
> > > 
> > > Could you tell me which part is missing? For keys like Tab/Enter/Backspace I
> > > purposely left them out because they are not printable.
> > 
> > Oh, I understand. You try convert from code to keyCode *only* for printable
> > keys. But I think that non-printable keys should be done that too since
> > there keyCode value is still necessary when Gecko handles such keys
> > internally.
> 
> TIP would fill keyCode for non-printable keys automatically. e.g. 0x8 when
> key is set to "Enter". This is described in MDN and also verified on the
> test.
> 
> Having them listed there sounds like a duplication to me.

Ah, I completely forgot that. You're right. Current code must be okay.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 103 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #101)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #92)
> > And how do you think about the support of modifier keys? I think that
> > keyboard apps should call modifier keydown and modifier keyup separately
> > when user toggles the state on the keyboard. If so, the new API should throw
> > an error when the key value is a modifier key's.
> 
> I've realized I didn't reply this.
> 
> I couldn't think of a good enough solution on top of my head to be honest.
> TIP.shareModifierStateOf() will not be able, and should not be used, to
> track modifier state across processes and pages.

I don't think modifier state should be shared between apps. Isn't it enough keeyboard apps to change modifier state separately? Does this cause some problems when keyboard apps are switched in an app?

> The state of modifiers
> should be managed by the app and the app alone, but the lifetime of the
> app(s) might be not last for the duration of input context (if the user
> switches apps or if the app crashes during a input context session).

forms.js cannot manage that?

> I would prefer if we could tackle this in a follow-up bug to avoid scope
> creep.

Anyway, I agree. We should discuss modifier state issue later. However, I think that new API should do nothing if keyboard apps try to send modifier key events because modifier state of TIP instance may be broken. Especially when the key is CapsLock, NumLock and etc...

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 104 • 9 years ago

Comment on attachment 8643554 [details] [diff] [review]
part 3 WIP, modification to sendKey()

>+function runTest() {
>+  let im = navigator.mozInputMethod;
>+  let eventDetails = [];
>+  let currentValue = '';
>+
>+  function assertEventDetail(expectedDetails, testName) {
>+    is(eventDetails.length, expectedDetails.length,
>+      testName + ' expects ' + expectedDetails.length + ' events');
>+
>+    expectedDetails.forEach((expectedDetail, j) => {
>+      for (let key in expectedDetail) {
>+        is(eventDetails[j][key], expectedDetail[key],
>+          testName + ' expects ' + key + ' of ' + eventDetails[j].type + ' to be equal to ' + expectedDetail[key]);
>+      }
>+    });
>+  }

I think that you should check if every modifier state is always false with .getModifeirState().

>+        if (testdata.expectedPrintable) {

Sounds odd. Non-printable keys except modifier keys should cause a keypress event if the preceding keydown event isn't consumed.

>+          expectedEventDetail.push({
>+            type: 'keypress',
>+            key: expectedValues.key,
>+            charCode: expectedValues.charCode,
>+            code: expectedValues.code || '',
>+            keyCode: expectedValues.keyCode < 0x20 ? expectedValues.keyCode : 0,

Odd. keyCode can be 0-255. If charCode is not 0, it must be 0.

>+        if (!testdata.expectedRepeat) {
>+          expectedEventDetail.push({
>+            type: 'keyup',
>+            key: expectedValues.key,
>+            charCode: 0,
>+            code: expectedValues.code || '',
>+            keyCode: expectedValues.keyCode || 0,
>+            location: 0,
>+            repeat: expectedValues.repeat || false,
>+            value: currentValue
>+          });
>+        }

I wonder when a key is released, how the keyup will be fired?

When a key is pressed,

keydown (reeat=false)
keypress (repeat=false, optional)

are fired. Then,

keydown (reeat=true)
keypress (repeat=true, optional)

are fired one or more times. Finally,

keyup (repeat=false)

should be fired.

>+  // Test the plain alphabets
>+  var codeA = 'A'.charCodeAt(0);
>+  for (var i = 0; i < 26; i++) {
>+    // callbacks in then() are deferred; must only reference these block-scoped
>+    // variable instead of i.
>+    let codePoint = codeA + i;
>+    let chr = String.fromCharCode(codePoint);
>+
>+    // Test plain upper caps alphabet
>+    promiseQueue = promiseQueue.then(() => {
>+      return sendKeyAndAssertResult({
>+        dict: {
>+          key: chr
>+        },
>+        expectedPrintable: true,
>+        expectedInput: chr,
>+        expectedValues: {
>+          key: chr, code: '',
>+          keyCode: 0,
>+          charCode: chr.charCodeAt(0)
>+        }
>+      });

Why should the keyCode be 0 in this case? Isn't the chr [A-Z]? Probably, I misunderstood the rule. I think that this is important for the compatibility with web applications. The .keyCode value of dictionary should be respected only when |!dict.code && !("keyCode" in dict)|, isn't it?

>+    });
>+
>+    // Test plain lower caps alphabet
>+    promiseQueue = promiseQueue.then(() => {
>+      return sendKeyAndAssertResult({
>+        dict: {
>+          key: chr.toLowerCase()
>+        },
>+        expectedPrintable: true,
>+        expectedInput: chr.toLowerCase(),
>+        expectedValues: {
>+          key: chr.toLowerCase(), code: '',
>+          keyCode: 0,

Same.

Other alphabet test cases look good. But I hope that you test some keys on Dvorak layout. E.g., 'O' key on Dvorak should input 'o' or 'O' and the keyCode should be DOM_VK_O. However, the code value should be "KeyS". So, any alphabet should be able to be input with KeyA or something.

>+    // Test plain symbol
>+    promiseQueue = promiseQueue.then(() => {
>+      return sendKeyAndAssertResult({
>+        dict: {
>+          key: chr
>+        },
>+        expectedPrintable: true,
>+        expectedInput: chr,
>+        expectedValues: {
>+          key: chr, code: '', keyCode: 0,

Same as alphabet's case.

And I wonder if dict has all of 'code', 'key' and 'keyCode' but the key value isn't [a-zA-Z0-9], shouldn't we respect the keyCode value coming from the keyboard apps?

And also I realized that you don't handle Numpad*. But it's okay to put off of fixing that case in another bug.

>+  // Test Greek letters
>+  var greekLetters =
>+    'ÎÎÎÎÎÎÎÎÎÎÎÎÎÎÎΠΡΣΤΥΦΧΨΩαβγδεζηθικλμνξοÏÏÏÏÏÏÏÏÏÏ';

Don't use non-ASCII characters directly become somebody may overwrite the file with non-UTF8-aware text editor. Please use "\uXXXX" instead.

>+    // Test plain alphabet
>+    promiseQueue = promiseQueue.then(() => {
>+      return sendKeyAndAssertResult({
>+        dict: {
>+          key: chr
>+        },
>+        expectedPrintable: true,
>+        expectedInput: chr,
>+        expectedValues: {
>+          key: chr, code: '', keyCode: 0,

So, in this case, keyCode should be 0 because there is really no hint of mapping to A-Z.

>+  // Test numbers
>+  var code0 = '0'.charCodeAt(0);
>+  for (var i = 0; i < 10; i++) {
>+    // callbacks in then() are deferred; must only reference these block-scoped
>+    // variable instead of i.
>+    let codePoint = code0 + i;
>+    let chr = String.fromCharCode(codePoint);
>+
>+    // Test plain number
>+    promiseQueue = promiseQueue.then(() => {
>+      return sendKeyAndAssertResult({
>+        dict: {
>+          key: chr
>+        },
>+        expectedPrintable: true,
>+        expectedInput: chr,
>+        expectedValues: {
>+          key: chr, code: '', keyCode: 0,

So, in this case, we should guess the keyCode value because of the chr is [0-9].

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 105 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #103)
> Anyway, I agree. We should discuss modifier state issue later. However, I
> think that new API should do nothing if keyboard apps try to send modifier
> key events because modifier state of TIP instance may be broken. Especially
> when the key is CapsLock, NumLock and etc...

Is that achievable if we block a set of |code| from being sent from MozKeyboard.js? Since there is currently no use case for that, we could do so first here.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #103)
> > I couldn't think of a good enough solution on top of my head to be honest.
> > TIP.shareModifierStateOf() will not be able, and should not be used, to
> > track modifier state across processes and pages.
> 
> I don't think modifier state should be shared between apps. Isn't it enough
> keeyboard apps to change modifier state separately? Does this cause some
> problems when keyboard apps are switched in an app?
> 
> > The state of modifiers
> > should be managed by the app and the app alone, but the lifetime of the
> > app(s) might be not last for the duration of input context (if the user
> > switches apps or if the app crashes during a input context session).
> 
> forms.js cannot manage that?
> 

It probably could. Let's do it in another bug then.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 106 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #104)
> >+    // Test plain upper caps alphabet
> >+    promiseQueue = promiseQueue.then(() => {
> >+      return sendKeyAndAssertResult({
> >+        dict: {
> >+          key: chr
> >+        },
> >+        expectedPrintable: true,
> >+        expectedInput: chr,
> >+        expectedValues: {
> >+          key: chr, code: '',
> >+          keyCode: 0,
> >+          charCode: chr.charCodeAt(0)
> >+        }
> >+      });
> 
> Why should the keyCode be 0 in this case? Isn't the chr [A-Z]? Probably, I
> misunderstood the rule. I think that this is important for the compatibility
> with web applications. The .keyCode value of dictionary should be respected
> only when |!dict.code && !("keyCode" in dict)|, isn't it?
> 

That's not what we discussed during our previous review. Just to confirm, your expectation is:

1. sendKey({ key: 'A' }) and sendKey({ key: 'A', keyCode: 0 }) => keyCode = DOM_VK_A
2. sendKey({ key: 'A', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_A
3. sendKey({ key: 'A', code: 'KeyX' }) => keyCode = DOM_VK_A
4. sendKey({ key: 'A', code: 'KeyX', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_A
5. sendKey({ key: 'Σ' }) and sendKey({ key: 'Σ', keyCode: 0 }) => keyCode = 0
6. sendKey({ key: 'Σ', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_Y
7. sendKey({ key: 'Σ', code: 'KeyX' }) => keyCode = DOM_VK_X
8. sendKey({ key: 'Σ', code: 'KeyX', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_X

(1) would help compatibility
(2) ignores incorrectly set keyCode
(3) helps Dvorak or AZERTY case
(4) helps Dvorak or AZERTY case and ignores incorrectly set keyCode
(5) keeps zero as there is no hint in anyway
(6) respects the keyCode set
(7) sets keyCode from code
(8) sets keyCode from code and ignore incorrectly set keyCode

I will codify this in _setKeyCode and update the test accordingly.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 107 • 9 years ago

... and

 9. sendKey({ key: '!' }) and sendKey({ key: '!', keyCode: 0 }) => keyCode = DOM_VK_1
10. sendKey({ key: '!', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_1
11. sendKey({ key: '!', code: 'KeyX' }) => keyCode = DOM_VK_X
12. sendKey({ key: '!', code: 'KeyX', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_X

In which the keyCode of (9) and (10) comes from _USKeyboardKeyCodesFromPrintableChars.

14. sendKey({ key: 'Σ', code: 'BracketLeft' }) => keyCode = DOM_VK_OPEN_BRACKET
15. sendKey({ key: 'Σ', code: 'BracketLeft', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_OPEN_BRACKET

In which the keyCodes comes from _USKeyboardKeyCodesFromPrintableCode.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 108 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #107)
> ... and
> 
> 10. sendKey({ key: '!', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode =
> DOM_VK_1

This should be DOM_VK_Y to help the Dvorak or AZERTY case?

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 109 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #107)
> 11. sendKey({ key: '!', code: 'KeyX' }) => keyCode = DOM_VK_X

Sorry, the change needed is (11). This needs to be DOM_VK_1 for us to support Dvorak case.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 110 • 9 years ago

Tests updated with Dvorak keys. Please inspect the test cases to see if this are the desired results.

1) Dvorak assign some symbols on Qwerty alphabet keys, and we would still have to emit keyCode for the symbol instead of the alphabet keys. Therefore, matches on |_USKeyboardKeyCodesFromPrintableChars| would still have to be done before matching |code|.

2) expectedPrintable should actually be expectedKeypress. My mistake, Sorry.

3) On repeat. You are right on the event sequences, but I cannot fix that with the current WIP. We would need the keyup() method on that.

Let's get the keyCode algorithm settled and tests verified, and I will follow up to finishing keyup() and keydown() methods, as previously commented.

Thank you for the feedback!

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 111 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #106)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #104)
> > >+    // Test plain upper caps alphabet
> > >+    promiseQueue = promiseQueue.then(() => {
> > >+      return sendKeyAndAssertResult({
> > >+        dict: {
> > >+          key: chr
> > >+        },
> > >+        expectedPrintable: true,
> > >+        expectedInput: chr,
> > >+        expectedValues: {
> > >+          key: chr, code: '',
> > >+          keyCode: 0,
> > >+          charCode: chr.charCodeAt(0)
> > >+        }
> > >+      });
> > 
> > Why should the keyCode be 0 in this case? Isn't the chr [A-Z]? Probably, I
> > misunderstood the rule. I think that this is important for the compatibility
> > with web applications. The .keyCode value of dictionary should be respected
> > only when |!dict.code && !("keyCode" in dict)|, isn't it?
> > 
> 
> That's not what we discussed during our previous review. Just to confirm,
> your expectation is:
> 
> 1. sendKey({ key: 'A' }) and sendKey({ key: 'A', keyCode: 0 }) => keyCode = DOM_VK_A
> 2. sendKey({ key: 'A', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_A
> 3. sendKey({ key: 'A', code: 'KeyX' }) => keyCode = DOM_VK_A
> 4. sendKey({ key: 'A', code: 'KeyX', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_A
> 5. sendKey({ key: 'Σ' }) and sendKey({ key: 'Σ', keyCode: 0 }) => keyCode = 0
> 6. sendKey({ key: 'Σ', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_Y
> 7. sendKey({ key: 'Σ', code: 'KeyX' }) => keyCode = DOM_VK_X
> 8. sendKey({ key: 'Σ', code: 'KeyX', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_X

#8 looks odd but such case should be a bug of the keyboard apps. So, it *might* be better to causes throwing exception. However, either is okay.

Anyway, that's what I'm expecting.

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #107)
> ... and
> 
>  9. sendKey({ key: '!' }) and sendKey({ key: '!', keyCode: 0 }) => keyCode = DOM_VK_1

Yeah, okay. Looks reasonable.

> 10. sendKey({ key: '!', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_1

Reasonable too.

> 11. sendKey({ key: '!', code: 'KeyX' }) => keyCode = DOM_VK_X

Hmm, such case shouldn't be... It's odd to input symbols with Key[A-Z]. But IIRC, there is such keyboard layout... In this case, we don't have no proper information. Can we set 0 in this case?

> 12. sendKey({ key: '!', code: 'KeyX', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_X

And in this case, I guess respecting .keyCode is better because guessing from code value may make some impossible case.

So, I think that if key is [a-zA-Z0-9], we should ignore odd keyCode value. However, for other cases, it's better to respect the keyboard app's keyCode value.

> 14. sendKey({ key: 'Σ', code: 'BracketLeft' }) => keyCode = DOM_VK_OPEN_BRACKET

Although, in this case, 0 is also okay. But probably, this is better for compatibility with Gecko for desktop in most cases.

> 15. sendKey({ key: 'Σ', code: 'BracketLeft', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_OPEN_BRACKET

So, if keyCode is *not* DOM_VK_[A-Z0-9], it seems that we should respect the specified keyCode. But such limitation must be redundant. I think that if keyCode is specified but key value isn't [a-zA-Z0-9], we should respect the specified value.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 112 • 9 years ago

So, I think that we should check that even if keyCode is specified but it doesn't match with key value when key value is [a-zA-Z0-9], we should ignore the keyCode value and compute it from the key value. Otherwise, keyCode value should be respected. If key isn't [a-zA-Z0-9] nor keyCode isn't specified, code value should be a hint of guessing keyCode.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 113 • 9 years ago

Comment on attachment 8644260 [details] [diff] [review]
part 3 WIP, modification to sendKey()

>+  // Test Dvorak layout emulation
>+  var qwertyCodeForDvorakKeys = [
>+    'KeyR', 'KeyT', 'KeyY', 'KeyU', 'KeyI', 'KeyO', 'KeyP',
>+    'KeyA', 'KeyS', 'KeyD', 'KeyF', 'KeyG',
>+    'KeyH', 'KeyJ', 'KeyK', 'KeyL', 'Semicolon',
>+    'KeyX', 'KeyC', 'KeyV', 'KeyB', 'KeyN',
>+    'KeyM', 'Comma', 'Period', 'Slash'];
>+  var dvorakKeys = 'PYFGCRL' +
>+    'AOEUIDHTNS' +
>+    'QJKXBMWVZ';
>+  for (var i = 0; i < dvorakKeys.length; i++) {
>+    // callbacks in then() are deferred; must only reference these block-scoped
>+    // variable instead of i.
>+    let keyCode = dvorakKeys.charCodeAt(i);
>+    let code = qwertyCodeForDvorakKeys[i];
>+
>+    [dvorakKeys.charAt(i), dvorakKeys.charAt(i).toLowerCase()]
>+    .forEach((chr) => {
>+      // Test alphabet with code set to Qwerty code,
>+      // expects keyCode to follow key value.
>+      promiseQueue = promiseQueue.then(() => {
>+        return sendKeyAndAssertResult({
>+          dict: {
>+            key: chr,
>+            code: code
>+          },
>+          expectedKeypress: true,
>+          expectedInput: chr,
>+          expectedValues: {
>+            key: chr, code: code,
>+            keyCode: keyCode,
>+            charCode: chr.charCodeAt(0)
>+          }
>+        });
>+      });
>+    });
>+  }

Looks good.

>+  var qwertyCodeForDvorakSymbols = [
>+    'Minus', 'Equal',
>+    'KeyQ', 'KeyW', 'KeyE', 'BracketLeft', 'BracketRight', 'Backslash',
>+    'Quote', 'KeyZ'];
>+
>+  var shiftDvorakSymbols = '{}\"<>?+|_:';
>+  var dvorakSymbols = '[]\',./=\\-;';
>+  var dvorakSymbolsKeyCodes = [
>+    KeyboardEvent.DOM_VK_OPEN_BRACKET,
>+    KeyboardEvent.DOM_VK_CLOSE_BRACKET,
>+    KeyboardEvent.DOM_VK_QUOTE,
>+    KeyboardEvent.DOM_VK_COMMA,
>+    KeyboardEvent.DOM_VK_PERIOD,
>+    KeyboardEvent.DOM_VK_SLASH,
>+    KeyboardEvent.DOM_VK_EQUALS,
>+    KeyboardEvent.DOM_VK_BACK_SLASH,
>+    KeyboardEvent.DOM_VK_HYPHEN_MINUS,
>+    KeyboardEvent.DOM_VK_SEMICOLON
>+  ];
>+
>+  for (var i = 0; i < dvorakSymbols.length; i++) {
>+    // callbacks in then() are deferred; must only reference these block-scoped
>+    // variable instead of i.
>+    let keyCode = dvorakSymbolsKeyCodes[i];
>+    let code = qwertyCodeForDvorakSymbols[i];
>+
>+    [dvorakSymbols.charAt(i), shiftDvorakSymbols.charAt(i)]
>+    .forEach((chr) => {
>+      // Test symbols with code set to Qwerty code,
>+      // expects keyCode to follow key value.
>+      promiseQueue = promiseQueue.then(() => {
>+        return sendKeyAndAssertResult({
>+          dict: {
>+            key: chr,
>+            code: code
>+          },

In this case, keyboard app *should* specify keyCode value explicitly. Otherwise, guessing from key value may be okay... (So, my previous comment may be wrong)

Anyway, we should document in MDN for keyboard app developers how code value and keyCode value should be specified.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 114 • 9 years ago

So, we should document following points (at least):

* New argument spec.
* New keyboard apps should be new style argument.
* If the keyboard app emulates physical keyboard, it should set proper code value.
* If the keyboard app does not emulate physical keyboard, it should not set code value (or set empty string).
* As far as possible, keyCode value should be specified:
  * If the keyboard app emulates physical keyboard, it should be same value as the result of Gecko for Desktop.
  * If the keyboard app does not emulate physical keyboard and the key value is a character in ASCII range, the keyCode value should be one of DOM_VK_* except DOM_VK_[A-Z0-9].
  * If the keyboard app does not emulate physical keyboard and the key value is not an ASCII character, the keyCode value should be 0.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 115 • 9 years ago

Masayuki,

I am a little confused. at comment 111 we already have our rules established. I agree we should document and educate how keyboard app should use the API, but it should be consistent with the rules of determine keyCode. I have rewritten comment 111 to the rules below and I have changed _getKeyCode() and tests to follow these rules:

Rule I: For [a-zA-Z0-9] (alphanumeric), always respect |key| regardless:

1. sendKey({ key: 'A' }) and sendKey({ key: 'A', keyCode: 0 }) => keyCode = DOM_VK_A
2. sendKey({ key: 'A', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_A
3. sendKey({ key: 'A', code: 'KeyX' }) => keyCode = DOM_VK_A
4. sendKey({ key: 'A', code: 'KeyX', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_A

Rule II: If keyCode is set to non-zero, always use it before considering |key| or |code|:

6. sendKey({ key: 'Σ', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_Y
8. sendKey({ key: 'Σ', code: 'KeyX', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_Y
10. sendKey({ key: '!', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_Y
12. sendKey({ key: '!', code: 'KeyX', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_Y
14. sendKey({ key: 'Σ', code: 'BracketLeft', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_Y

Rule III: If the |key| is a printable non-alphanumeric on US Keyboard, use the keyCode of it on US Keyboard:

9. sendKey({ key: '!' }) and sendKey({ key: '!', keyCode: 0 }) => keyCode = DOM_VK_1
11. sendKey({ key: '!', code: 'KeyX' }) => keyCode = DOM_VK_1

Rule IV: If |code| is set, use keyCode of the US Keyboard this |code| would generate:

7. sendKey({ key: 'Σ', code: 'KeyX' }) => keyCode = DOM_VK_X
13. sendKey({ key: 'Σ', code: 'BracketLeft' }) => keyCode = DOM_VK_OPEN_BRACKET

Rule V: Set keyCode = 0 when all of the rules above does not apply:

5. sendKey({ key: 'Σ' }) and sendKey({ key: 'Σ', keyCode: 0 }) => keyCode = 0


(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #113)
> >+    [dvorakSymbols.charAt(i), shiftDvorakSymbols.charAt(i)]
> >+    .forEach((chr) => {
> >+      // Test symbols with code set to Qwerty code,
> >+      // expects keyCode to follow key value.
> >+      promiseQueue = promiseQueue.then(() => {
> >+        return sendKeyAndAssertResult({
> >+          dict: {
> >+            key: chr,
> >+            code: code
> >+          },
> 
> In this case, keyboard app *should* specify keyCode value explicitly.
> Otherwise, guessing from key value may be okay... (So, my previous comment
> may be wrong)
> 
> Anyway, we should document in MDN for keyboard app developers how code value
> and keyCode value should be specified.

This is conflicting to me because our rules explicitly ensured Dvorak alphanumeric and symbols will generate the right keyCodes from |key|, since they are just the same set of characters on Qwerty. I thought the purpose of the tests are asserting the behavior, not to say what to do and re-test what's already tested on the alphanumeric section.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #114)
>   * If the keyboard app does not emulate physical keyboard and the key value
> is not an ASCII character, the keyCode value should be 0.

Are we going to allow keyCode to be set to zero? The current rule doesn't cover that and will always find a keyCode on US Keyboard for non-alphanumeric on US Keyboard.

Please clarify the right keyCode rules. Thanks.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 116 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #115)
> Masayuki,
> 
> I am a little confused. at comment 111 we already have our rules
> established.

Yeah, sorry. But I'm thinking that the rules we've considered isn't enough.

> I agree we should document and educate how keyboard app should
> use the API, but it should be consistent with the rules of determine
> keyCode.

Yes, of course.

> Rule I: For [a-zA-Z0-9] (alphanumeric), always respect |key| regardless:
> 
> 1. sendKey({ key: 'A' }) and sendKey({ key: 'A', keyCode: 0 }) => keyCode =
> DOM_VK_A
> 2. sendKey({ key: 'A', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode =
> DOM_VK_A
> 3. sendKey({ key: 'A', code: 'KeyX' }) => keyCode = DOM_VK_A
> 4. sendKey({ key: 'A', code: 'KeyX', keyCode: KeyboardEvent.DOM_VK_Y }) =>
> keyCode = DOM_VK_A

Yes, I completely agree with this case.

> Rule II: If keyCode is set to non-zero, always use it before considering
> |key| or |code|:
> 
> 6. sendKey({ key: 'Σ', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode =
> DOM_VK_Y
> 8. sendKey({ key: 'Σ', code: 'KeyX', keyCode: KeyboardEvent.DOM_VK_Y }) =>
> keyCode = DOM_VK_Y
> 10. sendKey({ key: '!', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode =
> DOM_VK_Y
> 12. sendKey({ key: '!', code: 'KeyX', keyCode: KeyboardEvent.DOM_VK_Y }) =>
> keyCode = DOM_VK_Y
> 14. sendKey({ key: 'Σ', code: 'BracketLeft', keyCode: KeyboardEvent.DOM_VK_Y
> }) => keyCode = DOM_VK_Y

I agree.

> Rule III: If the |key| is a printable non-alphanumeric on US Keyboard, use
> the keyCode of it on US Keyboard:
> 
> 9. sendKey({ key: '!' }) and sendKey({ key: '!', keyCode: 0 }) => keyCode =
> DOM_VK_1

I don't think that this is good. If keyCode is explicitly specified, we should respect that because if we don't do so, keyboard apps may not be able to emulate some physical keyboard layout completely.

> 11. sendKey({ key: '!', code: 'KeyX' }) => keyCode = DOM_VK_1

I believe that this computation isn't good for compatibility. As far as I know, there is no keyboard layout which causes '1' in an alphabet key position of QWERTY. So, I guess that in this case, we should compute the keyCode from the key value. In the rules of Gecko for Desktop, this key may cause DOM_VK_EXCLAMATION if it can be inputted without Shift key state or the key inputs a unicode character without shift. I think that only when the code value is one of Digit[0-9], DOM_VK_[0-9] are properly. Otherwise, we should guess from key value:

":", DOM_VK_COLON
";", DOM_VK_SEMICOLON
"<", DOM_VK_LESS_THAN
"=", DOM_VK_EQUALS
">", DOM_VK_GREATER_THAN
"?", DOM_VK_QUESTION_MARK
"@", DOM_VK_AT
"^", DOM_VK_CIRCUMFLEX
"!", DOM_VK_EXCLAMATION
"\"", DOM_VK_DOUBLE_QUOTE
"#", DOM_VK_HASH
"$", DOM_VK_DOLLAR
"%", DOM_VK_PERCENT
"&", DOM_VK_AMPERSAND
"_", DOM_VK_UNDERSCORE
"(", DOM_VK_OPEN_PAREN
")", DOM_VK_CLOSE_PAREN
"*", DOM_VK_ASTERISK
"+", DOM_VK_PLUS
"|", DOM_VK_PIPE
"-", DOM_VK_HYPHEN_MINUS
"{", DOM_VK_OPEN_CURLY_BRACKET
"}", DOM_VK_CLOSE_CURLY_BRACKET
"~", DOM_VK_TILDE
",", DOM_VK_COMMA
".", DOM_VK_PERIOD
"/", DOM_VK_SLASH
"`", DOM_VK_BACK_QUOTE
"[", DOM_VK_OPEN_BRACKET
"\\", DOM_VK_BACK_SLASH
"]", DOM_VK_CLOSE_BRACKET
"'", DOM_VK_QUOTE

As I mentioned above, in this case, keyboard apps should specify keyCode explicitly.

> Rule IV: If |code| is set, use keyCode of the US Keyboard this |code| would
> generate:
> 
> 7. sendKey({ key: 'Σ', code: 'KeyX' }) => keyCode = DOM_VK_X
> 13. sendKey({ key: 'Σ', code: 'BracketLeft' }) => keyCode =
> DOM_VK_OPEN_BRACKET

This is not solid, but must be better than 0. So, I agree.

> Rule V: Set keyCode = 0 when all of the rules above does not apply:
> 
> 5. sendKey({ key: 'Σ' }) and sendKey({ key: 'Σ', keyCode: 0 }) => keyCode = 0

Absolutely.

> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #113)
> > >+    [dvorakSymbols.charAt(i), shiftDvorakSymbols.charAt(i)]
> > >+    .forEach((chr) => {
> > >+      // Test symbols with code set to Qwerty code,
> > >+      // expects keyCode to follow key value.
> > >+      promiseQueue = promiseQueue.then(() => {
> > >+        return sendKeyAndAssertResult({
> > >+          dict: {
> > >+            key: chr,
> > >+            code: code
> > >+          },
> > 
> > In this case, keyboard app *should* specify keyCode value explicitly.
> > Otherwise, guessing from key value may be okay... (So, my previous comment
> > may be wrong)
> > 
> > Anyway, we should document in MDN for keyboard app developers how code value
> > and keyCode value should be specified.
> 
> This is conflicting to me because our rules explicitly ensured Dvorak
> alphanumeric and symbols will generate the right keyCodes from |key|, since
> they are just the same set of characters on Qwerty. I thought the purpose of
> the tests are asserting the behavior, not to say what to do and re-test
> what's already tested on the alphanumeric section.

I'd like the test checks if the ability to implement Dvorak keyboard layout. (Dvorak is just an example, not all of my worry)

Important things are:
* any characters can be input from any physical key.
* DOM_VK_[A-Z] shouldn't be *guessed* when neither key value is [a-zA-Z] nor code value is Key[A-Z].
* DOM_VK_[0-9] shouldn't be *guessed* when neither key value is [0-9] nor code value is Digit[0-9].
* If keyCode is explicitly specified and it's not odd (e.g., key=a, keyCode=DOM_VK_X), it should be respected.

> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #114)
> >   * If the keyboard app does not emulate physical keyboard and the key value
> > is not an ASCII character, the keyCode value should be 0.
> 
> Are we going to allow keyCode to be set to zero? The current rule doesn't
> cover that and will always find a keyCode on US Keyboard for
> non-alphanumeric on US Keyboard.

Yeah, I think that keyboard apps can specify keyCode 0 explicitly. For example, in Japan, the majority of VKB on mobile phone is 10 key style and each key can input 5 Hiragana characters (selected by swipe or toggled by tapping same key). In this case, each key event's keyCode should be 0 because such key shouldn't be mapped to any physical keyboard's key even for emulating for compatibility.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 117 • 9 years ago

At the last example, current your code also computes the keyCode as 0, but keyboard app developers must want that specifying 0 won't cause different keyCode value in the future releases. So, allowing 0 makes our API stabler for keyboard app develpers.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 118 • 9 years ago

And I apologize that I couldn't suggest the best approach first. But I'm still not sure what is the best. This is very complicated problem. But I should've been more careful.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 119 • 9 years ago

Hi Masayuki,

Please consider the updated rule here. I removed the original Rule III (hence the need of _USKeyboardKeyCodesFromPrintableChars table). The US keyboard symbols ('!' in our examples) now behaves the same way like Greek alphabets ('Σ' in our examples).

With these rules:

0. Rule I is in place to ensure consistency across apps for alphanumeric outputs.
1. Keyboard app emulating physical Dvorak keyboard would need to rely on rule II to generate correct keyCode, by explicitly set all the keyCodes that doesn't fall into rule I.
2. Keyboard app emulating physical standard US keyboard can forget rule II, and instead rely on rule III to generate keyCode from code.
3. Keyboard app *not* emulating physical keyboard can/should set keyCode to 0 and rely on rule II. Of course, it should not set a code value either.

===

Rule I: For [a-zA-Z0-9] (alphanumeric), always respect |key| regardless of |keyCode| or |code| set:

sendKey({ key: 'A' }) => keyCode = DOM_VK_A
sendKey({ key: 'A', keyCode: 0 }) => keyCode = DOM_VK_A
sendKey({ key: 'A', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_A
sendKey({ key: 'A', code: 'KeyX' }) => keyCode = DOM_VK_A
sendKey({ key: 'A', code: 'BracketLeft' }) => keyCode = DOM_VK_A
sendKey({ key: 'A', code: 'KeyX', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_A
sendKey({ key: 'A', code: 'BracketLeft', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_A
sendKey({ key: 'A', code: 'KeyX', keyCode: 0 }) => keyCode = DOM_VK_A
sendKey({ key: 'A', code: 'BracketLeft', keyCode: 0 }) => keyCode = DOM_VK_A

Rule II: If |keyCode| is set (even with zero), always use it before considering other |key| or |code|:

sendKey({ key: 'Σ', keyCode: 0 }) => keyCode = 0
sendKey({ key: 'Σ', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_Y
sendKey({ key: 'Σ', code: 'KeyX', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_Y
sendKey({ key: 'Σ', code: 'BracketLeft', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_Y
sendKey({ key: 'Σ', code: 'KeyX', keyCode: 0 }) => keyCode = 0
sendKey({ key: 'Σ', code: 'BracketLeft', keyCode: 0 }) => keyCode = 0
sendKey({ key: '!', keyCode: 0 }) => keyCode = 0
sendKey({ key: '!', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_Y
sendKey({ key: '!', code: 'KeyX', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_Y
sendKey({ key: '!', code: 'BracketLeft', keyCode: KeyboardEvent.DOM_VK_Y }) => keyCode = DOM_VK_Y
sendKey({ key: '!', code: 'KeyX', keyCode: 0 }) => keyCode = 0
sendKey({ key: '!', code: 'BracketLeft', keyCode: 0 }) => keyCode = 0

Rule III: If |code| is set, use keyCode of the US Keyboard this |code| would generate:

sendKey({ key: '!', code: 'KeyX' }) => keyCode = DOM_VK_X
sendKey({ key: '!', code: 'BracketLeft' }) => keyCode = DOM_VK_OPEN_BRACKET
sendKey({ key: 'Σ', code: 'KeyX' }) => keyCode = DOM_VK_X
sendKey({ key: 'Σ', code: 'BracketLeft' }) => keyCode = DOM_VK_OPEN_BRACKET

Rule IV: Set keyCode = 0 when all of the rules above does not apply:

sendKey({ key: '!' }) => keyCode = 0
sendKey({ key: 'Σ' }) => keyCode = 0

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 120 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #119)
> With these rules:
> 
> 0. Rule I is in place to ensure consistency across apps for alphanumeric
> outputs.

Absolutely.

> 1. Keyboard app emulating physical Dvorak keyboard would need to rely on
> rule II to generate correct keyCode, by explicitly set all the keyCodes that
> doesn't fall into rule I.

Sounds good.

> 2. Keyboard app emulating physical standard US keyboard can forget rule II,
> and instead rely on rule III to generate keyCode from code.

Basically, I agree, but I have some objects. See below.

> 3. Keyboard app *not* emulating physical keyboard can/should set keyCode to
> 0 and rely on rule II. Of course, it should not set a code value either.

Sounds good.

> Rule III: If |code| is set, use keyCode of the US Keyboard this |code| would
> generate:
> 
> sendKey({ key: '!', code: 'KeyX' }) => keyCode = DOM_VK_X

I think that DOM_VK_[A-Z0-9] shouldn't be computed from code value when key value is an ASCII character but not [a-zA-Z0-9]. If we do this, keyCode may become untrustable attribute.

> sendKey({ key: '!', code: 'BracketLeft' }) => keyCode = DOM_VK_OPEN_BRACKET

This could cause odd mapping. However, I think that it's enough that we should recommend that keyboard apps should specify keyCode by itself. So, this is okay for easier implementing US keyboard layout compatible layout.

> sendKey({ key: 'Σ', code: 'KeyX' }) => keyCode = DOM_VK_X

If key is a unicode character, I guess that it's better to guess from code.

> sendKey({ key: 'Σ', code: 'BracketLeft' }) => keyCode = DOM_VK_OPEN_BRACKET

This case looks odd, but keyboard apps should specify keyCode explicitly in this case. For allowing to create US keyboard layout compatible layout easier, we should allow this case (or if it makes your patch messy, 0 is better in this case).


So, I think that:

* When code is Key[A-Z] but the key is not [a-zA-Z] but is an ASCII character, keyCode should be 0.

The highest priority of my idea is that DOM_VK_[A-Z] should be set only when the key causes inputting [a-zA-Z]. That makes web apps believe the keyCode values. If the values may be set in some odd cases, they may be confused and complain about that.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 121 • 9 years ago

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

How about just giving up rule III and stop guessing keyCode from |code| altogether? We would give up this:

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #119)
> 2. Keyboard app emulating physical standard US keyboard can forget rule II,
> and instead rely on rule III to generate keyCode from code.

but we would get clarity on the patch and the behavior the method. WDYT?

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 122 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #121)

Rule III, while help us emulating physical standard US keyboard, generate odd mapping if not used carefully. Given the fact we already have rule II to delegate the responsibility to set a keyCode to all cases other than the rule I cases ([a-zA-Z0-9] keys), it's value is limited.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 123 • 9 years ago

> How about just giving up rule III and stop guessing keyCode from |code| altogether?

I think that it's resonable. Basically, web apps shouldn't handle key event with keyCode value except when the value is DOM_VK_[A-Z0-9]. So, other ASCII symbol characters cause keyCode 0 shouldn't break most part of the web.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 124 • 9 years ago

There is an approach. When we cannot guess keyCode value due to only code is specified, it *might* be better to throw an exception. Then, keyboard app developers must look for a good keyCode value in such case.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 125 • 9 years ago

Thank you for your comment. I will be completing part 3 with other methods and send you for review, with removal of rule III.

I don't think we should throw an exception because for non-printable keys the keyCode will be set by TIP in forms.js anyway -- we shouldn't be explicitly asking for a keyCode for content and then throw it away.

(The alternative would be again maintain list of non-printable keys in MozKeyboard.js and filter it properly, but that sound like an overkill to me.)

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 126 • 9 years ago

Patch updated with what's previously discussed.

- Removed the _getKeyCode() method and create a _getkeyboardEventDict() method to contain it's feature, since there is only two rules left and we would need to process dict in many methods.
- Added trailing optional argument on for setComposition() and endComposition() methods and pass the dict to forms.js to generate keyboard events during compositions.
- Added keydown() and keyup() methods. I am reusing the SendKey IPC message because most of the code are duplicates.
- Comments on WebIDL updated and move the definition of the dict into it's own MozInputMethodKeyboardEventDict.

There is one strange thing which require me to patch WebIDL.py: WebIDL refuse to process

interface MozInputContext: EventTarget {
  ...
  Promise<boolean> keydown(MozInputMethodKeyboardEventDict dict);
  ...
}

and shows "Dictionary argument or union argument containing a dictionary not followed by a required argument must be optional" during build without my change there to check if the dict is the first argument.

I am not entirely sure if a method which only takes a required dict property is something unforeseen by WebIDL, or I am just not using the right syntax in my IDL. Please correct me if that's wrong; it's it's really needed I would move that to another bug and patch the WebIDL rule separately (I hope I know Python enough for that ...).

The related line is here https://hg.mozilla.org/mozilla-central/annotate/38c1ea9ccae31700630f1fe0d651e94c0c5b9e1d/dom/bindings/parser/WebIDL.py#l4517

Thank you for your help!

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 127 • 9 years ago

Oh, I've also re-organized the tests into their own functions so it's easier to comment out a specific section. Assertion would also come with the section name.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 128 • 9 years ago

Comment on attachment 8646825 [details] [diff] [review]
part 3, Allow content to pass a dict representing the property of the keyboard event to send

Correct grammar of the commit message (updated desc on Bugzilla, patch updated offline).

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 129 • 9 years ago

So, false alarm on the WebIDL thing. :kanru pointed out I can use the |required| keyword on the dictionary to suppress the error. I can also use union types to kind-of overloading the sendKey() function. 

I am happy on the that we are going back to assert all types with WebIDL. I've removed unneeded type-checking code in JavaScript.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 130 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #129)
> So, false alarm on the WebIDL thing. :kanru pointed out I can use the
> |required| keyword on the dictionary to suppress the error. I can also use
> union types to kind-of overloading the sendKey() function. 
> 

Actually the |required| keyword cause setComposition() to throw if there isn't a dict. Need to investigate better way to write the IDL.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 131 • 9 years ago

Created another MozInputMethodRequiredKeyboardEventDict to workaround the problem stated in above comment.

There should be a better way to set up the WebIDL bindings but we could go through the review of the code first.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 132 • 9 years ago

Comment on attachment 8646990 [details] [diff] [review]
part 3, Allow content to pass a dict representing the property of the keyboard event to send

>+  // Take a MozInputMethodKeyboardEventDict dict, creates a keyboardEventDict
>+  // object that can be sent to forms.js
>+  _getkeyboardEventDict: function(dict) {
>+    if (typeof dict !== 'object' || !dict.key) {
>+      return;

Return undefined or something is clearer? I'm not sure up to you.

>+    if (/^[a-zA-Z0-9]$/.test(dict.key)) {
>+      // keyCode must follow the key value in this range;
>+      // disregard the keyCode from content.
>+      keyboardEventDict.keyCode = dict.key.toUpperCase().charCodeAt(0);
>+    } else if (typeof dict.keyCode === 'number') {
>+      // Allow keyCode to be specified for other key values.
>+      keyboardEventDict.keyCode = dict.keyCode;
>+
>+      // Allow keyCode to be explicitly set to zero.
>+      if (dict.keyCode === 0) {
>+        keyboardEventDict.flags |=
>+          Ci.nsITextInputProcessor.KEY_KEEP_KEYCODE_ZERO;
>+      }
>+    }

Oh, you specify nsITextInputProcessor.KEY_KEEP_KEYCODE_ZERO here but you don't remove this when the key is non-printable key. I guess that this causes non-printable keys don't set properly, isn't it?
http://mxr.mozilla.org/mozilla-central/source/dom/base/TextInputProcessor.cpp?rev=76a74dc6ccc9&mark=732-732,745-747#732

I guess that you should check |printable| here.

>-function guessKeyNameFromKeyCode(KeyboardEvent, aKeyCode) {
>+function guessKeyNameFromKeyCode(aKeyCode) {
>   switch (aKeyCode) {
>-    case KeyboardEvent.DOM_VK_CANCEL:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_CANCEL:
>       return "Cancel";
>-    case KeyboardEvent.DOM_VK_HELP:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_HELP:
>       return "Help";
>-    case KeyboardEvent.DOM_VK_BACK_SPACE:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_BACK_SPACE:
>       return "Backspace";
>-    case KeyboardEvent.DOM_VK_TAB:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_TAB:
>       return "Tab";
>-    case KeyboardEvent.DOM_VK_CLEAR:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_CLEAR:
>       return "Clear";
>-    case KeyboardEvent.DOM_VK_RETURN:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_RETURN:
>       return "Enter";
>-    case KeyboardEvent.DOM_VK_SHIFT:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_SHIFT:
>       return "Shift";
>-    case KeyboardEvent.DOM_VK_CONTROL:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_CONTROL:
>       return "Control";
>-    case KeyboardEvent.DOM_VK_ALT:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_ALT:
>       return "Alt";
>-    case KeyboardEvent.DOM_VK_PAUSE:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_PAUSE:
>       return "Pause";
>-    case KeyboardEvent.DOM_VK_EISU:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_EISU:
>       return "Eisu";
>-    case KeyboardEvent.DOM_VK_ESCAPE:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_ESCAPE:
>       return "Escape";
>-    case KeyboardEvent.DOM_VK_CONVERT:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_CONVERT:
>       return "Convert";
>-    case KeyboardEvent.DOM_VK_NONCONVERT:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_NONCONVERT:
>       return "NonConvert";
>-    case KeyboardEvent.DOM_VK_ACCEPT:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_ACCEPT:
>       return "Accept";
>-    case KeyboardEvent.DOM_VK_MODECHANGE:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_MODECHANGE:
>       return "ModeChange";
>-    case KeyboardEvent.DOM_VK_PAGE_UP:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_PAGE_UP:
>       return "PageUp";
>-    case KeyboardEvent.DOM_VK_PAGE_DOWN:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_PAGE_DOWN:
>       return "PageDown";
>-    case KeyboardEvent.DOM_VK_END:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_END:
>       return "End";
>-    case KeyboardEvent.DOM_VK_HOME:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_HOME:
>       return "Home";
>-    case KeyboardEvent.DOM_VK_LEFT:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_LEFT:
>       return "ArrowLeft";
>-    case KeyboardEvent.DOM_VK_UP:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_UP:
>       return "ArrowUp";
>-    case KeyboardEvent.DOM_VK_RIGHT:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_RIGHT:
>       return "ArrowRight";
>-    case KeyboardEvent.DOM_VK_DOWN:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_DOWN:
>       return "ArrowDown";
>-    case KeyboardEvent.DOM_VK_SELECT:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_SELECT:
>       return "Select";
>-    case KeyboardEvent.DOM_VK_PRINT:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_PRINT:
>       return "Print";
>-    case KeyboardEvent.DOM_VK_EXECUTE:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_EXECUTE:
>       return "Execute";
>-    case KeyboardEvent.DOM_VK_PRINTSCREEN:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_PRINTSCREEN:
>       return "PrintScreen";
>-    case KeyboardEvent.DOM_VK_INSERT:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_INSERT:
>       return "Insert";
>-    case KeyboardEvent.DOM_VK_DELETE:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_DELETE:
>       return "Delete";
>-    case KeyboardEvent.DOM_VK_WIN:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_WIN:
>       return "OS";
>-    case KeyboardEvent.DOM_VK_CONTEXT_MENU:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_CONTEXT_MENU:
>       return "ContextMenu";
>-    case KeyboardEvent.DOM_VK_SLEEP:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_SLEEP:
>       return "Standby";
>-    case KeyboardEvent.DOM_VK_F1:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F1:
>       return "F1";
>-    case KeyboardEvent.DOM_VK_F2:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F2:
>       return "F2";
>-    case KeyboardEvent.DOM_VK_F3:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F3:
>       return "F3";
>-    case KeyboardEvent.DOM_VK_F4:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F4:
>       return "F4";
>-    case KeyboardEvent.DOM_VK_F5:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F5:
>       return "F5";
>-    case KeyboardEvent.DOM_VK_F6:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F6:
>       return "F6";
>-    case KeyboardEvent.DOM_VK_F7:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F7:
>       return "F7";
>-    case KeyboardEvent.DOM_VK_F8:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F8:
>       return "F8";
>-    case KeyboardEvent.DOM_VK_F9:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F9:
>       return "F9";
>-    case KeyboardEvent.DOM_VK_F10:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F10:
>       return "F10";
>-    case KeyboardEvent.DOM_VK_F11:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F11:
>       return "F11";
>-    case KeyboardEvent.DOM_VK_F12:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F12:
>       return "F12";
>-    case KeyboardEvent.DOM_VK_F13:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F13:
>       return "F13";
>-    case KeyboardEvent.DOM_VK_F14:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F14:
>       return "F14";
>-    case KeyboardEvent.DOM_VK_F15:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F15:
>       return "F15";
>-    case KeyboardEvent.DOM_VK_F16:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F16:
>       return "F16";
>-    case KeyboardEvent.DOM_VK_F17:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F17:
>       return "F17";
>-    case KeyboardEvent.DOM_VK_F18:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F18:
>       return "F18";
>-    case KeyboardEvent.DOM_VK_F19:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F19:
>       return "F19";
>-    case KeyboardEvent.DOM_VK_F20:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F20:
>       return "F20";
>-    case KeyboardEvent.DOM_VK_F21:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F21:
>       return "F21";
>-    case KeyboardEvent.DOM_VK_F22:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F22:
>       return "F22";
>-    case KeyboardEvent.DOM_VK_F23:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F23:
>       return "F23";
>-    case KeyboardEvent.DOM_VK_F24:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_F24:
>       return "F24";
>-    case KeyboardEvent.DOM_VK_NUM_LOCK:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_NUM_LOCK:
>       return "NumLock";
>-    case KeyboardEvent.DOM_VK_SCROLL_LOCK:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_SCROLL_LOCK:
>       return "ScrollLock";
>-    case KeyboardEvent.DOM_VK_VOLUME_MUTE:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_VOLUME_MUTE:
>       return "VolumeMute";
>-    case KeyboardEvent.DOM_VK_VOLUME_DOWN:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_VOLUME_DOWN:
>       return "VolumeDown";
>-    case KeyboardEvent.DOM_VK_VOLUME_UP:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_VOLUME_UP:
>       return "VolumeUp";
>-    case KeyboardEvent.DOM_VK_META:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_META:
>       return "Meta";
>-    case KeyboardEvent.DOM_VK_ALTGR:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_ALTGR:
>       return "AltGraph";
>-    case KeyboardEvent.DOM_VK_ATTN:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_ATTN:
>       return "Attn";
>-    case KeyboardEvent.DOM_VK_CRSEL:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_CRSEL:
>       return "CrSel";
>-    case KeyboardEvent.DOM_VK_EXSEL:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_EXSEL:
>       return "ExSel";
>-    case KeyboardEvent.DOM_VK_EREOF:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_EREOF:
>       return "EraseEof";
>-    case KeyboardEvent.DOM_VK_PLAY:
>+    case Ci.nsIDOMKeyEvent.DOM_VK_PLAY:
>       return "Play";
>     default:
>       return "Unidentified";
>   }
> }

I don't understand why you need these changes. Could you explain the reason? I think that at least in this bug, you shouldn't do this change except if you really need this.

If you worry about the mismatch between KeyboardEvent.DOM_VK_* and nsIDOMKeyEvent.DOM_VK_*, I'll create an automated test for that.

>+            // We violate the spec here and ask TextInputProcessor not to infer
>+            // this value from value of key nor code so we could keep the original
>+            // behavior.
>+            keyCode: json.keyCode,

Hmm, don't we check keyCode value at least for [a-zA-Z0-9] case? Although, that may break compatibility with keyboard apps, but it must improve the compatibility with web apps. I think the latter is more important.

>+  // Test the plain alphabets
>+  var codeA = 'A'.charCodeAt(0);
>+  for (var i = 0; i < 26; i++) {
>+    // callbacks in then() are deferred; must only reference these block-scoped
>+    // variable instead of i.
>+    let codePoint = codeA + i;
>+    let code = 'Key' + String.fromCharCode(codePoint);
>+
>+    [String.fromCharCode(codePoint),
>+      String.fromCharCode(codePoint).toLowerCase()]
>+    .forEach((chr) => {
>+      // Test plain alphabet
>+      promiseQueue = promiseQueue.then(() => {
>+        return sendKeyAndAssertResult({
>+          dict: {
>+            key: chr
>+          },
>+          expectedKeypress: true,
>+          expectedInput: chr,
>+          expectedValues: {
>+            key: chr, code: '',
>+            keyCode: codePoint,
>+            charCode: chr.charCodeAt(0)
>+          }
>+        });
>+      });
>+
>+      // Test plain alphabet with keyCode set
>+      promiseQueue = promiseQueue.then(() => {
>+        return sendKeyAndAssertResult({
>+          dict: {
>+            key: chr,
>+            keyCode: codePoint
>+          },
>+          expectedKeypress: true,
>+          expectedInput: chr,
>+          expectedValues: {
>+            key: chr, code: '',
>+            keyCode: codePoint,
>+            charCode: chr.charCodeAt(0)
>+          }
>+        });
>+      });

How about to check keyCode adjustment? E.g.,

      promiseQueue = promiseQueue.then(() => {
        return sendKeyAndAssertResult({
          dict: {
            key: chr,
            keyCode: codePoint + 1
          },
          expectedKeypress: true,
          expectedInput: chr,
          expectedValues: {
            key: chr, code: '',
            keyCode: codePoint,
            charCode: chr.charCodeAt(0)
          }
        });
      });

Although, this could be tested by other cases, but this explains the new API behavior explicitly.

>+function runSendKeyNumberTests() {
>+  gTestDescription = 'runSendKeyNumberTests(): ';
>+  var promiseQueue = Promise.resolve();
>+
>+  // Test numbers
>+  var code0 = '0'.charCodeAt(0);
>+  for (var i = 0; i < 10; i++) {
>+    // callbacks in then() are deferred; must only reference these block-scoped
>+    // variable instead of i.
>+    let codePoint = code0 + i;
>+    let chr = String.fromCharCode(codePoint);
>+
>+    // Test plain number
>+    promiseQueue = promiseQueue.then(() => {
>+      return sendKeyAndAssertResult({
>+        dict: {
>+          key: chr
>+        },
>+        expectedKeypress: true,
>+        expectedInput: chr,
>+        expectedValues: {
>+          key: chr, code: '',
>+          keyCode: codePoint,
>+          charCode: chr.charCodeAt(0)
>+        }
>+      });
>+    });
>+
>+    // Test plain number with keyCode set
>+    promiseQueue = promiseQueue.then(() => {
>+      return sendKeyAndAssertResult({
>+        dict: {
>+          key: chr,
>+          keyCode: codePoint
>+        },
>+        expectedKeypress: true,
>+        expectedInput: chr,
>+        expectedValues: {
>+          key: chr, code: '',
>+          keyCode: codePoint,
>+          charCode: chr.charCodeAt(0)
>+        }
>+      });
>+    });

Similarly, here. keyCode: codePoint + 1 cases must be helpful.

>+function runSendKeyEnterTests() {
>+  gTestDescription = 'runSendKeyEnterTests(): ';
>+  var promiseQueue = Promise.resolve();
>+
>+  // Test Enter with code unset
>+  promiseQueue = promiseQueue.then(() => {
>+    return sendKeyAndAssertResult({
>+      dict: {
>+        key: 'Enter'
>+      },
>+      expectedKeypress: true,
>+      expectedInput: '\n',
>+      expectedValues: {
>+        key: 'Enter', code: '',
>+        keyCode: KeyboardEvent.DOM_VK_RETURN,
>+        charCode: 0
>+      }
>+    });
>+  });

Hmm, but this test computes keyCode value as expected. Do I misunderstand your patch??

>+     * Send a string/character with its key events. There are two ways of invocating 
>+     * the method for backward compability purpose.
>+     *
>+     * (1)
>+     * @param dictOrKeyCode keyCode of the key to send, should be one of the VK_ value in KeyboardEvent.
>+     * @param charCode charCode of the character, should be 0 for non-printable keys.
>+     * @param modifiers this paramater is no longer honored.
>+     * @param repeat indicates whether a key would be sent repeatedly.
>+     *
>+     * (2)
>+     * @param dictOrKeyCode See MozInputMethodKeyboardEventDict.
>+     * @param charCode disregarded
>+     * @param modifiers disregarded
>+     * @param repeat disregarded
>+     *
>+     * @return A promise. Resolve to true if succeeds.
>+     *                    Rejects to a string indicating the error.
>+     *
>+     * Note that, if you want to send a key n times repeatedly, make sure set
>+     * parameter repeat to true and invoke sendKey n times, and invoke keyup
>+     * after the end of the input.
>+     */

I hope that this document recommends (2) explicitly and explain why (1) is allowed (for compatibility).

>+    Promise<boolean> sendKey((MozInputMethodRequiredKeyboardEventDict or long) dictOrKeyCode, 

nit: Remove the last whitespace.

>+    /*
>+     * Send a string/character with keydown, and keypress events.
>+     * keyup should be called afterwards to ensure properly sequence.
>+     *
>+     * @param dict See MozInputMethodKeyboardEventDict.
>+     *
>+     * @return A promise. Resolve to true if succeeds.
>+     *                    Rejects to a string indicating the error.
>+     */
>+    Promise<boolean> keydown(MozInputMethodRequiredKeyboardEventDict dict);
>+
>+    /*
>+     * Send a keyup event. keydown should be called first to ensure properly sequence.
>+     *
>+     * @param dict See MozInputMethodKeyboardEventDict.
>+     *
>+     * @return A promise. Resolve to true if succeeds.
>+     *                    Rejects to a string indicating the error.
>+     *
>+     */
>+    Promise<boolean> keyup(MozInputMethodRequiredKeyboardEventDict dict);
> 
>     /*
>      * Set current composing text. This method will start composition or update
>      * composition if it has started. The composition will be started right
>      * before the cursor position and any selected text will be replaced by the
>      * composing text. When the composition is started, calling this method can
>      * update the text and move cursor winthin the range of the composing text.
>      * @param text The new composition text to show.
>      * @param cursor The new cursor position relative to the start of the
>      * composition text. The cursor should be positioned within the composition
>      * text. This means the value should be >= 0 and <= the length of
>      * composition text. Defaults to the lenght of composition text, i.e., the
>      * cursor will be positioned after the composition text.
>      * @param clauses The array of composition clause information. If not set,
>      * only one clause is supported.
>-     *
>+     * @param dict The properties of the keyboard event that cause the composition
>+     * to set.

Please add "keydown or keyup event will be fired if it's necessary. For compatibility, we recommend that you should always set this argument if it's caused by a key operation." or something like this.

>     /*
>      * End composition, clear the composing text and commit given text to
>      * current input field. The text will be committed before the cursor
>      * position.
>      * @param text The text to commited before cursor position. If empty string
>      * is given, no text will be committed.
>+     * @param dict The properties of the keyboard event that cause the composition
>+     * to end.

Same here.
>+dictionary MozInputMethodKeyboardEventDictBase {
>+  /*
>+   * String/character to output, or a registered name of non-printable key.
>+   * (To be defined in the inheriting dictionary types.)
>+   */
>+  // DOMString key;
>+  /*
>+   * String/char indicating the virtual hardware key pressed. Optional.
>+   * Must be a value defined in http://www.w3.org/TR/DOM-Level-3-Events-code/#keyboard-chording-virtual
>+   */
>+  DOMString code = "";

Please add this (or similar) comment: "If your keyboard app emulates physical keyboard layout, this value should not be empty string. Otherwise, it should be empty string."

>+  /*
>+   * keyCode of the keyboard event. Optional.
>+   * To be disregarded if |key| is an alphanumeric character.
>+   */
>+  long? keyCode;

Please add this (or similar) comment: "If the key causes inputting a character and if your keyboard app emulates a physical keyboard layout, this value should be same as the value used by Firefox for desktop. If the key causes inputting an ASCII character but if your keyboard app doesn't emulate any physical keyboard layouts, the value should be proper value for the key value."

>+/*
>+ * This seems to be the only way to prevent setComposition() to throw TypeError
>+ * when an "optional" dictionary is not passed into it? Why?
>+ */
>+dictionary MozInputMethodKeyboardEventDict : MozInputMethodKeyboardEventDictBase {
>+  DOMString? key;
>+};

Hmm, odd... Smaug might know something about this...

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 133 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #132)
> >+    if (/^[a-zA-Z0-9]$/.test(dict.key)) {
> >+      // keyCode must follow the key value in this range;
> >+      // disregard the keyCode from content.
> >+      keyboardEventDict.keyCode = dict.key.toUpperCase().charCodeAt(0);
> >+    } else if (typeof dict.keyCode === 'number') {
> >+      // Allow keyCode to be specified for other key values.
> >+      keyboardEventDict.keyCode = dict.keyCode;
> >+
> >+      // Allow keyCode to be explicitly set to zero.
> >+      if (dict.keyCode === 0) {
> >+        keyboardEventDict.flags |=
> >+          Ci.nsITextInputProcessor.KEY_KEEP_KEYCODE_ZERO;
> >+      }
> >+    }
> 
> Oh, you specify nsITextInputProcessor.KEY_KEEP_KEYCODE_ZERO here but you
> don't remove this when the key is non-printable key. I guess that this
> causes non-printable keys don't set properly, isn't it?
> http://mxr.mozilla.org/mozilla-central/source/dom/base/TextInputProcessor.
> cpp?rev=76a74dc6ccc9&mark=732-732,745-747#732
> 
> I guess that you should check |printable| here.
> 

We will only reach this block when the content explicitly set keyCode=0. KEY_KEEP_KEYCODE_ZERO is set to ensure we honor that, for both non-printable keys or printable keys.

If keyCode is unset in the |dict|, it is left unset in the |keyboardEventdict| sent to the remote, and TIP can fill the keyCode for non-printable keys.

Is that desirable?

> >-function guessKeyNameFromKeyCode(KeyboardEvent, aKeyCode) {
> >+function guessKeyNameFromKeyCode(aKeyCode) {
> >   switch (aKeyCode) {
> >-    case KeyboardEvent.DOM_VK_CANCEL:
> >+    case Ci.nsIDOMKeyEvent.DOM_VK_CANCEL:
> >       return "Cancel";
> 
> I don't understand why you need these changes. Could you explain the reason?
> I think that at least in this bug, you shouldn't do this change except if
> you really need this.
> 
> If you worry about the mismatch between KeyboardEvent.DOM_VK_* and
> nsIDOMKeyEvent.DOM_VK_*, I'll create an automated test for that.
> 

I am sorry, this should have been done on the part 2 patch. I just don't like a |KeyboardEvent| need to be passed into |guessKeyNameFromKeyCode()| just because we need the constants, and I thought |KeyboardEvent| and |nsIDOMKeyEvent| should have the same constants.

Is using constants from nsIDOMKeyEvent a real concern? How do I get a reference of KeyboardEvent w/o accessing that from a window, if that's the case?

> >+            // We violate the spec here and ask TextInputProcessor not to infer
> >+            // this value from value of key nor code so we could keep the original
> >+            // behavior.
> >+            keyCode: json.keyCode,
> 
> Hmm, don't we check keyCode value at least for [a-zA-Z0-9] case? Although,
> that may break compatibility with keyboard apps, but it must improve the
> compatibility with web apps. I think the latter is more important.
> 

This is just moving indent from part 2, for API call w/ old arguments.
I will check if |key| is [a-zA-Z0-9] too if that's what you asked.

> >+function runSendKeyEnterTests() {
> >+  gTestDescription = 'runSendKeyEnterTests(): ';
> >+  var promiseQueue = Promise.resolve();
> >+
> >+  // Test Enter with code unset
> >+  promiseQueue = promiseQueue.then(() => {
> >+    return sendKeyAndAssertResult({
> >+      dict: {
> >+        key: 'Enter'
> >+      },
> >+      expectedKeypress: true,
> >+      expectedInput: '\n',
> >+      expectedValues: {
> >+        key: 'Enter', code: '',
> >+        keyCode: KeyboardEvent.DOM_VK_RETURN,
> >+        charCode: 0
> >+      }
> >+    });
> >+  });
> 
> Hmm, but this test computes keyCode value as expected. Do I misunderstand
> your patch??
> 

The keyCode is filled by TIP since we dont fall into setting KEY_KEEP_KEYCODE_ZERO when running this case.

We however do fall to that in the next case (|sendKey({ key: 'Enter', keyCode: 0 })|).

> >+/*
> >+ * This seems to be the only way to prevent setComposition() to throw TypeError
> >+ * when an "optional" dictionary is not passed into it? Why?
> >+ */
> >+dictionary MozInputMethodKeyboardEventDict : MozInputMethodKeyboardEventDictBase {
> >+  DOMString? key;
> >+};
> 
> Hmm, odd... Smaug might know something about this...

I tried to make the argument nullable but that won't pass either.

    Promise<boolean> endComposition(optional DOMString text, 
                                    optional MozInputMethodRequiredKeyboardEventDict? dict);

What I am seeing is, the current setup

    Promise<boolean> endComposition(optional DOMString text, 
                                    optional MozInputMethodKeyboardEventDict dict);

will result WebIDL binding always creates an empty dict, even with "optional" set. Thus a dict def w/ "required" keyword on the |key| property will throw a TypeError because a "required" property is not set on the dict. Plainly, I don't think "optional" is honored for dictionary types at all. I will keep asking around in the office and maybe loop smaug if it's no avail.

I will address other part of review comments first. Please help me understand your thoughts more on the question asked in this comment (KEYCODE_ZERO and nsIDOMKeyEvent v.s. KeyboardEvent etc.). Thanks!

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 134 • 9 years ago

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #133)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #132)
> > >+    if (/^[a-zA-Z0-9]$/.test(dict.key)) {
> > >+      // keyCode must follow the key value in this range;
> > >+      // disregard the keyCode from content.
> > >+      keyboardEventDict.keyCode = dict.key.toUpperCase().charCodeAt(0);
> > >+    } else if (typeof dict.keyCode === 'number') {
> > >+      // Allow keyCode to be specified for other key values.
> > >+      keyboardEventDict.keyCode = dict.keyCode;
> > >+
> > >+      // Allow keyCode to be explicitly set to zero.
> > >+      if (dict.keyCode === 0) {
> > >+        keyboardEventDict.flags |=
> > >+          Ci.nsITextInputProcessor.KEY_KEEP_KEYCODE_ZERO;
> > >+      }
> > >+    }
> > 
> > Oh, you specify nsITextInputProcessor.KEY_KEEP_KEYCODE_ZERO here but you
> > don't remove this when the key is non-printable key. I guess that this
> > causes non-printable keys don't set properly, isn't it?
> > http://mxr.mozilla.org/mozilla-central/source/dom/base/TextInputProcessor.
> > cpp?rev=76a74dc6ccc9&mark=732-732,745-747#732
> > 
> > I guess that you should check |printable| here.
> > 
> 
> We will only reach this block when the content explicitly set keyCode=0.
> KEY_KEEP_KEYCODE_ZERO is set to ensure we honor that, for both non-printable
> keys or printable keys.

Ah, okay. Then, we should recommend that keyCode shouldn't be specified when keyboard apps try to send non-printable key event.

> > >-function guessKeyNameFromKeyCode(KeyboardEvent, aKeyCode) {
> > >+function guessKeyNameFromKeyCode(aKeyCode) {
> > >   switch (aKeyCode) {
> > >-    case KeyboardEvent.DOM_VK_CANCEL:
> > >+    case Ci.nsIDOMKeyEvent.DOM_VK_CANCEL:
> > >       return "Cancel";
> > 
> > I don't understand why you need these changes. Could you explain the reason?
> > I think that at least in this bug, you shouldn't do this change except if
> > you really need this.
> > 
> > If you worry about the mismatch between KeyboardEvent.DOM_VK_* and
> > nsIDOMKeyEvent.DOM_VK_*, I'll create an automated test for that.
> > 
> 
> I am sorry, this should have been done on the part 2 patch. I just don't
> like a |KeyboardEvent| need to be passed into |guessKeyNameFromKeyCode()|
> just because we need the constants, and I thought |KeyboardEvent| and
> |nsIDOMKeyEvent| should have the same constants.
> 
> Is using constants from nsIDOMKeyEvent a real concern? How do I get a
> reference of KeyboardEvent w/o accessing that from a window, if that's the
> case?

Cannot you use (window.)KeyboardEvent in the js file?

> > >+function runSendKeyEnterTests() {
> > >+  gTestDescription = 'runSendKeyEnterTests(): ';
> > >+  var promiseQueue = Promise.resolve();
> > >+
> > >+  // Test Enter with code unset
> > >+  promiseQueue = promiseQueue.then(() => {
> > >+    return sendKeyAndAssertResult({
> > >+      dict: {
> > >+        key: 'Enter'
> > >+      },
> > >+      expectedKeypress: true,
> > >+      expectedInput: '\n',
> > >+      expectedValues: {
> > >+        key: 'Enter', code: '',
> > >+        keyCode: KeyboardEvent.DOM_VK_RETURN,
> > >+        charCode: 0
> > >+      }
> > >+    });
> > >+  });
> > 
> > Hmm, but this test computes keyCode value as expected. Do I misunderstand
> > your patch??
> > 
> 
> The keyCode is filled by TIP since we dont fall into setting
> KEY_KEEP_KEYCODE_ZERO when running this case.
> 
> We however do fall to that in the next case (|sendKey({ key: 'Enter',
> keyCode: 0 })|).

Yeah, now I understand. Thanks!

> > >+/*
> > >+ * This seems to be the only way to prevent setComposition() to throw TypeError
> > >+ * when an "optional" dictionary is not passed into it? Why?
> > >+ */
> > >+dictionary MozInputMethodKeyboardEventDict : MozInputMethodKeyboardEventDictBase {
> > >+  DOMString? key;
> > >+};
> > 
> > Hmm, odd... Smaug might know something about this...
> 
> I tried to make the argument nullable but that won't pass either.
> 
>     Promise<boolean> endComposition(optional DOMString text, 
>                                     optional
> MozInputMethodRequiredKeyboardEventDict? dict);
> 
> What I am seeing is, the current setup
> 
>     Promise<boolean> endComposition(optional DOMString text, 
>                                     optional MozInputMethodKeyboardEventDict
> dict);
> 
> will result WebIDL binding always creates an empty dict, even with
> "optional" set. Thus a dict def w/ "required" keyword on the |key| property
> will throw a TypeError because a "required" property is not set on the dict.
> Plainly, I don't think "optional" is honored for dictionary types at all. I
> will keep asking around in the office and maybe loop smaug if it's no avail.

I feel it's a bug of webidl, but not sure. Smaug, do you know something about this?

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 135 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #134)
> > > >-function guessKeyNameFromKeyCode(KeyboardEvent, aKeyCode) {
> > > >+function guessKeyNameFromKeyCode(aKeyCode) {
> > > >   switch (aKeyCode) {
> > > >-    case KeyboardEvent.DOM_VK_CANCEL:
> > > >+    case Ci.nsIDOMKeyEvent.DOM_VK_CANCEL:
> > > >       return "Cancel";
> > > 
> > > I don't understand why you need these changes. Could you explain the reason?
> > > I think that at least in this bug, you shouldn't do this change except if
> > > you really need this.
> > > 
> > > If you worry about the mismatch between KeyboardEvent.DOM_VK_* and
> > > nsIDOMKeyEvent.DOM_VK_*, I'll create an automated test for that.
> > > 
> > 
> > I am sorry, this should have been done on the part 2 patch. I just don't
> > like a |KeyboardEvent| need to be passed into |guessKeyNameFromKeyCode()|
> > just because we need the constants, and I thought |KeyboardEvent| and
> > |nsIDOMKeyEvent| should have the same constants.
> > 
> > Is using constants from nsIDOMKeyEvent a real concern? How do I get a
> > reference of KeyboardEvent w/o accessing that from a window, if that's the
> > case?
> 
> Cannot you use (window.)KeyboardEvent in the js file?
> 

As written in the previous patch, I can steal the window object from the content and passed it into the function; if that's preferable I would just revert this part of code change.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 136 • 9 years ago

> As written in the previous patch, I can steal the window object from the content and passed it into
> the function; if that's preferable I would just revert this part of code change.

I just like the simplest code. So, if you cannot write it as "KeyboardEvent.DOM_VK_*" here, no problem to use Ci.nsIDOMKeyEvent.DOM_VK_*.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 137 • 9 years ago

All review comments addressed except the WebIDL part, where we should wait for confirmation from :smaug on that.

Assuming WebIDL parser is doing what's intended and we indeed need two dictionary types to denote two different kind of arguments, I have rewritten the comments to explain the situation as such.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 138 • 9 years ago

Comment on attachment 8647326 [details] [diff] [review]
part 3, Allow content to pass a dict representing the property of the keyboard event to send

Looks good to me.

I think that we should close this bug ASAP due to too long comments. Then, we should file follow up bugs for emulating Numpad* and handling modifiers.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 139 • 9 years ago

Comment on attachment 8647326 [details] [diff] [review]
part 3, Allow content to pass a dict representing the property of the keyboard event to send

Review of attachment 8647326 [details] [diff] [review]:
-----------------------------------------------------------------

I assume we need to super review this part 3 patch too since this touches WebIDL?

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 140 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #138)
> I think that we should close this bug ASAP due to too long comments. Then,
> we should file follow up bugs for emulating Numpad* and handling modifiers.

Agreed. Let's do this!

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

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 141 • 9 years ago

Minor correction here,

          if (!keyboardEventDict.keyCode) {
            flags |= tip.KEY_KEEP_KEYCODE_ZERO;
          }

flip the flag with the calculated keyCode, not the one passed.

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 142 • 9 years ago

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

> I feel it's a bug of webidl, but not sure. Smaug, do you know something
> about this?

Per spec http://heycam.github.io/webidl/
"If the type of an argument is a dictionary type or a union type that has a dictionary type as one of its flattened member types, and that dictionary type and its ancestors have no required members, and the argument is either the final argument or is followed only by optional arguments, then the argument MUST be specified as optional. Such arguments are always considered to have a default value of an empty dictionary"

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Reporter
	Comment 143 • 9 years ago

Thank you, smaug. I feel it's inconvenient, though.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 144 • 9 years ago

(In reply to Olli Pettay [:smaug] from comment #142)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #134)
> 
> > I feel it's a bug of webidl, but not sure. Smaug, do you know something
> > about this?
> 
> Per spec http://heycam.github.io/webidl/
> "If the type of an argument is a dictionary type or a union type that has a
> dictionary type as one of its flattened member types, and that dictionary
> type and its ancestors have no required members, and the argument is either
> the final argument or is followed only by optional arguments, then the
> argument MUST be specified as optional. Such arguments are always considered
> to have a default value of an empty dictionary"

I'll assume I am following the spec correctly at attachment 8647365 [details] [diff] [review] then, with two dicts fulfill two types of arguments. The naming could be improved though.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 145 • 9 years ago

Comment on attachment 8647365 [details] [diff] [review]
part 3, Allow content to pass a dict representing the property of the keyboard event to send

s/arugment/argument/


Interesting way to use required. handy.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 146 • 9 years ago

Typo corrected and r= and sr= added on commit message.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 147 • 9 years ago

Let's land this. Try push link can be found in comment 141.

	Ryan VanderMeulen [:RyanVM]
	Comment 148 • 9 years ago

Part 3 needs rebasing.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 149 • 9 years ago

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #148)
> Part 3 needs rebasing.

The patch to rebase is 

https://hg.mozilla.org/mozilla-central/diff/0f5421c28b14/dom/inputmethod/MozKeyboard.js (bug 904479) :'(

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 150 • 9 years ago

Rebased.

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

	Pulsebot
	Comment 151 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/81e93b60a622
https://hg.mozilla.org/integration/mozilla-inbound/rev/e48ed45d1c80
https://hg.mozilla.org/integration/mozilla-inbound/rev/83af10efcd3c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce86cf91f423

	Ryan VanderMeulen [:RyanVM]
	Comment 152 • 9 years ago

Backed out for causing intermittent Gij switching_test.js failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=13211240&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=10688997&repo=try

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa51330c9e09

	Ryan VanderMeulen [:RyanVM]
	Comment 153 • 9 years ago

*sigh* then that same failure hits on another branch. Thanks for the churn, unowned Gaia tests.

	Pulsebot
	Comment 154 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/692a0e1e46a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d47c8de6563
https://hg.mozilla.org/integration/mozilla-inbound/rev/e89f50c10fc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/478dd6079573

	Ryan VanderMeulen [:RyanVM]
	Comment 155 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/692a0e1e46a6
https://hg.mozilla.org/mozilla-central/rev/1d47c8de6563
https://hg.mozilla.org/mozilla-central/rev/e89f50c10fc2
https://hg.mozilla.org/mozilla-central/rev/478dd6079573