Closed Bug 958035 Opened 6 years ago Closed 6 years ago

[Keyboard][V1.4] The previous keyboard shows briefly if you switch keyboard between number field and text field.

Categories

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(b2g-v1.4 verified)

VERIFIED FIXED
Tracking Status
b2g-v1.4 --- verified

People

(Reporter: whsu, Assigned: rudyl)

References

Details

(Whiteboard: [FT:System-Platform], [3rd-party-keyboard])

Attachments

(5 files)

Attached video WP_20140109_009.mp4
* Description:
  This problem happened on v1.4 build (Mozilla Central).
  It seems to relate to Gecko rendering.
  If you switch keyboard between number field and text field, you will see the previous keyboard briefly shows on screen.
  Attaching the demo video (WP_20140109_009.mp4).

* Reproduction steps:
  1. Launch the Contact app
  2. Tap "+" to edit a new contact
  3. Tap "Name" field
  4. Tap "Phone" field
  5. Repeat step 3~4.

* Expected result:
  - Keyboard layout is displayed as normal.

* Actual result:
  - Previous keyboard briefly shows on screen.

* Reproduction build:( Mozilla Central - V1.4 )
 - Gaia      b7a7191f761933fd4878227488c75d09f5ba890c
 - Gecko     http://hg.mozilla.org/mozilla-central/rev/cf2d1bd796ea
 - BuildID   20140108040200
 - Version   29.0a1
blocking-b2g: --- → 1.4?
Whiteboard: [FT:System-Platform], [3rd-party-keyboard]
Fixed in bug 875963. Can you reverify?
Sure!! :)
Many thanks!
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 875963
After patch(bug 875963) landed on master, I still can reproduce this bug.
Remove the duplicate tag.
Thanks!
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Component: Gaia::Keyboard → Gaia::System::Input Mgmt
Hi all, 
   There is a WIP path for investigating this issue (just insert some logs).
   I found sometimes keyboard app gets |inputcontextchange| before Keyboard Manager, so keyboard app run |showkeyboard| with wrong |keyboardName| for |inputcontext.type|.

expect sequence:
01-16 16:53:09.261: keyboard_manager.js:332 in km_launchLayoutFrame: IMlog KMapp://keyboard.gaiamobile.org/index.html#en
01-16 16:53:09.301: keyboard.js:1634 in showKeyboard: IMlog inputcontextchange
01-16 16:53:09.301:keyboard.js:1669 in showKeyboard: IMlog keyboardName:numberLayout
01-16 16:53:09.311: keyboard.js:1670 in showKeyboard: IMlog inputContext:text
01-16 16:53:09.561: keyboard.js:1634 in showKeyboard: IMlog hashchange
01-16 16:53:09.761: keyboard.js:1634 in showKeyboard: IMlog inputcontextchange
01-16 16:53:09.761: keyboard.js:1669 in showKeyboard: IMlog keyboardName:en
01-16 16:53:09.761: keyboard.js:1670 in showKeyboard: IMlog inputContext:text


wrong sequence:
01-16 16:55:22.771: keyboard_manager.js:332 in km_launchLayoutFrame: IMlog KMapp://keyboard.gaiamobile.org/index.html#en
01-16 16:55:22.771: keyboard.js:1634 in showKeyboard: IMlog inputcontextchange
01-16 16:55:22.791: keyboard.js:1669 in showKeyboard: IMlog keyboardName:numberLayout
01-16 16:55:22.801: keyboard.js:1670 in showKeyboard: IMlog inputContext:text
01-16 16:55:23.091: keyboard.js:1634 in showKeyboard: IMlog hashchange
01-16 16:55:23.331: keyboard.js:1634 in showKeyboard: IMlog inputcontextchange
01-16 16:55:23.331: keyboard.js:1669 in showKeyboard: IMlog keyboardName:en
01-16 16:55:23.331: keyboard.js:1670 in showKeyboard: IMlog inputContext:text
Gary, do you know how to make changes in mozilla-central? I think the problem would go away if we change Keyboard.jsm's handleFocusChange function:

  handleFocusChange: function keyboardHandleFocusChange(msg) {
    this.forwardEvent('Keyboard:FocusChange', msg);

    // Chrome event, used also to render value selectors; that's why we need
    // the info about choices / min / max here as well...
    this.sendChromeEvent({
      type: 'inputmethod-contextchange',
      inputType: msg.data.type,
      value: msg.data.value,
      choices: JSON.stringify(msg.data.choices),
      min: msg.data.min,
      max: msg.data.max
    });
  },

Is the code now, but if we first send the chrome event, and then forward it to MozKeyboard.js, I think we will get the chrome event always earlier...

If you don't want to do Gecko I can test this :-)
Flags: needinfo?(gchen)
Sure, please take it.
Thank you very much!!!
Flags: needinfo?(gchen)
Hmm, it doesn't seem to be Gecko related. So you have it back ;-)
See Also: → 960948
Attached file keyboard.log
This is another log collected by Gary.

--
After you switch from a number fileld to text field:

   a.  Keyboard app would get inputcontextchange first and at the same time, its hash value is still the old one, say “number”.
 
   b.  Then KM would go through the normal flow, say hide and show the keyboard app.

   c. Keyboard would try to show the old keyboard first. 


Jan, Xulei,

Could you please help take a look at this situation, and see if we could guarantee that keyboard mananger would get notified before the keyboard app get inputcontextchange event?
Flags: needinfo?(xyuan)
Flags: needinfo?(janjongboom)
Attached patch test.patchSplinter Review
(In reply to Rudy Lu [:rudyl] from comment #9)
> Created attachment 8363417 [details]
> keyboard.log
> 
> This is another log collected by Gary.
> 
> --
> After you switch from a number fileld to text field:
> 
>    a.  Keyboard app would get inputcontextchange first and at the same time,
> its hash value is still the old one, say “number”.
>  
>    b.  Then KM would go through the normal flow, say hide and show the
> keyboard app.
> 
>    c. Keyboard would try to show the old keyboard first. 
> 
> 
> Jan, Xulei,
> 
> Could you please help take a look at this situation, and see if we could
> guarantee that keyboard mananger would get notified before the keyboard app
> get inputcontextchange event?

Did you test the way of comment 5 provided by :janjongboom?  The attachment is the patch.
Flags: needinfo?(xyuan)
If comment 5 doesn't work, Keyboard.jsm should send inputcontextchange event to keyboard manager only and then let keyboard manager forward the event to keyboard app.
Hi Xulei,
   Jan has tried the patch and it doesn't work please refer to Comment #7.
   Thanks~
Short reply, I'll dig into it later. I think the thing is that onhashchange is not sync. Thus the keyboard manager already knows the correct value but the hash change did not populate through yet.
Flags: needinfo?(janjongboom)
Alright so two things:

* If the hashchange is sync, but we're doing async stuff in the keyboard manager when the chrome event comes in, so before changing the hash, we should stop doing that.
* If the hashchange is async, we can do this:
The problem is that when the keyboard app gets an inputcontextchange event it relies on the hash to render the correct keyboard. When that one didn't change yet normally there is no problem. What you'll see is that you switch from text -> number and the keyboard takes 100 ms. or so to update (waiting for the hashchange to happen). In reality there are two render events: one for the contextchange (renders as text), then one for the hashchange (renders as number). All fine. The problem only shows during going from not focused -> focused. Because then we don't have a keyboard visible and we flash the old one. What we can do here is: introduce a new state in the keyboard manifest called 'blur' or something. We set the URL of the keyboard app to this state if there is no keyboard. That way we will *always* get a hashchange when a keyboard goes from hidden -> visible. Therefore if the keyboard gets a contextchange event when it's currently hidden, it knows that it should wait for the hashchange before rendering.
Flags: needinfo?(gchen)
(In reply to Jan Jongboom [:janjongboom] from comment #14)
> What we can do here is: introduce a new state in the
> keyboard manifest called 'blur' or something. We set the URL of the keyboard
> app to this state if there is no keyboard. That way we will *always* get a
> hashchange when a keyboard goes from hidden -> visible. Therefore if the
> keyboard gets a contextchange event when it's currently hidden, it knows
> that it should wait for the hashchange before rendering.

This is an interesting idea. What if the keyboard simply rewrite/reset it's own hash when it is being hidden? Will that achieve the same effect?
Component: Gaia::System::Input Mgmt → Gaia::Keyboard
I was thinking about separating number layout into its own html and avoid the need for hashchange.
Will update the result here.
Assignee: nobody → rlu
(In reply to Rudy Lu [:rudyl] from comment #16)
> I was thinking about separating number layout into its own html and avoid
> the need for hashchange.
> Will update the result here.

Hi Rudy,
   Good idea, I am looking forward to discussing with you after CNY. :)
Flags: needinfo?(gchen)
Update please?
Flags: needinfo?(rlu)
I have a testing branch here that would do:
https://github.com/RudyLu/gaia/tree/keyboard/Bug958035_investigation

 1. Simulate to get inputContext asynchronously by waitForInputContext(), so that keyboard app won't depend on inputcontextchange event.
 2. In keyboard manager, do not call keyboardFrame.setVisible(false) when it switch between the layouts of the same keyboard app, which could solve the issue that the background (homescreen) is leaked during the switch.

--
Jan, Xulei,

Could you guys help check the above point 1, that any chance we could modify the API so that we would have a API call to get the inputContext asyncly since we could not guarantee the event sequences?
Do we need to bring this API change discussion to #dev-webapi or elsewhere?

Thanks.
Flags: needinfo?(xyuan)
Flags: needinfo?(rlu)
Flags: needinfo?(janjongboom)
It looks odd to change API like that. If we do need the |inputcontextchange| event sent to keyboard app berfore keyboard manager, we could change the API implementation to do that. The |inputcontextchange| event from gecko will be first sent to keyboard manager, and then let keyboard manager forward it to the keyboard app.
Flags: needinfo?(xyuan)
I think we should keep the public API the same, communication between keyboard_manager and gecko already happens through chrome events which is undocumented anyway. Maybe do it like:

Gecko sends contextchange chrome event to Keyboard_manager
KM is done handling sends some event back to gecko
Gecko fires inputcontextchange event
Keyboard reacts to that
Everybody happy

Although I don't know how much overhead that would cost us due to IPC. But it sounds like implementable. I don't think forwarding from keyboard-manager to keyboard is viable solution, because then we would have to rely on hash-state or something, which won't work if anybody else wants to implement mozInputMethod.
Flags: needinfo?(janjongboom)
(In reply to Jan Jongboom [:janjongboom] from comment #21)
> I think we should keep the public API the same, communication between
> keyboard_manager and gecko already happens through chrome events which is
> undocumented anyway. Maybe do it like:
> 
> Gecko sends contextchange chrome event to Keyboard_manager
> KM is done handling sends some event back to gecko
> Gecko fires inputcontextchange event
> Keyboard reacts to that
> Everybody happy

It is doable and I like this way.
I'm thinking that we can extend the mozbrowser API to add a method called 
|sendInputMethodMessage| or something like the setInputMethodActive method
(https://bugzilla.mozilla.org/show_bug.cgi?id=905573). Keyboard_manager can 
use this method to forward chrome events from gecko to the keyboard app process.
The MozKeyobard.js of the keyboard app process is responsible to receive the 
chrome events and fires inputcontextchange event to keyboard finally.

This way will also solve the keyboard messages broadcasting issue (https://bugzilla.mozilla.org/show_bug.cgi?id=962233#c5).
Triage: 1.4 feature work.
blocking-b2g: 1.4? → 1.4+
This patch fixes the UI issue that the keyboard frame was hidden,  due to setVisible(false), and then reshow the previous layout.
- Not to invoke keyboardFrame.setVisible(false) when we reuse the
        frame.

Besides, we also invoke homescreen.fadeOut() to solve the annoying homescreen leak issue when showing/switching/hiding keyboard.

Tim,

Could you please review this patch?
Thank you.
Attachment #8376984 - Flags: review?(timdream)
Please note that I re-enabled the keyboard OOP in the patch to ease the testing.
Will remove that config before Bug 968991 is fixed.

Thanks.
Comment on attachment 8376984 [details] [review]
Patch V1 - pull request 16350

Addressed the review comment by Alive.
Alive, could you please help take a look? Thanks.
Attachment #8376984 - Flags: review?(alive)
Comment on attachment 8376984 [details] [review]
Patch V1 - pull request 16350

r+ if unit test is added.
Attachment #8376984 - Flags: review?(alive) → review+
This work has to be done for 3rd party keyboard to be FC in v1.4. But the feature work should not be the blocker. Remove blocking flag and link this bug to the 3rd party keyboard user story.
Blocks: 964670
blocking-b2g: 1.4+ → ---
Comment on attachment 8376984 [details] [review]
Patch V1 - pull request 16350

Patch updated to include a new unit test in homescreen_launcher.
Alive, thanks.
Comment on attachment 8376984 [details] [review]
Patch V1 - pull request 16350

Looks alright if manually tested.
Attachment #8376984 - Flags: review?(timdream) → review+
Landed to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/bad58be0cb67eae9293148125b871607f6fbbfd8
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Thanks for the help.
I cannot reproduce it on the latest build.

* Build Information: (V1.4)
 - Gaia      4c3b2f57f4229c5f36f0d8fd399e65f4db88f104
 - Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/3aaca223b673
 - BuildID   20140330160202
 - Version   30.0a2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.