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

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[Alice0775 White]

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

Comment 1

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

Hmm, looks like that the editor does something with "input" event. I'd like to see minimized testcase...

Comment 2

[Makoto Kato [:m_kato]]

element.textContent = <any value> is called by input event.  But I don't find why this issue is ATOK only.

Comment 3

[Makoto Kato [:m_kato]]

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.

Comment 4

[Makoto Kato [:m_kato]]

Attachment: test case

Comment 5

[Makoto Kato [:m_kato]]

Humm, selection is not single node, so we cannot replace composing string correctly.  Current editor code requires that selection must be single text node.

Comment 6

[Makoto Kato [:m_kato]]

I have to investigate whey selection becomes multiple text node using ATOK.  If using Microsoft IME, it keeps single text node.

Comment 7

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

eSelectionSet event may cause changing selection range.

Comment 8

[Makoto Kato [:m_kato]]

(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... :-<

Comment 9

[Makoto Kato [:m_kato]]

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

Comment 10

[Makoto Kato [:m_kato]]

Attachment: Bug 1310912 - Part 1. CompositionTransaction should support multiple text nodes.

Google Keep uses range.insertNode on input event.  So, text node will be inserted into current caret position.

Microsoft IME's caret is after composition string, but ATOK's caret is before it.  So when using ATOK, this issue occurs.  By range.insertNode, text nodes that have IME attribute becomes multiple text nodes.  But CompositionTransanction doesn't consider that IME range isn't single text node.  Although it replaces current composition string with new string on first text node, it doesn't delete composition string on other text node.

Review commit: https://reviewboard.mozilla.org/r/90942/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90942/

Comment 11

[Makoto Kato [:m_kato]]

Attachment: Bug 1310912 - Part 2. Add test.

Review commit: https://reviewboard.mozilla.org/r/90944/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90944/

Comment 12

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

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.

Comment 13

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

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.

Comment 14

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

> 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 15

[Makoto Kato [:m_kato]]

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.

Comment 16

[Makoto Kato [:m_kato]]

I need skip for android.  Android's mochitest is broken for IME. See Bug 1315898.

Comment 17

[Makoto Kato [:m_kato]]

(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.

Comment 18

[Makoto Kato [:m_kato]]

Comment on attachment 8808019 [details]
Bug 1310912 - Part 1. CompositionTransaction should support multiple text nodes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90942/diff/1-2/

Comment 19

[Makoto Kato [:m_kato]]

Comment on attachment 8808020 [details]
Bug 1310912 - Part 2. Add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90944/diff/1-2/

Comment 20

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

(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 21

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

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});|?

Comment 22

[Makoto Kato [:m_kato]]

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...

Comment 23

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

(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?

Comment 24

[Makoto Kato [:m_kato]]

(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.

Comment 25

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

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 26

[Makoto Kato [:m_kato]]

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

Comment 27

[Makoto Kato [:m_kato]]

Attachment: Bug 1310912 - Part 3. The selection into PlaceholderTransaction should be updated via RangeUpdater.

Since the selection into PlaceholderTransaction isn't updated via RangeUpdater, UndoTransaction may return error when selection/caret position is changed.  Then, redo is failed.

So we should add selection into PlaceholderTrancation to RangeUpdater.

Review commit: https://reviewboard.mozilla.org/r/92198/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92198/

Comment 28

[Makoto Kato [:m_kato]]

Attachment: Bug 1310912 - Part 4. RangeUpdater should be called on DoTransaction.

Although PlaceholderTransaction will use the selection on Merge, it is too late to use UpdateRange.  Because RangeUpdater will be used after DoTransaction is finished.  So we should update selection on DoTransaction.

Also, part 1 fix doesn't update selection correctly via RangeUpdater when IME composition is multiple node.

Review commit: https://reviewboard.mozilla.org/r/92200/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92200/

Comment 29

[Makoto Kato [:m_kato]]

Attachment: Bug 1310912 - Part 5. InsertTextIntoTextNodeImpl should use IMETextNode for listener.

When using CompositionTransaction, text node will be inserted into IMETextNode, not aTextNode of parameter.  So we should use it for listener.

Review commit: https://reviewboard.mozilla.org/r/92202/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92202/

Comment 30

[Makoto Kato [:m_kato]]

Attachment: Bug 1310912 - Part 6. Add redo test.

Review commit: https://reviewboard.mozilla.org/r/92204/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92204/

Comment 31

[Makoto Kato [:m_kato]]

Comment on attachment 8808019 [details]
Bug 1310912 - Part 1. CompositionTransaction should support multiple text nodes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90942/diff/2-3/

Comment 32

[Makoto Kato [:m_kato]]

Comment on attachment 8808020 [details]
Bug 1310912 - Part 2. Add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90944/diff/2-3/

Comment 33

[Makoto Kato [:m_kato]]

Comment on attachment 8809667 [details]
Bug 1310912 - Part 6. Add redo test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92204/diff/1-2/

Comment 34

[Makoto Kato [:m_kato]]

Comment on attachment 8809667 [details]
Bug 1310912 - Part 6. Add redo test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92204/diff/2-3/

Comment 35

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

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.

Comment 36

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

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.

Comment 37

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

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".

Comment 38

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

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!

Comment 39

[Pulsebot]

Pushed by [e-mail]:
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

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f0e042371dfe
https://hg.mozilla.org/mozilla-central/rev/94211fba111d
https://hg.mozilla.org/mozilla-central/rev/6e763c778781
https://hg.mozilla.org/mozilla-central/rev/62a4cc7525bc
https://hg.mozilla.org/mozilla-central/rev/856c6ccb09a1
https://hg.mozilla.org/mozilla-central/rev/bac6adefd6a2