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

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM: Events
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Chris Rebert, Assigned: jessica)

Tracking

(Blocks: 1 bug, {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

2 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

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

Updated

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

Updated

2 years ago
(Reporter)

Updated

2 years ago
(Reporter)

Updated

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

Updated

2 years ago
(Reporter)

Updated

2 years ago
Blocks: 344614
Whiteboard: [tw-dom]
(Assignee)

Comment 2

a year ago
I'd like to give it a try!
Assignee: nobody → jjong
(Assignee)

Comment 3

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

Comment 4

a year ago
Created attachment 8743158 [details] [diff] [review]
patch, v1.

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+
(Assignee)

Comment 6

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

Comment 7

a year ago
Created attachment 8743646 [details] [diff] [review]
patch, v2.
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+
(Assignee)

Comment 9

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

Comment 10

a year ago
Created attachment 8745169 [details] [diff] [review]
Part 1: Fire input event for type=checkbox,radio,file. r=smaug

Revised commit message.
Attachment #8743646 - Attachment is obsolete: true
Attachment #8745169 - Flags: review+
(Assignee)

Comment 11

a year ago
Created attachment 8745170 [details] [diff] [review]
Part 2: update web platform tests. r=smaug

Updated wpt based on spec: events should be trusted.
Attachment #8745170 - Flags: review?(bugs)
FYI, I filed https://github.com/whatwg/html/issues/1126
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+
(Assignee)

Comment 14

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

Comment 15

a year ago
try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e879e94c2903
(Assignee)

Comment 16

a year ago
try looks good.
Keywords: checkin-needed

Comment 17

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddb96fbc66c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/477b790ce198
Keywords: checkin-needed

Comment 18

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ddb96fbc66c9
https://hg.mozilla.org/mozilla-central/rev/477b790ce198
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.