838001 - Input Method can't be opened to type to input fields in a panel on Firefox 19+

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Pin Zhang [:pzhang] (inactive)]

Here is the test case:
    https://builder.addons.mozilla.org/package/172628/latest/

Comment 1

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

I'm able to type into the panel's textbox when I click the widget in Firefox 21 Nightly builds, for what it's worth. Can you explain the problem a bit more?

Comment 2

[Pin Zhang [:pzhang] (inactive)]

(In reply to Wes Kocher (:KWierso) from comment #1)
> I'm able to type into the panel's textbox when I click the widget in Firefox
> 21 Nightly builds, for what it's worth. Can you explain the problem a bit
> more?

Did you try any CJK IME to type Chinese/Japanese/Korean characters?

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

What's the "panel"? If it's XUL's <panel>, you must not be able to use IME on Firefox's bookmark panel too. How about that? I can use IME on the bookmark panel of Nightly.

Comment 4

[Pin Zhang [:pzhang] (inactive)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #3)
> What's the "panel"? If it's XUL's <panel>, you must not be able to use IME
> on Firefox's bookmark panel too. How about that? I can use IME on the
> bookmark panel of Nightly.

I don't know if the "panel" is implemented by the XUL's <panel>, but it works on FF 18-.

The Add-on SDK provides the panel model[1] to show a whatever page in a panel, for example, if I create a panel to show Google page, then I can't search any CJK keywords anymore, so obviously, this is a regression.

This bug will affect all the users in China, Japan, Korea and other foreign input method users.

[1] https://addons.mozilla.org/en-US/developers/docs/sdk/latest/modules/sdk/panel.html

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, this is a regression of bug 802896, bug 705057 and bug 805306.

I made nsPresContext::GetNearestWidget() for TextComposition class. I stole the code from nsDOMWindowUtils::GetWidget(). And the changes for bug 802896 and bug 805306, I removed nsIMEStateManager::GetWidget() and the made the callers (IME enabled state management code) use nsPresContext::GetNearestWidget().

By this change, when the panel's <input> gets focus, nsIMEStateManager changes wrong widget's IME state. nsIMEStateManager::GetWidget() had always changed the root widget of the nsPresContext. In most cases, the result of nsDOMWindowUtils::GetWidget() and nsIMEStateManager::GetWidget() are same except this case.

I still don't understand perfectly what happens in this case. I guess that the panel makes new nsPresContext and a child widget. And current code changes the IME state of the child widget. However, the root widget has native focus. Then, the root widget's IME state is referred in this case.

So, I think that I should make nsPresContext::GetRootWidget() which is same as the removed nsIMEStateManager::GetWidget(). And the callers which had called nsIMEStateManager::GetWidget() should use the new method.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=060b7561dece

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for Beta

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 710190 [details] [diff] [review]
Patch

In fact, this is a backout patch. nsIMEStateManager::GetWidget() is replaced with nsPresContext::GetRootWidget() if we have not landed the preceding patches.

FYI: There is already another user of GetNearestWidget(), so, we need to keep it.

And I guess that nsDOMWindowUtils::GetWidget() might be wrong at least for keyboard and IME API. I feel that it's odd nsIMEStateManager::GetWidget() and nsDOMWindowUtils::GetWidget() may return different widget.

Comment 9

[Alex Keybl [:akeybl]]

Please nominate for uplift asap, with user impact/risk. If we're going to take a fix here for FF19, it needs to be for tomorrow's beta and approved/uplifted tonight (PT).

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38218fd7cdaf

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 710190 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 802896 whose nsPresContext::GetNearestWidget() is introduced in bug 705057. And bug 805306 must cause similar symptom.

User impact if declined:
CJK users cannot use IME on the "panel" which is created by the Add-on SDK. This is a serious a11y regression. I'm not sure that such UI is used by how many add-ons though.

Testing completed (on m-c, etc.):
Just landed on m-i.

Risk to taking this patch (and alternatives if risky):
I think the risk is minimum. When I make nsPresContext::GetNearestWidget() for implementing TextComposition class, I stole the logic from nsDOMWindowUtils::GetWidget(). However, it's different logic from nsIMEStateManager::GetWidget(). But I hadn't found the difference of the result actually. Therefore, nsIMEStateManager::GetWidget() callers are replaced with nsPresContext::GetNearestWidget() in bug 802896 and bug 805306. Now, we find the different case of these methods' result. This patch makes nsPresContext::GetRootWidget() which is same as nsIMEStateManager::GetWidget(). And all IME related code which needs to access widget for accessing native IME use the new method. In other words, this backouts the change of nsIMEStateManager::GetWidget(). And the method is moved to nsPresContext for being shared with nsIMEStateManager and TextComposition.

String or UUID changes made by this patch:
Nothing.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 710272 [details] [diff] [review]
Patch for Beta

[Approval Request Comment]

See the previous comment. The difference between the patch and the patch for m-c and Aurora is, whether nsPresContext::GetRootWidget() uses nsViewManager or nsIViewManager.

nsIViewManager has been removed on Mozilla 20. Therefore, for Beta, we need this patch.

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 710190 [details] [diff] [review]
Patch

Approving this backout for Aurora and Beta. Can Mozilla Japan help test beta 5, once available, to ensure there haven't been any major CJK IME regressions from this? Thanks!

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5406b540b9d7
https://hg.mozilla.org/releases/mozilla-beta/rev/d37b7d53494f

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/38218fd7cdaf

Comment 16

[u279076]

Pin Zhang, can you please confirm this is now fixed for you? It should be fixed in the following builds:

Firefox Beta 19.0b5 (currently building)
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/19.0b5-candidates/build1/

Firefox Aurora 20.0a2 (as of 2013-02-07)
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-aurora

Firefox Nightly 21.0a1 (as of 2013-02-07)
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central

Thank you

Comment 17

[Pin Zhang [:pzhang] (inactive)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #16)
> Pin Zhang, can you please confirm this is now fixed for you? It should be
> fixed in the following builds:
> 
> Firefox Beta 19.0b5 (currently building)
> ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/19.0b5-candidates/
> build1/
> 
> Firefox Aurora 20.0a2 (as of 2013-02-07)
> ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-aurora
> 
> Firefox Nightly 21.0a1 (as of 2013-02-07)
> ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central
> 
> Thank you

I tested the 19.0b5, it perfectly works both on Linux and Windows, that's cool~

The latest Aurora and Nightly are still the builds of 2013-02-06 right now, I will verify them later today.

Thanks~

Comment 18

[u279076]

Thanks for your help, Pin.

Comment 19

[Pin Zhang [:pzhang] (inactive)]

(In reply to Pin Zhang [:pzhang] from comment #17)
> (In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #16)
> > Pin Zhang, can you please confirm this is now fixed for you? It should be
> > fixed in the following builds:
> > 
> > Firefox Beta 19.0b5 (currently building)
> > ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/19.0b5-candidates/
> > build1/
> > 
> > Firefox Aurora 20.0a2 (as of 2013-02-07)
> > ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-aurora
> > 
> > Firefox Nightly 21.0a1 (as of 2013-02-07)
> > ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central
> > 
> > Thank you
> 
> I tested the 19.0b5, it perfectly works both on Linux and Windows, that's
> cool~
> 
> The latest Aurora and Nightly are still the builds of 2013-02-06 right now,
> I will verify them later today.
> 
> Thanks~

The 2013-02-08 builds of Aurora and Nightly are verified on both Linux and Windows, the problem is fixed.

Comment 20

[u279076]

(In reply to Pin Zhang [:pzhang] from comment #19)
> The 2013-02-08 builds of Aurora and Nightly are verified on both Linux and
> Windows, the problem is fixed.

Thank you.