Bug 1295719 - input[type=range,number] does not fire 'change' event for some key combinations. r=smaug
authorStone Shih <sshih@mozilla.com>
Fri, 19 Aug 2016 09:19:35 +0800
changeset 312704 66c195197831d99fd420fb56523a6d5fc7719d58
parent 312703 5d11d2b3af0b81f14fd919110a3c2cff31e5324f
child 312705 5830a5488348c2eb891774f33968d12c2ac8d276
push id30653
push userphilringnalda@gmail.com
push dateMon, 05 Sep 2016 20:06:00 +0000
treeherdermozilla-central@3076fad24896 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1295719
milestone51.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 1295719 - input[type=range,number] does not fire 'change' event for some key combinations. r=smaug
dom/html/HTMLInputElement.cpp
dom/html/HTMLInputElement.h
dom/html/test/forms/test_change_event.html
dom/html/test/mochitest.ini
dom/html/test/test_bug1261673.html
dom/html/test/test_bug1261674-1.html
dom/html/test/test_bug1261674-2.html
dom/html/test/test_bug1295719_event_sequence_for_arrow_keys.html
dom/html/test/test_bug1295719_event_sequence_for_number_keys.html
--- a/dom/html/HTMLInputElement.cpp
+++ b/dom/html/HTMLInputElement.cpp
@@ -3800,29 +3800,16 @@ HTMLInputElement::PreHandleEvent(EventCh
         // from the theme should be removed from us (the repainting of the
         // sub-area occupied by the anon text control is not enough to do
         // that).
         frame->InvalidateFrame();
       }
     }
   }
 
-  if (aVisitor.mEvent->mMessage == eKeyUp && aVisitor.mEvent->IsTrusted()) {
-    WidgetKeyboardEvent* keyEvent = aVisitor.mEvent->AsKeyboardEvent();
-    if (MayFireChangeOnKeyUp(keyEvent->mKeyCode) &&
-        !(keyEvent->IsShift() || keyEvent->IsControl() || keyEvent->IsAlt() ||
-          keyEvent->IsMeta() || keyEvent->IsAltGraph() || keyEvent->IsFn() ||
-          keyEvent->IsOS())) {
-      // The up/down arrow key events fire 'change' events when released
-      // so that at the end of a series of up/down arrow key repeat events
-      // the value is considered to be "commited" by the user.
-      FireChangeEventIfNeeded();
-    }
-  }
-
   nsresult rv = nsGenericHTMLFormElementWithState::PreHandleEvent(aVisitor);
 
   // We do this after calling the base class' PreHandleEvent so that
   // nsIContent::PreHandleEvent doesn't reset any change we make to mCanHandle.
   if (mType == NS_FORM_INPUT_NUMBER &&
       aVisitor.mEvent->IsTrusted()  &&
       aVisitor.mEvent->mOriginalTarget != this) {
     // <input type=number> has an anonymous <input type=text> descendant. If
@@ -4298,16 +4285,17 @@ HTMLInputElement::PostHandleEvent(EventC
       // 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
@@ -4475,16 +4463,17 @@ HTMLInputElement::PostHandleEvent(EventC
                   // requires us to jump more.
                   newValue = value + std::max(step, (maximum - minimum) / Decimal(10));
                   break;
                 case  NS_VK_PAGE_DOWN:
                   newValue = value - std::max(step, (maximum - minimum) / Decimal(10));
                   break;
               }
               SetValueOfRangeForUserEvent(newValue);
+              FireChangeEventIfNeeded();
               aVisitor.mEventStatus = nsEventStatus_eConsumeNoDefault;
             }
           }
 
         } break; // eKeyPress || eKeyUp
 
         case eMouseDown:
         case eMouseUp:
@@ -4552,29 +4541,31 @@ HTMLInputElement::PostHandleEvent(EventC
               aVisitor.mEvent->IsTrusted() && IsMutable() && wheelEvent &&
               wheelEvent->mDeltaY != 0 &&
               wheelEvent->mDeltaMode != nsIDOMWheelEvent::DOM_DELTA_PIXEL) {
             if (mType == NS_FORM_INPUT_NUMBER) {
               nsNumberControlFrame* numberControlFrame =
                 do_QueryFrame(GetPrimaryFrame());
               if (numberControlFrame && numberControlFrame->IsFocused()) {
                 StepNumberControlForUserEvent(wheelEvent->mDeltaY > 0 ? -1 : 1);
+                FireChangeEventIfNeeded();
                 aVisitor.mEvent->PreventDefault();
               }
             } else if (mType == NS_FORM_INPUT_RANGE &&
                        nsContentUtils::IsFocusedContent(this) &&
                        GetMinimum() < GetMaximum()) {
               Decimal value = GetValueAsDecimal();
               Decimal step = GetStep();
               if (step == kStepAny) {
                 step = GetDefaultStep();
               }
               MOZ_ASSERT(value.isFinite() && step.isFinite());
               SetValueOfRangeForUserEvent(wheelEvent->mDeltaY < 0 ?
                                           value + step : value - step);
+              FireChangeEventIfNeeded();
               aVisitor.mEvent->PreventDefault();
             }
           }
           break;
         }
 #endif
         default:
           break;
@@ -8339,24 +8330,12 @@ HTMLInputElement::UpdateEntries(const ns
 
 void
 HTMLInputElement::GetWebkitEntries(nsTArray<RefPtr<FileSystemEntry>>& aSequence)
 {
   Telemetry::Accumulate(Telemetry::BLINK_FILESYSTEM_USED, true);
   aSequence.AppendElements(mEntries);
 }
 
-bool HTMLInputElement::MayFireChangeOnKeyUp(uint32_t aKeyCode) const
-{
-  switch (mType) {
-    case NS_FORM_INPUT_NUMBER:
-      return aKeyCode == NS_VK_UP || aKeyCode == NS_VK_DOWN;
-    case NS_FORM_INPUT_RANGE:
-      return aKeyCode == NS_VK_UP || aKeyCode == NS_VK_DOWN ||
-             aKeyCode == NS_VK_LEFT || aKeyCode == NS_VK_RIGHT;
-  }
-  return false;
-}
-
 } // namespace dom
 } // namespace mozilla
 
 #undef NS_ORIGINAL_CHECKED_VALUE
--- a/dom/html/HTMLInputElement.h
+++ b/dom/html/HTMLInputElement.h
@@ -1510,22 +1510,16 @@ private:
   }
 
   static bool MayFireChangeOnBlur(uint8_t aType) {
     return IsSingleLineTextControl(false, aType) ||
            aType == NS_FORM_INPUT_RANGE ||
            aType == NS_FORM_INPUT_NUMBER;
   }
 
-  /**
-   * Returns true if the key code may induce the input's value changed to fire
-   * a "change" event accordingly.
-   */
-  bool MayFireChangeOnKeyUp(uint32_t aKeyCode) const;
-
   struct nsFilePickerFilter {
     nsFilePickerFilter()
       : mFilterMask(0) {}
 
     explicit nsFilePickerFilter(int32_t aFilterMask)
       : mFilterMask(aFilterMask) {}
 
     nsFilePickerFilter(const nsString& aTitle,
--- a/dom/html/test/forms/test_change_event.html
+++ b/dom/html/test/forms/test_change_event.html
@@ -210,34 +210,60 @@ https://bugzilla.mozilla.org/show_bug.cg
     // Special case type=range
     var range = document.getElementById("input_range");
     range.focus();
     synthesizeKey("a", {});
     range.blur();
     is(rangeChange, 0, "Change event shouldn't be dispatched on range input element for key changes that don't change its value");
     range.focus();
     synthesizeKey("VK_HOME", {});
-    is(rangeChange, 0, "Change event shouldn't be dispatched on range input element for key changes until it loses focus");
+    is(rangeChange, 1, "Change event should be dispatched on range input element for key changes");
     range.blur();
-    is(rangeChange, 1, "Change event should be dispatched on range input element on blur");
+    is(rangeChange, 1, "Change event shouldn't be dispatched on range input element on blur");
     range.focus();
     var bcr = range.getBoundingClientRect();
     var centerOfRangeX = bcr.width / 2;
     var centerOfRangeY = bcr.height / 2;
     synthesizeMouse(range, centerOfRangeX - 10, centerOfRangeY, { type: "mousedown" });
     is(rangeChange, 1, "Change event shouldn't be dispatched on range input element for mousedown");
     synthesizeMouse(range, centerOfRangeX - 5, centerOfRangeY, { type: "mousemove" });
     is(rangeChange, 1, "Change event shouldn't be dispatched on range input element during drag of thumb");
     synthesizeMouse(range, centerOfRangeX, centerOfRangeY, { type: "mouseup" });
     is(rangeChange, 2, "Change event should be dispatched on range input element at end of drag");
     range.blur();
     is(rangeChange, 2, "Change event shouldn't be dispatched on range input element when range loses focus after a drag");
     synthesizeMouse(range, centerOfRangeX - 10, centerOfRangeY, {});
     is(rangeChange, 3, "Change event should be dispatched on range input element for a click that gives the range focus");
 
+    if (isDesktop) { // up/down arrow keys not supported on android/b2g
+      synthesizeKey("VK_UP", {});
+      is(rangeChange, 4, "Change event should be dispatched on range input element for key changes that change its value (VK_UP)");
+      synthesizeKey("VK_DOWN", {});
+      is(rangeChange, 5, "Change event should be dispatched on range input element for key changes that change its value (VK_DOWN)");
+      synthesizeKey("VK_RIGHT", {});
+      is(rangeChange, 6, "Change event should be dispatched on range input element for key changes that change its value (VK_RIGHT)");
+      synthesizeKey("VK_LEFT", {});
+      is(rangeChange, 7, "Change event should be dispatched on range input element for key changes that change its value (VK_LEFT)");
+      synthesizeKey("VK_UP", {shiftKey: true});
+      is(rangeChange, 8, "Change event should be dispatched on range input element for key changes that change its value (Shift+VK_UP)");
+      synthesizeKey("VK_DOWN", {shiftKey: true});
+      is(rangeChange, 9, "Change event should be dispatched on range input element for key changes that change its value (Shift+VK_DOWN)");
+      synthesizeKey("VK_RIGHT", {shiftKey: true});
+      is(rangeChange, 10, "Change event should be dispatched on range input element for key changes that change its value (Shift+VK_RIGHT)");
+      synthesizeKey("VK_LEFT", {shiftKey: true});
+      is(rangeChange, 11, "Change event should be dispatched on range input element for key changes that change its value (Shift+VK_LEFT)");
+      synthesizeKey("VK_PAGE_UP", {});
+      is(rangeChange, 12, "Change event should be dispatched on range input element for key changes that change its value (VK_PAGE_UP)");
+      synthesizeKey("VK_PAGE_DOWN", {});
+      is(rangeChange, 13, "Change event should be dispatched on range input element for key changes that change its value (VK_PAGE_DOWN");
+      synthesizeKey("VK_RIGHT", {shiftKey: true});
+      is(rangeChange, 14, "Change event should be dispatched on range input element for key changes that change its value (Shift+VK_PAGE_UP)");
+      synthesizeKey("VK_LEFT", {shiftKey: true});
+      is(rangeChange, 15, "Change event should be dispatched on range input element for key changes that change its value (Shift+VK_PAGE_DOWN)");
+    }
     //Input type change test.
     input = document.getElementById("input_checkbox");
     input.type = "text";
     input.focus();
     input.click();
     input.blur();
     is(NonTextInputChange[5], 1, "Change event shouldn't be dispatched for checkbox ---> text input type change");
 
--- a/dom/html/test/mochitest.ini
+++ b/dom/html/test/mochitest.ini
@@ -626,8 +626,11 @@ skip-if = buildapp == 'mulet' || buildap
 skip-if = (os != 'win' && os != 'linux')
 [test_bug1261674-1.html]
 skip-if = (os != 'win' && os != 'linux')
 [test_bug1261674-2.html]
 skip-if = (os != 'win' && os != 'linux')
 [test_bug1260704.html]
 [test_allowMedia.html]
 [test_bug1292522_same_domain_with_different_port_number.html]
+[test_bug1295719_event_sequence_for_arrow_keys.html]
+skip-if = os == "android" || appname == "b2g" # up/down arrow keys not supported on android/b2g
+[test_bug1295719_event_sequence_for_number_keys.html]
\ No newline at end of file
--- a/dom/html/test/test_bug1261673.html
+++ b/dom/html/test/test_bug1261673.html
@@ -37,31 +37,40 @@ function runTests() {
     {focus: true, deltaY: 1.0, deltaMode: WheelEvent.DOM_DELTA_PIXEL, valueChanged: 0},
     {focus: true, deltaY: -1.0, deltaMode: WheelEvent.DOM_DELTA_PIXEL, valueChanged: 0},
     {focus: false, deltaY: 1.0, deltaMode: WheelEvent.DOM_DELTA_LINE, valueChanged: 0},
     {focus: false, deltaY: -1.0, deltaMode: WheelEvent.DOM_DELTA_LINE, valueChanged: 0}
   ];
 
   let testIdx = 0;
   let result = parseInt(input.value);
+  let numberChange = 0;
+  let expectChange = 0;
+
+  input.addEventListener("change", () => {
+    ++numberChange;
+  }, false);
 
   function runNext() {
     let p = params[testIdx];
     (p["focus"]) ? input.focus() : input.blur();
+    expectChange = p["valueChanged"] == 0 ? expectChange : expectChange + 1;
     result += parseInt(p["valueChanged"]);
     synthesizeWheel(input, 1, 1, { deltaY: p["deltaY"], deltaMode: p["deltaMode"] });
     window.postMessage("finished", "http://mochi.test:8888");
     testIdx++;
   }
 
   window.addEventListener("message", event => {
     if (event.data == "finished") {
       ok(input.value == result,
         "Handle wheel in number input test-" + testIdx + " expect " + result +
         " get " + input.value);
+      ok(numberChange == expectChange,
+         "UA should fire change event when input's value changed, expect " + expectChange + " get " + numberChange);
       (testIdx >= params.length) ? SimpleTest.finish() : runNext();
     }
   });
   runNext();
 }
 
 </script>
 </body>
--- a/dom/html/test/test_bug1261674-1.html
+++ b/dom/html/test/test_bug1261674-1.html
@@ -38,24 +38,33 @@ function runTests() {
     {focus: true, deltaY: 1.0, deltaMode: WheelEvent.DOM_DELTA_PIXEL, valueChanged: 0},
     {focus: true, deltaY: -1.0, deltaMode: WheelEvent.DOM_DELTA_PIXEL, valueChanged: 0},
     {focus: false, deltaY: 1.0, deltaMode: WheelEvent.DOM_DELTA_LINE, valueChanged: 0},
     {focus: false, deltaY: -1.0, deltaMode: WheelEvent.DOM_DELTA_LINE, valueChanged: 0}
   ];
 
   let testIdx = 0;
   let result = parseInt(input.value);
+  let rangeChange = 0;
+  let expectChange = 0;
+
+  input.addEventListener("change", () => {
+    ++rangeChange;
+  }, false);
 
   function runNext() {
     let p = params[testIdx];
     (p["focus"]) ? input.focus() : input.blur();
+    expectChange = p["valueChanged"] == 0 ? expectChange : expectChange + 1;
     result += parseInt(p["valueChanged"]);
     sendWheelAndPaint(input, 1, 1, { deltaY: p["deltaY"], deltaMode: p["deltaMode"] }, () => {
       ok(input.value == result,
          "Handle wheel in range input test-" + testIdx + " expect " + result + " get " + input.value);
+      ok(rangeChange == expectChange,
+         "UA should fire change event when input's value changed, expect " + expectChange + " get " + rangeChange);
       (++testIdx >= params.length) ? SimpleTest.finish() : runNext();
     });
   }
 
   input.addEventListener("input", () => {
     ok(input.value == result,
        "Test-" + testIdx + " receive input event, expect " + result + " get " + input.value);
   }, false);
--- a/dom/html/test/test_bug1261674-2.html
+++ b/dom/html/test/test_bug1261674-2.html
@@ -36,23 +36,29 @@ function runTests() {
     {deltaY: 1.0, deltaMode: WheelEvent.DOM_DELTA_PIXEL},
     {deltaY: -1.0, deltaMode: WheelEvent.DOM_DELTA_PIXEL},
     {deltaY: 1.0, deltaMode: WheelEvent.DOM_DELTA_LINE},
     {deltaY: -1.0, deltaMode: WheelEvent.DOM_DELTA_LINE}
   ];
 
   let testIdx = 0;
   let result = parseInt(input.value);
+  let rangeChange = 0;
+
+  input.addEventListener("change", () => {
+    ++rangeChange;
+  }, false);
 
   function runNext() {
     let p = params[testIdx];
     (p["focus"]) ? input.focus() : input.blur();
     sendWheelAndPaint(input, 1, 1, { deltaY: p["deltaY"], deltaMode: p["deltaMode"] }, () => {
       ok(input.value == result,
          "Handle wheel in range input test-" + testIdx + " expect " + result + " get " + input.value);
+      ok(rangeChange == 0, "Wheel event should not trigger change event when max < min");
       testIdx++;
       (testIdx >= params.length) ? SimpleTest.finish() : runNext();
     });
   }
 
   input.addEventListener("input", () => {
     ok(false, "Wheel event should be no effect to range input element with max < min");
   }, false);
new file mode 100644
--- /dev/null
+++ b/dom/html/test/test_bug1295719_event_sequence_for_arrow_keys.html
@@ -0,0 +1,67 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1295719
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 1295719</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>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1295719">Mozilla Bug 1295719</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+</div>
+<input id="test_number" type="number" value=50>
+<input id="test_range" type="range" value=50 max=100 min=0>
+<script type="text/javascript">
+
+/** Test for Bug 1295719 **/
+SimpleTest.waitForExplicitFinish();
+SimpleTest.waitForFocus(runTests);
+
+function runTests() {
+  let number = window.document.getElementById("test_number");
+  let range = window.document.getElementById("test_range");
+  let waiting_event_sequence = ["keydown", "keypress", "input", "change"];
+
+  let waiting_event_idx = 0;
+  waiting_event_sequence.forEach((eventType) => {
+    number.addEventListener(eventType, (event) => {
+      let waiting_event = waiting_event_sequence[waiting_event_idx];
+      is(waiting_event, eventType, "Waiting " + waiting_event + " get " + eventType);
+      // Input element will fire input and change events when handling keypress
+      // with keycode=arrows. When user press and hold the keyboard, we expect
+      // that input element repeatedly fires "keydown", "keypress", "input", and
+      // "change" events until user release the keyboard. Using
+      // waiting_event_sequence as a circular buffer and reset waiting_event_idx
+      // when it point to the end of buffer.
+      waiting_event_idx = waiting_event_idx == waiting_event_sequence.length -1 ? 0 : waiting_event_idx + 1;
+    }, false);
+    range.addEventListener(eventType, (event) => {
+      let waiting_event = waiting_event_sequence[waiting_event_idx];
+      is(waiting_event, eventType, "Waiting " + waiting_event + " get " + eventType);
+      waiting_event_idx = waiting_event_idx == waiting_event_sequence.length - 1 ? 0 : waiting_event_idx + 1;
+    }, false);
+  });
+
+  number.focus();
+  synthesizeKey("VK_DOWN", {type: "keydown"});
+  synthesizeKey("VK_DOWN", {type: "keydown"});
+  synthesizeKey("VK_DOWN", {type: "keyup"});
+  number.blur();
+  range.focus();
+  waiting_event_idx = 0;
+  synthesizeKey("VK_DOWN", {type: "keydown"});
+  synthesizeKey("VK_DOWN", {type: "keydown"});
+  synthesizeKey("VK_DOWN", {type: "keyup"});
+
+  SimpleTest.finish();
+}
+
+</script>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/dom/html/test/test_bug1295719_event_sequence_for_number_keys.html
@@ -0,0 +1,65 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1295719
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 1295719</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>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1295719">Mozilla Bug 1295719</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+</div>
+<input id="test_number" type="number" value=50>
+<script type="text/javascript">
+
+/** Test for Bug 1295719 **/
+SimpleTest.waitForExplicitFinish();
+SimpleTest.waitForFocus(runTests);
+
+function runTests() {
+  let number = window.document.getElementById("test_number");
+  let waiting_event_sequence = ["keydown", "keypress", "input"];
+  let change_event_of_number = 0;
+  let keyup_event_of_number = 0;
+  let waiting_event_idx = 0;
+  waiting_event_sequence.forEach((eventType) => {
+    number.addEventListener(eventType, (event) => {
+      let waiting_event = waiting_event_sequence[waiting_event_idx];
+      is(eventType, waiting_event, "Waiting " + waiting_event + " get " + eventType);
+      // Input element will fire input event when handling keypress with
+      // keycode=numbers. When user press and hold the keyboard, we expect that
+      // input element repeatedly fires "keydown", "keypress", and "input" until
+      // user release the keyboard. Input element will fire change event when
+      // it's blurred. Using waiting_event_sequence as a circular buffer and
+      // reset waiting_event_idx when it point to the end of buffer.
+      waiting_event_idx = waiting_event_idx == waiting_event_sequence.length - 1 ? 0 : waiting_event_idx + 1;
+    }, false);
+  });
+  number.addEventListener("change", (event) => {
+    is(keyup_event_of_number, 1, "change event should be fired after blurred");
+    ++change_event_of_number;
+  }, false);
+  number.addEventListener("keyup", (event) => {
+    is(keyup_event_of_number, 0, "keyup event should be fired once");
+    is(change_event_of_number, 0, "keyup event should be fired before change event");
+    ++keyup_event_of_number;
+  }, false);
+  number.focus();
+  synthesizeKey("5", {type: "keydown"});
+  synthesizeKey("5", {type: "keydown"});
+  synthesizeKey("5", {type: "keyup"});
+  is(change_event_of_number, 0, "change event shouldn't be fired when input element is focused");
+  number.blur();
+  is(change_event_of_number, 1, "change event should be fired when input element is blurred");
+  SimpleTest.finish();
+}
+
+</script>
+</body>
+</html>