<select> does not fire input event

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: jakub.lopuszanski, Assigned: jdai)

Tracking

29 Branch
mozilla49
x86
Windows 7
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [tw-dom] btpp-active)

Attachments

(4 attachments, 6 obsolete attachments)

(Reporter)

Description

3 years ago
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

3 years ago
(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.
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Events
Ever confirmed: true
Product: Firefox → Core

Updated

3 years ago
Component: DOM: Events → DOM: Core & HTML
(Reporter)

Comment 2

3 years ago
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

2 years ago
(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

2 years ago
Created attachment 8585283 [details]
Small test page to check if input event is fired on select elements

Updated

2 years ago
Duplicate of this bug: 1250521
Whiteboard: [tw-dom]

Comment 6

a year ago
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
(Assignee)

Comment 7

a year ago
I'd like to take this bug.
Assignee: nobody → jdai
(Assignee)

Comment 8

a year ago
Created attachment 8744764 [details] [diff] [review]
wip, v1
(Assignee)

Comment 9

a year ago
Created attachment 8748587 [details] [diff] [review]
wip, v2

Add onInput in e10s.
Attachment #8744764 - Attachment is obsolete: true
(Assignee)

Comment 10

a year ago
Created attachment 8749139 [details] [diff] [review]
Part 1: Bug 1024350 - Remove redundant trailing spaces.
Attachment #8748587 - Attachment is obsolete: true
Attachment #8749139 - Flags: review?(bugs)
(Assignee)

Comment 11

a year ago
Created attachment 8749140 [details] [diff] [review]
Part 2: Bug 1024350 - Support fire input event for select element.
Attachment #8749140 - Flags: review?(bugs)
(Assignee)

Comment 12

a year ago
Created attachment 8749141 [details] [diff] [review]
Part 3: Bug 1024350 - Update mochitest testcase.

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=14e71e0ed900b96e573453d91da374b56cdc90b1
Attachment #8749141 - Flags: review?(bugs)
Whiteboard: [tw-dom] → [tw-dom] btpp-active

Updated

a year ago
Attachment #8749139 - Flags: review?(bugs) → review+
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.
Attachment #8749141 - Flags: review?(bugs) → review+
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>
Attachment #8749140 - Flags: review?(bugs) → review-
(Assignee)

Comment 15

a year ago
Created attachment 8757826 [details] [diff] [review]
Part 1: Bug 1024350 - Remove redundant trailing spaces. r=smaug
Attachment #8749139 - Attachment is obsolete: true
Attachment #8757826 - Flags: review+
(Assignee)

Comment 16

a year ago
Created attachment 8757846 [details] [diff] [review]
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.
Attachment #8749140 - Attachment is obsolete: true
Attachment #8757846 - Flags: review?(bugs)
(Assignee)

Comment 17

a year ago
Created attachment 8757849 [details] [diff] [review]
Part 3: Bug 1024350 - Add mochitest testcase. r=smaug

Address comment 14.

Hi Olli, may I have your review? Thank you.
Attachment #8749141 - Attachment is obsolete: true
Attachment #8757849 - Flags: review?(bugs)
Comment on attachment 8757849 [details] [diff] [review]
Part 3: Bug 1024350 - Add mochitest testcase. r=smaug

Why we need to skip on mac?
Attachment #8757849 - Flags: review?(bugs) → review+
(Assignee)

Comment 19

a year ago
(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 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+.
Attachment #8757846 - Flags: review?(bugs) → review+
(Assignee)

Comment 21

a year ago
Created attachment 8758584 [details] [diff] [review]
Part 2: Bug 1024350 - Support fire input event for select element. r=smaug

Address comment 20.
Attachment #8757846 - Attachment is obsolete: true
Attachment #8758584 - Flags: review+
(Assignee)

Comment 22

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

Add reviewer name.
Attachment #8757849 - Attachment description: Part 3: Bug 1024350 - Add mochitest testcase. v2 → Part 3: Bug 1024350 - Add mochitest testcase. r=smaug
(Assignee)

Comment 23

a year ago
try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bba7e1ec2985c04a46ecd206b3641c09775b65b5
Keywords: checkin-needed

Comment 24

a year ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed

Comment 25

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f58633cc1d45
https://hg.mozilla.org/mozilla-central/rev/2b8c4bcdaef9
https://hg.mozilla.org/mozilla-central/rev/8546d8cdf26f
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.