Backed out 2 changesets (bug 1620778) for causing test_autocomplete_mac_caret.xhtml failures
authorCiure Andrei <aciure@mozilla.com>
Sun, 15 Mar 2020 16:04:36 +0200
changeset 518821 0f2f823b599293166b82ceecd15b452de8b48382
parent 518820 00683827be1afd7193afc84f64f8b45523c3a2ae
child 518822 60d63a608908a41bd5afe59007e73484f6729894
push id37217
push userccoroiu@mozilla.com
push dateSun, 15 Mar 2020 21:37:59 +0000
treeherdermozilla-central@f9fc9427476e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1620778
milestone76.0a1
backs out66f97d1cf94abf84e2fafddfbf440d4dfd8fba35
eda75d901f4c6bbd902ec4a887865a3382287704
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
Backed out 2 changesets (bug 1620778) for causing test_autocomplete_mac_caret.xhtml failures Backed out changeset 66f97d1cf94a (bug 1620778) Backed out changeset eda75d901f4c (bug 1620778)
dom/html/HTMLInputElement.cpp
dom/html/HTMLInputElement.h
dom/html/TextControlState.cpp
editor/libeditor/tests/mochitest.ini
editor/libeditor/tests/test_bug1620778.html
toolkit/components/autocomplete/nsAutoCompleteController.cpp
toolkit/content/tests/chrome/test_autocomplete_mac_caret.xhtml
--- a/dom/html/HTMLInputElement.cpp
+++ b/dom/html/HTMLInputElement.cpp
@@ -3574,45 +3574,20 @@ nsresult HTMLInputElement::MaybeInitPick
   return NS_OK;
 }
 
 /**
  * Return true if the input event should be ignored because of its modifiers.
  * Control is treated specially, since sometimes we ignore it, and sometimes
  * we don't (for webcompat reasons).
  */
-static bool IgnoreInputEventWithModifier(const WidgetInputEvent& aEvent,
+static bool IgnoreInputEventWithModifier(WidgetInputEvent* aEvent,
                                          bool ignoreControl) {
-  return (ignoreControl && aEvent.IsControl()) || aEvent.IsAltGraph() ||
-         aEvent.IsFn() || aEvent.IsOS();
-}
-
-bool HTMLInputElement::StepsInputValue(const WidgetKeyboardEvent& aEvent) const {
-  if (mType != NS_FORM_INPUT_NUMBER) {
-    return false;
-  }
-  if (aEvent.mMessage != eKeyPress) {
-    return false;
-  }
-  if (!aEvent.IsTrusted()) {
-    return false;
-  }
-  if (aEvent.mKeyCode != NS_VK_UP && aEvent.mKeyCode != NS_VK_DOWN) {
-    return false;
-  }
-  if (IgnoreInputEventWithModifier(aEvent, false)) {
-    return false;
-  }
-  if (aEvent.DefaultPrevented()) {
-    return false;
-  }
-  if (!IsMutable()) {
-    return false;
-  }
-  return true;
+  return (ignoreControl && aEvent->IsControl()) || aEvent->IsAltGraph() ||
+         aEvent->IsFn() || aEvent->IsOS();
 }
 
 nsresult HTMLInputElement::PostHandleEvent(EventChainPostVisitor& aVisitor) {
   if (aVisitor.mEvent->mMessage == eFocus ||
       aVisitor.mEvent->mMessage == eBlur) {
     if (aVisitor.mEvent->mMessage == eBlur) {
       if (mIsDraggingRange) {
         FinishRangeThumbDrag();
@@ -3729,20 +3704,36 @@ nsresult HTMLInputElement::PostHandleEve
         }
       }
 #endif
     }
   }
 
   if (NS_SUCCEEDED(rv)) {
     WidgetKeyboardEvent* keyEvent = aVisitor.mEvent->AsKeyboardEvent();
-    if (keyEvent && StepsInputValue(*keyEvent)) {
-      StepNumberControlForUserEvent(keyEvent->mKeyCode == NS_VK_UP ? 1 : -1);
-      FireChangeEventIfNeeded();
-      aVisitor.mEventStatus = nsEventStatus_eConsumeNoDefault;
+    if (mType == NS_FORM_INPUT_NUMBER && keyEvent &&
+        keyEvent->mMessage == eKeyPress && aVisitor.mEvent->IsTrusted() &&
+        (keyEvent->mKeyCode == NS_VK_UP || keyEvent->mKeyCode == NS_VK_DOWN) &&
+        !IgnoreInputEventWithModifier(keyEvent, false)) {
+      // We handle the up/down arrow keys specially for <input type=number>.
+      // On some platforms the editor for the nested text control will
+      // process these keys to send the cursor to the start/end of the text
+      // control and as a result aVisitor.mEventStatus will already have been
+      // set to nsEventStatus_eConsumeNoDefault. However, we know that
+      // whenever the up/down arrow keys cause the value of the number
+      // control to change the string in the text control will change, and
+      // the cursor will be moved to the end of the text control, overwriting
+      // the editor's handling of up/down keypress events. For that reason we
+      // just ignore aVisitor.mEventStatus here and go ahead and handle the
+      // event to increase/decrease the value of the number control.
+      if (!aVisitor.mEvent->DefaultPreventedByContent() && IsMutable()) {
+        StepNumberControlForUserEvent(keyEvent->mKeyCode == NS_VK_UP ? 1 : -1);
+        FireChangeEventIfNeeded();
+        aVisitor.mEventStatus = nsEventStatus_eConsumeNoDefault;
+      }
     } else if (nsEventStatus_eIgnore == aVisitor.mEventStatus) {
       switch (aVisitor.mEvent->mMessage) {
         case eFocus: {
           // see if we should select the contents of the textbox. This happens
           // for text and password fields when the field was focused by the
           // keyboard or a navigation, the platform allows it, and it wasn't
           // just because we raised a window.
           nsIFocusManager* fm = nsFocusManager::GetFocusManager();
@@ -3945,17 +3936,17 @@ nsresult HTMLInputElement::PostHandleEve
                 aVisitor.mDOMEvent->StopPropagation();
               } else {
                 rv = NS_ERROR_FAILURE;
               }
             }
           }
           if (mType == NS_FORM_INPUT_NUMBER && aVisitor.mEvent->IsTrusted()) {
             if (mouseEvent->mButton == MouseButton::eLeft &&
-                !IgnoreInputEventWithModifier(*mouseEvent, false)) {
+                !IgnoreInputEventWithModifier(mouseEvent, false)) {
               nsNumberControlFrame* numberControlFrame =
                   do_QueryFrame(GetPrimaryFrame());
               if (numberControlFrame) {
                 if (aVisitor.mEvent->mMessage == eMouseDown && IsMutable()) {
                   switch (numberControlFrame->GetSpinButtonForPointerEvent(
                       aVisitor.mEvent->AsMouseEvent())) {
                     case nsNumberControlFrame::eSpinButtonUp:
                       StepNumberControlForUserEvent(1);
@@ -4092,17 +4083,17 @@ void HTMLInputElement::PostHandleEventFo
     case eTouchStart: {
       if (mIsDraggingRange) {
         break;
       }
       if (PresShell::GetCapturingContent()) {
         break;  // don't start drag if someone else is already capturing
       }
       WidgetInputEvent* inputEvent = aVisitor.mEvent->AsInputEvent();
-      if (IgnoreInputEventWithModifier(*inputEvent, true)) {
+      if (IgnoreInputEventWithModifier(inputEvent, true)) {
         break;  // ignore
       }
       if (aVisitor.mEvent->mMessage == eMouseDown) {
         if (aVisitor.mEvent->AsMouseEvent()->mButtons ==
             MouseButtonsFlag::eLeftFlag) {
           StartRangeThumbDrag(inputEvent);
         } else if (mIsDraggingRange) {
           CancelRangeThumbDrag();
--- a/dom/html/HTMLInputElement.h
+++ b/dom/html/HTMLInputElement.h
@@ -662,20 +662,16 @@ class HTMLInputElement final : public Te
   /**
    * Returns the current step value.
    * Returns kStepAny if the current step is "any" string.
    *
    * @return the current step value.
    */
   Decimal GetStep() const;
 
-  // Returns whether the given keyboard event steps up or down the value of an
-  // <input> element.
-  bool StepsInputValue(const WidgetKeyboardEvent&) const;
-
   already_AddRefed<nsINodeList> GetLabels();
 
   void Select();
 
   Nullable<uint32_t> GetSelectionStart(ErrorResult& aRv);
   MOZ_CAN_RUN_SCRIPT void SetSelectionStart(const Nullable<uint32_t>& aValue,
                                             ErrorResult& aRv);
 
--- a/dom/html/TextControlState.cpp
+++ b/dom/html/TextControlState.cpp
@@ -944,25 +944,16 @@ TextInputListener::HandleEvent(Event* aE
   }
 
   WidgetKeyboardEvent* widgetKeyEvent =
       aEvent->WidgetEventPtr()->AsKeyboardEvent();
   if (!widgetKeyEvent) {
     return NS_ERROR_UNEXPECTED;
   }
 
-  {
-    auto* input = HTMLInputElement::FromNode(mTxtCtrlElement);
-    if (input && input->StepsInputValue(*widgetKeyEvent)) {
-      // As an special case, don't handle key events that would step the value
-      // of our <input type=number>.
-      return NS_OK;
-    }
-  }
-
   KeyEventHandler* keyHandlers = ShortcutKeys::GetHandlers(
       mTxtCtrlElement->IsTextArea() ? HandlerType::eTextArea
                                     : HandlerType::eInput);
 
   RefPtr<nsAtom> eventTypeAtom =
       ShortcutKeys::ConvertEventToDOMEventType(widgetKeyEvent);
   for (KeyEventHandler* handler = keyHandlers; handler;
        handler = handler->GetNextHandler()) {
--- a/editor/libeditor/tests/mochitest.ini
+++ b/editor/libeditor/tests/mochitest.ini
@@ -222,17 +222,16 @@ skip-if = headless
 [test_bug1497480.html]
 skip-if = toolkit == 'android'
 [test_bug1543312.html]
 [test_bug1568996.html]
 [test_bug1574596.html]
 skip-if = os == "android" #Bug 1575739
 [test_bug1581337.html]
 [test_bug1619852.html]
-[test_bug1620778.html]
 [test_abs_positioner_appearance.html]
 [test_abs_positioner_positioning_elements.html]
 skip-if = os == 'android' # Bug 1525959
 [test_CF_HTML_clipboard.html]
 skip-if = os != 'mac' # bug 574005
 [test_composition_event_created_in_chrome.html]
 [test_contenteditable_focus.html]
 [test_cut_copy_delete_command_enabled.html]
deleted file mode 100644
--- a/editor/libeditor/tests/test_bug1620778.html
+++ /dev/null
@@ -1,27 +0,0 @@
-<!DOCTYPE html>
-<title>Test for Bug 1620778</title>
-<script src="/tests/SimpleTest/SimpleTest.js"></script>
-<script src="/tests/SimpleTest/EventUtils.js"></script>
-<link rel="stylesheet" href="/tests/SimpleTest/test.css">
-<input id=a value=abcd autocomplete=off>
-<input id=a value=abcd>
-<script>
-SimpleTest.waitForExplicitFinish();
-SimpleTest.waitForFocus(() => {
-  let expectedPosition = null;
-  for (let input of document.querySelectorAll("input")) {
-    input.focus();
-    input.selectionStart = 0;
-    synthesizeKey("KEY_ArrowRight");
-    synthesizeKey("KEY_ArrowRight");
-    synthesizeKey("KEY_ArrowDown");
-    if (expectedPosition === null)
-      expectedPosition = input.selectionStart;
-    isnot(input.selectionStart, 0);
-    is(input.selectionStart, expectedPosition, "autocomplete shouldn't make a difference on inputs that have no completion results of any kind");
-  }
-  SimpleTest.finish();
-});
-</script>
-</body>
-</html>
--- a/toolkit/components/autocomplete/nsAutoCompleteController.cpp
+++ b/toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ -415,22 +415,23 @@ nsAutoCompleteController::HandleKeyNavig
   bool disabled;
   input->GetDisableAutoComplete(&disabled);
   NS_ENSURE_TRUE(!disabled, NS_OK);
 
   if (aKey == dom::KeyboardEvent_Binding::DOM_VK_UP ||
       aKey == dom::KeyboardEvent_Binding::DOM_VK_DOWN ||
       aKey == dom::KeyboardEvent_Binding::DOM_VK_PAGE_UP ||
       aKey == dom::KeyboardEvent_Binding::DOM_VK_PAGE_DOWN) {
+    // Prevent the input from handling up/down events, as it may move
+    // the cursor to home/end on some systems
+    *_retval = true;
+
     bool isOpen = false;
     input->GetPopupOpen(&isOpen);
     if (isOpen) {
-      // Prevent the input from handling up/down events, as it may move
-      // the cursor to home/end on some systems
-      *_retval = true;
       bool reverse = aKey == dom::KeyboardEvent_Binding::DOM_VK_UP ||
                              aKey == dom::KeyboardEvent_Binding::DOM_VK_PAGE_UP
                          ? true
                          : false;
       bool page = aKey == dom::KeyboardEvent_Binding::DOM_VK_PAGE_UP ||
                           aKey == dom::KeyboardEvent_Binding::DOM_VK_PAGE_DOWN
                       ? true
                       : false;
@@ -482,81 +483,69 @@ nsAutoCompleteController::HandleKeyNavig
           SetValueOfInputTo(mSearchString,
                             nsIAutoCompleteInput::TEXTVALUE_REASON_REVERT);
           input->SelectTextRange(mSearchString.Length(),
                                  mSearchString.Length());
           mCompletedSelectionIndex = -1;
         }
       }
     } else {
-      // 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 shortcuts
-      // for up and down move to the beginning and end of the field otherwise.
-      if (aKey == dom::KeyboardEvent_Binding::DOM_VK_UP ||
-          aKey == dom::KeyboardEvent_Binding::DOM_VK_DOWN) {
-        const bool isUp = aKey == dom::KeyboardEvent_Binding::DOM_VK_UP;
-
-        int32_t start, end;
+#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
+      // shortcuts for up and down move to the beginning and end of the field
+      // otherwise.
+      int32_t start, end;
+      if (aKey == dom::KeyboardEvent_Binding::DOM_VK_UP) {
+        input->GetSelectionStart(&start);
+        input->GetSelectionEnd(&end);
+        if (start > 0 || start != end) *_retval = false;
+      } else if (aKey == dom::KeyboardEvent_Binding::DOM_VK_DOWN) {
+        nsAutoString text;
+        input->GetTextValue(text);
         input->GetSelectionStart(&start);
         input->GetSelectionEnd(&end);
+        if (start != end || end < (int32_t)text.Length()) *_retval = false;
+      }
+#endif
+      if (*_retval) {
+        nsAutoString oldSearchString;
+        uint16_t oldResult = 0;
 
-        if (isUp) {
-          if (start > 0 || start != end) {
-            return NS_OK;
+        // Open the popup if there has been a previous non-errored search, or
+        // else kick off a new search
+        if (!mResults.IsEmpty() &&
+            NS_SUCCEEDED(mResults[0]->GetSearchResult(&oldResult)) &&
+            oldResult != nsIAutoCompleteResult::RESULT_FAILURE &&
+            NS_SUCCEEDED(mResults[0]->GetSearchString(oldSearchString)) &&
+            oldSearchString.Equals(mSearchString,
+                                   nsCaseInsensitiveStringComparator())) {
+          if (mMatchCount) {
+            OpenPopup();
           }
         } else {
-          nsAutoString text;
-          input->GetTextValue(text);
-          if (start != end || end < (int32_t)text.Length()) {
+          // Stop all searches in case they are async.
+          StopSearch();
+
+          if (!mInput) {
+            // 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 #395344 for more details
             return NS_OK;
           }
-        }
-      }
-
-      nsAutoString oldSearchString;
-      uint16_t oldResult = 0;
-
-      // Open the popup if there has been a previous non-errored search, or
-      // else kick off a new search
-      if (!mResults.IsEmpty() &&
-          NS_SUCCEEDED(mResults[0]->GetSearchResult(&oldResult)) &&
-          oldResult != nsIAutoCompleteResult::RESULT_FAILURE &&
-          NS_SUCCEEDED(mResults[0]->GetSearchString(oldSearchString)) &&
-          oldSearchString.Equals(mSearchString,
-                                 nsCaseInsensitiveStringComparator())) {
-        if (mMatchCount) {
-          OpenPopup();
-        }
-      } else {
-        // Stop all searches in case they are async.
-        StopSearch();
 
-        if (!mInput) {
-          // 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 #395344 for more details
-          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);
+          SetSearchStringInternal(value);
 
-        // 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);
-        SetSearchStringInternal(value);
-
-        StartSearches();
-      }
-
-      bool isOpen = false;
-      input->GetPopupOpen(&isOpen);
-      if (isOpen) {
-        // Prevent the default action if we opened the popup in any of the code
-        // paths above.
-        *_retval = true;
+          StartSearches();
+        }
       }
     }
   } else if (aKey == dom::KeyboardEvent_Binding::DOM_VK_LEFT ||
              aKey == dom::KeyboardEvent_Binding::DOM_VK_RIGHT
 #ifndef XP_MACOSX
              || aKey == dom::KeyboardEvent_Binding::DOM_VK_HOME
 #endif
   ) {
@@ -992,19 +981,17 @@ nsresult nsAutoCompleteController::Start
     }
   }
 
   return NS_OK;
 }
 
 void nsAutoCompleteController::AfterSearches() {
   mResultCache.Clear();
-  if (mSearchesFailed == mSearches.Length()) {
-    PostSearchCleanup();
-  }
+  if (mSearchesFailed == mSearches.Length()) PostSearchCleanup();
 }
 
 NS_IMETHODIMP
 nsAutoCompleteController::StopSearch() {
   // Stop the timer if there is one
   ClearSearchTimer();
 
   // Stop any ongoing asynchronous searches
--- a/toolkit/content/tests/chrome/test_autocomplete_mac_caret.xhtml
+++ b/toolkit/content/tests/chrome/test_autocomplete_mac_caret.xhtml
@@ -17,18 +17,18 @@
 
 SimpleTest.waitForExplicitFinish();
 
 function keyCaretTest()
 {
   var autocomplete = $("autocomplete");
 
   autocomplete.focus();
-  checkKeyCaretTest("KEY_ArrowUp", 0, 0, true, "no value up");
-  checkKeyCaretTest("KEY_ArrowDown", 0, 0, true, "no value down");
+  checkKeyCaretTest("KEY_ArrowUp", 0, 0, false, "no value up");
+  checkKeyCaretTest("KEY_ArrowDown", 0, 0, false, "no value down");
 
   autocomplete.value = "Sample";
 
   autocomplete.selectionStart = 3;
   autocomplete.selectionEnd = 3;
   checkKeyCaretTest("KEY_ArrowUp", 0, 0, true, "value up with caret in middle");
   checkKeyCaretTest("KEY_ArrowUp", 0, 0, false, "value up with caret in middle again");