Bug 1620778 - Fix interaction of up/down keys with autocomplete and <input type=number>. r=masayuki,smaug
☠☠ backed out by 0f2f823b5992 ☠ ☠
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sun, 15 Mar 2020 12:11:09 +0000
changeset 518817 eda75d901f4c6bbd902ec4a887865a3382287704
parent 518816 429fb5cee0650c3e72e12c1865ca4b4cb1f80072
child 518818 bada9ae2d10cd0dc9ce8b14a729819aa18f01fbd
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)
reviewersmasayuki, smaug
bugs1620778
milestone76.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 1620778 - Fix interaction of up/down keys with autocomplete and <input type=number>. r=masayuki,smaug Differential Revision: https://phabricator.services.mozilla.com/D66011
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/chrome.ini
toolkit/content/tests/chrome/test_autocomplete_mac_caret.xhtml
--- a/dom/html/HTMLInputElement.cpp
+++ b/dom/html/HTMLInputElement.cpp
@@ -3574,20 +3574,45 @@ 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(WidgetInputEvent* aEvent,
+static bool IgnoreInputEventWithModifier(const WidgetInputEvent& aEvent,
                                          bool ignoreControl) {
-  return (ignoreControl && aEvent->IsControl()) || aEvent->IsAltGraph() ||
-         aEvent->IsFn() || aEvent->IsOS();
+  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;
 }
 
 nsresult HTMLInputElement::PostHandleEvent(EventChainPostVisitor& aVisitor) {
   if (aVisitor.mEvent->mMessage == eFocus ||
       aVisitor.mEvent->mMessage == eBlur) {
     if (aVisitor.mEvent->mMessage == eBlur) {
       if (mIsDraggingRange) {
         FinishRangeThumbDrag();
@@ -3704,36 +3729,20 @@ nsresult HTMLInputElement::PostHandleEve
         }
       }
 #endif
     }
   }
 
   if (NS_SUCCEEDED(rv)) {
     WidgetKeyboardEvent* keyEvent = aVisitor.mEvent->AsKeyboardEvent();
-    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;
-      }
+    if (keyEvent && StepsInputValue(*keyEvent)) {
+      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();
@@ -3936,17 +3945,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);
@@ -4083,17 +4092,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,16 +662,20 @@ 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,16 +944,25 @@ 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,16 +222,17 @@ 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]
new file mode 100644
--- /dev/null
+++ b/editor/libeditor/tests/test_bug1620778.html
@@ -0,0 +1,27 @@
+<!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,23 +415,22 @@ 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;
@@ -483,69 +482,81 @@ nsAutoCompleteController::HandleKeyNavig
           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
-      // 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);
+      // 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;
         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;
 
-        // 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();
+        if (isUp) {
+          if (start > 0 || start != end) {
+            return NS_OK;
           }
         } 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
+          nsAutoString text;
+          input->GetTextValue(text);
+          if (start != end || end < (int32_t)text.Length()) {
             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);
+      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();
 
-          StartSearches();
+        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);
+
+        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;
       }
     }
   } 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
   ) {
@@ -981,17 +992,19 @@ 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/chrome.ini
+++ b/toolkit/content/tests/chrome/chrome.ini
@@ -189,13 +189,12 @@ support-files = window_intrinsic_size.xh
 # textboxes and lists only, so skip this test on Mac
 [test_panel_focus.xhtml]
 support-files = window_panel_focus.xhtml
 skip-if = toolkit == "cocoa"
 [test_chromemargin.xhtml]
 support-files = window_chromemargin.xhtml
 skip-if = toolkit == "cocoa"
 [test_autocomplete_mac_caret.xhtml]
-skip-if = toolkit != "cocoa"
 [test_cursorsnap.xhtml]
 disabled =
 #skip-if = os != "win"
 support-files = window_cursorsnap_dialog.xhtml window_cursorsnap_wizard.xhtml
--- 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, false, "no value up");
-  checkKeyCaretTest("KEY_ArrowDown", 0, 0, false, "no value down");
+  checkKeyCaretTest("KEY_ArrowUp", 0, 0, true, "no value up");
+  checkKeyCaretTest("KEY_ArrowDown", 0, 0, true, "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");