Bug 93674 – change event is sent to <input type=radio> before click event

• Component: DOM
• Status: NEW

[Glenn Maynard]

When a radio button (or checkbox) is clicked, the onchange event is sent before onclick instead of after.

If onclick is defaultPrevented, the page has now received onchange in the temporary checked state, even though the "canceled activation steps" reset the checked state to false.

Opera and Firefox behave per spec: onclick is sent before onchange, and if onclick is stopped, the onchange event is never fired.  (Firing onclick is the activation behavior of the element, which happens after an uncancelled click.)

IE9 matches what WebKit is doing.

[Alexey Proskuryakov]

Weird! But if IE and WebKit are doing the same thing, then nearly every browser works like that.

[Glenn Maynard]

Opera and Firefox don't.

[Alexey Proskuryakov]

Got that, I was counting not by browser name, but by installed base.

That's just a data point - Firefox behavior seems much more logical, unless there is an explanation for IE behavior that I don't know.

[Glenn Maynard]

At least, since cancelling onchange doesn't do anything, people aren't expecting preventDefault on onchange to prevent onclick.  I wonder if Mozilla has received any bug reports for following the spec.

[Alexey Proskuryakov]

See also: bug 93760.

[Rohit]

Created attachment 253697 [details]
Patch

[WebKit Commit Bot]

Attachment 253697 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 253697 [details]
Patch

Attachment 253697 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6352494187773952

New failing tests:
fast/forms/radio/radio-group-keyboard-change-event.html

[Build Bot]

Created attachment 253700 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 253697 [details]
Patch

Attachment 253697 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6332516818485248

New failing tests:
fast/forms/radio/radio-group-keyboard-change-event.html

[Build Bot]

Created attachment 253701 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Rohit]

Created attachment 253702 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 253702 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253702&action=review

> Source/WebCore/html/CheckboxInputType.cpp:86
> +    } else {
> +        element().dispatchChangeEventIfNeeded(); 
>      }

nit: No curly braces arouns a single line statement.

> Source/WebCore/html/HTMLInputElement.cpp:862
> +        setTextAsOfLastFormControlChangeEvent(String());

Why is this call to setTextAsOfLastFormControlChangeEvent necessary even we've already called it in HTMLInputElement::setChecked?
It seems strange that we need to call it in both places.

> Source/WebCore/html/RadioInputType.cpp:184
> +    } else {
> +        element().dispatchChangeEventIfNeeded();
>      }

nit: No curly braces around a single line statement.

[Rohit]

Created attachment 253780 [details]
Patch

[Rohit]

Comment on attachment 253702 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253702&action=review

>> Source/WebCore/html/CheckboxInputType.cpp:86
>>      }
> 
> nit: No curly braces arouns a single line statement.

Done.

>> Source/WebCore/html/HTMLInputElement.cpp:862
>> +        setTextAsOfLastFormControlChangeEvent(String());
> 
> Why is this call to setTextAsOfLastFormControlChangeEvent necessary even we've already called it in HTMLInputElement::setChecked?
> It seems strange that we need to call it in both places.

Removed.
I added setTextAsOfLastFormControlChangeEvent(String()) in both as setChecked() and dispatchChangeEventIfNeeded() are getting called in if-else statements. But as setChecked() is already getting called in willDispatchClick for both radio and checkbox, it makes more sense to remove it from dispatchChangeEventIfNeeded().

>> Source/WebCore/html/RadioInputType.cpp:184
>>      }
> 
> nit: No curly braces around a single line statement.

Done.

[Antonio Gomes]

Comment on attachment 253780 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253780&action=review

code looks reasonable, but you need to add some background/context to your commit message.

> Source/WebCore/ChangeLog:5
> +

please
- add the motivation of the spec (match the spec, add an url)
- any possible real world web site behavior that the patch fixes
- also mention that webkit now matches firefox and open (presto or chromium?) behavior?, and does not match IE anymore.

[Rohit]

Created attachment 254793 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 253702 [details]
Patch

Cleared review? from obsolete attachment 253702 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Kent Tamura]

Comment on attachment 254793 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254793&action=review

> Source/WebCore/html/HTMLInputElement.cpp:862
> +        dispatchFormControlChangeEvent();

This causes a regression.  See https://code.google.com/p/chromium/issues/detail?id=528985

[Chris Dumez]

(In reply to comment #19)
> Comment on attachment 254793 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254793&action=review
> 
> > Source/WebCore/html/HTMLInputElement.cpp:862
> > +        dispatchFormControlChangeEvent();
> 
> This causes a regression.  See
> https://code.google.com/p/chromium/issues/detail?id=528985

Thanks Tamura.

[Chris Rebert]

See also HTML spec bug:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=28985

[Chris Rebert]

(In reply to comment #19)
> Comment on attachment 254793 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254793&action=review
> 
> > Source/WebCore/html/HTMLInputElement.cpp:862
> > +        dispatchFormControlChangeEvent();
> 
> This causes a regression.  See
> https://code.google.com/p/chromium/issues/detail?id=528985

The regression was subsequently fixed:
https://code.google.com/p/chromium/issues/detail?id=528985#c4
https://src.chromium.org/viewvc/blink?view=revision&revision=202190

[Lucas Forschler]

Mass move bugs into the DOM component.