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.
Weird! But if IE and WebKit are doing the same thing, then nearly every browser works like that.
Opera and Firefox don't.
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.
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.
See also: bug 93760.
Created attachment 253697 [details] Patch
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.
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
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
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
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
Created attachment 253702 [details] Patch
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.
Created attachment 253780 [details] Patch
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.
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.
Created attachment 254793 [details] Patch
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).
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
(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.
See also HTML spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=28985
(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
Mass move bugs into the DOM component.