<input> of type=checkbox,radio, file doesn't fire `input` events

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: mozilla, Assigned: jessica)

Tracking

({html5, testcase})

Trunk
mozilla49
html5, testcase
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Per the following portions of the HTML specification:
* https://html.spec.whatwg.org/multipage/forms.html#checkbox-state-(type=checkbox):event-input-input
* https://html.spec.whatwg.org/multipage/forms.html#radio-button-state-(type=radio):event-input-input
* https://html.spec.whatwg.org/multipage/forms.html#file-upload-state-(type=file):event-input-input

when the user changes the checkedness of an <input type="checkbox"> or <input type="radio">, or when a user changes the selected files of an <input type="file">, the browser is supposed to fire an `input` event (https://developer.mozilla.org/en-US/docs/Web/Events/input ) at that <input> element.
Firefox 42 doesn't comply with this, and doesn't fire `input` events in these cases.

Testcase: http://output.jsbin.com/jojoji
(Reporter)

Updated

4 years ago
Confirmed also in Firefox 41
Component: Untriaged → DOM: Events
Product: Firefox → Core
Version: Trunk → 41 Branch
(Reporter)

Updated

4 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Updated

3 years ago
Keywords: html5
Version: 41 Branch → Trunk
(Reporter)

Updated

3 years ago
(Reporter)

Updated

3 years ago
Blocks: 344614
Whiteboard: [tw-dom]
I'd like to give it a try!
Assignee: nobody → jjong
Per [1], checkbox, radio and file input types have a defined activation behaviour. In the defined activation behaviour described in each of their section, 'input' event is fired before 'change' event, so I'm going to add firing 'input' events before 'change' events.

[1] https://html.spec.whatwg.org/multipage/forms.html#common-input-element-events
Whiteboard: [tw-dom] → [tw-dom] btpp-active
Posted patch patch, v1. (obsolete) — Splinter Review
Olli, may I have your feedback on this? Thanks.
Attachment #8743158 - Flags: feedback?(bugs)
Comment on attachment 8743158 [details] [diff] [review]
patch, v1.

So 
https://html.spec.whatwg.org/multipage/forms.html#checkbox-state-(type=checkbox)
https://html.spec.whatwg.org/multipage/forms.html#radio-button-state-(type=radio)
https://html.spec.whatwg.org/multipage/forms.html#file-upload-state-(type=file)


Don't we dispatch the "change" for checkbox and radiobutton 
http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#3852
Dispatching "input" in the same place would be good.
And the spec talks about activation behavior, which in Gecko terminology is pretty much PostHandleEvent.

Also, dispatching the event in SetCheckedInternal may cause us to dispatch the event twice if 'click' which is changing the state is cancelled (.preventDefault()), at least if I read the code right.

You could add a test for checkbox and radiobutton that (1) when doing click(), the click event is handled before 
 "change" and "input" and (2) that if click event is cancelled, neither of the events is dispatched.
Attachment #8743158 - Flags: feedback?(bugs) → feedback+
(In reply to Olli Pettay [:smaug] from comment #5)
> Comment on attachment 8743158 [details] [diff] [review]
> patch, v1.
> 
> So 
> https://html.spec.whatwg.org/multipage/forms.html#checkbox-state-
> (type=checkbox)
> https://html.spec.whatwg.org/multipage/forms.html#radio-button-state-
> (type=radio)
> https://html.spec.whatwg.org/multipage/forms.html#file-upload-state-
> (type=file)
> 
> 
> Don't we dispatch the "change" for checkbox and radiobutton 
> http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.
> cpp#3852
> Dispatching "input" in the same place would be good.
> And the spec talks about activation behavior, which in Gecko terminology is
> pretty much PostHandleEvent.

Thanks, that's a useful note.

> 
> Also, dispatching the event in SetCheckedInternal may cause us to dispatch
> the event twice if 'click' which is changing the state is cancelled
> (.preventDefault()), at least if I read the code right.

Yes, you are right, the events were dispatched twice.

> 
> You could add a test for checkbox and radiobutton that (1) when doing
> click(), the click event is handled before 
>  "change" and "input" and (2) that if click event is cancelled, neither of
> the events is dispatched.

I have added a test case for (2), do you think it covers (1) as well? (I'll upload my updated patch right away).
Posted patch patch, v2. (obsolete) — Splinter Review
Attachment #8743158 - Attachment is obsolete: true
Attachment #8743646 - Flags: review?(bugs)
Comment on attachment 8743646 [details] [diff] [review]
patch, v2.

Is there really no web platform tests for this stuff. If so, surprising.
Attachment #8743646 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8743646 [details] [diff] [review]
> patch, v2.
> 
> Is there really no web platform tests for this stuff. If so, surprising.

Oh, actually there is. They are under testing/web-platform/tests/html/semantics/forms/the-input-element/ and are expected to fail right now.
But after running with the proposed patch, I still get an error:
"assert_false: click()-initiated input event shouldn't be trusted expected false got true"

But per spec, a simple event is trusted [1], unless I am missing something here.

[1] https://html.spec.whatwg.org/multipage/webappapis.html#fire-a-simple-event
Revised commit message.
Attachment #8743646 - Attachment is obsolete: true
Attachment #8745169 - Flags: review+
Updated wpt based on spec: events should be trusted.
Attachment #8745170 - Flags: review?(bugs)
Comment on attachment 8745170 [details] [diff] [review]
Part 2: update web platform tests. r=smaug

Thanks.
If the specification bug is fixed, checkbox.html and radio.html may be changed back to the old behavior, but that is something for a different bug.
Attachment #8745170 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #13)
> Comment on attachment 8745170 [details] [diff] [review]
> Part 2: update web platform tests. r=smaug
> 
> Thanks.
> If the specification bug is fixed, checkbox.html and radio.html may be
> changed back to the old behavior, but that is something for a different bug.

Got it. Thanks for filing the bug.
try looks good.
Keywords: checkin-needed

Comment 18

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ddb96fbc66c9
https://hg.mozilla.org/mozilla-central/rev/477b790ce198
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Comment 19

a year ago
Is this really fixed?

Please see this test page, there is one failing test:
http://w3c-test.org/html/semantics/forms/the-input-element/checkbox-click-events.html
That failing test doesn't seem to have anything to do with input events.
But feel free to file a new bug about the failure.

Comment 21

a year ago
Alright, I opened bug 1434268
You need to log in before you can comment on or make changes to this bug.