Bug 1507328 - 2. Make new text input tests pass; r=esawin
authorJim Chen <nchen@mozilla.com>
Fri, 16 Nov 2018 10:29:30 +0000
changeset 503171 afc6a9d15ad88381cd9c0159fa68770cbcb97a07
parent 503170 109c6162b9c2ad32611016fa680077e0e1dcbb7c
child 503172 b061ebcf3ba7c86654f6324fb4888a4da8053cde
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersesawin
bugs1507328
milestone65.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 1507328 - 2. Make new text input tests pass; r=esawin Make some fixes in GeckoEditable and GeckoEditableSupport to make the new tests pass under e10s. Differential Revision: https://phabricator.services.mozilla.com/D11989
mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java
mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testInputConnection.java
widget/android/GeckoEditableSupport.cpp
widget/android/GeckoEditableSupport.h
--- a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java
+++ b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java
@@ -100,18 +100,21 @@ import android.view.inputmethod.EditorIn
     private int mIMEState = // Used by IC thread.
             SessionTextInput.EditableListener.IME_STATE_DISABLED;
     private String mIMETypeHint = ""; // Used by IC/UI thread.
     private String mIMEModeHint = ""; // Used by IC thread.
     private String mIMEActionHint = ""; // Used by IC thread.
     private int mIMEFlags; // Used by IC thread.
 
     private boolean mIgnoreSelectionChange; // Used by Gecko thread
-    private int mLastTextChangeStart = -1; // Used by Gecko thread
-    private int mLastTextChangeEnd = -1; // Used by Gecko thread
+    // Combined offsets from the previous batch of onTextChange calls; valid
+    // between the onTextChange calls and the next onSelectionChange call.
+    private int mLastTextChangeStart = Integer.MAX_VALUE; // Used by Gecko thread
+    private int mLastTextChangeOldEnd = -1; // Used by Gecko thread
+    private int mLastTextChangeNewEnd = -1; // Used by Gecko thread
     private boolean mLastTextChangeReplacedSelection; // Used by Gecko thread
 
     // Prevent showSoftInput and hideSoftInput from being called multiple times in a row,
     // including reentrant calls on some devices. Used by UI/IC thread.
     /* package */ final AtomicInteger mSoftInputReentrancyGuard = new AtomicInteger();
 
     private static final int IME_RANGE_CARETPOSITION = 1;
     private static final int IME_RANGE_RAWINPUT = 2;
@@ -1131,42 +1134,48 @@ import android.view.inputmethod.EditorIn
         if (DEBUG) {
             Log.d(LOGTAG, "reply: Action(" +
                           getConstantName(Action.class, "TYPE_", action.mType) + ")");
         }
         switch (action.mType) {
         case Action.TYPE_REPLACE_TEXT: {
             final Spanned currentText = mText.getCurrentText();
             final int actionNewEnd = action.mStart + action.mSequence.length();
-            if (mLastTextChangeStart < 0 || mLastTextChangeEnd > currentText.length() ||
-                action.mStart < mLastTextChangeStart || actionNewEnd > mLastTextChangeEnd) {
+            if (mLastTextChangeStart > mLastTextChangeNewEnd ||
+                mLastTextChangeNewEnd > currentText.length() ||
+                action.mStart < mLastTextChangeStart || actionNewEnd > mLastTextChangeNewEnd) {
                 // Replace-text action doesn't match our text change.
                 break;
             }
 
             int indexInText = TextUtils.indexOf(currentText, action.mSequence,
-                                                action.mStart, mLastTextChangeEnd);
+                                                action.mStart, mLastTextChangeNewEnd);
             if (indexInText < 0 && action.mStart != mLastTextChangeStart) {
                 final String changedText = TextUtils.substring(
                         currentText, mLastTextChangeStart, actionNewEnd);
                 indexInText = changedText.lastIndexOf(action.mSequence.toString());
                 if (indexInText >= 0) {
                     indexInText += mLastTextChangeStart;
                 }
             }
             if (indexInText < 0) {
                 // Replace-text action doesn't match our current text.
                 break;
             }
 
+            final int selStart = Selection.getSelectionStart(currentText);
+            final int selEnd = Selection.getSelectionEnd(currentText);
+
             // Replace-text action matches our current text; copy the new spans to the
             // current text.
             mText.currentReplace(indexInText,
                                  indexInText + action.mSequence.length(),
                                  action.mSequence);
+            // Make sure selection is preserved.
+            mText.currentSetSelection(selStart, selEnd);
 
             // The text change is caused by the replace-text event. If the text change
             // replaced the previous selection, we need to rely on Gecko for an updated
             // selection, so don't ignore selection change. However, if the text change
             // did not replace the previous selection, we can ignore the Gecko selection
             // in favor of the Java selection.
             mIgnoreSelectionChange = !mLastTextChangeReplacedSelection;
             break;
@@ -1177,16 +1186,24 @@ import android.view.inputmethod.EditorIn
             if (action.mStart > len || action.mEnd > len ||
                     !TextUtils.substring(mText.getCurrentText(), action.mStart,
                                          action.mEnd).equals(action.mSequence)) {
                 if (DEBUG) {
                     Log.d(LOGTAG, "discarding stale set span call");
                 }
                 break;
             }
+            if ((action.mSpanObject == Selection.SELECTION_START ||
+                 action.mSpanObject == Selection.SELECTION_END) &&
+                (action.mStart < mLastTextChangeStart && action.mEnd < mLastTextChangeStart ||
+                 action.mStart > mLastTextChangeOldEnd && action.mEnd > mLastTextChangeOldEnd)) {
+                // Use the Java selection if, between text-change notification and replace-text
+                // processing, we specifically set the selection to outside the replaced range.
+                mLastTextChangeReplacedSelection = false;
+            }
             mText.currentSetSpan(action.mSpanObject, action.mStart, action.mEnd, action.mSpanFlags);
             break;
 
         case Action.TYPE_REMOVE_SPAN:
             mText.currentRemoveSpan(action.mSpanObject);
             break;
 
         case Action.TYPE_SET_HANDLER:
@@ -1622,18 +1639,19 @@ import android.view.inputmethod.EditorIn
         if (mIgnoreSelectionChange) {
             mIgnoreSelectionChange = false;
         } else {
             mText.currentSetSelection(start, end);
         }
 
         // We receive selection change notification after receiving replies for pending
         // events, so we can reset text change bounds at this point.
-        mLastTextChangeStart = -1;
-        mLastTextChangeEnd = -1;
+        mLastTextChangeStart = Integer.MAX_VALUE;
+        mLastTextChangeOldEnd = -1;
+        mLastTextChangeNewEnd = -1;
         mLastTextChangeReplacedSelection = false;
 
         mIcPostHandler.post(new Runnable() {
             @Override
             public void run() {
                 icSyncShadowText();
             }
         });
@@ -1668,18 +1686,19 @@ import android.view.inputmethod.EditorIn
             // newly-focused editors). Simply replace the text in that case; replace in
             // two steps to properly clear composing spans that span the whole range.
             mText.currentReplace(0, currentLength, "");
             mText.currentReplace(0, 0, text);
 
             // Don't ignore the next selection change because we are re-syncing with Gecko
             mIgnoreSelectionChange = false;
 
-            mLastTextChangeStart = -1;
-            mLastTextChangeEnd = -1;
+            mLastTextChangeStart = Integer.MAX_VALUE;
+            mLastTextChangeOldEnd = -1;
+            mLastTextChangeNewEnd = -1;
             mLastTextChangeReplacedSelection = false;
 
         } else if (!geckoIsSameText(start, oldEnd, text)) {
             final Spanned currentText = mText.getCurrentText();
             final int selStart = Selection.getSelectionStart(currentText);
             final int selEnd = Selection.getSelectionEnd(currentText);
 
             // True if the selection was in the middle of the replaced text; in that case
@@ -1689,31 +1708,33 @@ import android.view.inputmethod.EditorIn
                 (selStart >= start && selStart <= oldEnd) ||
                 (selEnd >= start && selEnd <= oldEnd);
 
             // Gecko side initiated the text change. Replace in two steps to properly
             // clear composing spans that span the whole range.
             mText.currentReplace(start, oldEnd, "");
             mText.currentReplace(start, start, text);
 
-            mLastTextChangeStart = start;
-            mLastTextChangeEnd = newEnd;
+            mLastTextChangeStart = Math.min(start, mLastTextChangeStart);
+            mLastTextChangeOldEnd = Math.max(oldEnd, mLastTextChangeOldEnd);
+            mLastTextChangeNewEnd = Math.max(newEnd, mLastTextChangeNewEnd);
 
         } else {
             // Nothing to do because the text is the same. This could happen when
             // the composition is updated for example, in which case we want to keep the
             // Java selection.
             final Action action = mActions.peek();
             mIgnoreSelectionChange = mIgnoreSelectionChange || (action != null &&
                     (action.mType == Action.TYPE_REPLACE_TEXT ||
                      action.mType == Action.TYPE_SET_SPAN ||
                      action.mType == Action.TYPE_REMOVE_SPAN));
 
-            mLastTextChangeStart = start;
-            mLastTextChangeEnd = newEnd;
+            mLastTextChangeStart = Math.min(start, mLastTextChangeStart);
+            mLastTextChangeOldEnd = Math.max(oldEnd, mLastTextChangeOldEnd);
+            mLastTextChangeNewEnd = Math.max(newEnd, mLastTextChangeNewEnd);
         }
 
         // onTextChange is always followed by onSelectionChange, so we let
         // onSelectionChange schedule a shadow text sync.
     }
 
     @Override // IGeckoEditableParent
     public void onDefaultKeyEvent(final IBinder token, final KeyEvent event) {
--- a/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testInputConnection.java
+++ b/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testInputConnection.java
@@ -460,17 +460,17 @@ public class testInputConnection extends
             });
 
             // Bug 1254629, crash when hiding input during input.
             ic.commitText("foo", 1);
             assertTextAndSelectionAt("Can commit text (hiding)", ic, "foo", 3);
 
             ic.commitText("!", 1);
             // The '!' key causes the input to hide in robocop_input.html,
-            assertTextAndSelectionAt("Can handle hiding input", ic, "foo!", 4);
+            assertTextAndSelectionAt("Can handle hiding input", ic, "foo", 3);
 
             // Bug 1401737, Editable does not behave correctly after disconnecting from Gecko.
             getJS().syncCall("blur_hiding_input");
             processGeckoEvents();
             processInputConnectionEvents();
 
             ic.setComposingRegion(0, 3);
             ic.commitText("bar", 1);
--- a/widget/android/GeckoEditableSupport.cpp
+++ b/widget/android/GeckoEditableSupport.cpp
@@ -796,16 +796,22 @@ GeckoEditableSupport::PostFlushIMEChange
 
 void
 GeckoEditableSupport::FlushIMEChanges(FlushChangesFlag aFlags)
 {
     // Only send change notifications if we are *not* masking events,
     // i.e. if we have a focused editor,
     NS_ENSURE_TRUE_VOID(!mIMEMaskEventsCount);
 
+    if (mIMEDelaySynchronizeReply && mIMEActiveCompositionCount > 0) {
+        // We are still expecting more composition events to be handled. Once
+        // that happens, FlushIMEChanges will be called again.
+        return;
+    }
+
     nsCOMPtr<nsIWidget> widget = GetWidget();
     NS_ENSURE_TRUE_VOID(widget);
 
     struct TextRecord
     {
         nsString text;
         int32_t start;
         int32_t oldEnd;
@@ -916,16 +922,17 @@ GeckoEditableSupport::FlushIMEChanges(Fl
     }
 
     while (mIMEDelaySynchronizeReply && mIMEActiveSynchronizeCount) {
         mIMEActiveSynchronizeCount--;
         mEditable->NotifyIME(EditableListener::NOTIFY_IME_REPLY_EVENT);
     }
     mIMEDelaySynchronizeReply = false;
     mIMEActiveSynchronizeCount = 0;
+    mIMEActiveCompositionCount = 0;
 
     if (mIMESelectionChanged) {
         mIMESelectionChanged = false;
         mEditable->OnSelectionChange(selStart, selEnd);
         flushOnException();
     }
 }
 
@@ -1063,17 +1070,17 @@ GeckoEditableSupport::DoReplaceText(int3
                 } else {
                     mDispatcher->MaybeDispatchKeypressEvents(*event, status);
                 }
                 if (!mDispatcher || widget->Destroyed()) {
                     break;
                 }
             }
             mIMEKeyEvents.Clear();
-            return true;
+            return false;
         }
 
         if (aStart != aEnd) {
             // Perform a deletion first.
             WidgetContentCommandEvent event(
                     true, eContentCommandDelete, widget);
             event.mTime = PR_Now() / 1000;
             widget->DispatchEvent(&event, status);
@@ -1102,20 +1109,22 @@ GeckoEditableSupport::DoReplaceText(int3
         if (!mDispatcher || widget->Destroyed()) {
             return false;
         }
     }
 
     if (composing) {
         mDispatcher->SetPendingComposition(string, mIMERanges);
         mDispatcher->FlushPendingComposition(status);
+        mIMEActiveCompositionCount++;
         // Ensure IME ranges are empty.
         mIMERanges->Clear();
     } else if (!string.IsEmpty() || mDispatcher->IsComposing()) {
         mDispatcher->CommitComposition(status, &string);
+        mIMEActiveCompositionCount++;
         textChanged = true;
     }
     if (!mDispatcher || widget->Destroyed()) {
         return false;
     }
 
     if (sDispatchKeyEventsInCompositionForAnyApps ||
         mInputContext.mMayBeIMEUnaware) {
@@ -1171,27 +1180,26 @@ GeckoEditableSupport::DoUpdateCompositio
     }
 
     nsCOMPtr<nsIWidget> widget = GetWidget();
     nsEventStatus status = nsEventStatus_eIgnore;
     NS_ENSURE_TRUE(mDispatcher && widget, false);
 
     const bool keepCurrent = !!(aFlags &
             java::GeckoEditableChild::FLAG_KEEP_CURRENT_COMPOSITION);
-    bool compositionChanged = false;
 
     // A composition with no ranges means we want to set the selection.
     if (mIMERanges->IsEmpty()) {
         if (keepCurrent && mDispatcher->IsComposing()) {
             // Don't set selection if we want to keep current composition.
             return false;
         }
 
         MOZ_ASSERT(aStart >= 0 && aEnd >= 0);
-        compositionChanged = RemoveComposition();
+        const bool compositionChanged = RemoveComposition();
 
         WidgetSelectionEvent selEvent(true, eSetSelection, widget);
         selEvent.mOffset = std::min(aStart, aEnd);
         selEvent.mLength = std::max(aStart, aEnd) - selEvent.mOffset;
         selEvent.mReversed = aStart > aEnd;
         selEvent.mExpandToClusterBoundary = false;
         widget->DispatchEvent(&selEvent, status);
         return compositionChanged;
@@ -1216,17 +1224,16 @@ GeckoEditableSupport::DoUpdateCompositio
             // Don't start a new composition if we want to keep the current one.
             mIMERanges->Clear();
             return false;
         }
 
         // Only start new composition if we don't have an existing one,
         // or if the existing composition doesn't match the new one.
         RemoveComposition();
-        compositionChanged = true;
 
         {
             WidgetSelectionEvent event(true, eSetSelection, widget);
             event.mOffset = uint32_t(aStart);
             event.mLength = uint32_t(aEnd - aStart);
             event.mExpandToClusterBoundary = false;
             event.mReason = nsISelectionListener::IME_REASON;
             widget->DispatchEvent(&event, status);
@@ -1253,17 +1260,17 @@ GeckoEditableSupport::DoUpdateCompositio
 
     if (NS_WARN_IF(NS_FAILED(BeginInputTransaction(mDispatcher)))) {
         mIMERanges->Clear();
         return false;
     }
     mDispatcher->SetPendingComposition(string, mIMERanges);
     mDispatcher->FlushPendingComposition(status);
     mIMERanges->Clear();
-    return compositionChanged;
+    return true;
 }
 
 void
 GeckoEditableSupport::OnImeRequestCursorUpdates(int aRequestMode)
 {
     if (aRequestMode == EditableClient::ONE_SHOT) {
         UpdateCompositionRects();
         return;
@@ -1363,16 +1370,17 @@ GeckoEditableSupport::NotifyIME(TextEven
             mIMEFocusCount--;
             MOZ_ASSERT(mIMEFocusCount >= 0);
 
             RefPtr<GeckoEditableSupport> self(this);
             nsAppShell::PostEvent([this, self] {
                 if (!mIMEFocusCount) {
                     mIMEDelaySynchronizeReply = false;
                     mIMEActiveSynchronizeCount = 0;
+                    mIMEActiveCompositionCount = 0;
                     mEditable->NotifyIME(EditableListener::NOTIFY_IME_OF_BLUR);
                     OnRemovedFrom(mDispatcher);
                 }
             });
 
             // Mask events because we lost focus. Unmask on the next focus.
             mIMEMaskEventsCount++;
             break;
@@ -1397,16 +1405,24 @@ GeckoEditableSupport::NotifyIME(TextEven
             mIMESelectionChanged = true;
             AddIMETextChange(IMETextChange(aNotification));
             break;
         }
 
         case NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED: {
             ALOGIME("IME: NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED");
 
+            // We often only get one event-handled notification after a pair of
+            // update-composition then replace-text calls. Therefore, only count
+            // the number of composition events for replace-text calls to reduce
+            // the chance of mismatch.
+            if (!(--mIMEActiveCompositionCount) && mIMEDelaySynchronizeReply) {
+                FlushIMEChanges();
+            }
+
             // Hardware keyboard support requires each string rect.
             if (mIMEMonitorCursor) {
                 UpdateCompositionRects();
             }
             break;
         }
 
         default:
--- a/widget/android/GeckoEditableSupport.h
+++ b/widget/android/GeckoEditableSupport.h
@@ -93,16 +93,17 @@ class GeckoEditableSupport final
     InputContext mInputContext;
     AutoTArray<UniquePtr<mozilla::WidgetEvent>, 4> mIMEKeyEvents;
     AutoTArray<IMETextChange, 4> mIMETextChanges;
     RefPtr<TextRangeArray> mIMERanges;
     int32_t mIMEMaskEventsCount; // Mask events when > 0.
     int32_t mIMEFocusCount; // We are focused when > 0.
     bool mIMEDelaySynchronizeReply; // We reply asynchronously when true.
     int32_t mIMEActiveSynchronizeCount; // The number of replies being delayed.
+    int32_t mIMEActiveCompositionCount; // The number of compositions expected.
     bool mIMESelectionChanged;
     bool mIMETextChangedDuringFlush;
     bool mIMEMonitorCursor;
 
     static bool sDispatchKeyEventsInCompositionForAnyApps;
 
     void ObservePrefs();