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

--HG--
extra : moz-landing-system : lando

dom/html/HTMLInputElement.cpp

@@ -3576,10 +3576,35 @@ nsresult HTMLInputElement::MaybeInitPickers(EventChainPostVisitor& aVisitor) {
  * 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) {
@@ -3706,26 +3731,10 @@ nsresult HTMLInputElement::PostHandleEvent(EventChainPostVisitor& aVisitor) {
 
   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()) {
+    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: {
@@ -3938,7 +3947,7 @@ nsresult HTMLInputElement::PostHandleEvent(EventChainPostVisitor& aVisitor) {
           }
           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) {
@@ -4085,7 +4094,7 @@ void HTMLInputElement::PostHandleEventForRangeThumb(
         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) {

dom/html/HTMLInputElement.h

@@ -667,6 +667,10 @@ class HTMLInputElement final : public TextControlElement,
    */
   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();

dom/html/TextControlState.cpp

@@ -950,6 +950,15 @@ TextInputListener::HandleEvent(Event* aEvent) {
     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);

editor/libeditor/tests/mochitest.ini

@@ -227,6 +227,7 @@ skip-if = toolkit == 'android'
 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

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>

toolkit/components/autocomplete/nsAutoCompleteController.cpp

@@ -420,13 +420,12 @@ nsAutoCompleteController::HandleKeyNavigation(uint32_t aKey, bool* _retval) {
       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
@@ -488,25 +487,30 @@ nsAutoCompleteController::HandleKeyNavigation(uint32_t aKey, bool* _retval) {
         }
       }
     } 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.
+      // 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;
-      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) {
+
+        if (isUp) {
+          if (start > 0 || start != end) {
+            return NS_OK;
+          }
+        } else {
           nsAutoString text;
           input->GetTextValue(text);
-        input->GetSelectionStart(&start);
-        input->GetSelectionEnd(&end);
-        if (start != end || end < (int32_t)text.Length()) *_retval = false;
+          if (start != end || end < (int32_t)text.Length()) {
+            return NS_OK;
           }
-#endif
-      if (*_retval) {
+        }
+      }
+
       nsAutoString oldSearchString;
       uint16_t oldResult = 0;
 
@@ -541,6 +545,13 @@ nsAutoCompleteController::HandleKeyNavigation(uint32_t aKey, bool* _retval) {
 
         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 ||
@@ -986,7 +997,9 @@ nsresult nsAutoCompleteController::StartSearch(uint16_t aSearchType) {
 
 void nsAutoCompleteController::AfterSearches() {
   mResultCache.Clear();
-  if (mSearchesFailed == mSearches.Length()) PostSearchCleanup();
+  if (mSearchesFailed == mSearches.Length()) {
+    PostSearchCleanup();
+  }
 }
 
 NS_IMETHODIMP