1197700 - Correct mistakes in InputMethod.webidl

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

InputMethod.webidl does not actually describe what the API do currently. Preferably we should normalize that first before working on anything else.

My patch will probably be a little bit opinionated but I hope we are going toward the right direction.

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: bug1197700.patch

This patch re-formats the Web IDL file to normalize the style of the comment, indents, and properly document and enforce the dictionary and enum types. The value of InputContext#type is finally correctly implemented too (see fixed test).

I've also remove the permission check in MozKeyboard.js entirely and rely on Web IDL binding to enforce that. The permission checking on IPC messages remains.

Jan, I wonder if you have some spare time for the review? One thing to watch out is the the "inputType" v.s. "type" in the IPC message sent -- I think I've changed all of them. Unfortunately we can't test mozChromeEvents -- I intend to remove them one by one after this bug.

Kanru, could you help me review the permission bits? Since :masayuki said he is not familiar with B2G permissions...

Olli, I am sorry that the IDL contains many white space changes. As stated the while space changes are about styling so the part to super-review is the newly added dictionaries and enums, and permissions.

Thanks!

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8653963 [details] [diff] [review]
bug1197700.patch

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

I only checked the webidl file. The permissions looks fine. But the patch introduced more trailing white spaces in the comments, maybe by accident?

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Kan-Ru Chen [:kanru] from comment #2)
> Comment on attachment 8653963 [details] [diff] [review]
> bug1197700.patch
> 
> Review of attachment 8653963 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I only checked the webidl file. The permissions looks fine. But the patch
> introduced more trailing white spaces in the comments, maybe by accident?

Thanks, white space removed.

Comment 4

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

Comment on attachment 8653963 [details] [diff] [review]
bug1197700.patch

It is a bit odd to have so finegrained  permission handling in MozInputMethod
(like, why doesn't input-manage have access to all non-chrome-only stuff).
But fine.

Comment 5

[Jan Jongboom [:janjongboom] (inactive)]

I guess you removed it already, but:

bug1197700.patch:344: trailing whitespace.
 * element and the input content (a.k.a. input app, virtual keyboard app,
bug1197700.patch:347: trailing whitespace.
 * The API also contains a few Gaia System app only methods
bug1197700.patch:514: trailing whitespace.
   * OS should sliently ignore this request if the app is currently not the
bug1197700.patch:523: trailing whitespace.
   * OS should sliently ignore this request if the app is currently not the
bug1197700.patch:537: trailing whitespace.
   *

Building now...

Comment 6

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8653963 [details] [diff] [review]
bug1197700.patch

I really think this should have been in 2 separate patches tbh.

Anyway: we should change the way 'blur' works. It was messy already but that is not cleaned up, just adds more code and the if statement that we have in handleFocusChange is a sign that this is not a clean protocol from forms.js -> MozKeyboard -> system app. I think we should add a new message type in forms.js:unhandleFocus like "Forms:Input:Blur" or something.

> * Group "text". Be reminded that contenteditable element is assigned with
> * a type of "textarea".
Should be 'an input type of', at least it was not clear from me from the patch file. If you're just reading the IDL file it might be alright.

Anyway: based on this patch I'm really wondering why we need the 'type' field on a context anyway? We're not ever gonna show a different keyboard whether it's input, textarea or contenteditable. Can we just remove the type property? Do we even use it in Gaia? I can't really think of a place where we do...

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #6)
> Comment on attachment 8653963 [details] [diff] [review]
> bug1197700.patch
> 
> I really think this should have been in 2 separate patches tbh.
> 
> Anyway: we should change the way 'blur' works. It was messy already but that
> is not cleaned up, just adds more code and the if statement that we have in
> handleFocusChange is a sign that this is not a clean protocol from forms.js
> -> MozKeyboard -> system app. I think we should add a new message type in
> forms.js:unhandleFocus like "Forms:Input:Blur" or something.
> 

I can fix this in this patch if you want.

> > * Group "text". Be reminded that contenteditable element is assigned with
> > * a type of "textarea".
> Should be 'an input type of', at least it was not clear from me from the
> patch file. If you're just reading the IDL file it might be alright.
> 
> Anyway: based on this patch I'm really wondering why we need the 'type'
> field on a context anyway? We're not ever gonna show a different keyboard
> whether it's input, textarea or contenteditable. Can we just remove the type
> property? Do we even use it in Gaia? I can't really think of a place where
> we do...

You are right, we don't use that in Gaia Keyboard because it recognizes every possible |inputType| and |inputType| of each of the |type| doesn't overlap. It's possible some other keyboard doesn't handle |inputType| though, and I am just finishing up what's originally planned for |type|. Do you still think I should remove it?

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: bug1197700.patch

This patches address the problem raised in the previous comment. It is indeed more clear.

The local build have not finish yet but I have pushed to try first.

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

(Carrying over superreivew+ from the previous patch)

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8658514 [details] [diff] [review]
bug1197700.patch

... and it doesn't work locally.

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: bug1197700.patch

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

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #7)
> You are right, we don't use that in Gaia Keyboard because it recognizes
> every possible |inputType| and |inputType| of each of the |type| doesn't
> overlap. It's possible some other keyboard doesn't handle |inputType|
> though, and I am just finishing up what's originally planned for |type|. Do
> you still think I should remove it?

Actually, since we assign contenteditable with inputType=textarea, the only way to distinguish the two would be to inspect the |type| attribute. You can see that by looking at the test.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #11)
> Actually, since we assign contenteditable with inputType=textarea, the only
> way to distinguish the two would be to inspect the |type| attribute. You can
> see that by looking at the test.

... the test of bug 1201407 which is more exhaust than test_basic.html.

Comment 13

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8658519 [details] [diff] [review]
bug1197700.patch

r=me for dom/inputmethod/.

@tim, I'd add the following to this patch:

diff --git a/dom/inputmethod/mochitest/test_bug1059163.html b/dom/inputmethod/mochitest/test_bug1059163.html
index 79da5d9..12b1173 100644
--- a/dom/inputmethod/mochitest/test_bug1059163.html
+++ b/dom/inputmethod/mochitest/test_bug1059163.html
@@ -54,6 +54,9 @@ function runTest() {
     mm.loadFrameScript('data:,(' + encodeURIComponent(appFrameScript.toString()) + ')();', false);

     im.oninputcontextchange = function() {
+      is(im.inputcontext.type, 'contenteditable', 'type');
+      is(im.inputcontext.inputType, 'textarea', 'inputType');
+
       if (im.inputcontext) {
         im.oninputcontextchange = null;
         register();

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch to commit

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd40cb58dca3

Comment 16

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures in https://treeherder.mozilla.org/logviewer.html#?job_id=13873434&repo=mozilla-inbound

Comment 17

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47dc98dd7dbf

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: bug1197700.patch

I think below is what's needed since the patch now hides the constructors behind a permission. Strangely, MozInputMethod (constructor of navigator.mozInputMethod) is not listed in the test.

:smaug, could you bless the additional change with review? Thanks.

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

diff --git a/dom/tests/mochitest/general/test_interfaces.html b/dom/tests/mochitest/general/test_interfaces.html
--- a/dom/tests/mochitest/general/test_interfaces.html
+++ b/dom/tests/mochitest/general/test_interfaces.html
@@ -781,19 +781,19 @@ var interfaceNamesInGlobalScope =
     "MozContactChangeEvent",
 // IMPORTANT: Do not change this list without review from a DOM peer!
     "MozCSSKeyframeRule",
 // IMPORTANT: Do not change this list without review from a DOM peer!
     "MozCSSKeyframesRule",
 // IMPORTANT: Do not change this list without review from a DOM peer!
     {name: "MozEmergencyCbModeEvent", b2g: true},
 // IMPORTANT: Do not change this list without review from a DOM peer!
-    {name: "MozInputContext", b2g: true},
-// IMPORTANT: Do not change this list without review from a DOM peer!
-    {name: "MozInputMethodManager", b2g: true},
+    {name: "MozInputContext", b2g: true, permission: ["input"]},
+// IMPORTANT: Do not change this list without review from a DOM peer!
+    {name: "MozInputMethodManager", b2g: true, permission: ["input"]},
 // IMPORTANT: Do not change this list without review from a DOM peer!
     "MozMmsMessage",
 // IMPORTANT: Do not change this list without review from a DOM peer!
     {name: "MozMobileCellInfo", b2g: true},
 // IMPORTANT: Do not change this list without review from a DOM peer!
     {name: "MozMobileConnection", b2g: true},
 // IMPORTANT: Do not change this list without review from a DOM peer!
     {name: "MozMobileConnectionArray", b2g: true},

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch to comment

Fix one error spotted in bug 1201407 comment 16.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc7a86c09438

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dc7a86c09438