Bug 32013 - radiogroup: onchange not detected when triggered by keyboard
Summary: radiogroup: onchange not detected when triggered by keyboard
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Erik Arvidsson
URL:
Keywords:
: 33328 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-12-01 02:56 PST by arno.
Modified: 2011-05-09 15:49 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description arno. 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.
Comment 1 Alexey Proskuryakov 2010-01-07 10:45:15 PST
*** Bug 33328 has been marked as a duplicate of this bug. ***
Comment 2 Alexey Proskuryakov 2010-01-07 10:54:15 PST
Confirmed with a local ToT build.
Comment 3 Erik Arvidsson 2011-05-06 15:24:46 PDT
Created attachment 92648 [details]
Patch
Comment 4 Alexey Proskuryakov 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.
Comment 5 Erik Arvidsson 2011-05-06 17:40:30 PDT
Thanks for the feedback. Those are all very valuable comments.
Comment 6 Erik Arvidsson 2011-05-09 12:44:26 PDT
Created attachment 92830 [details]
Patch
Comment 7 Erik Arvidsson 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.
Comment 8 Erik Arvidsson 2011-05-09 12:52:17 PDT
Created attachment 92833 [details]
Fix typo
Comment 9 WebKit Commit Bot 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>
Comment 10 WebKit Commit Bot 2011-05-09 14:42:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 WebKit Review Bot 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