Closed Bug 1081993 Opened 8 years ago Closed 8 years ago

[TSF] Candidate window position is always the top-left of browser window or just disappeared if IMM IME is active in TSF mode (~ATOK 2010, Japanist)

Categories

(Core :: Widget: Win32, defect)

35 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- unaffected
firefox35 + fixed
firefox36 --- fixed

People

(Reporter: alice0775, Assigned: masayuki)

References

Details

(Keywords: inputmethod, regression, reproducible)

Attachments

(2 files)

Using the old Atok, Candidate window position is always opened the top-left of browser window.

Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16998eb738aa
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20141002222135
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ec43ac8b65c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20141002233435
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=16998eb738aa&tochange=3ec43ac8b65c

Triggered by: Bug 975383
Attached image screenshot
Similar problem is observed with Atok 2014(latest version).
Candidate window position is always bottom-left of browser window.
[Tracking Requested - why for this release]: UI broken due to Bug 975383
Flags: needinfo?(masayuki)
This is NOT a new regression. However, this bug was hidden by another bug which was removed by bug 975383.

At bug 975383, a caller of nsIMM32Handler::SetIMERelatedWindowsPos() immediately after dispatching NS_COMPOSITION_UPDATE is removed.  However, for making nsIMM32Handler e10s-aware, we shouldn't call it synchronously. The correct approach is, the method should be called when NOTIFY_IME_OF_COMPOSITION_UPDATE is notified by TextComposition.

However, in TSF mode, WinIMEHandler::NotifyIME() never calls nsIMM32Handler::OnUpdateComposition(). This is the cause of this bug.

This bug shouldn't be reproduced on most users of Aurora, Beta and Release. But somebody may enable TSF. For such people, we need to uplift this.
Flags: needinfo?(masayuki)
Summary: Candidate window position is always opened the top-left of browser window under Atok2006 → Candidate window position is always the top-left of browser window or just disappeared if IMM IME is active in TSF mode (~ATOK 2010, Japanist)
This will fix the design of WinIMEHandler. That is necessary for enabling TSF in Aurora for IMM IME users.
Assignee: nobody → masayuki
Blocks: 1049488
Status: NEW → ASSIGNED
Summary: Candidate window position is always the top-left of browser window or just disappeared if IMM IME is active in TSF mode (~ATOK 2010, Japanist) → [TSF] Candidate window position is always the top-left of browser window or just disappeared if IMM IME is active in TSF mode (~ATOK 2010, Japanist)
Attached patch PatchSplinter Review
The cause of this bug is, WinIMEHandler doesn't send notification of composition updates to nsIMM32Handler. Therefore, nsIMM32Handler never calls SetIMERelatedWindowsPos(). The reason why this is regressed by bug 975383 is that NS_COMPOSITION_UPDATE event dispatcher called SetIMERelatedWindowsPos() unexpectedly and that was removed by the change.

WinIMEHandler was designed before I realized that even in TSF mode, we need to use nsIMM32Handler for handling IMM IME. Therefore, there are some bugs. All of them should be fixed here:

1. IMEHandler::IsComposing() and IsComposingOn() don't check if nsIMM32Handler has composition in TSF mode. This causes dispatching key events during composition.

2. IMEHandler::NotifyIME() doesn't send notifications in TSF mode.

3. IMEHandler::GetOpenState() queries IME open state to TSF even if IMM IME is active.
Attachment #8504802 - Flags: review?(VYV03354)
Comment on attachment 8504802 [details] [diff] [review]
Patch

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

::: widget/windows/WinIMEHandler.cpp
@@ +189,5 @@
> +        // composition window position.
> +        if (IsIMMActive()) {
> +          nsIMM32Handler::OnUpdateComposition(aWindow);
> +        }
> +        return NS_OK;

When IMM IME is inactive, this patch will change the return value from NS_ERROR_NOT_IMPLEMENTED to NS_OK. Is it intentional?

@@ +208,5 @@
>        case REQUEST_TO_COMMIT_COMPOSITION:
>          if (nsTextStore::IsComposingOn(aWindow)) {
>            nsTextStore::CommitComposition(false);
> +        } else if (IsIMMActive()) {
> +          nsIMM32Handler::CommitComposition(aWindow);

Is nsIMM32Handler::IsComposingOn() unnecessary?
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> WinIMEHandler was designed before I realized that even in TSF mode, we need
> to use nsIMM32Handler for handling IMM IME.

Out of curiosity, why is it needed?
(In reply to Masatoshi Kimura [:emk] from comment #7)
> Comment on attachment 8504802 [details] [diff] [review]
> Patch
> 
> Review of attachment 8504802 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/WinIMEHandler.cpp
> @@ +189,5 @@
> > +        // composition window position.
> > +        if (IsIMMActive()) {
> > +          nsIMM32Handler::OnUpdateComposition(aWindow);
> > +        }
> > +        return NS_OK;
> 
> When IMM IME is inactive, this patch will change the return value from
> NS_ERROR_NOT_IMPLEMENTED to NS_OK. Is it intentional?

Yes. The result shouldn't depend on active IME. Currently, the caller doesn't change the behavior with the result, but if it would do that, depending on active IME type may have caused unexpected bug.

> @@ +208,5 @@
> >        case REQUEST_TO_COMMIT_COMPOSITION:
> >          if (nsTextStore::IsComposingOn(aWindow)) {
> >            nsTextStore::CommitComposition(false);
> > +        } else if (IsIMMActive()) {
> > +          nsIMM32Handler::CommitComposition(aWindow);
> 
> Is nsIMM32Handler::IsComposingOn() unnecessary?

Yes. It will be checked in the method:
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsIMM32Handler.cpp#199
and currently, NotifyIME() doesn't check it in IMM mode:
http://mxr.mozilla.org/mozilla-central/source/widget/windows/WinIMEHandler.cpp#198

(In reply to Masatoshi Kimura [:emk] from comment #8)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> > WinIMEHandler was designed before I realized that even in TSF mode, we need
> > to use nsIMM32Handler for handling IMM IME.
> 
> Out of curiosity, why is it needed?

Because IMM IMEs don't cause accessing ITextStoreACP. Instead, they send/post WM_IME_* messages and respond to ImmFoo() APIs. So, actually, IMM IME is still handled by nsIMM32Handler. See bug 951966.
Comment on attachment 8504802 [details] [diff] [review]
Patch

Thanks for the clarifications.
Attachment #8504802 - Flags: review?(VYV03354) → review+
https://hg.mozilla.org/mozilla-central/rev/e59d5635eef2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Looks like we need an aurora uplift here to prevent shipping this regression, please nominate.
Flags: needinfo?(masayuki)
Right. I'll check the patch can be applied to Aurora.
Flags: needinfo?(masayuki)
Comment on attachment 8504802 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: bug 975383
[User impact if declined]: A candidate window of IME is very important UI. It should be near the caret position. However, this bug makes the window top-left corner of the window or hidden (depends on IME because we don't set candidate window position, so, the result is the default behavior of IME)
[Describe test coverage new/current, TBPL]: I tested manually. We cannot test native IME behavior with automated tests.
[Risks and why]: Risk isn't so high. Even if this has other regressions, it affects only few testers who enable TSF mode in Aurora, Beta or Release channel (TSF is now only enabled on Nightly in default settings) and the user uses IME implemented with IMM. This patch checks if active IME is not a IME of TSF aware. If so, it just falls back to IMM event handler (IMM is old IME framework of Windows).
[String/UUID change made/needed]: Nothing.
Attachment #8504802 - Flags: approval-mozilla-aurora?
Attachment #8504802 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.