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