[webkit-reviews] review denied: [Bug 32013] radiogroup: onchange not detected when triggered by keyboard : [Attachment 92648] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri May 6 17:04:11 PDT 2011
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.
More information about the webkit-reviews
mailing list