Bug 1295719 - input[type=range,number] does not fire 'change' event for some key combinations. r=smaug, a=ritu
authorStone Shih <sshih@mozilla.com>
Fri, 19 Aug 2016 09:19:35 +0800
changeset 350651 97b73c8ba503bf0fb54136258a8c9653e5b50e9b
parent 350650 61f5a6000c752b08ba01a18d64a186d500cb91a4
child 350652 63d6e12ae53374f06ad39535803962a37abcb2c4
push id1230
push userjlund@mozilla.com
push dateMon, 31 Oct 2016 18:13:35 +0000
treeherdermozilla-release@5e06e3766db2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug, ritu
bugs1295719
milestone50.0
Bug 1295719 - input[type=range,number] does not fire 'change' event for some key combinations. r=smaug, a=ritu
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
@@ -3773,29 +3773,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
@@ -4271,16 +4258,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
@@ -4448,16 +4436,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:
@@ -4525,29 +4514,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;
@@ -8152,24 +8143,12 @@ HTMLInputElement::UpdateEntries(const ns
 
 void
 HTMLInputElement::GetWebkitEntries(nsTArray<RefPtr<Entry>>& 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
@@ -1492,22 +1492,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
@@ -624,8 +624,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>