986992 - Remove navigator.mozKeyboard

Status: RESOLVED FIXED

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

Description

[(no longer active)]

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?

Comment 1

[Yuan Xulei [:yxl] (Leave from May 18)]

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.

Comment 2

[(no longer active)]

Thanks a lot for taking this, Yuan!

Comment 3

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: gecko_v1.patch

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.

Comment 4

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: gaia v1.patch

Comment 5

[[:fabrice] Fabrice Desré]

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?

Comment 6

[Yuan Xulei [:yxl] (Leave from May 18)]

(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.

Comment 7

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

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?

Comment 8

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

BTW, can we dupe bug 941448 to this one?

Comment 10

[Yuan Xulei [:yxl] (Leave from May 18)]

(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;)

Comment 11

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8396411 [details] [review]
gaia v1.patch

This looks good to me for API changes.
Thanks.

Comment 12

[Yuan Xulei [:yxl] (Leave from May 18)]

Rudy, thanks:)

Comment 13

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part1 gecko v1

Comment 14

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 gecko v1

Comment 15

[Yuan Xulei [:yxl] (Leave from May 18)]

Hi Sheriff,

Based on comment 11,
I slitted the gecko patch into two. Please land the gecko path of part1 first.

Comment 16

[Yuan Xulei [:yxl] (Leave from May 18)]

Sorry for the typo...

I split the gecko patch into two. Please land the gecko patch of part1 first.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8399302 [details] [diff] [review]
Part1 gecko v1

https://hg.mozilla.org/integration/b2g-inbound/rev/17b6288baf8f

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/17b6288baf8f

Comment 19

[Yuan Xulei [:yxl] (Leave from May 18)]

Ryan, thanks. Please help to check in the gecko path of part2.

@Rudy, please help to merge gaia patch.

Comment 20

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

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.

Comment 21

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: 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.

Comment 22

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Gaia patch landed,
https://github.com/mozilla-b2g/gaia/commit/a077a10d1082da86d17b3ab1e791af2bba1a1d4e

--
Please help land the gecko patch part 2.
Thanks.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8399303 [details] [diff] [review]
Part2 gecko v1

https://hg.mozilla.org/integration/b2g-inbound/rev/aa47acfdbdae

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8399303 [details] [diff] [review]
Part2 gecko v1

This broke packaging. Backed out.
https://hg.mozilla.org/integration/b2g-inbound/rev/ba30e8f5d6b4

https://tbpl.mozilla.org/php/getParsedLog.php?id=37158408&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=37158497&tree=B2g-Inbound

Might want to have a build peer take a look on the next patch revision.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

This also had mochitest orange on B2G desktop builds.
https://tbpl.mozilla.org/php/getParsedLog.php?id=37159324&tree=B2g-Inbound

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=37163179&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=37162932&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=37163634&tree=B2g-Inbound

These look like fallout, too.

Comment 27

[Firefox Commit Bot]

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

Comment 28

[Zibi Braniecki [:zbraniecki][:gandalf]]

Can you backout Gaia part as well? Currently the master is broken and does not pass marionette tests due to this.

Comment 29

[Zibi Braniecki [:zbraniecki][:gandalf]]

What is the status of the bug?

Comment 30

[Yuan Xulei [:yxl] (Leave from May 18)]

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.

Comment 31

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 gecko v2

Fix packaging and mochitest.

https://tbpl.mozilla.org/?tree=Try&rev=298fac612cde

Comment 32

[Yuan Xulei [:yxl] (Leave from May 18)]

Hi, Rudy. Please backout the gaia patch. I need add "input" permission back to system app. Thanks.

Comment 33

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Yuan, why do we need to back out the Gaia part?
Does that really cause the TBPL fail?

Comment 34

[Yuan Xulei [:yxl] (Leave from May 18)]

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;

Comment 35

[Yuan Xulei [:yxl] (Leave from May 18)]

Rudy, the third option is wait for my new gecko patch of part 2. It will be ready in two days.

Comment 36

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 gecko v3

Update patch to fix permission tests failure.

https://tbpl.mozilla.org/?tree=Try&rev=2a86e1f4802e

Comment 37

[Yuan Xulei [:yxl] (Leave from May 18)]

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.

Comment 38

[Zibi Braniecki [:zbraniecki][:gandalf]]

Yuan: no, with the latest Gecko I don't see the bug anymore so it seems like not a blocker.

Comment 39

[Yuan Xulei [:yxl] (Leave from May 18)]

Sheriff, please land Part2 gecko v3.

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/99b1ab87b230

Comment 41

[Carsten Book [:Tomcat]]

Comment on attachment 8404400 [details] [diff] [review]
Part2 gecko v3

did the checkin

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/99b1ab87b230

Comment 43

[Yuan Xulei [:yxl] (Leave from May 18)]

Carsten, thanks:)

Comment 44

[Youngjun Kim]

Can these patches be landed on V1.4?

Comment 45

[Wayne Chang [: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.

Comment 47

[Jean-Yves Perrier [:teoli]]

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)