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

RESOLVED FIXED in Firefox 42

Status

()

--
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kylepark77, Assigned: masayuki)

Tracking

({inputmethod, regression})

41 Branch
mozilla44
All
Windows
inputmethod, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41- wontfix, firefox42+ fixed, firefox43+ fixed, firefox44+ fixed, firefox-esr38 unaffected)

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
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>
(Reporter)

Updated

4 years ago
Severity: normal → critical
OS: Unspecified → Windows
Hardware: Unspecified → All

Comment 1

4 years ago
This bug may related to bug 1199224. The reason of this bug may be TSF.
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?
Severity: critical → major
Flags: needinfo?(kylepark77)
Keywords: inputmethod
Ah, okay. I reproduced it on Win10 with MS-IME.
Assignee: nobody → masayuki
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(kylepark77)
Summary: In contenteditable, moving caret & characters to end of previous textnode when typing korean at start position inside P tag → [TSF] MS-IME for Korean inserts composition string to the end of previous textnode when you type text at start of <p>
[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>.
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox43: --- → affected
status-firefox44: --- → affected
status-firefox-esr38: --- → unaffected
tracking-firefox41: --- → ?
tracking-firefox42: --- → ?
tracking-firefox43: --- → ?
tracking-firefox44: --- → ?
Keywords: regression
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.
Attachment #8667334 - Flags: review?(VYV03354)
I do not believe this bug meets the bar for 41, too late, it will be a wontfix.
status-firefox41: affected → wontfix
tracking-firefox41: ? → -
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.
tracking-firefox42: ? → +
tracking-firefox43: ? → +
tracking-firefox44: ? → +
Flags: needinfo?(masayuki)

Comment 9

4 years ago
 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?
(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.
(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.
Flags: needinfo?(masayuki) → needinfo?(VYV03354)
Flags: needinfo?(VYV03354)
Attachment #8667334 - Flags: review?(VYV03354) → review+
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
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.
Attachment #8668958 - Flags: review?(VYV03354)
Attachment #8668958 - Flags: review?(VYV03354) → review+
https://hg.mozilla.org/mozilla-central/rev/a25e01ac86a2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Duplicate of this bug: 1210494
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.
Attachment #8667334 - Flags: approval-mozilla-beta?
Attachment #8667334 - Flags: approval-mozilla-aurora?
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.
Attachment #8668958 - Flags: approval-mozilla-beta?
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.
Attachment #8667334 - Flags: approval-mozilla-beta?
Attachment #8667334 - Flags: approval-mozilla-beta+
Attachment #8667334 - Flags: approval-mozilla-aurora?
Attachment #8667334 - Flags: approval-mozilla-aurora+
Attachment #8668958 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 1210494

Updated

4 years ago
Duplicate of this bug: 1213811
You need to log in before you can comment on or make changes to this bug.