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

Status: RESOLVED FIXED

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

Description

[Jonathan Watt [:jwatt]]

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>.

Comment 1

[Jonathan Watt [:jwatt]]

I'm thinking specifically of the text here:

http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-element-attributes.html#event-input-input

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.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch for 'input' event changes

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch for 'change' event changes

The 'change' event changes are maybe a bit more contentious and less important for release, so I've split them into a separate patch.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch for 'change' event changes

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9883b8930f30

Comment 6

[Jonathan Watt [:jwatt]]

9883b8930f30 has nothing to do with this bug. It was checked in with the wrong bug number. It should have been bug 945874.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 8

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64f359e4e054
https://hg.mozilla.org/integration/mozilla-inbound/rev/29db440280c8

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f23fd773d7d1

https://tbpl.mozilla.org/php/getParsedLog.php?id=31507132&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=31506475&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=31506497&tree=Mozilla-Inbound

Comment 10

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b9c628c3c53
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9e9debc9b1

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.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b9c628c3c53
https://hg.mozilla.org/mozilla-central/rev/8a9e9debc9b1