[webkit-reviews] review denied: [Bug 32013] radiogroup: onchange not detected when triggered by keyboard : [Attachment 92648] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Erik Arvidsson
<arv at chromium.org>'s request for review:
Bug 32013: radiogroup: onchange not detected when triggered by keyboard
https://bugs.webkit.org/show_bug.cgi?id=32013

Attachment 92648: Patch
https://bugs.webkit.org/attachment.cgi?id=92648&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.