Closed Bug 1310912 Opened 3 years ago Closed 3 years ago

[TSF] Duplicate chars are committed to the edit area if you enter through ATOK

Categories

(Core :: Editor, defect, P3)

x86
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: alice0775, Assigned: m_kato)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

(Keywords: inputmethod, Whiteboard: [parity-chrome][tpi:+])

Attachments

(7 files)

Build Identifier:
https://hg.mozilla.org/mozilla-central/rev/94b0fddf96b43942bdd851a3275042909ea37e09
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0 ID:20161017030209

Using ATOK2016, Windows10 home x64.
Problem occurs regardless of the e10s.
Duplicate chars problem seems to occur only with TSF enabled. 

Reproducible : always

Steps To Reproduce:
1. Open https://keep.google.com/ and logged in
2. Click on input field [Take a note...]
3. Click on three dots menu(more) in the box
4. Chose "Show checkboxes"  (The problem does not occur if the checkboxes is not shown)
5. Type any text and Done

6. Click on the note
7. IME on
8. Type text , do conversion and then commit
   ---- Observe, Duplicate chars are committed 


Actual Results:
Duplicate chars are committed to the edit area. 
  See screen capture https://youtu.be/oXMvitV9nso


Expected Results:
not doubled
Flags: needinfo?(masayuki)
See Also: → 1310913
Whiteboard: [parity-chrome][parity-edge] → [parity-chrome]
Hmm, looks like that the editor does something with "input" event. I'd like to see minimized testcase...
Blocks: 1284419
Flags: needinfo?(masayuki)
Keywords: testcase-wanted
element.textContent = <any value> is called by input event.  But I don't find why this issue is ATOK only.
Priority: -- → P3
Whiteboard: [parity-chrome] → [parity-chrome][tpi:+]
Ignore comment #2.  Maybe, I think that root cause is same as bug 1310913.  Google keep seems to want to use like contenteditable="plaintext-only".  So they use Range object to update current editable.  But the behaviour is different of Chrome.
Attached file test case
Attachment #8802777 - Attachment is patch: false
Attachment #8802777 - Attachment mime type: text/plain → text/html
Keywords: testcase-wanted
Humm, selection is not single node, so we cannot replace composing string correctly.  Current editor code requires that selection must be single text node.
Assignee: nobody → m_kato
Summary: [TSF] Duplicate chars are committed to the edit area if you enter through the IME → [TSF] Duplicate chars are committed to the edit area if you enter through ATOK
I have to investigate whey selection becomes multiple text node using ATOK.  If using Microsoft IME, it keeps single text node.
eSelectionSet event may cause changing selection range.
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #7)
> eSelectionSet event may cause changing selection range.

This root cause is caret...  When using ATOK, IME caret is head of composing string.  But MS-IME, it is tail... :-<
Component: Widget: Win32 → Editor
Comment on attachment 8808019 [details]
Bug 1310912 - Part 1. CompositionTransaction should support multiple text nodes.

https://reviewboard.mozilla.org/r/90942/#review90724

::: editor/libeditor/CompositionTransaction.cpp:72
(Diff revision 1)
>      nsresult rv = mTextNode->InsertData(mOffset, mStringToInsert);
>      if (NS_WARN_IF(NS_FAILED(rv))) {
>        return rv;
>      }
>    } else {
> +    uint32_t length = mTextNode->TextLength() - mOffset;

Although, |length| is unclear name. How about |replaceableLength| or something?

::: editor/libeditor/CompositionTransaction.cpp:84
(Diff revision 1)
> +    // If IME text node is multiple node, ReplaceData doesn't remove all IME
> +    // text.  So we need remove remained text into other text node.
> +    if (length < mReplaceLength) {
> +      int32_t remainLength = mReplaceLength - length;
> +      nsCOMPtr<nsINode> node = mTextNode->GetNextSibling();
> +      while (node && node->NodeType() == nsIDOMNode::TEXT_NODE &&

Why don't you use nsINode::IsNodeOfType(nsINode::eTEXT)?

::: editor/libeditor/CompositionTransaction.cpp:86
(Diff revision 1)
> +    if (length < mReplaceLength) {
> +      int32_t remainLength = mReplaceLength - length;
> +      nsCOMPtr<nsINode> node = mTextNode->GetNextSibling();
> +      while (node && node->NodeType() == nsIDOMNode::TEXT_NODE &&
> +             remainLength > 0) {
> +        Text* content = static_cast<Text*>(node.get());

|content| is usually used for ptr to nsIContent. How about just |text|?

::: editor/libeditor/CompositionTransaction.cpp:88
(Diff revision 1)
> +      nsCOMPtr<nsINode> node = mTextNode->GetNextSibling();
> +      while (node && node->NodeType() == nsIDOMNode::TEXT_NODE &&
> +             remainLength > 0) {
> +        Text* content = static_cast<Text*>(node.get());
> +        uint32_t textLength = content->TextLength();
> +        content->DeleteData(0, remainLength);

Hmm, this is very terrible change.  Although, undoing each text node completely is not possible, this change *might* break something. But perhaps, this is enough safe since composition transaction must be undone/redone with a transaction committing composition.

However, anyway, please text if this won't break undo/redo before landing this patch.
Attachment #8808019 - Flags: review?(masayuki) → review+
Comment on attachment 8808020 [details]
Bug 1310912 - Part 2. Add test.

https://reviewboard.mozilla.org/r/90944/#review90734

::: editor/libeditor/tests/test_bug1310912.html:26
(Diff revision 1)
> +  elm.addEventListener("input", () => {
> +    let range = window.getSelection().getRangeAt(0);
> +    range.insertNode(document.createTextNode(""));
> +  });

Although, looks like that such input event handler should ignore input events whose isComposing is true. Should we also contact Google?

::: editor/libeditor/tests/test_bug1310912.html:35
(Diff revision 1)
> +  let tip = SpecialPowers.Cc["@mozilla.org/text-input-processor;1"].
> +            createInstance(SpecialPowers.Ci.nsITextInputProcessor);
> +
> +  tip.beginInputTransactionForTests(window);
> +  tip.startComposition();
> +  let composingText = "DEF";
> +  tip.setPendingCompositionString(composingText);
> +  tip.appendClauseToPendingComposition(composingText.length,
> +                                       tip.ATTR_RAW_CLAUSE);
> +  tip.setCaretInPendingComposition(3);
> +  tip.flushPendingComposition();
> +  ok(elm.textContent == "ABCDEF", "composing text should be set");

Why don't you use EventUtils's synthesizeCompositionChange()?

::: editor/libeditor/tests/test_bug1310912.html:78
(Diff revision 1)
> +  tip.setCaretInPendingComposition(1);
> +  tip.flushPendingComposition();
> +
> +  ok(elm.textContent == "ABCMNO", "composing text should be replaced");
> +
> +  tip.cancelComposition();

You can do this with synthesizeComposition() of EventUtils with "compositioncommit" and empty data.

::: editor/libeditor/tests/test_bug1310912.html:82
(Diff revision 1)
> +
> +  tip.cancelComposition();
> +
> +  ok(elm.textContent == "ABC", "composing text should be reset");
> +
> +  SimpleTest.finish();

Could you test undo/redo here? Synthesizing Accel+Z and Accel+Shift+Z is enough to kick them.
Attachment #8808020 - Flags: review?(masayuki) → review-
> Could you test undo/redo here? Synthesizing Accel+Z and Accel+Shift+Z is enough to kick them.

Oops, it doesn't make sense to test undo/redo after canceling composition. Could you commit something before that?
Comment on attachment 8808019 [details]
Bug 1310912 - Part 1. CompositionTransaction should support multiple text nodes.

https://reviewboard.mozilla.org/r/90942/#review90724

> Hmm, this is very terrible change.  Although, undoing each text node completely is not possible, this change *might* break something. But perhaps, this is enough safe since composition transaction must be undone/redone with a transaction committing composition.
> 
> However, anyway, please text if this won't break undo/redo before landing this patch.

OK, I will add test into part 2 that you comment.
Blocks: 1315898
No longer blocks: 1315898
Depends on: 1315898
I need skip for android.  Android's mochitest is broken for IME. See Bug 1315898.
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #13)
> Although, looks like that such input event handler should ignore input
> events whose isComposing is true. Should we also contact Google?

This issue is our bug.  I don't think whether google fixes. (If this occurs via Google IME, they may consider it....).  Also, I don't know why they uses Range.insertNode.
(In reply to Makoto Kato [:m_kato] from comment #17)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #13)
> > Although, looks like that such input event handler should ignore input
> > events whose isComposing is true. Should we also contact Google?
> 
> This issue is our bug.  I don't think whether google fixes. (If this occurs
> via Google IME, they may consider it....).  Also, I don't know why they uses
> Range.insertNode.

I meant that it's bad idea to modify DOM tree during composition for compatibility between browsers. I guess that the developer of this point perhaps doesn't know InputEvent.isComposing.
Comment on attachment 8808020 [details]
Bug 1310912 - Part 2. Add test.

https://reviewboard.mozilla.org/r/90944/#review91210

::: editor/libeditor/tests/test_bug1310912.html:36
(Diff revision 2)
> +
> +  elm.focus();
> +  let sel = window.getSelection();
> +  sel.collapse(elm.childNodes[0], elm.textContent.length);
> +
> +  synthesizeComposition({ type: "compositionstart" });

You can omit to synthesize compositionstart because nsITextInputProcessor will dispatch compositionstart automatically when you make non-empty composition string.

::: editor/libeditor/tests/test_bug1310912.html:39
(Diff revision 2)
> +  sel.collapse(elm.childNodes[0], elm.textContent.length);
> +
> +  synthesizeComposition({ type: "compositionstart" });
> +  synthesizeCompositionChange({
> +    composition: {
> +      string: "DEF", 

Please remove the whitespace at the end of this line.

::: editor/libeditor/tests/test_bug1310912.html:50
(Diff revision 2)
> +  });
> +  ok(elm.textContent == "ABCDEF", "composing text should be set");
> +
> +  synthesizeCompositionChange({
> +    composition: {
> +      string: "GHI", 

ditto.

::: editor/libeditor/tests/test_bug1310912.html:61
(Diff revision 2)
> +  });
> +  ok(elm.textContent == "ABCGHI", "composing text should be replaced");
> +
> +  synthesizeCompositionChange({
> +    composition: {
> +      string: "JKL", 

ditto.

::: editor/libeditor/tests/test_bug1310912.html:72
(Diff revision 2)
> +  });
> +  ok(elm.textContent == "ABCJKL", "composing text should be replaced");
> +
> +  synthesizeCompositionChange({
> +    composition: {
> +      string: "MNO", 

ditto.

::: editor/libeditor/tests/test_bug1310912.html:84
(Diff revision 2)
> +  synthesizeKey("Z", { accelKey: true });
> +  ok(elm.textContent == "ABC", "text should be undoed");
> +
> +  SimpleTest.finish();

Could you test redo with |synthesizeKey("Z", { accelKey: true, shiftKey: true});|?
Attachment #8808020 - Flags: review?(masayuki) → review+
Comment on attachment 8808020 [details]
Bug 1310912 - Part 2. Add test.

https://reviewboard.mozilla.org/r/90944/#review91210

> Could you test redo with |synthesizeKey("Z", { accelKey: true, shiftKey: true});|?

Since SelectionState::RestoreSelection is failed on UndoTransaction, redo is failure... humm...
(In reply to Makoto Kato [:m_kato] from comment #22)
> Since SelectionState::RestoreSelection is failed on UndoTransaction, redo is
> failure... humm...

Is it a bug of this change?
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #23)
> (In reply to Makoto Kato [:m_kato] from comment #22)
> > Since SelectionState::RestoreSelection is failed on UndoTransaction, redo is
> > failure... humm...
> 
> Is it a bug of this change?

No, it is new issue.  I think that we need improve RangeUpdater.
I meant if the breaks undo/redo transactions, we shouldn't land them. Otherwise, it's already broken, it's okay to land it for now.
Comment on attachment 8809664 [details]
Bug 1310912 - Part 3. The selection into PlaceholderTransaction should be updated via RangeUpdater.

https://reviewboard.mozilla.org/r/92198/#review92214

Oh, this is big change... But I guess this should work well. Anyway, we have much time to test this in Aurora stage. Let's take this.
Attachment #8809664 - Flags: review?(masayuki) → review+
Comment on attachment 8809665 [details]
Bug 1310912 - Part 4. RangeUpdater should be called on DoTransaction.

https://reviewboard.mozilla.org/r/92200/#review92218

::: editor/libeditor/CompositionTransaction.h:20
(Diff revision 1)
>  class TextRangeArray;
> +class RangeUpdater;

RangeUpdater should be before TextRangeArray. (A -> Z order)

::: editor/libeditor/CompositionTransaction.h:95
(Diff revision 1)
>    nsString mStringToInsert;
>  
>    // The editor, which is used to get the selection controller.
>    EditorBase& mEditorBase;
>  
> +  RangeUpdater* mRangeUpdater;

Hmm, the lifecycle of mRangeUpdater and *Transaction may not match. *Transaction is held by TransactionManager, but mRangeUpdater is managed by EditorBase.

Anyway, without EditorBase instance, transaction won't work. So, this actually doesn't have any problems. Actually, similar code is in other transaction class.

Ideally, we should redesign around this, though, of course, this is not a scope of this bug.

::: editor/libeditor/InsertTextTransaction.cpp:56
(Diff revision 1)
>    // Only set selection to insertion point if editor gives permission
>    if (mEditorBase.GetShouldTxnSetSelection()) {
>      RefPtr<Selection> selection = mEditorBase.GetSelection();
>      NS_ENSURE_TRUE(selection, NS_ERROR_NULL_POINTER);
>      DebugOnly<nsresult> rv =
>        selection->Collapse(mTextNode, mOffset + mStringToInsert.Length());
>      NS_ASSERTION(NS_SUCCEEDED(rv),
>                   "Selection could not be collapsed after insert");
>    } else {
>      // Do nothing - DOM Range gravity will adjust selection
>    }
> +  mRangeUpdater->SelAdjInsertText(*mTextNode, mOffset, mStringToInsert);

Shouldn't we do this in the preceding if block? But this must keep current behavior. So, even if my concern is right, it should be fixed in another bug.
Attachment #8809665 - Flags: review?(masayuki) → review+
Comment on attachment 8809666 [details]
Bug 1310912 - Part 5. InsertTextIntoTextNodeImpl should use IMETextNode for listener.

https://reviewboard.mozilla.org/r/92202/#review92222

::: editor/libeditor/EditorBase.cpp:2441
(Diff revision 1)
> -  int32_t replacedOffset = 0;
> +  RefPtr<Text> replacedTextNode = &aTextNode;
> +  int32_t replacedOffset = aOffset;

nit: I like "inserted" (as the method name said) or just "target" is better than "replaced".
Attachment #8809666 - Flags: review?(masayuki) → review+
Comment on attachment 8809667 [details]
Bug 1310912 - Part 6. Add redo test.

https://reviewboard.mozilla.org/r/92204/#review92226

I wonder, your this nice fix might improve the behavior in some SNS's complicated rich text fields (I really hope so!).

Anyway, thank you very much!
Attachment #8809667 - Flags: review?(masayuki) → review+
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0e042371dfe
Part 1. CompositionTransaction should support multiple text nodes. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/94211fba111d
Part 2. Add test. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e763c778781
Part 3. The selection into PlaceholderTransaction should be updated via RangeUpdater. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/62a4cc7525bc
Part 4. RangeUpdater should be called on DoTransaction. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/856c6ccb09a1
Part 5. InsertTextIntoTextNodeImpl should use IMETextNode for listener. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/bac6adefd6a2
Part 6. Add redo test. r=masayuki
Depends on: 1328023
You need to log in before you can comment on or make changes to this bug.