Bug 98480

Summary: Add behavior tests for input[type=date] with multiple fields
Product: WebKit Reporter: Kent Tamura <tkent>
Component: Tools / TestsAssignee: Kent Tamura <tkent>
Status: RESOLVED FIXED    
Severity: Normal CC: haraken, yutak
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 98226    
Attachments:
Description Flags
Patch
none
Patch 2
none
Patch 3 haraken: review+

Description Kent Tamura 2012-10-04 21:48:36 PDT
Add behavior tests for input[type=date] with multiple fields
Comment 1 Kent Tamura 2012-10-09 23:14:58 PDT
Created attachment 167930 [details]
Patch
Comment 2 Build Bot 2012-10-09 23:42:58 PDT
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
Comment 3 Kent Tamura 2012-10-09 23:49:15 PDT
Created attachment 167937 [details]
Patch 2

Fix number-spinbutton-change-and-input-events.html
Comment 4 Kentaro Hara 2012-10-10 01:10:52 PDT
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 5 Kent Tamura 2012-10-10 01:58:55 PDT
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().
Comment 6 Kent Tamura 2012-10-10 02:08:11 PDT
Created attachment 167960 [details]
Patch 3

Follow reviewer comments.
Comment 7 Kentaro Hara 2012-10-10 03:22:25 PDT
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
Comment 8 Kent Tamura 2012-10-10 03:52:42 PDT
Committed r130884: <http://trac.webkit.org/changeset/130884>