Closed Bug 1208043 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: DOM: Editor, defect)

41 Branch
All
Windows
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox41 - wontfix
firefox42 + fixed
firefox43 + fixed
firefox44 + fixed
firefox-esr38 --- unaffected

People

(Reporter: kylepark77, Assigned: masayuki)

References

()

Details

(Keywords: inputmethod, regression)

Attachments

(3 files)

Attached video 2015_09_24_22_26_22_250.mp4 (deleted) —
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>
Severity: normal → critical
OS: Unspecified → Windows
Hardware: Unspecified → All
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>.
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.
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.
Flags: needinfo?(masayuki)
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+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: