Bug 1462257 - TextComposition should dispatch eCompositionChange event when eCompositionCommit is being fired immediately after eCompositionStart r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Wed, 11 Jul 2018 23:05:39 +0900
changeset 426468 37e818a03a1b0be87616b50d702a2e7406cc9172
parent 426443 254564563107faac0f16dd48fa6927ddbfac955c
child 426469 1b52153377e34adda9c64c5637d5335ca2f8f403
push id34274
push usernerli@mozilla.com
push dateFri, 13 Jul 2018 21:51:36 +0000
treeherdermozilla-central@e37eeb019cf8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1462257
milestone63.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1462257 - TextComposition should dispatch eCompositionChange event when eCompositionCommit is being fired immediately after eCompositionStart r=m_kato TextEditor modifies composition string or selected string when first eCompositionChange event is received. However, TextComposition dispatches eCompositionChange event ("text" event of DOM) only when composition string becomes non-empty if current composition string is empty. So, when IME dispatches only eCompositionStart and eCompositionCommit events for removing selected text, TextEditor does nothing. This hacky behavior is used by MS Pinyin on Windows 10 at least. For supporting this behavior, we need to make TextComposition dispatch eCompositionChange event when eCompositionChange(AsIs) event is fired even before dispatching eCompositionChange event. Although from point of view of web apps, the hacky composition should be merged into the previous composition if it's possible but it's out of scope of this bug. MozReview-Commit-ID: 7QfeBJamGTU
dom/events/TextComposition.cpp
dom/events/TextComposition.h
editor/libeditor/tests/test_bug1230473.html
widget/tests/window_composition_text_querycontent.xul
--- a/dom/events/TextComposition.cpp
+++ b/dom/events/TextComposition.cpp
@@ -62,16 +62,17 @@ TextComposition::TextComposition(nsPresC
   , mCompositionStartOffsetInTextNode(UINT32_MAX)
   , mCompositionLengthInTextNode(UINT32_MAX)
   , mIsSynthesizedForTests(aCompositionEvent->mFlags.mIsSynthesizedForTests)
   , mIsComposing(false)
   , mIsEditorHandlingEvent(false)
   , mIsRequestingCommit(false)
   , mIsRequestingCancel(false)
   , mRequestedToCommitOrCancel(false)
+  , mHasDispatchedDOMTextEvent(false)
   , mHasReceivedCommitEvent(false)
   , mWasNativeCompositionEndEventDiscarded(false)
   , mAllowControlCharacters(
       Preferences::GetBool("dom.compositionevent.allow_control_characters",
                            false))
   , mWasCompositionStringEmpty(true)
 {
   MOZ_ASSERT(aCompositionEvent->mNativeIMEContext.IsValid());
@@ -103,16 +104,23 @@ TextComposition::MaybeDispatchCompositio
                    const WidgetCompositionEvent* aCompositionEvent)
 {
   MOZ_RELEASE_ASSERT(!mTabParent);
 
   if (!IsValidStateForComposition(aCompositionEvent->mWidget)) {
     return false;
   }
 
+  // Note that we don't need to dispatch eCompositionUpdate event even if
+  // mHasDispatchedDOMTextEvent is false and eCompositionCommit event is
+  // dispatched with empty string immediately after eCompositionStart
+  // because composition string has never been changed from empty string to
+  // non-empty string in such composition even if selected string was not
+  // empty string (mLastData isn't set to selected text when this receives
+  // eCompositionStart).
   if (mLastData == aCompositionEvent->mData) {
     return true;
   }
   CloneAndDispatchAs(aCompositionEvent, eCompositionUpdate);
   return IsValidStateForComposition(aCompositionEvent->mWidget);
 }
 
 BaseEventFlags
@@ -351,20 +359,25 @@ TextComposition::DispatchCompositionEven
   }
 
   bool dispatchEvent = true;
   bool dispatchDOMTextEvent = aCompositionEvent->CausesDOMTextEvent();
 
   // When mIsComposing is false but the committing string is different from
   // the last data (E.g., previous eCompositionChange event made the
   // composition string empty or didn't have clause information), we don't
-  // need to dispatch redundant DOM text event.
+  // need to dispatch redundant DOM text event.  (But note that we need to
+  // dispatch eCompositionChange event if we have not dispatched
+  // eCompositionChange event yet and commit string replaces selected string
+  // with empty string since selected string hasn't been replaced with empty
+  // string yet.)
   if (dispatchDOMTextEvent &&
       aCompositionEvent->mMessage != eCompositionChange &&
-      !mIsComposing && mLastData == aCompositionEvent->mData) {
+      !mIsComposing && mHasDispatchedDOMTextEvent &&
+      mLastData == aCompositionEvent->mData) {
     dispatchEvent = dispatchDOMTextEvent = false;
   }
 
   // widget may dispatch redundant eCompositionChange event
   // which modifies neither composition string, clauses nor caret
   // position.  In such case, we shouldn't dispatch DOM events.
   if (dispatchDOMTextEvent &&
       aCompositionEvent->mMessage == eCompositionChange &&
@@ -382,20 +395,24 @@ TextComposition::DispatchCompositionEven
 
   if (dispatchEvent) {
     // If the composition event should cause a DOM text event, we should
     // overwrite the event message as eCompositionChange because due to
     // the limitation of mapping between event messages and DOM event types,
     // we cannot map multiple event messages to a DOM event type.
     if (dispatchDOMTextEvent &&
         aCompositionEvent->mMessage != eCompositionChange) {
+      mHasDispatchedDOMTextEvent = true;
       aCompositionEvent->mFlags =
         CloneAndDispatchAs(aCompositionEvent, eCompositionChange,
                            aStatus, aCallBack);
     } else {
+      if (aCompositionEvent->mMessage == eCompositionChange) {
+        mHasDispatchedDOMTextEvent = true;
+      }
       DispatchEvent(aCompositionEvent, aStatus, aCallBack);
     }
   } else {
     *aStatus = nsEventStatus_eConsumeNoDefault;
   }
 
   if (!IsValidStateForComposition(aCompositionEvent->mWidget)) {
     return;
--- a/dom/events/TextComposition.h
+++ b/dom/events/TextComposition.h
@@ -364,16 +364,19 @@ private:
 
   // mRequestedToCommitOrCancel is true *after* we requested IME to commit or
   // cancel the composition.  In other words, we already requested of IME that
   // it commits or cancels current composition.
   // NOTE: Before this is set to true, both mIsRequestingCommit and
   //       mIsRequestingCancel are set to false.
   bool mRequestedToCommitOrCancel;
 
+  // Set to true if the instance dispatches an eCompositionChange event.
+  bool mHasDispatchedDOMTextEvent;
+
   // Before this dispatches commit event into the tree, this is set to true.
   // So, this means if native IME already commits the composition.
   bool mHasReceivedCommitEvent;
 
   // mWasNativeCompositionEndEventDiscarded is true if this composition was
   // requested commit or cancel itself but native compositionend event is
   // discarded by PresShell due to not safe to dispatch events.
   bool mWasNativeCompositionEndEventDiscarded;
--- a/editor/libeditor/tests/test_bug1230473.html
+++ b/editor/libeditor/tests/test_bug1230473.html
@@ -46,16 +46,25 @@ SimpleTest.waitForFocus(()=>{
         aEditor.value = "";
       } else {
         aEditor.textContent = "";
       }
     }
 
     clear();
 
+    // FYI: Chrome commits composition if blur() and focus() are called during
+    //      composition.  But note that if they are called by compositionupdate
+    //      listener, the behavior is unstable.  On Windows, composition is
+    //      canceled.  On Linux and macOS, the composition is committed
+    //      internally but the string keeps underlined.  If they are called
+    //      by input event listener, committed on any platforms though.
+    //      On the other hand, Edge and Safari keeps composition even with
+    //      calling both blur() and focus().
+
     // Committing at compositionstart
     aEditor.focus();
     aEditor.addEventListener("compositionstart", committer, true);
     synthesizeCompositionChange({ composition: { string: "a", clauses: [{length: 1, attr: COMPOSITION_ATTR_RAW_CLAUSE }] },
                                   caret: { start: 1, length: 0 }, key: { key: "a" }});
     aEditor.removeEventListener("compositionstart", committer, true);
     ok(!isComposing(), "composition in " + aEditor.id + " should be committed by compositionstart event handler");
     is(value(), "", "composition in " + aEditor.id + " shouldn't insert any text since it's committed at compositionstart");
@@ -63,17 +72,17 @@ SimpleTest.waitForFocus(()=>{
 
     // Committing at first compositionupdate
     aEditor.focus();
     aEditor.addEventListener("compositionupdate", committer, true);
     synthesizeCompositionChange({ composition: { string: "a", clauses: [{length: 1, attr: COMPOSITION_ATTR_RAW_CLAUSE }] },
                                   caret: { start: 1, length: 0 }, key: { key: "a" }});
     aEditor.removeEventListener("compositionupdate", committer, true);
     ok(!isComposing(), "composition in " + aEditor.id + " should be committed by compositionupdate event handler");
-    is(value(), "", "composition in " + aEditor.id + " shouldn't have inserted any text since it's committed at first compositionupdate");
+    is(value(), "a", "composition in " + aEditor.id + " should have \"a\" since IME committed with it");
     clear();
 
     // Committing at first text (eCompositionChange)
     aEditor.focus();
     aEditor.addEventListener("text", committer, true);
     synthesizeCompositionChange({ composition: { string: "a", clauses: [{length: 1, attr: COMPOSITION_ATTR_RAW_CLAUSE }] },
                                   caret: { start: 1, length: 0 }, key: { key: "a" }});
     aEditor.removeEventListener("text", committer, true);
@@ -88,32 +97,32 @@ SimpleTest.waitForFocus(()=>{
                                   caret: { start: 1, length: 0 }, key: { key: "a" }});
     ok(isComposing(), "composition should be in " + aEditor.id + " before dispatching second compositionupdate");
     is(value(), "a", "composition in " + aEditor.id + " should be 'a' before dispatching second compositionupdate");
     aEditor.addEventListener("compositionupdate", committer, true);
     synthesizeCompositionChange({ composition: { string: "ab", clauses: [{length: 2, attr: COMPOSITION_ATTR_RAW_CLAUSE }] },
                                   caret: { start: 2, length: 0 }, key: { key: "b" }});
     aEditor.removeEventListener("compositionupdate", committer, true);
     ok(!isComposing(), "composition in " + aEditor.id + " should be committed by compositionupdate event handler");
-    todo_is(value(), "a", "composition in " + aEditor.id + " shouldn't have been modified since it's committed at second compositionupdate");
+    is(value(), "ab", "composition in " + aEditor.id + " should have \"ab\" since IME committed with it");
     clear();
 
     // Committing at second text (eCompositionChange)
     aEditor.focus();
     // FYI: "compositionstart" will be dispatched automatically.
     synthesizeCompositionChange({ composition: { string: "a", clauses: [{length: 1, attr: COMPOSITION_ATTR_RAW_CLAUSE }] },
                                   caret: { start: 1, length: 0 }, key: { key: "a" }});
     ok(isComposing(), "composition should be in " + aEditor.id + " before dispatching second text");
     is(value(), "a", "composition in " + aEditor.id + " should be 'a' before dispatching second text");
     aEditor.addEventListener("text", committer, true);
     synthesizeCompositionChange({ composition: { string: "ab", clauses: [{length: 2, attr: COMPOSITION_ATTR_RAW_CLAUSE }] },
                                   caret: { start: 2, length: 0 }, key: { key: "b" }});
     aEditor.removeEventListener("text", committer, true);
     ok(!isComposing(), "composition in " + aEditor.id + " should be committed by text event handler");
-    todo_is(value(), "a", "composition in " + aEditor.id + " shouldn't have been modified since it's committed at second text");
+    is(value(), "ab", "composition in " + aEditor.id + " should have \"ab\" since IME committed with it");
     clear();
   }
   runTest(document.getElementById("input"));
   runTest(document.getElementById("textarea"));
   runTest(document.getElementById("div"));
   SimpleTest.finish();
 });
 </script>
--- a/widget/tests/window_composition_text_querycontent.xul
+++ b/widget/tests/window_composition_text_querycontent.xul
@@ -879,16 +879,73 @@ function runCompositionCommitTest()
   synthesizeComposition({ type: "compositioncommit", data: "\u3043", key: { key: "KEY_Enter", type: "keyup" } });
 
   is(result.compositionupdate, true, "runCompositionCommitTest: compositionupdate should be fired after dispatching compositioncommit #3");
   is(result.compositionend, true, "runCompositionCommitTest: compositionend should be fired after dispatching compositioncommit #3");
   is(result.text, true, "runCompositionCommitTest: text should be fired after dispatching compositioncommit #3");
   is(result.input, true, "runCompositionCommitTest: input should be fired after dispatching compositioncommit #3");
   is(textarea.value, "\u3043", "runCompositionCommitTest: textarea doesn't have committed string #3");
 
+  // inserting empty string with simple composition.
+  textarea.value = "abc";
+  textarea.setSelectionRange(3, 3);
+  synthesizeComposition({ type: "compositionstart" });
+
+  clearResult();
+  synthesizeComposition({ type: "compositioncommit", data: "" });
+
+  is(result.compositionupdate, false,
+     "runCompositionCommitTest: compositionupdate should not be fired when interting empty string with composition");
+  is(result.compositionend, true,
+     "runCompositionCommitTest: compositionend should be fired when interting empty string with composition");
+  is(result.text, true,
+     "runCompositionCommitTest: text should be fired when interting empty string with composition");
+  is(result.input, true,
+     "runCompositionCommitTest: input should be fired when interting empty string with composition");
+  is(textarea.value, "abc",
+     "runCompositionCommitTest: textarea should keep original value when interting empty string with composition");
+
+  // replacing selection with empty string with simple composition.
+  textarea.value = "abc";
+  textarea.setSelectionRange(0, 3);
+  synthesizeComposition({ type: "compositionstart" });
+
+  clearResult();
+  synthesizeComposition({ type: "compositioncommit", data: "" });
+
+  is(result.compositionupdate, false,
+     "runCompositionCommitTest: compositionupdate should not be fired when replacing selection with empty string with composition");
+  is(result.compositionend, true,
+     "runCompositionCommitTest: compositionend should be fired when replacing selection with empty string with composition");
+  is(result.text, true,
+     "runCompositionCommitTest: text should be fired when replacing selection with empty string with composition");
+  is(result.input, true,
+     "runCompositionCommitTest: input should be fired when replacing selection with empty string with composition");
+  is(textarea.value, "",
+     "runCompositionCommitTest: textarea should become empty when replacing selection with empty string with composition");
+
+  // replacing selection with same string with simple composition.
+  textarea.value = "abc";
+  textarea.setSelectionRange(0, 3);
+  synthesizeComposition({ type: "compositionstart" });
+
+  clearResult();
+  synthesizeComposition({ type: "compositioncommit", data: "abc" });
+
+  is(result.compositionupdate, true,
+     "runCompositionCommitTest: compositionupdate should be fired when replacing selection with same string with composition");
+  is(result.compositionend, true,
+     "runCompositionCommitTest: compositionend should be fired when replacing selection with same string with composition");
+  is(result.text, true,
+     "runCompositionCommitTest: text should be fired when replacing selection with same string with composition");
+  is(result.input, true,
+     "runCompositionCommitTest: input should be fired when replacing selection with same string with composition");
+  is(textarea.value, "abc",
+     "runCompositionCommitTest: textarea should keep same value when replacing selection with same string with composition");
+
   // compositioncommit with non-empty composition string.
   textarea.value = "";
   synthesizeCompositionChange(
     { "composition":
       { "string": "\u3042",
         "clauses":
         [
           { "length": 1, "attr": COMPOSITION_ATTR_RAW_CLAUSE }
@@ -6416,23 +6473,25 @@ function runRemoveContentTest()
 
     synthesizeComposition({ type: "compositionstart" });
 
     events = [];
     parent.removeChild(textarea);
 
     hitEventLoop(function () {
       // XXX Currently, "input" event isn't fired on removed content.
-      is(events.length, 1,
+      is(events.length, 2,
          "runRemoveContentTest: wrong event count #2");
-      is(events[0].type, "compositionend",
+      is(events[0].type, "text",
+         "runRemoveContentTest: the 1st event must be text #2");
+      is(events[1].type, "compositionend",
          "runRemoveContentTest: the 1st event must be compositionend #2");
-      is(events[0].data, "",
+      is(events[1].data, "",
          "runRemoveContentTest: compositionupdate has wrong data #2");
-      is(events[0].target, textarea,
+      is(events[1].target, textarea,
          "runRemoveContentTest: The 1st event was fired on wrong event target #2");
       ok(!getEditor(textarea).isComposing,
          "runRemoveContentTest: the textarea still has composition #2");
       is(textarea.value, "",
          "runRemoveContentTest: the textarea has the committed text? #2");
 
       parent.insertBefore(textarea, nextSibling);