1234120 - [IMM] Either Google Japanese Input on Win7 or Japanist 2013 doesn't show suggest window at correct position after previous composition string is committed

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

On Win7, Google Japanese Input is installed as an IMM-IME.

At committing composition, IMMHandler fails to update selection cache since NOTIFY_IME_OF_SELECTION_CHANGE won't be notified if it's caused by composition. Therefore, IMR_QUERYCHARPOSITION result becomes wrong value (not modified to new caret position after the previous commit).

According to the original reporter, this is a regression since 42. I believe that this must be a regression of bug 1184449.

[Tracking Requested - why for this release]:

This makes Google Japanese Input users on Win Vista dn Win 7 inconvenient and this must be able to be fixed by simple fix. So, we should fix this on any branches.

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e0b4ebd1d7c

Comment 2

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

This is reproduced with Japanist 2013 which is not so major IME, but it's one of most famous IMEs supporting special keyboard layout, Thumb-shift keyboard. So, this bug may be serious for the users.

Comment 3

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

Attachment: part.1 IMEHandler should request all notifications which are requested by either IMMHander or TSFTextStore when IMM is available in TSF mode

Currently, nsIWidget::GetUpdatePreference() is called only immediately after an editor gets focus and in TSF mode, IMEHandler returns the result of TSFTextStore::GetUpdatePreference().  However, if IMM-IME is active at moving focus or becomes active after moving focus, IMMHandler starts to handle IMM messages.  Ideally, while IMMHandler works, IMEContentObsever and ContentCacheInParent should work with IMMHandler::GetUpdatePreference(). But unfortunately, we don't have a way to modify the behavior of them when IMM-IME becomes active after moving focus.

Therefore, when both IMM and TSF are enabled, IMEHandler should return both flags of IMMHandler::GetIMEUpdatePreference() and TSFTextStore::GetIMEUpdatePreference().

With this change, selection and text change notifications caused by composition are notified even in TSF mode.

Comment 4

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

Attachment: part.2 TSFTextStore should ignore unnecessary text and selection changes which are caused by composition

However, TSFTextStore shouldn't send selection/text change notifications to TSF/TIP since TSF spec defines so.

So, TSFTextStore should ignore if notifications are caused by composition.

Comment 5

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

Attachment: part.3 IMMHandler should forget caret position specified by IME when IMMHandler doesn't set caret range to eCompositionChange event

After we apply those patches, we meet new regression caused by bug 555642.

If caret is in selected clause, IMMHandler doesn't add caret range to eCompositionChange event. Then, IMETextTxn sets normal selection to the end of composition string and hide the caret. Therefore, IMMHandler is notified the selection range as the end of composition string.

This causes that IMMHandler::SetIMERelatedWindowsPos() sets candidate window position at the end of composition string (via IMMHandler::GetCaretRect() with the normal selection range). This is too bad. The candidate window should be positioned at start of selected clause.

For fixing this bug, IMMHandler should forget caret position specified by IME if IMMHandler doesn't set the caret position to eCompositionChange event. Because not setting the caret position to eCompositionChange event means that the caret position information is discarded. So, at setting the position of the candidate window, IMMHandler should decide the position without caret offset information.

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b03892eed2756b02ab61318e7f850d7c205332af
Bug 1234120 part.1 IMEHandler should request all notifications which are requested by either IMMHander or TSFTextStore when IMM is available in TSF mode r=m_kato

https://hg.mozilla.org/integration/mozilla-inbound/rev/43da4474ff14ef50c69be1101a3fc6a3e64541ee
Bug 1234120 part.2 TSFTextStore should ignore unnecessary text and selection changes which are caused by composition r=m_kato

https://hg.mozilla.org/integration/mozilla-inbound/rev/003a52668561ad43a757c83fd1df73d52c159488
Bug 1234120 part.3 IMMHandler should forget caret position specified by IME when IMMHandler doesn't set caret range to eCompositionChange event r=m_kato

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b03892eed275
https://hg.mozilla.org/mozilla-central/rev/43da4474ff14
https://hg.mozilla.org/mozilla-central/rev/003a52668561

Comment 8

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

Comment on attachment 8700602 [details] [diff] [review]
part.1 IMEHandler should request all notifications which are requested by either IMMHander or TSFTextStore when IMM is available in TSF mode

Approval Request Comment
[Feature/regressing bug #]: bug 1184449
[User impact if declined]: IMM-IME users such as users of Google Japanese Input on Win7 or ealier or all Japanist 2013 users see candidate window of IME at wrong position. This may cause composition string is covered by the window.
[Describe test coverage new/current, TreeHerder]: Landed on m-c. Tested manually.
[Risks and why]: The risk is low. This patch makes XP part takes all requests of notifications for both IMMHander and TSFTextStore. Unnecessary notifications can be ignored by themselves. 
[String/UUID change made/needed]: No.

Comment 9

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

Comment on attachment 8700603 [details] [diff] [review]
part.2 TSFTextStore should ignore unnecessary text and selection changes which are caused by composition

Approval Request Comment
[Feature/regressing bug #]: bug 1184449
[User impact if declined]: See above.
[Describe test coverage new/current, TreeHerder]: Landed on m-c and tested manually.
[Risks and why]: Nothing. This makes TSFTextStore ignore unnecessary notifications caused by the previous patch.
[String/UUID change made/needed]: Nothing.

Comment 10

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

Comment on attachment 8700605 [details] [diff] [review]
part.3 IMMHandler should forget caret position specified by IME when IMMHandler doesn't set caret range to eCompositionChange event

Approval Request Comment
[Feature/regressing bug #]: bug 555642
[User impact if declined]: If above patches are applied, users meet this regression. Due to this regression, candidate window of IME is always positioned at the end of composition string rather than start of selected clause in the composition string.
[Describe test coverage new/current, TreeHerder]: Landed on m-c and tested manually.
[Risks and why]: Low. This patch resets cached caret position specified by IME when IMMHandler doesn't send the caret position to our XP part. (The cached caret position causes IMMHandler tries to retrieve actual caret rect in Gecko, but the caret is positioned at the end of composition string if caret position specified by IME isn't sent to our XP part.)
[String/UUID change made/needed]: Nothing.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8700602 [details] [diff] [review]
part.1 IMEHandler should request all notifications which are requested by either IMMHander or TSFTextStore when IMM is available in TSF mode

IME specific so seems low risk. Beta44+, Aurora45+

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I don't feel the need to track this as it should not be release blocking but I am happy to uplift the patches to 44 as it seems to significantly impact Japanese IME users.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8700603 [details] [diff] [review]
part.2 TSFTextStore should ignore unnecessary text and selection changes which are caused by composition

Beta44+, Aurora45+

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8700605 [details] [diff] [review]
part.3 IMMHandler should forget caret position specified by IME when IMMHandler doesn't set caret range to eCompositionChange event

Beta44+, Aurora45+

Comment 15

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/ebc0418a8110
https://hg.mozilla.org/releases/mozilla-aurora/rev/a68652738f3c
https://hg.mozilla.org/releases/mozilla-aurora/rev/2fd7f7a794fb

Comment 16

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

https://hg.mozilla.org/releases/mozilla-beta/rev/a96441fac6aa
https://hg.mozilla.org/releases/mozilla-beta/rev/aac00a87efa6
https://hg.mozilla.org/releases/mozilla-beta/rev/7449cf7509b2

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/a96441fac6aa
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/aac00a87efa6
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/7449cf7509b2