Bug 98480 – Add behavior tests for input[type=date] with multiple fields

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Kent Tamura]

Add behavior tests for input[type=date] with multiple fields

[Kent Tamura]

Created attachment 167930 [details]
Patch

[Build Bot]

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

[Kent Tamura]

Created attachment 167937 [details]
Patch 2

Fix number-spinbutton-change-and-input-events.html

[Kentaro Hara]

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?

[Kent Tamura]

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().

[Kent Tamura]

Created attachment 167960 [details]
Patch 3

Follow reviewer comments.

[Kentaro Hara]

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

[Kent Tamura]

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