radiogroup: onchange not detected when triggered by keyboard

• Component: Forms
• Status: RESOLVED FIXED

[arno.]

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.

[Alexey Proskuryakov]

*** Bug 33328 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

Confirmed with a local ToT build.

[Erik Arvidsson]

Created attachment 92648 [details]
Patch

[Alexey Proskuryakov]

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]

Thanks for the feedback. Those are all very valuable comments.

[Erik Arvidsson]

Created attachment 92830 [details]
Patch

[Erik Arvidsson]

(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]

Created attachment 92833 [details]
Fix typo

[WebKit Commit Bot]

Comment on attachment 92833 [details]
Fix typo

Clearing flags on attachment: 92833

Committed r86088: <http://trac.webkit.org/changeset/86088>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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