Bug 94196 - [Tests] Adding tests for multiple fields time input UI
Summary: [Tests] Adding tests for multiple fields time input UI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yosin
URL:
Keywords:
Depends on: 94287 94293
Blocks: 94195
  Show dependency treegraph
 
Reported: 2012-08-16 00:33 PDT by yosin
Modified: 2012-08-17 01:13 PDT (History)
1 user (show)

See Also:


Attachments
Patch 1 (82.93 KB, patch)
2012-08-16 00:41 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 2 (142.83 KB, patch)
2012-08-16 02:41 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 3 (143.09 KB, patch)
2012-08-16 03:01 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 4 (143.01 KB, patch)
2012-08-16 03:04 PDT, yosin
no flags Details | Formatted Diff | Diff
input type "number" with readonly attribute (34.29 KB, image/png)
2012-08-16 18:31 PDT, yosin
no flags Details
Patch 5 (16.18 KB, patch)
2012-08-16 21:22 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 6 (16.58 KB, patch)
2012-08-17 01:06 PDT, yosin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yosin 2012-08-16 00:33:15 PDT
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.
Comment 1 yosin 2012-08-16 00:41:56 PDT
Created attachment 158735 [details]
Patch 1
Comment 2 yosin 2012-08-16 00:42:23 PDT
Comment on attachment 158735 [details]
Patch 1

Could you review this patch?
Thanks in advance.
Comment 3 Kent Tamura 2012-08-16 01:46:16 PDT
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.
Comment 4 yosin 2012-08-16 02:41:00 PDT
Created attachment 158758 [details]
Patch 2
Comment 5 yosin 2012-08-16 02:43:20 PDT
(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.
Comment 6 yosin 2012-08-16 03:01:08 PDT
Created attachment 158760 [details]
Patch 3
Comment 7 yosin 2012-08-16 03:04:40 PDT
Created attachment 158761 [details]
Patch 4
Comment 8 yosin 2012-08-16 03:06:32 PDT
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
Comment 9 Kent Tamura 2012-08-16 03:20:57 PDT
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.
Comment 10 Kent Tamura 2012-08-16 03:25:36 PDT
(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?
Comment 11 yosin 2012-08-16 03:26:36 PDT
(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.
Comment 12 Kent Tamura 2012-08-16 03:32:23 PDT
(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.
Comment 13 yosin 2012-08-16 18:31:40 PDT
Created attachment 158968 [details]
input type "number" with readonly attribute

For comparison with readonly multiple field time input UI appearance.
Comment 14 yosin 2012-08-16 18:33:37 PDT
(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.
Comment 15 Kent Tamura 2012-08-16 18:46:50 PDT
(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.
Comment 16 yosin 2012-08-16 21:22:06 PDT
Created attachment 158989 [details]
Patch 5
Comment 17 yosin 2012-08-16 21:23:20 PDT
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.
Comment 18 Kent Tamura 2012-08-16 21:50:52 PDT
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().
Comment 19 yosin 2012-08-17 01:06:08 PDT
Created attachment 159036 [details]
Patch 6
Comment 20 yosin 2012-08-17 01:08:09 PDT
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
Comment 21 Kent Tamura 2012-08-17 01:11:20 PDT
Comment on attachment 159036 [details]
Patch 6

ok
Comment 22 yosin 2012-08-17 01:13:15 PDT
Comment on attachment 159036 [details]
Patch 6

Clearing flags on attachment: 159036

Committed r125867: <http://trac.webkit.org/changeset/125867>
Comment 23 yosin 2012-08-17 01:13:22 PDT
All reviewed patches have been landed.  Closing bug.