Bug 102861

Summary: Implement ValidityState::badInput
Product: WebKit Reporter: Kent Tamura <tkent>
Component: FormsAssignee: Kent Tamura <tkent>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, allan.jensen, cmarcelo, gyuyoung.kim, haraken, macpherson, menard, michelangelo, mifenton, morrita, ojan, rakuco, rwlbuis, syoichi, tonikitoo, webkit-ews, webkit.review.bot
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 103018, 103033    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch 2
none
Patch 3
none
Patch 4
none
Patch 5 morrita: review+

Description Kent Tamura 2012-11-20 19:21:55 PST 
The WHATWG HTML specification has introduced ValidityState::badInput. We should implement it.

http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#dom-validitystate-badinput
Comment 1 Kent Tamura 2012-11-25 22:18:48 PST 
Created attachment 175923 [details]
Patch
Comment 2 Kent Tamura 2012-11-25 22:25:13 PST 
Created attachment 175924 [details]
Patch 2

Added a missing file
Comment 3 Early Warning System Bot 2012-11-25 22:47:40 PST 
Comment on attachment 175924 [details]
Patch 2

Attachment 175924 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15000148
Comment 4 Early Warning System Bot 2012-11-25 22:49:04 PST 
Comment on attachment 175924 [details]
Patch 2

Attachment 175924 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14989467
Comment 5 Kent Tamura 2012-11-25 23:08:50 PST 
Created attachment 175931 [details]
Patch 3

Qt build fix
Comment 6 EFL EWS Bot 2012-11-25 23:24:51 PST 
Comment on attachment 175931 [details]
Patch 3

Attachment 175931 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14982537
Comment 7 Kent Tamura 2012-11-25 23:33:44 PST 
Created attachment 175933 [details]
Patch 4

EFL build fix
Comment 8 Build Bot 2012-11-26 01:05:02 PST 
Comment on attachment 175933 [details]
Patch 4

Attachment 175933 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14986501

New failing tests:
fast/forms/ValidityState-001.html
Comment 9 Kentaro Hara 2012-11-26 01:08:08 PST 
Comment on attachment 175933 [details]
Patch 4

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

Looks good.

morrita-san: would you take another look?

> LayoutTests/fast/forms/number/number-validity-badinput.html:13
> +<p id="description"></p>
> +<div id="console"></div>

Nit: Remove this.

> LayoutTests/fast/forms/number/number-validity-badinput.html:29
> +shouldBeTrue('colorOf(number) != invalidStyleColor');

Nit: Use !==, or ShouldNotBe('colorOf(number)', 'invalidStyleColor')

> LayoutTests/fast/forms/number/number-validity-badinput.html:41
> +shouldBeTrue('colorOf(number) != invalidStyleColor');

Ditto.

> LayoutTests/fast/forms/resources/multiple-fields-validity-badinput.js:28
> +    if (type == 'date' || type== 'datetime' || type == 'datetime-local') {

Nit: type == 'datetime'. === would be better.
Comment 10 Kent Tamura 2012-11-26 01:15:17 PST 
Created attachment 175940 [details]
Patch 5

Addressed reviewer comments. Fix a test failure.
Comment 11 Kent Tamura 2012-11-26 01:16:59 PST 
Comment on attachment 175933 [details]
Patch 4

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

>> LayoutTests/fast/forms/number/number-validity-badinput.html:13
>> +<div id="console"></div>
> 
> Nit: Remove this.

Fixed.
FYI: I wanted to minimize the difference from number-unacceptable-style.html.

>> LayoutTests/fast/forms/number/number-validity-badinput.html:29
>> +shouldBeTrue('colorOf(number) != invalidStyleColor');
> 
> Nit: Use !==, or ShouldNotBe('colorOf(number)', 'invalidStyleColor')

Fixed.

>> LayoutTests/fast/forms/number/number-validity-badinput.html:41
>> +shouldBeTrue('colorOf(number) != invalidStyleColor');
> 
> Ditto.

Fixed.

>> LayoutTests/fast/forms/resources/multiple-fields-validity-badinput.js:28
>> +    if (type == 'date' || type== 'datetime' || type == 'datetime-local') {
> 
> Nit: type == 'datetime'. === would be better.

Fixed.
Comment 12 Hajime Morrita 2012-11-27 00:55:14 PST 
Comment on attachment 175940 [details]
Patch 5

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

> Source/WebCore/ChangeLog:17
> +        interactive validation tells it.

Better to use consistent tense.

> Source/WebCore/ChangeLog:27
> +        will be added later.

The bug# might be worth being mentioned here.

> Source/WebCore/html/FormAssociatedElement.h:79
>      // stepMismatch, tooLong and valueMissing must call willValidate method.

Vaguely feel that it's time to fold these into one method which returns bit flags.
Comment 13 Kent Tamura 2012-11-27 01:10:07 PST 
Committed r135836: <http://trac.webkit.org/changeset/135836>
Comment 14 Kent Tamura 2012-11-27 01:21:24 PST 
Comment on attachment 175940 [details]
Patch 5

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

>> Source/WebCore/html/FormAssociatedElement.h:79
>>      // stepMismatch, tooLong and valueMissing must call willValidate method.
> 
> Vaguely feel that it's time to fold these into one method which returns bit flags.

MO, returning bit flags won't be nice.  When JS code gets input.validity.tooLong, we don't need to compute other validation states.
Comment 15 Kent Tamura 2012-11-27 01:22:05 PST 
(In reply to comment #14)
> MO, returning bit flags won't be nice.  When JS code gets input.validity.tooLong, we don't need to compute other validation states.

MO -> IMO