1208043 - [TSF] MS-IME for Korean inserts composition string to the end of previous textnode when you type text at start of <p>

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[kylepark77]

Attachment: 2015_09_24_22_26_22_250.mp4

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/45.0.2454.99 Safari/537.36

Steps to reproduce:

1. typing characters (korean) inside P tag 
<p>가[caret]</p>
2. enter (at this time, P tag splitted and caret is located next P tag)
<p>가</p><p>[caret]</p>
3. continue to type characters (korean) in next P tag



Actual results:

moving caret & text to end of previous P tag
<p>가[caret]</p><p></p>
->
<p>가나[caret]</p><p></p>

if there is a textnode between two P tag, caret is placed end of a textnode when typing korean characters
<p>가</p>
<p>나[caret]</p>
->
<p>가</p>
가[caret]
<p>나</p>



Expected results:

fixed caret inside next P tag and text written
<p>가</p><p>나[caret]</p>

Comment 1

[Maesil]

This bug may related to bug 1199224. The reason of this bug may be TSF.

Comment 2

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

I guess that you use Firefox 41, right?

Which IME are use using? MS-IME for Korean?

Changing "intl.tsf.enable" to false (restarting Firefox required) helps you?

Comment 3

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

Ah, okay. I reproduced it on Win10 with MS-IME.

Comment 4

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

[Tracking Requested - why for this release]:
This must be a serious regression of enabling TSF. If we can fix this safely, we should uplift the patch as far as possible. I'll investigate the behavior of MS-IME for Korean tomorrow.

# But I'm not sure how many web apps creates <p> elements into a contenteditable element. If <p> isn't in the contenteditable element, Enter key causes inserting <br>.

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=96f392728a71

Comment 6

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

Attachment: If composition is started with InsertTextAtSelection() and OnStartComposition() is called later, RecordCompositionStartAction() should cancel the last pending compositionend

MS Korean IME starts composition with following steps:

1. Calling InsertTextAtSelection() with first composition string.
2. Calling OnStartComposition() with the range from selection start before #1 to the end of composition string inserted at #1.

This order isn't expected by TSFTextStore. Therefore, at #1, we append pending compositionstart with the selected range and compositionend with inserted string. I.e., it emulates inserting text without composing state. After #1, the selection range is collapsed immediately after the inserted string. Then, we try to start composition again at #2. In this time, the range is different from the selection range. Therefore, we set selection with the specified range. This causes moving insertion point to the end of the previous text node. The reason is, our flat text conversion doesn't insert any special character (like \n) between <p> elements, therefore, the end of preceding text node and the start of following text node is same offset.

For avoiding this, we need not to dispatch eSetSelection event. We can do that if we cancel the pending compositionend which is created by #1 and initialize mComposition with the specified range at #2.

In TSFTextStore::RecordCompositionStartAction(), it checks if InsertTextAtSelection() is called immediately before that and the specified range matches the information stored by InsertTextAtSelection().

If it matches the case of this bug, TSFTextStore::Content::RestoreCommittedComposition() initializes mComposition (mSelection shouldn't be modified because current selection should be as expected by TIP).

Finally, I found an existing bug of TSFTextStore::InsertTextAtSelectionInternal(). It doesn't initialize PendingAction::mAdjustSelection of COMPOSITION_START. Therefore, we're dispatching unnecessary eSetSelection event in this case. We need to fix this bug for not modifying selection with eSetSelection in the case of this bug.

Unfortunately, this patch is pretty risky for other TIPs especially for the other language users. If we need to uplift this for release too, we should restrict the new behavior should work only with MS Korean IME. However, fortunately, this is enough safe for other branches.

Comment 7

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

I do not believe this bug meets the bar for 41, too late, it will be a wontfix.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 42+. 

Masayuki, should we have a separate patch for beta 42 to restrict the fix to only MS Korean IME as you mention in comment 6?   We are heading towards the beta 3 build tomorrow; best if you talk with Sylvestre about beta uplift.

Comment 9

[tjtncks]

 I think that this bug dosen't happening while use windows 8 touch keyboard, but osk touch keyboard still affected by this bug. can you check about this?

Comment 10

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

(In reply to tjtncks from comment #9)
>  I think that this bug dosen't happening while use windows 8 touch keyboard,
> but osk touch keyboard still affected by this bug. can you check about this?

Did you test the patch with the patched build? If so, please file a new bug. We need to separate to each bug for different causes.

Comment 11

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

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #8)
> Tracking for 42+. 
> 
> Masayuki, should we have a separate patch for beta 42 to restrict the fix to
> only MS Korean IME as you mention in comment 6?

Sure, sounds reasonable even on Beta.

> We are heading towards the
> beta 3 build tomorrow; best if you talk with Sylvestre about beta uplift.

Unfortunately, this hasn't been reviewed yet.


Kimura-san, if you don't have much time to review the patch, I'll request the review to Kato-san. Feel free to cancel the request if you so.

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a25e01ac86a20da64b336892831f8e1932547913
Bug 1208043 If composition is started with InsertTextAtSelection() and OnStartComposition() is called later, RecordCompositionStartAction() should cancel the last pending compositionend r=emk

Comment 13

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

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

Comment 14

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

Attachment: part.2 Restrict the fix for Korean IMEs to them

This patch restricts the new behavior to MS-IME for Korean (only for Beta).

On Vista, Win8.1 and Win10, "Microsoft IME" is their default TIP which has same GUID.

On Win7, "Microsoft IME 2010" is its default TIP whose GUID is different from "Microsoft IME"'s.

On Win8.1 and Win10, "Microsoft Old Hangul" is also available, the GUID is different from both "Microsoft IME" and "Microsoft IME 2010".

I'm not sure about 3rd party's TIPs in Korea, though.

Comment 15

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

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

Comment 17

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

Comment on attachment 8667334 [details] [diff] [review]
If composition is started with InsertTextAtSelection() and OnStartComposition() is called later, RecordCompositionStartAction() should cancel the last pending compositionend

Approval Request Comment
[Feature/regressing bug #]:　New regression of enabling TSF in 41.
[User impact if declined]: Korean users cannot use some web services which use contenteditable which includes <p> elements.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: A little bit risky if there were some IMEs which behave similar to MS-IME for Korean but some of them wouldn't expect our new behavior. In strictly speaking, MS-IME for Korean behavior is odd since it inserts fixed string first and starting composition with the inserted string. Other IMEs start composition first with selected range and insert text at selection. Therefore, we should land following patch too only for Beta to reduce the risk.
[String/UUID change made/needed]: No.

Comment 18

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

Comment on attachment 8668958 [details] [diff] [review]
part.2 Restrict the fix for Korean IMEs to them

Approval Request Comment
[Describe test coverage new/current, TreeHerder]: Similar patch has been landed m-c. I checked all MS-IME for Korean on WinVista, Win7, Win8.1 and Win10 manually.
[Risks and why]: For reducing the risk of the previous patch, this restricts the new behavior only to MS-IME for Korean (like white-listed).
[String/UUID change made/needed]: No.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8667334 [details] [diff] [review]
If composition is started with InsertTextAtSelection() and OnStartComposition() is called later, RecordCompositionStartAction() should cancel the last pending compositionend

OK, let's try that.
Should be in 42 beta 4.

Comment 20

[Carsten Book [:Tomcat]]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=1db50a858c60

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6a830c5afb5d

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/effd8a2c9aff