1199224 - [TSF] MS Korean IME doesn't replace composition string properly after ASCII character

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Maesil]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20150826004006

Steps to reproduce:

input "모질라 '파ㅍ" in textbox.


Actual results:

"모질라 '파ㅍ" is shown in textbox.


Expected results:

"모질라 '파" should be shown.

when I write "모질라 '파이어폭스'" in textbox, "모질라 '파이어폭스'ㅍ" is shown in textbox.
when I write "모질라'파이어폭스'" in textbox, it is shown normally.

Comment 1

[Maesil]

There is a mistake in above write.
-------------
Steps to reproduce:

input "모질라 '파" in textbox. <-- changed
introduction.
1) ㅁ, 모, 몾, 모지, 모질, 모질라, 모질라 ', 모질라 '파

Actual results:

"모질라 '파ㅍ" is shown in textbox.
1) ㅁ, 모, 몾, 모지, 모질, 모질라, 모질라 ', 모질라 '파ㅍ

Expected results:

"모질라 '파" should be shown.

--------------

I tested it in Firefox Developer Version 42.0a2 64bit version, Windows 8.1 64bit. There is a problem not only in textbox, but in search box.

It works propertly in Firefox 40.0.0.2.

Comment 2

[Maesil]

It has same problem in Nightly 43.0a1 (2015-08-25)

Comment 3

[Loic]

Did you test with e10s enabled or disabled?

Comment 4

[Alice0775 White]

Assumed IME problem

Comment 5

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

We need more info:

* Which IME are you using?
* If this is reproduced on Beta, it might be a bug of TSF mode. However, even if so, we cannot put it off from Firefox 41 because it's important for vertical writing mode support and may be too late to fix this on Beta stage.
* Could you tell us how to input as your STR with QWERTY keyboard layout? We're not sure how to input Hangul characters with Korean IME.

Comment 6

[Maesil]

(In reply to Loic from comment #3)
> Did you test with e10s enabled or disabled?

I tested it with e10s disabled in Dev. Edition 42, and e10 enabled e10s in Nightly 43. It has same problem when e10s disabled in Nightly 43. e10s is disabled in Dev. Edition 42: An accessibility tool is or was active. See bug 1115956.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #5)
> We need more info:
> 
> * Which IME are you using?
I use 'Korean - Microsoft IME'.

> * If this is reproduced on Beta, it might be a bug of TSF mode. However,
> even if so, we cannot put it off from Firefox 41 because it's important for
> vertical writing mode support and may be too late to fix this on Beta stage.

> * Could you tell us how to input as your STR with QWERTY keyboard layout?
> We're not sure how to input Hangul characters with Korean IME.

How to set Korean IME : http://www.koreanfluent.com/cross_cultural/korean_keyboard/korean_keyboard.htm

When you write "모질라 '파", type ㅁ, ㅗ, ㅈ, ㅣ, ㄹ, ㄹ, ㅏ ,  ,' ,ㅍ, ㅏ.

There is no way to input Korean in QWERTY keyboard layout as I know. Korean IME must be installed.

Comment 7

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

>> * Could you tell us how to input as your STR with QWERTY keyboard layout?
>> We're not sure how to input Hangul characters with Korean IME.
> 
> How to set Korean IME : http://www.koreanfluent.com/cross_cultural/korean_keyboard/korean_keyboard.htm
> 
> When you write "모질라 '파", type ㅁ, ㅗ, ㅈ, ㅣ, ㄹ, ㄹ, ㅏ ,  ,' ,ㅍ, ㅏ.
> 
> There is no way to input Korean in QWERTY keyboard layout as I know. Korean IME must be installed.

Thanks. I just want to know which key of QWERY inputs each key. So,

a, h, w, l, f, f, k, <space>, ', v, k,

right?

Comment 8

[Maesil]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #7)

> Thanks. I just want to know which key of QWERY inputs each key. So,
> 
> a, h, w, l, f, f, k, <space>, ', v, k,
> 
> right?

That's right. By the way, if you use backspace key when type it, this bug sometimes doesn't appear.

Comment 9

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

Okay, I reproduced this bug on Win7/Win8.1/Win10. However, I can reproduce this bug only in the input field in about:newtab.

So, something doing in about:newtab causes this bug.

Comment 10

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

Hmm, I reproduced in the search bar too. However, I cannot reproduce this bug in normal web pages.

Additionally, when I'm typing slowly, I cannot reproduce this bug.

Comment 11

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

Okay, I see the cause. MS Korean TIP behaves odd. When inserting the composition string after "'", it inserts first character first ("ㅍ"). Then, it starts composition with the range of the inserted character.

However, due to our limitation, we creates dummy composition at a call of InsertTextAtSelection(). But after that, Korean TIP tries to start new composition.

Comment 12

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

> TSF: 0x1d598d40   Locking (TS_LF_READWRITE) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> TSF: 0x1d598d40 TSFTextStore::GetSelection(ulIndex=4294967295, ulCount=1, pSelection=0x2ce650, pcFetched=0x2ce6d0)
> TSF: 0x1d598d40   TSFTextStore::CurrentSelection(): acpStart=5, acpEnd=5 (length=0), reverted=false
> TSF: 0x1d598d40   TSFTextStore::GetSelection() succeeded
> TSF: 0x1d598d40 TSFTextStore::InsertTextAtSelection(dwFlags=0, pchText=0x688c154 "ㅍ", cch=1, pacpStart=0x2ce5e4, pacpEnd=0x2ce5e8, pChange=0x2ce5c0), IsComposing()=false
> TSF: 0x1d598d40   TSFTextStore::InsertTextAtSelectionInternal(aInsertStr="ㅍ", aTextChange=0x2ce5c0), IsComposing=false
> TSF: 0x1d598d40   TSFTextStore::CurrentSelection(): acpStart=5, acpEnd=5 (length=0), reverted=false
> TSF: 0x1d598d40   TSFTextStore::LockedContent(): mLockedContent={ mText.Length()=4, mLastCompositionString="", mMinTextModifiedOffset=4294967295 }
> TSF: 0x1d598d40   TSFTextStore::InsertTextAtSelectionInternal() succeeded: mWidget=0xdd0f000, mWidget->Destroyed()=false, aTextChange={ acpStart=5, acpOldEnd=5, acpNewEnd=6 }
> TSF: 0x1d598d40   TSFTextStore::InsertTextAtSelection() succeeded: *pacpStart=5, *pacpEnd=6, *pChange={ acpStart=5, acpOldEnd=5, acpNewEnd=6 })
> TSF: 0x1d598d40 TSFTextStore::GetText(acpStart=0, acpEnd=-1, pchPlain=0x77774c10, cchPlainReq=128, pcchPlainOut=0x2ce1a4, prgRunInfo=0x77774d10, ulRunInfoReq=33, pulRunInfoOut=0x777748ac, pacpNext=0x777748b0), mComposition={ mStart=3, mString.Length()=0, IsComposing()=false }
> TSF: 0x1d598d40   TSFTextStore::CurrentSelection(): acpStart=6, acpEnd=6 (length=0), reverted=false
> TSF: 0x1d598d40   TSFTextStore::LockedContent(): mLockedContent={ mText.Length()=5, mLastCompositionString="", mMinTextModifiedOffset=5 }
> TSF: 0x1d598d40   TSFTextStore::GetText() succeeded: pcchPlainOut=0x2ce1a4, *prgRunInfo={ uCount=5, type=TS_RT_PLAIN }, *pulRunInfoOut=2004306092, *pacpNext=2004306096)
> TSF: 0x1d598d40 TSFTextStore::OnStartComposition(pComposition=0xf634a0, pfOk=0x2ce61c), mComposition.mView=0x0
> TSF: 0x1d598d40   TSFTextStore::RecordCompositionStartAction(aComposition=0xf634a0, aRange=0xf664ac, aPreserveSelection=false), mComposition.mView=0x0
> TSF: 0x1d598d40   TSFTextStore::RecordCompositionStartAction(aComposition=0xf634a0, aStart=5, aLength=1, aPreserveSelection=false), mComposition.mView=0x0
> TSF: 0x1d598d40   TSFTextStore::CurrentSelection(): acpStart=6, acpEnd=6 (length=0), reverted=false
> TSF: 0x1d598d40   TSFTextStore::LockedContent(): mLockedContent={ mText.Length()=5, mLastCompositionString="", mMinTextModifiedOffset=5 }
> TSF: 0x1d598d40   TSFTextStore::CurrentSelection(): acpStart=6, acpEnd=6 (length=0), reverted=false
> TSF: 0x1d598d40   TSFTextStore::RecordCompositionStartAction() succeeded: mComposition={ mStart=5, mString.Length()=0, mSelection={ acpStart=5, acpEnd=5, style.ase=TS_AE_END, style.fInterimChar=false } }
> TSF: 0x1d598d40   TSFTextStore::OnStartComposition() succeeded
> TSF: 0x1d598d40 TSFTextStore::GetStatus(pdcs=0x2ce6c8)
> TSF: 0x1d598d40 TSFTextStore::GetText(acpStart=0, acpEnd=-1, pchPlain=0x77774c10, cchPlainReq=128, pcchPlainOut=0x2ce250, prgRunInfo=0x77774d10, ulRunInfoReq=33, pulRunInfoOut=0x777748ac, pacpNext=0x777748b0), mComposition={ mStart=5, mString.Length()=0, IsComposing()=true }
> TSF: 0x1d598d40   TSFTextStore::CurrentSelection(): acpStart=5, acpEnd=5 (length=0), reverted=false
> TSF: 0x1d598d40   TSFTextStore::LockedContent(): mLockedContent={ mText.Length()=5, mLastCompositionString="", mMinTextModifiedOffset=5 }
> TSF: 0x1d598d40   TSFTextStore::GetText() succeeded: pcchPlainOut=0x2ce250, *prgRunInfo={ uCount=5, type=TS_RT_PLAIN }, *pulRunInfoOut=2004306092, *pacpNext=2004306096)
> TSF: 0x1d598d40 TSFTextStore::OnEndComposition(pComposition=0xf634a0), mComposition={ mView=0xf634a0, mString="" }
> TSF: 0x1d598d40   TSFTextStore::RecordCompositionEndAction(), mComposition={ mView=0xf634a0, mString="" }
> TSF: 0x1d598d40   TSFTextStore::CurrentSelection(): acpStart=5, acpEnd=5 (length=0), reverted=false
> TSF: 0x1d598d40   TSFTextStore::LockedContent(): mLockedContent={ mText.Length()=5, mLastCompositionString="", mMinTextModifiedOffset=5 }
> TSF: 0x1d598d40   TSFTextStore::RecordCompositionEndAction(), succeeded
> TSF: 0x1d598d40   TSFTextStore::OnEndComposition(), succeeded
> TSF: 0x1d598d40   Unlocked (TS_LF_READWRITE) <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

Oh, and accidentally, mLockedContent is too short. The selection is [5-5] but the content length is 4. This might be the cause.

Comment 13

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

[Tracking Requested - why for this release]:

This is a serious bug of TSF mode (TSF mode is enabled starting from 41) for Korean users. Additionally, this bug may cause odd behavior with other language's IMEs because the cause of this bug is a simple mistake at designing of TSFTextStore. TSFTextStore caches the whole contents of the focused editor while TSF locks the content. When the lock is read-write lock, it manages the cached content correctly. However, when the lock is read-only lock, it forgets to clear the cache at unlocking. Therefore, this bug is caused by following steps:

1. TSF requests read-only lock
2. TSF retrieves text contents during the lock (cached the contents)
3. unlocking the read-only lock (but cache isn't cleared)
4. User inputs text without IME (then, actual content which is expected by TSF and the cache become different)
5. User inputs text with IME (TSF tries to input new character, but the selection is out of the cached content)

So, we can fix this bug if we guarantee that the cache is cleared after read-only lock. So, the patch becomes simply.

Unfortunately, we need different patch for Beta and the others but both of them are simple.

Comment 14

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

Attachment: TSFTextStore should clear mLockedContent unless it needs to wait the content to be modified asynchronously

# If you're busy, feel free to let me know. We need to fix this bug ASAP for uplift

This patch renames mPendingClearLockedContent to mDeferClearingLockedContent and the meaning is reverted.

When mLockedContent is initialized in TSFTextStore::LockedContent(), it's always set false. I.e., in usual cases, it should be cleared immediately after unlocking the document.

For e10s mode, it's set true only when FlushPendingAction() dispatches events which will modify the content and cause NOTIFY_IME_OF_COMPOSITION_UPDATE. After that, mLockedContent won't be cleared until OnUpdateCompositionInternal() is called.

So, if the lock is read-only, FlushPendingAction() won't be called and mDeferClearingLockedContent won't be set true.

However, if mLockedContent is already initialized by previous document lock, the read-only lock doesn't cause clearing mLockedContent because mDeferClearingLockedContent is initialized *only* when mLockedContent is initialized.

Comment 15

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

Attachment: nsTextStore should clear mLockedContent at unlocking read-only lock

This is the patch for beta. This must be enough simple and safe!

Comment 16

[Masatoshi Kimura [:emk]]

Comment on attachment 8654037 [details] [diff] [review]
TSFTextStore should clear mLockedContent unless it needs to wait the content to be modified asynchronously

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

::: widget/windows/TSFTextStore.cpp
@@ +1773,5 @@
>            action.mRanges->AppendElement(wholeRange);
>          }
>          compositionChange.mRanges = action.mRanges;
>          // When the NS_COMPOSITION_CHANGE causes a DOM text event,
> +        // NOTIFY_IME_OF_COMPOSITION_UPDATE will be notified.  In such case,

the IME will be notified of NOTIFY_IME_OF_COMPOSITION_UPDATE

@@ +1774,5 @@
>          }
>          compositionChange.mRanges = action.mRanges;
>          // When the NS_COMPOSITION_CHANGE causes a DOM text event,
> +        // NOTIFY_IME_OF_COMPOSITION_UPDATE will be notified.  In such case,
> +        // we should wait to clear the locked content until it's notified.

we should not clear the locked content until we notify the IME of the composition update

Comment 17

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

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/ef0e6d94ee9aa9319396eb43fbd0e51f7af85eaa
changeset:  ef0e6d94ee9aa9319396eb43fbd0e51f7af85eaa
user:       Masayuki Nakano <[e-mail]>
date:       Fri Aug 28 23:17:37 2015 +0900
description:
Bug 1199224 TSFTextStore should clear mLockedContent unless it needs to wait the content to be modified asynchronously r=emk

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Masayuki, are you planning to request the uplift soon? Thanks

Comment 20

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

Comment on attachment 8654037 [details] [diff] [review]
TSFTextStore should clear mLockedContent unless it needs to wait the content to be modified asynchronously

Approval Request Comment
[Feature/regressing bug #]: TSF mode is new feature starting from 41.
[User impact if declined]: Korean IME users may hit this bug when they type ASCII character before typing a Hangul character. At this time, an unexpected character is committed and expected character is inserted before it.
[Describe test coverage new/current, TreeHerder]: Landed on m-c. And I tested manually and checked the log of the behavior.
[Risks and why]: Not so risky. Even if this patch has a bug, it must cause regression only in e10s mode because the reason why this patch is a little bit complicated than the patch for Beta is, including some fixes for e10s mode.
[String/UUID change made/needed]: Nothing.

Comment 21

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

Comment on attachment 8654048 [details] [diff] [review]
nsTextStore should clear mLockedContent at unlocking read-only lock

Approval Request Comment
[Feature/regressing bug #]: See above.
[User impact if declined]: See above.
[Describe test coverage new/current, TreeHerder]: Tested manually.
[Risks and why]: Really low. In 41, we don't support TSF in e10s mode. Therefore, this patch is very simple. This just adds the code to clear the cache at unlocking the document. This is what this bug is.
[String/UUID change made/needed]: No.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8654037 [details] [diff] [review]
TSFTextStore should clear mLockedContent unless it needs to wait the content to be modified asynchronously

Let's take it. We will have time to fix potential regressions.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ef5f85d45d35

Comment 24

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

Comment on attachment 8654048 [details] [diff] [review]
nsTextStore should clear mLockedContent at unlocking read-only lock

Since this is limited to TSF, I am assuming regressions would be isolated instead of it being widespread. We do want TSF in FF41 experience to be good, so as such it meets the bar.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8c4dafb1058a