931746 - The composition string is always cleared abnormally in the location bar of the browser

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Luke Chang [:lchang] (inactive)]

gaia: 65050448048799c749601d22955bdf24bb4bc72a
gecko: 9e2ec4ca9896a4a5a4698d49a1a053c1c1c07ff9

The composition string will show in the location bar shortly and then disappear right away when the keyboard app is using "setComposition()".

You can try this issue by using pinyin IME since it is the only layout engine which uses new composition APIs so far.

As Gary mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=924846#c11 , I found that this bug is also caused by OOP issue. It will work fine if I enable OOP in the browser app but I don't know why.

Comment 1

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

We need a fix/workaround in koi+. Luke, are you still investigating the Gecko part or we need help?

Comment 2

[Luke Chang [:lchang] (inactive)]

Tim, I can't find any workaround from my perspective. I think we need help from gecko team.

Comment 3

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

Luke, thank you.

Kanru or SC, do you have extra bandwidth to work on the issue? If not I can try to talk to managers...

Comment 4

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

I'll take a look and see if it's an easy fix or workaround.

Comment 5

[Ivan Tsay (:ITsay)]

Tim, may I know who will take the ownership of this bug.

Comment 6

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

Kanru is investigating for now.

Comment 7

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

I/GeckoDump( 1075): jspinyin: Deactivate.
I/GeckoDump( 1075): jspinyin: empty.
I/GeckoDump( 1075): jspinyin: SendPendingSymbol: 
I/GeckoDump( 1075): jspinyin: SendPendingSymbol endComposition
I/GeckoDump( 1075): jspinyin: 
E/GeckoConsole( 1075): [JavaScript Error: "TypeError: inputContext is null" {file: "app://keyboard.gaiamobile.org/js/keyboard.js" line: 1839}]
I/GeckoDump( 1075): jspinyin: Activate. Input type: text
E/GeckoConsole( 1075): [JavaScript Error: "Return value is not an object."]

Probably loosing focus?

Comment 8

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

Each time I input a character jspinyin receive two deactivate call.

Comment 9

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

forms.js was loaded twice.

When we focus the search bar, both forms.js receive "focus" event and addEditorObserver self. Only the second forms.js receives the Forms:SetComposition message and triggers EditAction that makes the first forms.js call sendKeyboardState.

Comment 10

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

(In reply to Kan-Ru Chen [:kanru] from comment #9)
> forms.js was loaded twice.
> 
> When we focus the search bar, both forms.js receive "focus" event and
> addEditorObserver self. Only the second forms.js receives the
> Forms:SetComposition message and triggers EditAction that makes the first
> forms.js call sendKeyboardState.

Where did you test it? On b2g-desktop, there are two form.js files loaded; while on the device, there seems only one.

Comment 11

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

(In reply to Yuan Xulei [:yxl] from comment #10)
> (In reply to Kan-Ru Chen [:kanru] from comment #9)
> > forms.js was loaded twice.
> > 
> > When we focus the search bar, both forms.js receive "focus" event and
> > addEditorObserver self. Only the second forms.js receives the
> > Forms:SetComposition message and triggers EditAction that makes the first
> > forms.js call sendKeyboardState.
> 
> Where did you test it? On b2g-desktop, there are two form.js files loaded;
> while on the device, there seems only one.

On the device (unagi).

Comment 12

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

4796:I/Gecko   ( 4658): focus evt currentTarget[object ContentFrameMessageManager @ 0x47327200 (native @ 0x47ba62e0)]
4808:I/Gecko   ( 4658): focus evt currentTarget[object ContentFrameMessageManager @ 0x453325c0 (native @ 0x45330ee0)]

The event is received by two different frame message manager. I guess the event is captured by the system iframe then captured by the browser app iframe since they are in the same tree.

Comment 13

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

Attachment: Only handle events from our direct descendants

I don't know if there is a more efficient way to do this :S

Comment 14

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

Comment on attachment 826656 [details] [diff] [review]
Only handle events from our direct descendants

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

It looks good for me, but I don't test it on devices.
Please make sure the patch doesn't break bug 914100 and bug 811177.

Comment 15

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

Bug 914100 looks good. I can't test bug 811177 because IME tests app is broken for a while.

Comment 16

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

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

Comment 17

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

Hi Kan-Ru, the patch broke the |UI Tests| app. With the patch, I cannot open the keyboard with the keyboard page of the |UI Tests| app.

Without that path, that test app works fine.

Comment 18

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

(In reply to Yuan Xulei [:yxl] from comment #17)
> Hi Kan-Ru, the patch broke the |UI Tests| app. With the patch, I cannot open
> the keyboard with the keyboard page of the |UI Tests| app.
> 
> Without that path, that test app works fine.

Thanks, backout'ed
https://hg.mozilla.org/integration/b2g-inbound/rev/2620144506dc

Comment 19

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

Clearly the element in an iframe is not a descendant of the document.body.

Comment 20

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

Attachment: Only handle the event from our descendants

This version looks better :)

Comment 21

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

Comment on attachment 827302 [details] [diff] [review]
Only handle the event from our descendants

I tested the patch on b2g-desktop of mozilla-central with gaia master. It fixed the location bar issue, but I still cannot open keyboard on the keyboard page of the UI tests app. I got "too much recursion" errors in the javascript console.

Steps to reproduce:

  1. Launch UI tests app
  2. Open the Keyboard page
  3. Tap the first text area

Actual:
  
  The virtual keyboard doesn't show.

Expected:

  The virtual keyboard shows.

Comment 22

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

(In reply to Yuan Xulei [:yxl] from comment #21)
> Comment on attachment 827302 [details] [diff] [review]
> Only handle the event from our descendants
> 
> I tested the patch on b2g-desktop of mozilla-central with gaia master. It
> fixed the location bar issue, but I still cannot open keyboard on the
> keyboard page of the UI tests app. I got "too much recursion" errors in the
> javascript console.

Hmm, strange. I avoided any recursion in the patch already and it works locally. I will do more test.

Comment 23

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

So on b2g-desktop, in the UI tests app, only one focus event was fired yet the target has different top window with the content.window. How could it be..

Comment 24

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

The EventTarget chain does sometimes include the frame message manager (nsInProcessTabChildGlobal) of the UI tests app but never when we focus it. So the event always dispatches to the system app frame message manager only. This is weird, I don't know why we miss the inner frame message manager in the EventTarget chain only on b2g-desktop.

Comment 25

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

On b2g-desktop, the UI tests app is not OOPed. May it be the cause?

Comment 26

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

The mozbrowser's DocShell has typeContent so it's frame message handler shouldn't be used as the chrome event handler. But why we get the event on the phone? We are getting close..

Comment 27

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

Attachment: Make sure all mozbrowser mm are in the target chain

In this setup:
<iframe id="root" mozbrowser>
  #document
  <iframe id="app" mozbrowser>
    #document
    <input id="input1">
    <iframe id="content">
      #document
      <input id="input2">

Events dispatched to input1 has following target chain (some omitted):
 chromeWindow->rootMM->appMM->input1

Events dispatched to input2 has following target chain (some omitted):
 chromeWindow->rootMM->input2

Because the content iframe does have an MM so it set its mParentTarget to its mChromeEventHandler, the rootMM. This patch adds the enclosing mozbrowser's MM to the target chain, so after this patch events dispatched to input2 has following target chain:
 chromeWindow->rootMM->appMM->input2

This make the appMM have a chance to receive or capture the events from its content.

Comment 28

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

Correction: ...the content iframe does *not* have an MM...

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

Comment 29

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

Comment on attachment 830754 [details] [diff] [review]
Make sure all mozbrowser mm are in the target chain

Could you please add still some test that the parent document of the
iframe doesn't get the event. And that the message managers do get them.

Comment 30

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

Comment on attachment 830754 [details] [diff] [review]
Make sure all mozbrowser mm are in the target chain

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

::: dom/base/nsGlobalWindow.cpp
@@ +2801,5 @@
>      TryGetTabChildGlobalAsEventTarget(frameElement);
>  
>    if (!eventTarget) {
> +    nsGlobalWindow* topWin = GetScriptableTop();
> +    frameElement = topWin->GetFrameElementInternal();

You need a check to ensure |topWin| is not nullptr.

Comment 31

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

uh, indeed.

Comment 32

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

@Kan-ru, thanks for digging into this tough bug. You're almost there:)

Comment 33

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

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

Comment 34

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

Attachment: Make sure all mozbrowser mm are in the target chain

Add test case and fix the null check.

Comment 35

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

Comment on attachment 831413 [details] [diff] [review]
Make sure all mozbrowser mm are in the target chain

Could you add a click event listener to the main document too and make sure
it is not called.

Also, if synthesizeMouseAtCenter doesn't succeed everywhere, 
you may need to do similar hack I did in 
https://bug847763.bugzilla.mozilla.org/attachment.cgi?id=829768
(the synthesizeMouseAtPoint part)

Comment 36

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

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

Looks like we don't need the hack here.

Comment 37

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

Uh.. it does need the hack.

Comment 38

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

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

Dispatch click event directly

Comment 39

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

Update: Kanru told me he still have trouble debug Emulator test run locally, so he will work something out.

Comment 40

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

Attachment: Make sure all mozbrowser mm are in the target chain

Final version that works on both desktop and emulator. Tree closed, will push when it open.

Comment 41

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

https://hg.mozilla.org/integration/b2g-inbound/rev/2fe365696178
https://hg.mozilla.org/integration/b2g-inbound/rev/84c1a344132b

Comment 42

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2fe365696178
https://hg.mozilla.org/mozilla-central/rev/84c1a344132b

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/07185f63dc39
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/6cff38d1f936

Comment 45

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

Attachment: backout-84c1a344132b

Until bug 941468 is fixed, we need to back this out.

Comment 46

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

Actually I think load forms.js only once in the top browser could resolve both problem.. patches coming.