Closed Bug 986992 Opened 6 years ago Closed 6 years ago

Remove navigator.mozKeyboard

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: ehsan, Assigned: xyuan)

References

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 3 obsolete files)

This is what I get when I look for mozKeyboard usage in gaia:

$ git grep -w mozKeyboard
apps/keyboard/js/keyboard.js:1644:// This is called when we get an event from mozKeyboard.
apps/system/js/keyboard_manager.js:3:// If we get a focuschange event from mozKeyboard for an element with
apps/system/js/value_selector/value_selector.js:215:      window.navigator.mozKeyboard.setSelectedOption(singleOptionIndex);
apps/system/js/value_selector/value_selector.js:225:      window.navigator.mozKeyboard.setSelectedOptions(optionIndices);
apps/system/js/value_selector/value_selector.js:250:    window.navigator.mozKeyboard.removeFocus();
apps/system/js/value_selector/value_selector.js:261:        window.navigator.mozKeyboard.setValue(timeValue);
apps/system/js/value_selector/value_selector.js:269:        window.navigator.mozKeyboard.setValue(dateValue);
apps/system/js/value_selector/value_selector.js:321:          window.navigator.mozKeyboard.setValue(datetimeValue);
apps/system/js/value_selector/value_selector.js:326:    window.navigator.mozKeyboard.removeFocus();
apps/system/test/unit/value_selector_test.js:34:    realKeyboard = window.navigator.mozKeyboard;
apps/system/test/unit/value_selector_test.js:35:    window.navigator.mozKeyboard = sinon.stub();
apps/system/test/unit/value_selector_test.js:41:    window.navigator.mozKeyboard = realKeyboard;
shared/js/keyboard_helper.js:646:        //    use mozKeyboard API
tests/python/gaia-ui-tests/gaiatest/apps/keyboard/app.py:326:        self.marionette.execute_script('navigator.mozKeyboard.removeFocus();')

Yuan, can we kill navigator.mozKeyboard and just remove these tests?
Flags: needinfo?(xyuan)
Blocks: 981845
No, if only remove the tests. The value selector of gaia system depends on mozKeyboard.  The method "setSelectedOptions" of mozKeyboard is required by the system app. We need remove that method before kill mozKeyboard.
Flags: needinfo?(xyuan)
Assignee: nobody → xyuan
Thanks a lot for taking this, Yuan!
Attached patch gecko_v1.patch (obsolete) — Splinter Review
Remove navigator.mozKeyboard and move its methods to navigator.mozInputMethod. The methods, such as "setValue", "setSelectedOption", etc. should only be used by the system app with the permission "input-manage". Other keyboard apps are forbidden to access these methods.

The _ignoreEditActionOnce in the forms.js is only used by mozKeyboard.sendKey. As we don't use that method any more, I removed it as well.
Attachment #8396369 - Flags: review?(fabrice)
Attached file gaia v1.patch
Attachment #8396411 - Flags: review?(rlu)
Comment on attachment 8396369 [details] [diff] [review]
gecko_v1.patch

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

lgtm, but I'd like to know why you switched to weak listeners here.

::: dom/inputmethod/MozKeyboard.js
@@ +133,5 @@
>      Services.obs.addObserver(this, "inner-window-destroyed", false);
> +    cpmm.addWeakMessageListener('Keyboard:FocusChange', this);
> +    cpmm.addWeakMessageListener('Keyboard:SelectionChange', this);
> +    cpmm.addWeakMessageListener('Keyboard:GetContext:Result:OK', this);
> +    cpmm.addWeakMessageListener('Keyboard:LayoutsChange', this);

why did you move to weak listeners?
(In reply to Fabrice Desré [:fabrice] from comment #5)
> Comment on attachment 8396369 [details] [diff] [review]
> gecko_v1.patch
> 
> Review of attachment 8396369 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm, but I'd like to know why you switched to weak listeners here.
> 
> ::: dom/inputmethod/MozKeyboard.js
> @@ +133,5 @@
> >      Services.obs.addObserver(this, "inner-window-destroyed", false);
> > +    cpmm.addWeakMessageListener('Keyboard:FocusChange', this);
> > +    cpmm.addWeakMessageListener('Keyboard:SelectionChange', this);
> > +    cpmm.addWeakMessageListener('Keyboard:GetContext:Result:OK', this);
> > +    cpmm.addWeakMessageListener('Keyboard:LayoutsChange', this);
> 
> why did you move to weak listeners?
Either strong or weak listeners works here. I prefer to the weak one to avoid potential leak in the future.
I guess we would need to land the Gecko patch first, and at the time, the old Gaia code would still reference Gecko's mozKeyboard API, right?

So I think we would need 2-steps landing for this to happen:
 1. Land the Gecko patch to add the functions, "setValue", "setSelectedOption", etc., to mozInputMethod.
 2. Land the Gaia patch here.
 3. Land the Gecko patch to remove those function in #1 from mozKeyboard.

Yuan, does this make sense?
Flags: needinfo?(xyuan)
BTW, can we dupe bug 941448 to this one?
Duplicate of this bug: 941448
(In reply to Rudy Lu [:rudyl] from comment #7)
> I guess we would need to land the Gecko patch first, and at the time, the
> old Gaia code would still reference Gecko's mozKeyboard API, right?
> 
> So I think we would need 2-steps landing for this to happen:
>  1. Land the Gecko patch to add the functions, "setValue",
> "setSelectedOption", etc., to mozInputMethod.
>  2. Land the Gaia patch here.
>  3. Land the Gecko patch to remove those function in #1 from mozKeyboard.
> 
> Yuan, does this make sense?

Yes, I'll split the gecko patch into two to land;)
Flags: needinfo?(xyuan)
Comment on attachment 8396411 [details] [review]
gaia v1.patch

This looks good to me for API changes.
Thanks.
Attachment #8396411 - Flags: review?(rlu) → review+
Rudy, thanks:)
Attachment #8396369 - Flags: review?(fabrice) → review+
Attached patch Part1 gecko v1Splinter Review
Attachment #8396369 - Attachment is obsolete: true
Attachment #8399302 - Flags: review+
Attached patch Part2 gecko v1 (obsolete) — Splinter Review
Attachment #8399303 - Flags: review+
Attachment #8399303 - Attachment description: Part2 v1 gecko → Part2 gecko v1
Hi Sheriff,

Based on comment 11,
I slitted the gecko patch into two. Please land the gecko path of part1 first.
Status: NEW → ASSIGNED
Sorry for the typo...

I split the gecko patch into two. Please land the gecko patch of part1 first.
Ryan, thanks. Please help to check in the gecko path of part2.

@Rudy, please help to merge gaia patch.
Flags: needinfo?(rlu)
Will check if the patch is ready on m-c and then land the gaia patch.
Please don't land gecko part 2 before the gaia patch landed.
Attached file Pull request 17893
gaia v1.1 patch
To update the unit tests part.

Carry forward the r+, will land this after travis is all green.
Attachment #8400559 - Flags: review+
Gaia patch landed,
https://github.com/mozilla-b2g/gaia/commit/a077a10d1082da86d17b3ab1e791af2bba1a1d4e

--
Please help land the gecko patch part 2.
Thanks.
Flags: needinfo?(rlu)
Attachment #8400559 - Flags: checkin+
The following changeset is now in Firefox Nightly:

> 17b6288baf8f Bug 986992 - Part 1: Merge navigator.mozKeyboard to navigator.mozInputMethod. r=fabrice

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central
Can you backout Gaia part as well? Currently the master is broken and does not pass marionette tests due to this.
What is the status of the bug?
Flags: needinfo?(xyuan)
gandalf, I'll fix the gecko patch of part2 soon. 

Are marionette tests still broken? If so, could you show me the link and ask rudy to backout the gaia patch.

I checked the https://tbpl.mozilla.org/?tree=B2g-Inbound and din't find the marionette or marionette-webapi tests broken by this.
Flags: needinfo?(xyuan)
Attached patch Part2 gecko v2 (obsolete) — Splinter Review
Fix packaging and mochitest.

https://tbpl.mozilla.org/?tree=Try&rev=298fac612cde
Attachment #8399303 - Attachment is obsolete: true
Attachment #8403090 - Flags: review+
Hi, Rudy. Please backout the gaia patch. I need add "input" permission back to system app. Thanks.
Flags: needinfo?(rlu)
Yuan, why do we need to back out the Gaia part?
Does that really cause the TBPL fail?
Flags: needinfo?(rlu) → needinfo?(xyuan)
We need add "input" permission back to system app temporarily. Otherwise if MozInputMethod unloads in the system app, the system app process will be killed.

To solve the issue, we have two options:

1. backout the gaia patch

2. apply the following patch:

diff --git a/apps/system/manifest.webapp b/apps/system/manifest.webapp
index 6cd6ea7..f44fad3 100644
--- a/apps/system/manifest.webapp
+++ b/apps/system/manifest.webapp
@@ -40,6 +40,7 @@
     "audio-channel-content":{},
     "audio-channel-ringer":{},
     "cellbroadcast":{},
+    "input":{},
     "input-manage":{},
     "nfc":{ "access": "readonly" },
     "nfc-manager":{},
diff --git a/shared/js/keyboard_helper.js b/shared/js/keyboard_helper.js
index 935a5d9..2648d96 100644
--- a/shared/js/keyboard_helper.js
+++ b/shared/js/keyboard_helper.js
@@ -644,6 +644,11 @@ var KeyboardHelper = exports.KeyboardHelper = {
           return;
         }
 
+        //XXX remove this hard code check if one day system app no longer
+        //    use "input" permission.
+        if (app.origin === 'app://system.gaiamobile.org') {
+          return;
+        }
         // all keyboard apps should define its layout(s) in "inputs" section
         if (!app.manifest.inputs) {
           return;
Flags: needinfo?(xyuan)
Rudy, the third option is wait for my new gecko patch of part 2. It will be ready in two days.
Attached patch Part2 gecko v3Splinter Review
Update patch to fix permission tests failure.

https://tbpl.mozilla.org/?tree=Try&rev=2a86e1f4802e
Attachment #8403090 - Attachment is obsolete: true
Comment on attachment 8404400 [details] [diff] [review]
Part2 gecko v3

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

Fabrice, this patch made fix with the tests under dom/permission/tests.
Attachment #8404400 - Flags: review?(fabrice)
Yuan: no, with the latest Gecko I don't see the bug anymore so it seems like not a blocker.
Attachment #8404400 - Flags: review?(fabrice) → review+
Sheriff, please land Part2 gecko v3.
Attachment #8404400 - Flags: checkin?(ryanvm)
Comment on attachment 8404400 [details] [diff] [review]
Part2 gecko v3

did the checkin
Attachment #8404400 - Flags: checkin?(ryanvm) → checkin+
https://hg.mozilla.org/mozilla-central/rev/99b1ab87b230
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Carsten, thanks:)
Can these patches be landed on V1.4?
Flags: needinfo?(wchang)
(In reply to Youngjun Kim from comment #44)
> Can these patches be landed on V1.4?

No, sorry but we're wrapping up 1.4 now so we're unable to include changes like this to 1.4.
Flags: needinfo?(wchang)
Depends on: 1000095
Blocks: 970044
Duplicate of this bug: 969897
Depends on: 1027127
There is a mention in: 
https://developer.mozilla.org/en-US/Firefox/Releases/31/Site_Compatibility

I think this doesn't need more info (it has never been documented)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.