WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
32013
radiogroup: onchange not detected when triggered by keyboard
https://bugs.webkit.org/show_bug.cgi?id=32013
Summary
radiogroup: onchange not detected when triggered by keyboard
arno.
Reported
2009-12-01 02:56:44 PST
Created
attachment 44061
[details]
testcase Hi, I've a radiogroup with onchange handlers. Those handlers are triggered correctly when radiogroup selection is changed with mouse. But they are not when selection is changed with keyboard. same behaviour with webkit-gtk and google chrome, so I assume it's a webkit bug.
Attachments
testcase
(732 bytes, text/html)
2009-12-01 02:56 PST
,
arno.
no flags
Details
Patch
(3.78 KB, patch)
2011-05-06 15:24 PDT
,
Erik Arvidsson
no flags
Details
Formatted Diff
Diff
Patch
(6.19 KB, patch)
2011-05-09 12:44 PDT
,
Erik Arvidsson
no flags
Details
Formatted Diff
Diff
Fix typo
(6.19 KB, patch)
2011-05-09 12:52 PDT
,
Erik Arvidsson
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1
2010-01-07 10:45:15 PST
***
Bug 33328
has been marked as a duplicate of this bug. ***
Alexey Proskuryakov
Comment 2
2010-01-07 10:54:15 PST
Confirmed with a local ToT build.
Erik Arvidsson
Comment 3
2011-05-06 15:24:46 PDT
Created
attachment 92648
[details]
Patch
Alexey Proskuryakov
Comment 4
2011-05-06 17:04:12 PDT
Comment on
attachment 92648
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=92648&action=review
This is an awesome fix! I think that tests need some improvement though.
> LayoutTests/fast/forms/radio-group-keyboard-change-event.html:8 > +if (window.layoutTestController) > + layoutTestController.dumpAsText();
Bad indentation.
> LayoutTests/fast/forms/radio-group-keyboard-change-event.html:10 > +var expexted = 'bc';
Typo: expected.
> LayoutTests/fast/forms/radio-group-keyboard-change-event.html:20 > +if (window.eventSender) {
The test should have an explanation of what to do to verify its behavior manually, or at the very least, an explanation that it doesn't work in a browser. We strongly prefer tests that can be run in a browser, so that anyone could easily compare the behavior to other browsers.
> LayoutTests/fast/forms/radio-group-keyboard-change-event.html:29 > +else > + result = 'FAIL: Expected "' + expexted + '" but got "' + actual + '"';
Bad indentation.
> Source/WebCore/ChangeLog:13 > + (WebCore::RadioInputType::handleKeydownEvent): Ensure that we do not check the radio input before we simulate the click > + event. The simulated click event will check it for us but more importantly it will fire the "change" event as > + expected.
I think that a change like that should have a test verifying that event.preventDefault() in click event handler didn't prevent setting the value before the fix, but does now. The test should pass in Firefox and IE.
Erik Arvidsson
Comment 5
2011-05-06 17:40:30 PDT
Thanks for the feedback. Those are all very valuable comments.
Erik Arvidsson
Comment 6
2011-05-09 12:44:26 PDT
Created
attachment 92830
[details]
Patch
Erik Arvidsson
Comment 7
2011-05-09 12:50:00 PDT
(In reply to
comment #4
)
> > Source/WebCore/ChangeLog:13 > > + (WebCore::RadioInputType::handleKeydownEvent): Ensure that we do not check the radio input before we simulate the click > > + event. The simulated click event will check it for us but more importantly it will fire the "change" event as > > + expected. > > I think that a change like that should have a test verifying that event.preventDefault() in click event handler didn't prevent setting the value before the fix, but does now. The test should pass in Firefox and IE.
Now we match other browsers and that is that calling preventDefault in the click event DOES prevent the checked radio button in the group from being changed. There are still browser differences when a change event is triggered. I'll start a discusion on whatwg to try to resolve this. We currently match IE here and that is that we fire change and then revert the value back in case of a call to preventDefault.
Erik Arvidsson
Comment 8
2011-05-09 12:52:17 PDT
Created
attachment 92833
[details]
Fix typo
WebKit Commit Bot
Comment 9
2011-05-09 14:42:54 PDT
Comment on
attachment 92833
[details]
Fix typo Clearing flags on attachment: 92833 Committed
r86088
: <
http://trac.webkit.org/changeset/86088
>
WebKit Commit Bot
Comment 10
2011-05-09 14:42:59 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 11
2011-05-09 15:49:43 PDT
http://trac.webkit.org/changeset/86088
might have broken SnowLeopard Intel Release (WebKit2 Tests) The following tests are not passing: fast/forms/radio-group-keyboard-change-event.html fast/frames/flattening/frameset-flattening-subframesets.html
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug