Bug 1407085 - nsAutoCompleteController shouldn't restore original value after somebody changes the input value even when Escape key is pressed r?mak draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Tue, 19 Dec 2017 16:46:20 +0900
changeset 717517 e04efa22d7bdf67034841f97d7816ed295dc36e0
parent 717516 8eb1fcc3bdcc2a15829c206ef05ec15203f939e1
child 745289 ebaf46249a252a56ce6446ab239d82ce7340f932
push id94714
push usermasayuki@d-toybox.com
push dateTue, 09 Jan 2018 07:21:57 +0000
reviewersmak
bugs1407085
milestone59.0a1
Bug 1407085 - nsAutoCompleteController shouldn't restore original value after somebody changes the input value even when Escape key is pressed r?mak When Escape key is pressed, nsAutoCompleteController needs to restore last string which was default value of the input or typed by the user. However, somebody may change the value, e.g., an event listener which handles Escape key. In this case, nsAutoCompleteController shouldn't restore the last string. Unfortunately, when JS sets input value, DOM "input" event won't be fired. Therefore, nsAutoCompleteController doesn't have a chance to modify mSearchString in this case. Therefore, nsAutoCompleteController needs to store expected input string for checking if somebody modified the input value. For solving this issue, this patch adds a new member, mSetValue which is modified when the input value is modified by nsAutoCompleteController itself or mSearchString is modified. Even with this patch, if user temporarily selects an item of the popup and JS sets same value as the selected item from JS, nsAutoCompleteController restores the input value with mSearchString. However, this must be rare case and I don't have idea to fix this issue with simple patches. MozReview-Commit-ID: lig8c7xvD7
toolkit/components/autocomplete/nsAutoCompleteController.cpp
toolkit/components/autocomplete/nsAutoCompleteController.h
toolkit/content/tests/mochitest/mochitest.ini
toolkit/content/tests/mochitest/test_bug1407085.html
--- a/toolkit/components/autocomplete/nsAutoCompleteController.cpp
+++ b/toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ -21,30 +21,16 @@
 #include "mozilla/Services.h"
 #include "mozilla/ModuleUtils.h"
 #include "mozilla/Unused.h"
 
 static const char *kAutoCompleteSearchCID = "@mozilla.org/autocomplete/search;1?name=";
 
 using namespace mozilla;
 
-namespace {
-
-void
-SetTextValue(nsIAutoCompleteInput* aInput,
-             const nsString& aValue,
-             uint16_t aReason) {
-  nsresult rv = aInput->SetTextValueWithReason(aValue, aReason);
-  if (NS_FAILED(rv)) {
-    aInput->SetTextValue(aValue);
-  }
-}
-
-} // anon namespace
-
 NS_IMPL_CYCLE_COLLECTION_CLASS(nsAutoCompleteController)
 
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsAutoCompleteController)
   tmp->SetInput(nullptr);
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsAutoCompleteController)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mInput)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSearches)
@@ -77,16 +63,28 @@ nsAutoCompleteController::nsAutoComplete
 {
 }
 
 nsAutoCompleteController::~nsAutoCompleteController()
 {
   SetInput(nullptr);
 }
 
+void
+nsAutoCompleteController::SetValueOfInputTo(const nsString& aValue,
+                                            uint16_t aReason)
+{
+  mSetValue = aValue;
+  nsCOMPtr<nsIAutoCompleteInput> input(mInput);
+  nsresult rv = input->SetTextValueWithReason(aValue, aReason);
+  if (NS_FAILED(rv)) {
+    input->SetTextValue(aValue);
+  }
+}
+
 ////////////////////////////////////////////////////////////////////////
 //// nsIAutoCompleteController
 
 NS_IMETHODIMP
 nsAutoCompleteController::GetSearchStatus(uint16_t *aSearchStatus)
 {
   *aSearchStatus = mSearchStatus;
   return NS_OK;
@@ -144,17 +142,19 @@ nsAutoCompleteController::SetInput(nsIAu
 
   // Nothing more to do if the input was just being set to null.
   if (!mInput) {
     return NS_OK;
   }
   nsCOMPtr<nsIAutoCompleteInput> input(mInput);
 
   // Reset the current search string.
-  input->GetTextValue(mSearchString);
+  nsAutoString value;
+  input->GetTextValue(value);
+  SetSearchStringInternal(value);
 
   // Clear out this reference in case the new input's popup has no tree
   mTree = nullptr;
 
   // Initialize our list of search objects
   uint32_t searchCount;
   input->GetSearchCount(&searchCount);
   mResults.SetCapacity(searchCount);
@@ -203,34 +203,34 @@ nsAutoCompleteController::ResetInternalS
 {
   // Clear out the current search context
   if (mInput) {
     nsAutoString value;
     mInput->GetTextValue(value);
     // Stop all searches in case they are async.
     Unused << StopSearch();
     Unused << ClearResults();
-    mSearchString = value;
+    SetSearchStringInternal(value);
   }
 
   mPlaceholderCompletionString.Truncate();
   mDefaultIndexCompleted = false;
   mProhibitAutoFill = false;
   mSearchStatus = nsIAutoCompleteController::STATUS_NONE;
   mRowCount = 0;
   mDelayedRowCountDelta = 0;
   mCompletedSelectionIndex = -1;
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsAutoCompleteController::StartSearch(const nsAString &aSearchString)
 {
-  mSearchString = aSearchString;
+  SetSearchStringInternal(aSearchString);
   StartSearches();
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsAutoCompleteController::HandleText(bool *_retval)
 {
   *_retval = false;
@@ -316,17 +316,17 @@ nsAutoCompleteController::HandleText(boo
       ClearResults();
     }
     mProhibitAutoFill = true;
     mPlaceholderCompletionString.Truncate();
   } else {
     mProhibitAutoFill = false;
   }
 
-  mSearchString = newValue;
+  SetSearchStringInternal(newValue);
 
   // Don't search if the value is empty
   if (newValue.Length() == 0) {
     // If autocomplete popup was closed by compositionstart event handler,
     // we should reopen it forcibly even if the value is empty.
     if (popupClosedByCompositionStart && handlingCompositionCommit) {
       bool cancel;
       HandleKeyNavigation(nsIDOMKeyEvent::DOM_VK_DOWN, &cancel);
@@ -521,31 +521,31 @@ nsAutoCompleteController::HandleKeyNavig
             // If the result is the previously autofilled string, then restore
             // the search string and selection that existed when the result was
             // autofilled.  Else, fill the result and move the caret to the end.
             int32_t start;
             if (value.Equals(mPlaceholderCompletionString,
                              nsCaseInsensitiveStringComparator())) {
               start = mSearchString.Length();
               value = mPlaceholderCompletionString;
-              SetTextValue(input, value,
-                           nsIAutoCompleteInput::TEXTVALUE_REASON_COMPLETEDEFAULT);
+              SetValueOfInputTo(
+                value, nsIAutoCompleteInput::TEXTVALUE_REASON_COMPLETEDEFAULT);
             } else {
               start = value.Length();
-              SetTextValue(input, value,
-                           nsIAutoCompleteInput::TEXTVALUE_REASON_COMPLETESELECTED);
+              SetValueOfInputTo(
+                value, nsIAutoCompleteInput::TEXTVALUE_REASON_COMPLETESELECTED);
             }
 
             input->SelectTextRange(start, value.Length());
           }
           mCompletedSelectionIndex = selectedIndex;
         } else {
           // Nothing is selected, so fill in the last typed value
-          SetTextValue(input, mSearchString,
-                       nsIAutoCompleteInput::TEXTVALUE_REASON_REVERT);
+          SetValueOfInputTo(
+            mSearchString, nsIAutoCompleteInput::TEXTVALUE_REASON_REVERT);
           input->SelectTextRange(mSearchString.Length(), mSearchString.Length());
           mCompletedSelectionIndex = -1;
         }
       }
     } else {
 #ifdef XP_MACOSX
       // on Mac, only show the popup if the caret is at the start or end of
       // the input and there is no selection, so that the default defined key
@@ -587,17 +587,17 @@ nsAutoCompleteController::HandleKeyNavig
             return NS_OK;
           }
 
           // Some script may have changed the value of the text field since our
           // last keypress or after our focus handler and we don't want to search
           // for a stale string.
           nsAutoString value;
           input->GetTextValue(value);
-          mSearchString = value;
+          SetSearchStringInternal(value);
 
           StartSearches();
         }
       }
     }
   } else if (   aKey == nsIDOMKeyEvent::DOM_VK_LEFT
              || aKey == nsIDOMKeyEvent::DOM_VK_RIGHT
 #ifndef XP_MACOSX
@@ -633,18 +633,18 @@ nsAutoCompleteController::HandleKeyNavig
       int32_t selectedIndex;
       popup->GetSelectedIndex(&selectedIndex);
       bool shouldComplete;
       input->GetCompleteDefaultIndex(&shouldComplete);
       if (selectedIndex >= 0) {
         // The pop-up is open and has a selection, take its value
         nsAutoString value;
         if (NS_SUCCEEDED(GetResultValueAt(selectedIndex, false, value))) {
-          SetTextValue(input, value,
-                       nsIAutoCompleteInput::TEXTVALUE_REASON_COMPLETESELECTED);
+          SetValueOfInputTo(
+            value, nsIAutoCompleteInput::TEXTVALUE_REASON_COMPLETESELECTED);
           input->SelectTextRange(value.Length(), value.Length());
         }
       }
       else if (shouldComplete) {
         // We usually try to preserve the casing of what user has typed, but
         // if he wants to autocomplete, we will replace the value with the
         // actual autocomplete result. Note that the autocomplete input can also
         // be showing e.g. "bar >> foo bar" if the search matched "bar", a
@@ -659,33 +659,33 @@ nsAutoCompleteController::HandleKeyNavig
           int32_t pos = inputValue.Find(" >> ");
           if (pos > 0) {
             inputValue.Right(suggestedValue, inputValue.Length() - pos - 4);
           } else {
             suggestedValue = inputValue;
           }
 
           if (value.Equals(suggestedValue, nsCaseInsensitiveStringComparator())) {
-            SetTextValue(input, value,
-                         nsIAutoCompleteInput::TEXTVALUE_REASON_COMPLETEDEFAULT);
+            SetValueOfInputTo(
+              value, nsIAutoCompleteInput::TEXTVALUE_REASON_COMPLETEDEFAULT);
             input->SelectTextRange(value.Length(), value.Length());
           }
         }
       }
 
       // Close the pop-up even if nothing was selected
       ClearSearchTimer();
       ClosePopup();
     }
     // Update last-searched string to the current input, since the input may
     // have changed.  Without this, subsequent backspaces look like text
     // additions, not text deletions.
     nsAutoString value;
     input->GetTextValue(value);
-    mSearchString = value;
+    SetSearchStringInternal(value);
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsAutoCompleteController::HandleDelete(bool *_retval)
 {
@@ -845,17 +845,17 @@ nsAutoCompleteController::GetFinalComple
   NS_ENSURE_SUCCESS(rv, rv);
 
   return result->GetFinalCompleteValueAt(rowIndex, _retval);
 }
 
 NS_IMETHODIMP
 nsAutoCompleteController::SetSearchString(const nsAString &aSearchString)
 {
-  mSearchString = aSearchString;
+  SetSearchStringInternal(aSearchString);
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsAutoCompleteController::GetSearchString(nsAString &aSearchString)
 {
   aSearchString = mSearchString;
   return NS_OK;
@@ -1569,19 +1569,19 @@ nsAutoCompleteController::EnterMatch(boo
     }
   }
 
   nsCOMPtr<nsIObserverService> obsSvc = services::GetObserverService();
   NS_ENSURE_STATE(obsSvc);
   obsSvc->NotifyObservers(input, "autocomplete-will-enter-text", nullptr);
 
   if (!value.IsEmpty()) {
-    SetTextValue(input, value, nsIAutoCompleteInput::TEXTVALUE_REASON_ENTERMATCH);
+    SetValueOfInputTo(value, nsIAutoCompleteInput::TEXTVALUE_REASON_ENTERMATCH);
     input->SelectTextRange(value.Length(), value.Length());
-    mSearchString = value;
+    SetSearchStringInternal(value);
   }
 
   obsSvc->NotifyObservers(input, "autocomplete-did-enter-text", nullptr);
   ClosePopup();
 
   bool cancel;
   input->OnTextEntered(aEvent, &cancel);
 
@@ -1592,33 +1592,41 @@ nsresult
 nsAutoCompleteController::RevertTextValue()
 {
   // StopSearch() can call PostSearchCleanup() which might result
   // in a blur event, which could null out mInput, so we need to check it
   // again.  See bug #408463 for more details
   if (!mInput)
     return NS_OK;
 
-  nsAutoString oldValue(mSearchString);
   nsCOMPtr<nsIAutoCompleteInput> input(mInput);
 
+  // If current input value is different from what we have set, it means
+  // somebody modified the value like JS of the web content.  In such case,
+  // we shouldn't overwrite it with the old value.
+  nsAutoString currentValue;
+  input->GetTextValue(currentValue);
+  if (currentValue != mSetValue) {
+    SetSearchStringInternal(currentValue);
+    return NS_OK;
+  }
+
   bool cancel = false;
   input->OnTextReverted(&cancel);
 
   if (!cancel) {
     nsCOMPtr<nsIObserverService> obsSvc = services::GetObserverService();
     NS_ENSURE_STATE(obsSvc);
     obsSvc->NotifyObservers(input, "autocomplete-will-revert-text", nullptr);
 
-    nsAutoString inputValue;
-    input->GetTextValue(inputValue);
     // Don't change the value if it is the same to prevent sending useless events.
     // NOTE: how can |RevertTextValue| be called with inputValue != oldValue?
-    if (!oldValue.Equals(inputValue)) {
-      SetTextValue(input, oldValue, nsIAutoCompleteInput::TEXTVALUE_REASON_REVERT);
+    if (mSearchString != currentValue) {
+      SetValueOfInputTo(
+        mSearchString, nsIAutoCompleteInput::TEXTVALUE_REASON_REVERT);
     }
 
     obsSvc->NotifyObservers(input, "autocomplete-did-revert-text", nullptr);
   }
 
   return NS_OK;
 }
 
@@ -1947,18 +1955,18 @@ nsAutoCompleteController::CompleteValue(
 
   if (aValue.IsEmpty() ||
       StringBeginsWith(aValue, mSearchString,
                        nsCaseInsensitiveStringComparator())) {
     // aValue is empty (we were asked to clear mInput), or mSearchString
     // matches the beginning of aValue.  In either case we can simply
     // autocomplete to aValue.
     mPlaceholderCompletionString = aValue;
-    SetTextValue(input, aValue,
-                 nsIAutoCompleteInput::TEXTVALUE_REASON_COMPLETEDEFAULT);
+    SetValueOfInputTo(
+      aValue, nsIAutoCompleteInput::TEXTVALUE_REASON_COMPLETEDEFAULT);
   } else {
     nsresult rv;
     nsCOMPtr<nsIIOService> ios = do_GetService(NS_IOSERVICE_CONTRACTID, &rv);
     NS_ENSURE_SUCCESS(rv, rv);
     nsAutoCString scheme;
     if (NS_SUCCEEDED(ios->ExtractScheme(NS_ConvertUTF16toUTF8(aValue), scheme))) {
       // Trying to autocomplete a URI from somewhere other than the beginning.
       // Only succeed if the missing portion is "http://"; otherwise do not
@@ -1970,26 +1978,28 @@ nsAutoCompleteController::CompleteValue(
           !scheme.LowerCaseEqualsLiteral("http") ||
           !Substring(aValue, findIndex, mSearchStringLength).Equals(
             mSearchString, nsCaseInsensitiveStringComparator())) {
         return NS_OK;
       }
 
       mPlaceholderCompletionString = mSearchString +
         Substring(aValue, mSearchStringLength + findIndex, endSelect);
-      SetTextValue(input, mPlaceholderCompletionString,
-                   nsIAutoCompleteInput::TEXTVALUE_REASON_COMPLETEDEFAULT);
+      SetValueOfInputTo(
+        mPlaceholderCompletionString,
+        nsIAutoCompleteInput::TEXTVALUE_REASON_COMPLETEDEFAULT);
 
       endSelect -= findIndex; // We're skipping this many characters of aValue.
     } else {
       // Autocompleting something other than a URI from the middle.
       // Use the format "searchstring >> full string" to indicate to the user
       // what we are going to replace their search string with.
-      SetTextValue(input, mSearchString + NS_LITERAL_STRING(" >> ") + aValue,
-                   nsIAutoCompleteInput::TEXTVALUE_REASON_COMPLETEDEFAULT);
+      SetValueOfInputTo(
+        mSearchString + NS_LITERAL_STRING(" >> ") + aValue,
+        nsIAutoCompleteInput::TEXTVALUE_REASON_COMPLETEDEFAULT);
 
       endSelect = mSearchString.Length() + 4 + aValue.Length();
 
       // Reset the last search completion.
       mPlaceholderCompletionString.Truncate();
     }
   }
 
--- a/toolkit/components/autocomplete/nsAutoCompleteController.h
+++ b/toolkit/components/autocomplete/nsAutoCompleteController.h
@@ -37,16 +37,31 @@ public:
   NS_DECL_NSITIMERCALLBACK
   NS_DECL_NSINAMED
 
   nsAutoCompleteController();
 
 protected:
   virtual ~nsAutoCompleteController();
 
+  /**
+   * SetValueOfInputTo() sets value of mInput to aValue and notifies the input
+   * of setting reason.
+   */
+  void SetValueOfInputTo(const nsString& aValue, uint16_t aReason);
+
+  /**
+   * SetSearchStringInternal() sets both mSearchString and mSetValue to
+   * aSearchString.
+   */
+  void SetSearchStringInternal(const nsAString& aSearchString)
+  {
+    mSearchString = mSetValue = aSearchString;
+  }
+
   nsresult OpenPopup();
   nsresult ClosePopup();
 
   nsresult StartSearch(uint16_t aSearchType);
 
   nsresult BeforeSearches();
   nsresult StartSearches();
   void AfterSearches();
@@ -133,18 +148,29 @@ protected:
   // search.  This is needed to allow the searches to reuse the previous result,
   // since otherwise the first search clears mResults.
   nsCOMArray<nsIAutoCompleteResult> mResultCache;
 
   nsCOMPtr<nsITimer> mTimer;
   nsCOMPtr<nsITreeSelection> mSelection;
   nsCOMPtr<nsITreeBoxObject> mTree;
 
+  // mSearchString stores value which is the original value of the input or
+  // typed by the user.  When user is choosing an item from the popup, this
+  // is NOT modified by the item because this is used for reverting the input
+  // value when user cancels choosing an item from the popup.
+  // This should be set through only SetSearchStringInternal().
   nsString mSearchString;
   nsString mPlaceholderCompletionString;
+  // mSetValue stores value which is expected in the input.  So, if input's
+  // value and mSetValue are different, it means somebody has changed the
+  // value like JS of the web content.
+  // This is set only by SetValueOfInputTo() or when modifying mSearchString
+  // through SetSearchStringInternal().
+  nsString mSetValue;
   bool mDefaultIndexCompleted;
   bool mPopupClosedByCompositionStart;
 
   // Whether autofill is allowed for the next search. May be retrieved by the
   // search through the "prohibit-autofill" searchParam.
   bool mProhibitAutoFill;
 
   // Indicates whether the user cleared the autofilled part, returning to the
--- a/toolkit/content/tests/mochitest/mochitest.ini
+++ b/toolkit/content/tests/mochitest/mochitest.ini
@@ -1,11 +1,12 @@
+[test_bug1407085.html]
+
 [test_autocomplete_change_after_focus.html]
 skip-if = toolkit == "android"
 [test_mousecapture.xhtml]
 support-files =
   file_mousecapture.html
   file_mousecapture2.html
   file_mousecapture3.html
   file_mousecapture4.html
   file_mousecapture5.html
 skip-if = toolkit == "android" || webrender # bug 1424752 for webrender
-
new file mode 100644
--- /dev/null
+++ b/toolkit/content/tests/mochitest/test_bug1407085.html
@@ -0,0 +1,38 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <title>Test for Bug 1407085</title>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+<p id="display"></p>
+
+<div id="content">
+  <input id="input" value="original value">
+</div>
+
+<pre id="test">
+<script class="testbody" type="text/javascript">
+
+SimpleTest.waitForExplicitFinish();
+SimpleTest.waitForFocus(runTests);
+
+function runTests() {
+  let input = document.getElementById("input");
+  input.focus();
+  input.addEventListener("keydown", () => {
+    input.value = "new value";
+  }, { once: true });
+  synthesizeKey("KEY_Escape", { code: "Escape" });
+  is(input.value, "new value",
+     "New <input> value changed by an Escape key event listener shouldn't be " +
+     "overwritten by original value even if Escape key is pressed");
+  SimpleTest.finish();
+}
+
+</script>
+</pre>
+</body>
+</html>