IME candidate window position is always top-left of the desktop when using any addon providing input field in its popup panel
Categories
(Core :: DOM: UI Events & Focus Handling, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox-esr68 | --- | verified |
firefox67 | --- | unaffected |
firefox68 | --- | wontfix |
firefox69 | --- | wontfix |
firefox70 | --- | verified |
People
(Reporter: yuki, Assigned: masayuki)
References
(Regression)
Details
(Keywords: inputmethod, regression)
Attachments
(2 files)
709.18 KB,
image/png
|
Details | |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-esr68+
|
Details | Review |
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
- Install Firefox 68.0.
- Install https://addons.mozilla.org/firefox/addon/second-search/
- Click the added toolbar button.
- 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
Updated•5 years ago
|
Comment 1•5 years ago
|
||
Hmm, Henri seems to be PTO. This is regression by bug 1543363 according to mozregression.
Updated•5 years ago
|
Assignee | ||
Comment 2•5 years ago
|
||
Well, I could investigate after bug 1548389. However, I'm really afraid to uplift patches for bug 1543363 regressions without Henri's review...
Updated•5 years ago
|
Updated•5 years ago
|
Comment 3•5 years ago
|
||
Henri, could you handle this since this is regression by bug 1543363?
Comment 4•5 years ago
|
||
(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()
?
Assignee | ||
Comment 5•5 years ago
|
||
(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()
andGetWidget()
.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 usedGetWidget()
.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 preferGetDocWidget()
overGetWidget()
?
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•5 years ago
|
||
(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 preferGetDocWidget()
overGetWidget()
?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 withnsPresContext::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()
andnsPresContext::GetRootWidget()
may return different widget since only the former usesnsContentUtils::WidgetForDocument()
. I'm still not sure which one returns expected widget, but I guess,BrowserParent::GetTopLevelWidget()
returns what is exactly same asnsPresContext::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•5 years ago
|
||
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•5 years ago
|
||
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.
Assignee | ||
Comment 9•5 years ago
|
||
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()
toGetWidget()
. Migrating the other callers onGetDocWidget()
toGetRootWidget()
still risks mismatches. What would be the expected effect of migrating the rest ofGetDocWidget()
callers toGetWidget()
and madeIMEStateManager
call something other thannsPresContext::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...
Assignee | ||
Comment 10•5 years ago
|
||
Okay, now I have a patch. The direction is correct. I guess that Henri missed some lines to change.
Assignee | ||
Comment 11•5 years ago
|
||
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.
Assignee | ||
Comment 12•5 years ago
|
||
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•5 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/b282fe75003a Make `BrowserParent` use `nsPresContext::GetRootWidget()` when handling IME related messages r=hsivonen
Comment 14•5 years ago
|
||
bugherder |
Comment 15•5 years ago
|
||
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?
Assignee | ||
Comment 16•5 years ago
|
||
(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.
Updated•5 years ago
|
Comment 17•5 years ago
|
||
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!
Reporter | ||
Comment 18•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
Is this something we should consider uplifting to ESR68?
Assignee | ||
Comment 21•5 years ago
|
||
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
andIMEStateManager
usensPresContext::GetRootWidget()
with wrappingnsPresContext::GetTextInputHandlingWidget()
which returns correct widget for communicating with native IME. - String or UUID changes made by this patch: None
Comment 22•5 years ago
|
||
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•5 years ago
|
||
bugherder uplift |
Comment 24•5 years ago
|
||
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!
Reporter | ||
Comment 25•5 years ago
|
||
ESR68.2.0 with this fix is not released yet, so I'll verify it after released.
Reporter | ||
Comment 26•5 years ago
|
||
I've tried this with ESR68.2.0 and I've confirmed it has been fixed. Thanks a lot!
Description
•