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
2020-03-15 12:11:09 +00:00 · 2020-03-15 12:11:09 +00:00 · 749ef068e9
--- a/dom/html/HTMLInputElement.cpp
+++ b/dom/html/HTMLInputElement.cpp
@ -3579,10 +3579,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) {
@ -3709,26 +3734,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()) {
-        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: {
@ -3941,7 +3950,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) {
@ -4088,7 +4097,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) {
--- a/dom/html/HTMLInputElement.h
+++ b/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();
--- a/dom/html/TextControlState.cpp
+++ b/dom/html/TextControlState.cpp
@ -949,6 +949,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);
--- a/editor/libeditor/tests/mochitest.ini
+++ b/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
--- a/editor/libeditor/tests/test_bug1620778.html
+++ 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
@ -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,60 +487,72 @@ 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.
-      int32_t start, end;
-      if (aKey == dom::KeyboardEvent_Binding::DOM_VK_UP) {
+      // 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 > 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;

-        // 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
+        if (isUp) {
+          if (start > 0 || start != end) {
+            return NS_OK;
+          }
+        } else {
+          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);
-
-          StartSearches();
        }
      }
+
+      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);
+
+        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
@ -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
--- a/toolkit/content/tests/chrome/chrome.ini
+++ b/toolkit/content/tests/chrome/chrome.ini
@ -194,7 +194,6 @@ skip-if = toolkit == "cocoa"
 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"
--- a/toolkit/content/tests/chrome/test_autocomplete_mac_caret.xhtml
+++ b/toolkit/content/tests/chrome/test_autocomplete_mac_caret.xhtml
@ -22,8 +22,8 @@ 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";