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)
Tracking
()
RESOLVED
FIXED
mozilla44
People
(Reporter: kylepark77, Assigned: masayuki)
References
()
Details
(Keywords: inputmethod, regression)
Attachments
(3 files)
(deleted),
video/mp4
|
Details | |
(deleted),
patch
|
emk
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
emk
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•9 years ago
|
Severity: normal → critical
OS: Unspecified → Windows
Hardware: Unspecified → All
This bug may related to bug 1199224. The reason of this bug may be TSF.
Assignee | ||
Comment 2•9 years ago
|
||
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?
Assignee | ||
Comment 3•9 years ago
|
||
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>
Assignee | ||
Comment 4•9 years ago
|
||
[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
Assignee | ||
Comment 5•9 years ago
|
||
Assignee | ||
Comment 6•9 years ago
|
||
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?
Assignee | ||
Comment 10•9 years ago
|
||
(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.
Assignee | ||
Comment 11•9 years ago
|
||
(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)
Updated•9 years ago
|
Flags: needinfo?(VYV03354)
Attachment #8667334 -
Flags: review?(VYV03354) → review+
Assignee | ||
Comment 12•9 years ago
|
||
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
Assignee | ||
Comment 13•9 years ago
|
||
Assignee | ||
Comment 14•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8668958 -
Flags: review?(VYV03354) → review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Assignee | ||
Comment 17•9 years ago
|
||
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?
Assignee | ||
Comment 18•9 years ago
|
||
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 19•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8668958 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 20•9 years ago
|
||
Comment 21•9 years ago
|
||
Comment 22•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•