Closed
Bug 958035
Opened 11 years ago
Closed 11 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)
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)
* 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
Reporter | ||
Updated•11 years ago
|
blocking-b2g: --- → 1.4?
Whiteboard: [FT:System-Platform], [3rd-party-keyboard]
Comment 1•11 years ago
|
||
Fixed in bug 875963. Can you reverify?
Reporter | ||
Comment 2•11 years ago
|
||
Sure!! :)
Many thanks!
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Reporter | ||
Comment 3•11 years ago
|
||
After patch(bug 875963) landed on master, I still can reproduce this bug.
Remove the duplicate tag.
Thanks!
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Assignee | ||
Updated•11 years ago
|
Component: Gaia::Keyboard → Gaia::System::Input Mgmt
Comment 4•11 years ago
|
||
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
Comment 5•11 years ago
|
||
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)
Comment 7•11 years ago
|
||
Hmm, it doesn't seem to be Gecko related. So you have it back ;-)
Comment 8•11 years ago
|
||
Got it~ :)
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 9•11 years ago
|
||
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)
Comment 10•11 years ago
|
||
(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)
Comment 11•11 years ago
|
||
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.
Comment 12•11 years ago
|
||
Hi Xulei,
Jan has tried the patch and it doesn't work please refer to Comment #7.
Thanks~
Comment 13•11 years ago
|
||
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)
Comment 14•11 years ago
|
||
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.
Updated•11 years ago
|
Flags: needinfo?(gchen)
Comment 15•11 years ago
|
||
(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?
Updated•11 years ago
|
Component: Gaia::System::Input Mgmt → Gaia::Keyboard
Assignee | ||
Comment 16•11 years ago
|
||
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
Comment 17•11 years ago
|
||
(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)
Assignee | ||
Comment 19•11 years ago
|
||
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)
Comment 20•11 years ago
|
||
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)
Comment 21•11 years ago
|
||
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)
Comment 22•11 years ago
|
||
(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).
Assignee | ||
Comment 24•11 years ago
|
||
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)
Assignee | ||
Comment 25•11 years ago
|
||
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.
Assignee | ||
Comment 26•11 years ago
|
||
Comment on attachment 8376984 [details]
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 27•11 years ago
|
||
Comment on attachment 8376984 [details]
Patch V1 - pull request 16350
r+ if unit test is added.
Attachment #8376984 -
Flags: review?(alive) → review+
Comment 28•11 years ago
|
||
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+ → ---
Assignee | ||
Comment 29•11 years ago
|
||
Comment on attachment 8376984 [details]
Patch V1 - pull request 16350
Patch updated to include a new unit test in homescreen_launcher.
Alive, thanks.
Comment 30•11 years ago
|
||
Comment on attachment 8376984 [details]
Patch V1 - pull request 16350
Looks alright if manually tested.
Attachment #8376984 -
Flags: review?(timdream) → review+
Assignee | ||
Comment 31•11 years ago
|
||
Landed to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/bad58be0cb67eae9293148125b871607f6fbbfd8
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 32•11 years ago
|
||
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
status-b2g-v1.4:
--- → verified
You need to log in
before you can comment on or make changes to this bug.
Description
•