1564639 - IME candidate window position is always top-left of the desktop when using any addon providing input field in its popup panel

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P2)

Description

[YUKI "Piro" Hiroshi]

Attachment: A screenshot describing how the problem appears

This looks a reappearance of the bug 1419285. When an addon provides a toolbar button and input fields in its popup panel, composition text of a text input software is shown an independent dialog window instead of an inline text in the field. I've confirmed this problem on some addons:

https://addons.mozilla.org/firefox/addon/second-search/
https://addons.mozilla.org/firefox/addon/hatena-bookmark/

On the other hand, this problem doesn't happen on input fields in sidebar contents. I've confirmed that this problem doesn't happen on following sidebar addons like:

https://addons.mozilla.org/firefox/addon/sidebar-note/
https://addons.mozilla.org/firefox/addon/sidebar_plus/

Steps to reproduce

1. Install Firefox 68.0.
2. Install https://addons.mozilla.org/firefox/addon/second-search/
3. Click the added toolbar button.
4. Activate your IME and input any text.

Expected result

Composition text is shown as an inline text at the input field.

Actual result

Composition text is shown in a separate dialog window at top-left of the desktop.

Environment

• Windows 10 Pro 1809
• Firefox 68.0 (64bit)
• Nightly 70.0a1 (64bit) Build ID: 20190709153742

Comment 1

[Makoto Kato [:m_kato]]

Hmm, Henri seems to be PTO. This is regression by bug 1543363 according to mozregression.

Comment 2

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

Well, I could investigate after bug 1548389. However, I'm really afraid to uplift patches for bug 1543363 regressions without Henri's review...

Comment 3

[Makoto Kato [:m_kato]]

Henri, could you handle this since this is regression by bug 1543363?

Comment 4

[Henri Sivonen (:hsivonen)]

(In reply to Makoto Kato [:m_kato] from comment #1)

This is regression by bug 1543363 according to mozregression.

That bug was an assertion mismatch between GetDocWidget() and GetWidget().

I lack sufficient understanding of which one to use when. It seems that IME-related IPC code in BrowserParent uses a mix of the two. GetDocWidget() is used exclusively in IME-related code, but there's some IME code that even before bug 1543363 used GetWidget().

Is there a rule about which one to use in which IME-related situation? Should we change all IME-related code to use GetDocWidget()? Why does IME code prefer GetDocWidget() over GetWidget()?

Comment 5

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

(In reply to Henri Sivonen (:hsivonen) from comment #4)

(In reply to Makoto Kato [:m_kato] from comment #1)

This is regression by bug 1543363 according to mozregression.

That bug was an assertion mismatch between GetDocWidget() and GetWidget().

I lack sufficient understanding of which one to use when. It seems that IME-related IPC code in BrowserParent uses a mix of the two. GetDocWidget() is used exclusively in IME-related code, but there's some IME code that even before bug 1543363 used GetWidget().

Is there a rule about which one to use in which IME-related situation? Should we change all IME-related code to use GetDocWidget()? Why does IME code prefer GetDocWidget() over GetWidget()?

Ah... Before e10s, we always used top-level widget (I mean if a XUL popup has focus, its parent widget, not top of the popup widget) for communication between our code and native IME since only top-level widget gets focus and its native IME context is used. Therefore, IMEStateManager retrieves widget with nsPresContext::GetRootWidget(). I assume that it returns the top-level widget which I expect. This should still be true in the main process.

But now, nsContentUtils::WidgetForDocument() and nsPresContext::GetRootWidget() may return different widget since only the former uses nsContentUtils::WidgetForDocument(). I'm still not sure which one returns expected widget, but I guess, BrowserParent::GetTopLevelWidget() returns what is exactly same as nsPresContext::GetRootWidget().

Comment 6

[Henri Sivonen (:hsivonen)]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Still struggling with the pain, but becoming better) from comment #5)

(In reply to Henri Sivonen (:hsivonen) from comment #4)

Is there a rule about which one to use in which IME-related situation? Should we change all IME-related code to use GetDocWidget()? Why does IME code prefer GetDocWidget() over GetWidget()?

Ah... Before e10s, we always used top-level widget (I mean if a XUL popup has focus, its parent widget, not top of the popup widget) for communication between our code and native IME since only top-level widget gets focus and its native IME context is used. Therefore, IMEStateManager retrieves widget with nsPresContext::GetRootWidget(). I assume that it returns the top-level widget which I expect. This should still be true in the main process.

But now, nsContentUtils::WidgetForDocument() and nsPresContext::GetRootWidget() may return different widget since only the former uses nsContentUtils::WidgetForDocument(). I'm still not sure which one returns expected widget, but I guess, BrowserParent::GetTopLevelWidget() returns what is exactly same as nsPresContext::GetRootWidget().

I started an attempt to make IME-related code use BrowserParent::GetTopLevelWidget() on Linux, this made things worse: With the change, when the IME candidate popup opens after changing focus between widgets, the popup initially flashes in a bogus location and then moves to the right location. Presumably there exists at least one IME-related thing that should not use BrowserParent::GetTopLevelWidget(): Whatever determines the initial location of the candidate popup on Linux.

I'm quite worried that I don't understand the issues here and will make things worse.

Also, this bug does not reproduce with IBus on Linux. Why might that be?

Comment 7

[Henri Sivonen (:hsivonen)]

Even though this patch is already known to introduce a new bug on Linux, let's see if it fixes this bug on Windows to get an idea of whether it's at all in the right direction for this bug:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bc3165cf823389a7ec3ea2eb9997b21b6f173b9

Comment 8

[Henri Sivonen (:hsivonen)]

Also, bug 1543363 switched from GetDocWidget() to GetWidget(). Migrating the other callers on GetDocWidget() to GetRootWidget() still risks mismatches. What would be the expected effect of migrating the rest of GetDocWidget() callers to GetWidget() and made IMEStateManager call something other than nsPresContext::GetRootWidget()?

That is, bug 1543363 was about IMEStateManager somehow using the widget of the pop-up and I was assuming that that was the right thing to do and migrated the thing being asserted against to use the widget of the pop-up, too.

If we don't want to use the widget of the popup but the widget for the (topmost?) main window, we should probably revert bug 1543363 and figure out where IMEStateManager got the widget of the popup.

Masayuki, can you give me further guidance?

With the change, when the IME candidate popup opens after changing focus between widgets, the popup initially flashes in a bogus location

Speficially, the popup first appears where it has previously appeared.

Comment 9

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

With the change, when the IME candidate popup opens after changing focus between widgets, the popup initially flashes in a bogus location and then moves to the right location.

Sigh, such kind of bugs is what we don't like to exist in release channel since it really looks Gecko engine's quality is poor for IME users.

Also, bug 1543363 switched from GetDocWidget() to GetWidget(). Migrating the other callers on GetDocWidget() to GetRootWidget() still risks mismatches. What would be the expected effect of migrating the rest of GetDocWidget() callers to GetWidget() and made IMEStateManager call something other than nsPresContext::GetRootWidget()?

Yeah, that's really non-understandable fact.

If we don't want to use the widget of the popup but the widget for the (topmost?) main window, we should probably revert bug 1543363 and figure out where IMEStateManager got the widget of the popup.

Indeed.

Masayuki, can you give me further guidance?

From point of view of native IME handlers in each platform-specific widget, notifications and requests for IME should be called in the widget which has composition. When popup is open, its parent window's top level widget has native focus and it handles native IME events in my understanding. However, when we need to compute text/caret coordinates for making IME show its own windows to expected position, IME handling widget dispatches WidgetQueryContentEvent. The result is relative position in popup widget, but it also returns the popup widget. Therefore, widget can convert the result to required coordinates.
https://searchfox.org/mozilla-central/rev/30b01f4f60dbcbd6b01500a26b3100c28005cf62/widget/windows/IMMHandler.cpp#1565-1574
https://searchfox.org/mozilla-central/rev/30b01f4f60dbcbd6b01500a26b3100c28005cf62/widget/windows/TSFTextStore.cpp#4616-4629
https://searchfox.org/mozilla-central/rev/30b01f4f60dbcbd6b01500a26b3100c28005cf62/widget/cocoa/TextInputHandler.mm#4244-4252
https://searchfox.org/mozilla-central/rev/30b01f4f60dbcbd6b01500a26b3100c28005cf62/widget/gtk/IMContextWrapper.cpp#2778-2792

But Cocoa and GTK widgets don't refer mFocusedWidget that might cause the new bug...

With the change, when the IME candidate popup opens after changing focus between widgets, the popup initially flashes in a bogus location

Speficially, the popup first appears where it has previously appeared.

Well, sounds like that we notify IME of the location too early time...

Comment 10

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

Okay, now I have a patch. The direction is correct. I guess that Henri missed some lines to change.

Comment 11

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

And I hit assertion in cocoa widget with current Nightly debug build when I focus <input> in an addon. That's caused by wrong widget receives notifications for IME.

Comment 12

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

Attachment: Bug 1564639 - Make `BrowserParent` use `nsPresContext::GetRootWidget()` when handling IME related messages

When contents notify IME or requests something to IME, they need to use
an nsIWidget instance which may have focus if active, and handles
native keyboard/IME events since that knows correct native IME context.

Currently, such widget exactly matches with the result of
nsPresContext::GetRootWidget(). However, this is unclear for most developers.
Therefore, this patch creates a wrapper method of it named
nsPresContext::GetUserInputHandlingWidget(). Then, also adds
BrowserParent::GetUserInputHandlingWidget() wraps it. Finally, makes
IMEStateManager call GetUserInputHandlingWidget() of them.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b282fe75003a
Make `BrowserParent` use `nsPresContext::GetRootWidget()` when handling IME related messages r=hsivonen

Comment 14

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/b282fe75003a

Comment 15

[Ryan VanderMeulen [:RyanVM]]

This sounds like a scary thing to uplift this late in the cycle :( (RC week is next week). Can we live with this bug for another release?

Comment 16

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)

This sounds like a scary thing to uplift this late in the cycle :( (RC week is next week). Can we live with this bug for another release?

Yeah, I think so.

Comment 17

[Peter Magyari (Desktop QA)]

Hi,

Unfortunately I am unable to reproduce this issue on my end on either of the affected versions of Firefox.
Since I can't reproduce it, I cannot confirm that it has been fixed myself.
YUKI "Piro" Hiroshi could you please verify the fix using the latest Firefox Nightly?

Thank you!

Comment 18

[YUKI "Piro" Hiroshi]

I've confirmed that this has been fixed on:

• Nightly 71.0a1 build ID 20190903094847
• Firefox Developer Edition 70.0b2 build ID 20190830040415

Comment 19

[Peter Magyari (Desktop QA)]

As per Comment 18 I'm going to set this as verified:fixed.
Please reset the qe-verify+ flag if this gets fixed on ESR 68

Thanks for verifying Yuki!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting to ESR68?

Comment 21

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

Comment on attachment 9086030 [details]
Bug 1564639 - Make BrowserParent use nsPresContext::GetRootWidget() when handling IME related messages

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Not a security bug, but this fixes a basic behavior of IME on addon's popup window.
• User impact if declined: Without this fix, IME users cannot use IME comfortable in addon's popup window.
• Fix Landed on Version: 70
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This just makes all IME/keyboard handlers in BrowserParent and IMEStateManager use nsPresContext::GetRootWidget() with wrapping nsPresContext::GetTextInputHandlingWidget() which returns correct widget for communicating with native IME.
• String or UUID changes made by this patch: None

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9086030 [details]
Bug 1564639 - Make BrowserParent use nsPresContext::GetRootWidget() when handling IME related messages

Fixes an IME regression. No known issues with it on Beta so far. Approved for 68.2esr.

Comment 23

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/a597dc76a8fe

Comment 24

[Peter Magyari (Desktop QA)]

Hi Yuki,

Could you also verify the fix on Firefox ESR 68?
You can find it here: https://www.mozilla.org/en-US/firefox/organizations/all/

Thanks again!

Comment 25

[YUKI "Piro" Hiroshi]

ESR68.2.0 with this fix is not released yet, so I'll verify it after released.

Comment 26

[YUKI "Piro" Hiroshi]

I've tried this with ESR68.2.0 and I've confirmed it has been fixed. Thanks a lot!

Comment 27

[Peter Magyari (Desktop QA)]

Thank you for verifying Yuki!