Closed Bug 945784 Opened 7 years ago Closed 7 years ago

Fire 'input' and 'change' events for <input type=number> more frequently, per the new HTML5 rules


(Core :: DOM: Core & HTML, defect)

Not set



Tracking Status
firefox28 --- disabled
firefox29 --- fixed


(Reporter: jwatt, Assigned: jwatt)



(Whiteboard: [qa-])


(2 files, 1 obsolete file)

The HTML5 text for <input type=range> was clarified recently to be clearer about when 'input' and 'change' events are fired for that input type. <input type=number> is conceptually very similar (both input types control a number value), so it would seem to make sense to make <input type=number> dispatch events consistently with <input type=range>.
I'm thinking specifically of the text here:

where it says:

"A third example of a user interface with a commit action would be a Range controls that use a slider. While the user is dragging the control's knob, input events would fire whenever the position changed, whereas the change event would only fire when the user let go of the knob, committing to a specific value."

Analogous for <input type=number> would be that while the mouse is down on one of the spin buttons an the spinner is spinning, 'input' events would fire. Maybe less analogously the releasing of a spin button should be considered a "commit" action, and a 'change' events should fire. That would make us dispatch 'change' events for each click in a rapid series of clicks, and it could be argued there is no "commit" between those clicks, but I think that's okay. Adding logic to try to gauge when we're seeing a rapid series of clicks is probably not worth it, and besides, Chrome is very aggressive in dispatching 'change' events.

I also think we should change things so that repeat NS_VK_UP/NS_VK_DOWN events should fire 'input' events.
Attachment #8341873 - Flags: review?(bugs)
Attachment #8341873 - Attachment description: patch → patch for 'input' event changes
Attached patch patch for 'change' event changes (obsolete) — Splinter Review
The 'change' event changes are maybe a bit more contentious and less important for release, so I've split them into a separate patch.
Attachment #8341873 - Attachment is obsolete: true
Attachment #8341873 - Flags: review?(bugs)
Attachment #8341877 - Flags: review?(bugs)
Attachment #8341873 - Attachment is obsolete: false
Attachment #8341873 - Flags: review?(bugs)
Attachment #8341877 - Attachment is obsolete: true
Attachment #8341877 - Flags: review?(bugs)
Attachment #8341942 - Flags: review?(bugs)
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
9883b8930f30 has nothing to do with this bug. It was checked in with the wrong bug number. It should have been bug 945874.
Resolution: FIXED → ---
Attachment #8341873 - Flags: review?(bugs) → review+
Comment on attachment 8341942 [details] [diff] [review]
patch for 'change' event changes

> HTMLInputElement::StepNumberControlForUserEvent(int32_t aDirection)
> {
>-  ApplyStep(aDirection);
>+  // We cannot call ApplyStep here because that eventually calls SetValue, and
>+  // that sets mFocusedValue. We are handling a user action here, not a script
>+  // action, so we do not want to reset mFocusedValue or else we will fail to
>+  // dispatch 'change' events correctly.
Please add some comment to ApplyStep that if it is changed, this method may need to be changed too
Attachment #8341942 - Flags: review?(bugs) → review+

The test_input_number_key_events.html failures were caused by stepping when we shouldn't have. I fixed that by changing the second patch to split the body of ApplyStep out into a separate GetValueIfStepped method and then using that in StepNumberControlForUserEvent.

The test_change_event.html and test_input_event.html on android and b2g non-desktop were caused by the failure of the new up/down arrow key tests that expect the value to be increased/decreased. These aren't valid there because the IME keyboard doesn't forward those keys, so I'd disabled them from running on android and b2g non-desktop.
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.