Bug 94196 – [Tests] Adding tests for multiple fields time input UI

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

These tests are running for ports which enable ENABLE_INPUT_TYPE_TIME and ENABLE_INPUT_TYPE_TIME_MULTIPLE_FIELDS.
At this time, such ports are Chromium ports except for Android.

Created attachment 158735 [details]
Patch 1

Comment on attachment 158735 [details]
Patch 1

Could you review this patch?
Thanks in advance.

[Kent Tamura]

Comment on attachment 158735 [details]
Patch 1

View in context: https://bugs.webkit.org/attachment.cgi?id=158735&action=review

> LayoutTests/ChangeLog:38
> +

We should have a pixel test for the default appearance of <input type=time readonly> and <input type=time disabled>

platform/chromium/TestExpectations should have a failing entry for non-Linux.

> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-apperance-basic.html:3
> +<link rel="stylesheet" type="text/css" href="../resources/time-multiple-fields-apperance.css" />

Is this needed?

> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-apperance-pseudo-elements.html:10
> +    <li><span class="after">After</span><input type="time" value="12:34:56" class="after"></li>
> +    <li><span class="before">Before</span><input type="time" value="12:34:56" class="before"></li>

Why do you set after/before to the <span> elements?

> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-apperance-pseudo-elements.html:11
> +    <li><p id="first-letter">first letter</p><p id="first-letter"><input type="time" value="12:34:56" class="first-letter"></li></p>

Having multiple element with identical ID is not good.

Created attachment 158758 [details]
Patch 2

(In reply to comment #3)
> (From update of attachment 158735 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158735&action=review
> 
> > LayoutTests/ChangeLog:38
> > +
> 
> We should have a pixel test for the default appearance of <input type=time readonly> and <input type=time disabled>
> 
Added time-multiple-fields-appearance-disabled-readonly.html


> platform/chromium/TestExpectations should have a failing entry for non-Linux.
> 

Done

> > LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-apperance-basic.html:3
> > +<link rel="stylesheet" type="text/css" href="../resources/time-multiple-fields-apperance.css" />
> 
> Is this needed?
Yes. This CSS reduces image size by setting style for h1 and h2 elements.

> > LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-apperance-pseudo-elements.html:10
> > +    <li><span class="after">After</span><input type="time" value="12:34:56" class="after"></li>
> > +    <li><span class="before">Before</span><input type="time" value="12:34:56" class="before"></li>
> 
> Why do you set after/before to the <span> elements?
Removed. They were used for CSS selector matched.

> 
> > LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-apperance-pseudo-elements.html:11
> > +    <li><p id="first-letter">first letter</p><p id="first-letter"><input type="time" value="12:34:56" class="first-letter"></li></p>
> 
> Having multiple element with identical ID is not good.
Remove "p" elements. They aren't needed.

Created attachment 158760 [details]
Patch 3

Created attachment 158761 [details]
Patch 4

Comment on attachment 158761 [details]
Patch 4

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Fix typo of "appearance"
* Add time-multiple-fields-appearance-disabled-readonly.html for default appearance of disabled/readonly.
* Remove redundant elements in time-multiple-fields-appearance-pseudo-elements.html

[Kent Tamura]

Comment on attachment 158761 [details]
Patch 4

View in context: https://bugs.webkit.org/attachment.cgi?id=158761&action=review

Off topic: I feel we need some margin between the last field and the spin button.

> LayoutTests/ChangeLog:18
> +        * fast/forms/resources/time-multiple-fields-appearance.css: Added.

This should be put into fast/forms/time-multiple-fields/resources/.

> LayoutTests/ChangeLog:36
> +        * platform/chromium-linux/fast/forms/time-multiple-fields/time-multiple-fields-appearance-disabled-readonly-expected.png: Added.

Is this result expected? The readonly appearance is almost identical with the normal appearance except gray spinbutton.

> LayoutTests/fast/forms/resources/time-multiple-fields-appearance.css:90
> +    x-margin-bottom: -1px;

What's x-?

> LayoutTests/platform/chromium/TestExpectations:180
> +BUGWK94196 MAC WIN : fast/forms/time-multiple-fields/time-multiple-fields-appearance-disabled-readonly.html = IMAGE TEXT IMAGE+TEXT PASS
> +BUGWK94196 MAC WIN : fast/forms/time-multiple-fields/time-multiple-fields-appearance-pseudo-classes.html = IMAGE TEXT IMAGE+TEXT PASS
> +BUGWK94196 MAC WIN : fast/forms/time-multiple-fields/time-multiple-fields-appearance-style.html = IMAGE TEXT IMAGE+TEXT PASS

Need MISSING.

[Kent Tamura]

(In reply to comment #5)
> > > LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-apperance-basic.html:3
> > > +<link rel="stylesheet" type="text/css" href="../resources/time-multiple-fields-apperance.css" />
> > 
> > Is this needed?
> Yes. This CSS reduces image size by setting style for h1 and h2 elements.

Why does it reduce image size?

(In reply to comment #10)
> (In reply to comment #5)
> > > > LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-apperance-basic.html:3
> > > > +<link rel="stylesheet" type="text/css" href="../resources/time-multiple-fields-apperance.css" />
> > > 
> > > Is this needed?
> > Yes. This CSS reduces image size by setting style for h1 and h2 elements.
> 
> Why does it reduce image size?

Make font-size of h1 and h2 smaller.

[Kent Tamura]

(In reply to comment #11)
> > > > Is this needed?
> > > Yes. This CSS reduces image size by setting style for h1 and h2 elements.
> > 
> > Why does it reduce image size?
> 
> Make font-size of h1 and h2 smaller.

You can remove these h1 and h2 if you want to reduce the size.  They are not essential in the test.

Created attachment 158968 [details]
input type "number" with readonly attribute

For comparison with readonly multiple field time input UI appearance.

(In reply to comment #9)
> (From update of attachment 158761 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158761&action=review
> > LayoutTests/ChangeLog:36
> > +        * platform/chromium-linux/fast/forms/time-multiple-fields/time-multiple-fields-appearance-disabled-readonly-expected.png: Added.
> 
> Is this result expected? The readonly appearance is almost identical with the normal appearance except gray spinbutton.
> 
It seems this appearance is as same as "number" input type. Could you see the attachment which is screenshot of non-readonly and readonly "number" input type on Windows.

[Kent Tamura]

(In reply to comment #14)
> (In reply to comment #9)
> > (From update of attachment 158761 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=158761&action=review
> > > LayoutTests/ChangeLog:36
> > > +        * platform/chromium-linux/fast/forms/time-multiple-fields/time-multiple-fields-appearance-disabled-readonly-expected.png: Added.
> > 
> > Is this result expected? The readonly appearance is almost identical with the normal appearance except gray spinbutton.
> > 
> It seems this appearance is as same as "number" input type. Could you see the attachment which is screenshot of non-readonly and readonly "number" input type on Windows.

I see. I confirmed the default appearance of <input type=text readonly> was same as <input type=text>.  Please go ahead as is.

Created attachment 158989 [details]
Patch 5

Comment on attachment 158989 [details]
Patch 5

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Move appearance tests to bug 	 94287. This patch contains functional tests only.

[Kent Tamura]

Comment on attachment 158989 [details]
Patch 5

View in context: https://bugs.webkit.org/attachment.cgi?id=158989&action=review

> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-change-type-on-focus-expected.txt:1
> +Bug 46950: Assertion failure by changing type from type=time in focus event.

Bug 46950 is unrelated to type=time.

> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-keyboard-events.html:46
> +beginTest('Digit keys');
> +keyDown('7');
> +keyDown('5');
> +keyDown('6');
> +keyDown('A');
> +shouldBeEqualToString('input.value', '07:56');

We should test typeAheadTimeout behavior. You can use eventSender.leapForward().

Created attachment 159036 [details]
Patch 6

Comment on attachment 159036 [details]
Patch 6

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Update time-multiple-fields-change-type-on-focus-expected.txt following common-change-type-on-focus.js changes
* Add FIXME comment for type ahead timeout in time-multiple-fields-keyboard-events.html

[Kent Tamura]

Comment on attachment 159036 [details]
Patch 6

ok

Comment on attachment 159036 [details]
Patch 6

Clearing flags on attachment: 159036

Committed r125867: <http://trac.webkit.org/changeset/125867>

All reviewed patches have been landed.  Closing bug.