Bug 98480

Created attachment 167930 [details] Patch

Comment on attachment 167930 [details] Patch Attachment 167930 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14254048 New failing tests: fast/forms/number/number-spinbutton-change-and-input-events.html

Created attachment 167937 [details] Patch 2 Fix number-spinbutton-change-and-input-events.html

Comment on attachment 167937 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=167937&action=review > LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-ax-aria-attributes.html:11 > +function enableAccessibility() Nit: This function just checks window.accessibilityController. You can remove this and write the check directly in the global scope. > LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-ax-aria-attributes.html:22 > + if (!window.accessibilityController) > + return; Nit: Remove this, as you've already checked window.accessibilityController. > LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-ax-aria-attributes.html:23 > + var element = accessibilityController.focusedElement Nit: ';' is missing. > LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-ax-aria-attributes.html:58 > +debug(''); > +testInput.parentNode.removeChild(testInput); What is it for? > LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-ax-value-changed-notification.html:11 > +function enableAccessibility() Nit: initializeAccessibility() might be a better name. > LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-ax-value-changed-notification.html:44 > + debug(''); > + testInput.parentNode.removeChild(testInput); Do you need to remove it? If it is needed, debug('Remove the input element') would be better so that we can observe the removal result in the expected file. > LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-mouse-events.html:7 > +<p id="description"></p> Nit: This is not needed. This will be added automatically. > LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-mouse-events.html:33 > + eventSender.mouseMoveTo(x + input.offsetLeft, y + input.offsetTop); Nit: Maybe you can change this to eventSender.mouseMoveTo(x + input.offsetLeft, input.offsetHeight / 2 + input.offsetTop); Then you can remove 'y' from the arguments of this function. > LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-mouse-events.html:39 > +input.blur(); Nit: Is this needed? > LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-mouse-events.html:91 > + > +debug(''); Nit: Remove this. > LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-reset-value-after-reloads.html:14 > +var testInput1; > +var testInput2; Nit: These can be local variables. > LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-reset-value-after-reloads.html:19 > + iframe.contentDocument.getElementById('test1').value = '2012-10-01'; > + iframe.contentDocument.getElementById('test2').value = '2012-11-01'; Just in case, I want to check the values of these inputs not only in runOnIFrameLoad() but also here. > LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-reset-value-after-reloads.html:30 > + iframe.parentNode.removeChild(iframe); Nit: Is this needed? > LayoutTests/fast/forms/date/date-reset-value.html:3 > +<!-- Confirm appearance after reset() is same as the initial appearance. --> I would prefer writing this test using js-test-pre.js. > LayoutTests/fast/forms/date/date-reset-value.html:11 > +if (testRunner) > + testRunner.waitUntilDone(); > +window.onload = function() { Why does this test need to be async? > LayoutTests/fast/forms/resources/multiple-fields-blur-and-focus-events.js:1 > +var testInput; Nit: This can be a local variable. > LayoutTests/fast/forms/resources/multiple-fields-blur-and-focus-events.js:39 > + if (window.eventSender) { Nit: This is not needed, as it is checked in keyDown(). > LayoutTests/fast/forms/resources/multiple-fields-blur-and-focus-events.js:50 > + document.body.removeChild(document.getElementById("container")); Nit: Is it needed?

Comment on attachment 167937 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=167937&action=review >> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-ax-aria-attributes.html:11 >> +function enableAccessibility() > > Nit: This function just checks window.accessibilityController. You can remove this and write the check directly in the global scope. Done. >> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-ax-aria-attributes.html:22 >> + return; > > Nit: Remove this, as you've already checked window.accessibilityController. Done. >> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-ax-aria-attributes.html:23 >> + var element = accessibilityController.focusedElement > > Nit: ';' is missing. Done. >> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-ax-aria-attributes.html:58 >> +testInput.parentNode.removeChild(testInput); > > What is it for? We'd like to avoid to pollute text result. >> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-ax-value-changed-notification.html:11 >> +function enableAccessibility() > > Nit: initializeAccessibility() might be a better name. Removed. >> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-mouse-events.html:7 >> +<p id="description"></p> > > Nit: This is not needed. This will be added automatically. This and <div id="console"> below are intentional. We'd like to show the description before the long explanations below, and show the console output after the long explanations. >> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-mouse-events.html:33 >> + eventSender.mouseMoveTo(x + input.offsetLeft, y + input.offsetTop); > > Nit: Maybe you can change this to eventSender.mouseMoveTo(x + input.offsetLeft, input.offsetHeight / 2 + input.offsetTop); Then you can remove 'y' from the arguments of this function. I'd like to keep it as is. We'd like to move this function to fast/forms/resources/common.js in the future. >> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-mouse-events.html:39 >> +input.blur(); > > Nit: Is this needed? Removed. >> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-mouse-events.html:91 >> +debug(''); > > Nit: Remove this. This is intentional to improve test result readability. >> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-reset-value-after-reloads.html:14 >> +var testInput2; > > Nit: These can be local variables. No. They must be global because they are used in shouldBe*(). >> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-reset-value-after-reloads.html:19 >> + iframe.contentDocument.getElementById('test2').value = '2012-11-01'; > > Just in case, I want to check the values of these inputs not only in runOnIFrameLoad() but also here. Done. >> LayoutTests/fast/forms/date/date-reset-value.html:3 >> +<!-- Confirm appearance after reset() is same as the initial appearance. --> > > I would prefer writing this test using js-test-pre.js. The purpose of this test is to confirm placeholder strings for each of sub-fields are correctly re-appear. So dumpAsText() doesn't work. >> LayoutTests/fast/forms/date/date-reset-value.html:11 >> +window.onload = function() { > > Why does this test need to be async? To make sure elements are laid out and painted. >> LayoutTests/fast/forms/resources/multiple-fields-blur-and-focus-events.js:1 >> +var testInput; > > Nit: This can be a local variable. No. >> LayoutTests/fast/forms/resources/multiple-fields-blur-and-focus-events.js:39 >> + if (window.eventSender) { > > Nit: This is not needed, as it is checked in keyDown(). keyDown() is not called before this line. So, we should remove eventSender check in keyDown().

Created attachment 167960 [details] Patch 3 Follow reviewer comments.

Comment on attachment 167960 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=167960&action=review > LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-preserve-value-after-history-back.html:64 > + for (var index = 0; index < testCases.length; ++ index) { Nit: ++index

Committed r130884: <http://trac.webkit.org/changeset/130884>