1024350 - <select> does not fire input event

Status: RESOLVED FIXED

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

Description

[jakub.lopuszanski]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140506152807

Steps to reproduce:

Assume a simple <select> with two <options> : 
http://codepen.io/anon/pen/uLAmv 
Attach event listeners to the <select> element for 'change' an 'input' events.
Select (using mouse) one of the options.


Actual results:

only 'change' event was triggered


Expected results:

'input' followed by 'change' should be fired.

The docs at https://developer.mozilla.org/en-US/docs/Web/Reference/Events/input link to the http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-element-attributes.html#event-input-input which says:

When the input and change events apply (which is the case for all input controls other than buttons and those with the type attribute in the Hidden state), the events are fired to indicate that the user has interacted with the control. The input event fires whenever the user has modified the data of the control. The change event fires when the value is committed, if that makes sense for the control, or else when the control loses focus. In all cases, the input event comes before the corresponding change event (if any).

Comment 1

[:Gijs (he/him)]

(In reply to jakub.lopuszanski from comment #0)
> The docs at
> https://developer.mozilla.org/en-US/docs/Web/Reference/Events/input link to
> the
> http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-
> element-attributes.html#event-input-input which says:
> 
> When the input and change events apply (which is the case for all input
> controls other than buttons and those with the type attribute in the Hidden
> state)

But the <select> element isn't an <input> element.

I think you rather mean:

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#send-select-update-notifications

In particular:

When the user agent is to send select update notifications, queue a task to first fire a simple event that bubbles named input at the select element, and then fire a simple event that bubbles named change at the select element, using the user interaction task source as the task source.

Comment 2

[jakub.lopuszanski]

Actually, I was misled by the fact that red words input and button appeared in the same statement, so I assumed that they were using "input" in a broad sense of "ui elements which user can use to input, such as input and button". Now, I understand that this statement was about a <input type="button"> and not about a <button>.

Anyway. The thing is that Firefox's controls work differently than Chrome's, which is particularly annoying, when one has to write a javascript to handle <form> controls, before deciding if some filed should be <input type="text"> or <select> or <textarea> etc. I want to react as soon as possible to actions of user, so using the input event seemed logical choice, and worked in Chrome.

I think that it would be nice if all form controls (<input>,<textarea>,<select>..) had similar "interface" so one could abstract away some "ifology".

Comment 3

[psa.pub.0]

(In reply to :Gijs Kruitbosch (away 26-30/3) from comment #1) 
> But the <select> element isn't an <input> element.
> 
> I think you rather mean:
> 
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-
> element.html#send-select-update-notifications
> 
> In particular:
> 
> When the user agent is to send select update notifications, queue a task to
> first fire a simple event that bubbles named input at the select element,
> and then fire a simple event that bubbles named change at the select
> element, using the user interaction task source as the task source.

I'm currently writing a cross-browser web application that uses these events to do a number of tasks, including checking whether or not the form is valid (so that it can provide an indication to the user of whole-form validity), so I'm interested in understanding the way that this should be working.  It appears to me that the above quote about select update notifications says that a bubbling input event should be fired prior to firing the change event from the select, which would imply that Firefox should still fire an input event.  I have done some cross-browser testing on this, and as far as I can tell Firefox (Gecko)/Konqueror(KHTML)/MSIE all follow this behavior, while all of the Webkit-based browser that I have been able to access fire both input and change.

Comment 4

[psa.pub.0]

Attachment: Small test page to check if input event is fired on select elements

Comment 6

[github]

When this is fixed, the documentation of the `input` event should also need to be modified to state that `<select>` also fires it. (https://developer.mozilla.org/en-US/docs/Web/Events/input)

I hope this gets fixed soon. The `input` event is perfect for validation purposes, we use it for Parsley.js.

Thanks

Comment 7

[John Dai[:jdai]]

I'd like to take this bug.

Comment 8

[John Dai[:jdai]]

Attachment: wip, v1

Comment 9

[John Dai[:jdai]]

Attachment: wip, v2

Add onInput in e10s.

Comment 10

[John Dai[:jdai]]

Attachment: Part 1: Bug 1024350 - Remove redundant trailing spaces.

Comment 11

[John Dai[:jdai]]

Attachment: Part 2: Bug 1024350 - Support fire input event for select element.

Comment 12

[John Dai[:jdai]]

Attachment: Part 3: Bug 1024350 - Update mochitest testcase.

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=14e71e0ed900b96e573453d91da374b56cdc90b1

Comment 13

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

Comment on attachment 8749141 [details] [diff] [review]
Part 3: Bug 1024350 - Update mochitest testcase.

I wonder if there are some web-platform-tests for this. Maybe not.

Comment 14

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

Comment on attachment 8749140 [details] [diff] [review]
Part 2: Bug 1024350 - Support fire input event for select element.

>+// Send out an onInput and onChange notification.
>+void
>+nsListControlFrame::FireOnInputAndOnChange()
>+{
>+  if (mComboboxFrame) {
>+    // Return hit without changing anything
>+    int32_t index = mComboboxFrame->UpdateRecentIndex(NS_SKIP_NOTIFY_INDEX);
>+    if (index == NS_SKIP_NOTIFY_INDEX)
>+      return;
>+
>+    // See if the selection actually changed
>+    if (index == GetSelectedIndex())
>+      return;
Nit, in C++ 'if' should always have {}, so
if (...) {
  ...
}
Some old code may not be so strict.


But, do we really need separate methods for input+change and change firing.
I'd prefer having just one method which takes then some enum value as param
eFireInputAndChange or eFireChange. And each case when eFireChange is 
used, why we don't want to dispatch also input? I think we need to annotate each such case separately.
(by annotation I mean adding some comment in the code)
Though, currently I'm having hard time to find a case where the spec says only change should be dispatched, since
I see only https://html.spec.whatwg.org/multipage/forms.html#send-select-update-notifications defining those events. But am I missing some case where input shouldn't be fired but change should?

(Btw, I did file https://github.com/whatwg/html/issues/1092 some time ago.)

Do we have tests both for <select size=1> (==dropdown) and <select size="{something_larger_than_1}"> (list)
Need also some test for <select size="{something_larger_than_1}" multiple>

Comment 15

[John Dai[:jdai]]

Attachment: Part 1: Bug 1024350 - Remove redundant trailing spaces. r=smaug

Comment 16

[John Dai[:jdai]]

Attachment: Part 2: Bug 1024350 - Support fire input event for select element. v2

Address comment 14. I didn't find a case which is only fire change event without input event on other browser. 

Hi Olli, may I have your review? Thank you.

Comment 17

[John Dai[:jdai]]

Attachment: Part 3: Bug 1024350 - Add mochitest testcase. r=smaug

Address comment 14.

Hi Olli, may I have your review? Thank you.

Comment 18

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

Comment on attachment 8757849 [details] [diff] [review]
Part 3: Bug 1024350 - Add mochitest testcase. r=smaug

Why we need to skip on mac?

Comment 19

[John Dai[:jdai]]

(In reply to Olli Pettay [:smaug] from comment #18)
> Comment on attachment 8757849 [details] [diff] [review]
> Part 3: Bug 1024350 - Add mochitest testcase. v2
> 
> Why we need to skip on mac?
Bug 1272012 cause disabled dropdown testcase on OSX and it will be enabled in bug 1275513.

Comment 20

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

Comment on attachment 8757846 [details] [diff] [review]
Part 2: Bug 1024350 - Support fire input event for select element. v2

Oh, e10s got input event in Bug 1266575.
A tad hard to follow what events get added where when e10s isn't following the non-e10s behavior :/


> 
>+  // Dispatch the input event.
>+  nsContentUtils::DispatchTrustedEvent(mContent->OwnerDoc(), mContent,
>+                                       NS_LITERAL_STRING("input"), true,
>+                                       false);
>   // Dispatch the change event.
>   nsContentUtils::DispatchTrustedEvent(mContent->OwnerDoc(), mContent,
>                                        NS_LITERAL_STRING("change"), true,
>                                        false);
so mContent may point to a deleted object here.
I would have nsCOMPtr<nsIContent> content = mContent; before dispatching "input" and then use that variable
for both events to get OwnerDoc() and to use it as target, and not use mContent at all after the assignment.
That fixed, r+.

Comment 21

[John Dai[:jdai]]

Attachment: Part 2: Bug 1024350 - Support fire input event for select element. r=smaug

Address comment 20.

Comment 22

[John Dai[:jdai]]

Comment on attachment 8757849 [details] [diff] [review]
Part 3: Bug 1024350 - Add mochitest testcase. r=smaug

Add reviewer name.

Comment 23

[John Dai[:jdai]]

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bba7e1ec2985c04a46ecd206b3641c09775b65b5

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f58633cc1d45
Remove redundant trailing spaces.r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b8c4bcdaef9
Support fire input event for select element.r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8546d8cdf26f
Add mochitest testcase.r=smaug

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f58633cc1d45
https://hg.mozilla.org/mozilla-central/rev/2b8c4bcdaef9
https://hg.mozilla.org/mozilla-central/rev/8546d8cdf26f