Bug 1462257 - TextComposition should dispatch eCompositionChange event when eCompositionCommit is being fired immediately after eCompositionStart r=m_kato a=lizzard
authorMasayuki Nakano <masayuki@d-toybox.com>
Wed, 11 Jul 2018 23:05:39 +0900
changeset 480567 3743c968feeb2a137fb950df3d9b762be0641b10
parent 480566 a59b0ee25b6ebca42259baf20b95b9a57d80a3fc
child 480568 f9542916a0bb4d55873991ef42ad2ae1fe5feef3
push id1757
push userffxbld-merge
push dateFri, 24 Aug 2018 17:02:43 +0000
treeherdermozilla-release@736023aebdb1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato, lizzard
bugs1462257
milestone62.0
Bug 1462257 - TextComposition should dispatch eCompositionChange event when eCompositionCommit is being fired immediately after eCompositionStart r=m_kato a=lizzard 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);