Bug 1650705 - Don't dispatch unnecessary input event when we dispatch composition start with selected text. r=geckoview-reviewers,agi
authorMakoto Kato <m_kato@ga2.so-net.ne.jp>
Sun, 21 Feb 2021 17:12:13 +0000
changeset 568181 ce73c96906f465b19cfddba088129f790c402ec5
parent 568180 ffe056c7c496395708f643aeb00910622d689f64
child 568182 8ff75502852496095290c8b8edcea6c08b31eaaf
push id136886
push userm_kato@ga2.so-net.ne.jp
push dateSun, 21 Feb 2021 17:14:34 +0000
treeherderautoland@ce73c96906f4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgeckoview-reviewers, agi
bugs1650705, 1499076
milestone87.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 1650705 - Don't dispatch unnecessary input event when we dispatch composition start with selected text. r=geckoview-reviewers,agi Actually, before starting composition, we always remove current selected text. So it causes that input event is fired by this operation. This is different of Blink. `TextEventDispatcher` can set composition string using selected text when starting composition. So we can reduce this input event. Also, I mistake bug 1499076 fix. By this fix, we always fires input event when having selected text even if it is unnecessary. This changeset has this fix too. Differential Revision: https://phabricator.services.mozilla.com/D105464
mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/TextInputDelegateTest.kt
widget/android/GeckoEditableSupport.cpp
--- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/TextInputDelegateTest.kt
+++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/TextInputDelegateTest.kt
@@ -918,9 +918,35 @@ class TextInputDelegateTest : BaseSessio
         setComposingText(ic, "a", 1)
         assertTextAndSelectionAt("Can set composition string after calling blur and focus",
                                  ic, "ba", 2)
 
         pressKey(ic, KeyEvent.KEYCODE_R)
         assertText("Can set input string by keypress after calling blur and focus",
                    ic, "bar")
     }
+
+    @WithDisplay(width = 512, height = 512) // Child process updates require having a display.
+    @Test fun inputConnection_bug1650705() {
+        // no way on designmode.
+        assumeThat("Not in designmode", id, not(equalTo("#designmode")))
+
+        setupContent("")
+        val ic = mainSession.textInput.onCreateInputConnection(EditorInfo())!!
+
+        commitText(ic, "foo", 1)
+        setSelection(ic, 0, 3)
+
+        mainSession.evaluateJS("""
+            input_event_count = 0;
+            document.querySelector('$id').addEventListener('input', () => {
+                input_event_count++;
+            })
+        """)
+
+        setComposingText(ic, "barbaz", 1)
+
+        val count = mainSession.evaluateJS("input_event_count") as Double;
+        assertThat("input event is once", count, equalTo(1.0))
+
+        finishComposingText(ic)
+    }
 }
--- a/widget/android/GeckoEditableSupport.cpp
+++ b/widget/android/GeckoEditableSupport.cpp
@@ -909,17 +909,20 @@ bool GeckoEditableSupport::DoReplaceText
 
   RefPtr<TextComposition> composition(GetComposition());
   MOZ_ASSERT(!composition || !composition->IsEditorHandlingEvent());
 
   nsString string(aText->ToString());
   const bool composing = !mIMERanges->IsEmpty();
   nsEventStatus status = nsEventStatus_eIgnore;
   bool textChanged = composing;
-  bool performDeletion = true;
+  // Whether deleting content before setting or committing composition text.
+  bool performDeletion = false;
+  // Dispatch composition start to set current composition.
+  bool needDispatchCompositionStart = false;
 
   if (!mIMEKeyEvents.IsEmpty() || !composition || !mDispatcher->IsComposing() ||
       uint32_t(aStart) != composition->NativeOffsetOfStartComposition() ||
       uint32_t(aEnd) != composition->NativeOffsetOfStartComposition() +
                             composition->String().Length()) {
     // Only start a new composition if we have key events,
     // if we don't have an existing composition, or
     // the replaced text does not match our composition.
@@ -983,18 +986,24 @@ bool GeckoEditableSupport::DoReplaceText
           break;
         }
       }
       mIMEKeyEvents.Clear();
       return textChanged;
     }
 
     if (aStart != aEnd) {
-      // Perform a deletion first.
-      performDeletion = true;
+      if (composing) {
+        // Actually Gecko doesn't start composition, so it is unnecessary to
+        // delete content before setting composition string.
+        needDispatchCompositionStart = true;
+      } else {
+        // Perform a deletion first.
+        performDeletion = true;
+      }
     }
   } else if (composition->String().Equals(string)) {
     /* If the new text is the same as the existing composition text,
      * the NS_COMPOSITION_CHANGE event does not generate a text
      * change notification. However, the Java side still expects
      * one, so we manually generate a notification. */
     IMETextChange dummyChange;
     dummyChange.mStart = aStart;
@@ -1009,17 +1018,24 @@ bool GeckoEditableSupport::DoReplaceText
           intl_ime_hack_on_any_apps_fire_key_events_for_composition() ||
       mInputContext.mMayBeIMEUnaware) {
     SendIMEDummyKeyEvent(widget, eKeyDown);
     if (!mDispatcher || widget->Destroyed()) {
       return false;
     }
   }
 
-  if (performDeletion) {
+  if (needDispatchCompositionStart) {
+    // StartComposition sets composition string from selected string.
+    nsEventStatus status = nsEventStatus_eIgnore;
+    mDispatcher->StartComposition(status);
+    if (!mDispatcher || widget->Destroyed()) {
+      return false;
+    }
+  } else if (performDeletion) {
     WidgetContentCommandEvent event(true, eContentCommandDelete, widget);
     event.mTime = PR_Now() / 1000;
     widget->DispatchEvent(&event, status);
     if (!mDispatcher || widget->Destroyed()) {
       return false;
     }
     textChanged = true;
   }