Bug 234039 – Improve <type="datetime-local"> value parsing and sanitization

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Chris Dumez]

Allow using a space as date / time separator in <type="datetime-local">, instead of simply allowing a 'T'. This would align our behavior with Gecko and allow us to pass more tests in WPT.

[Chris Dumez]

Created attachment 446453 [details]
Patch

[Chris Dumez]

Created attachment 446560 [details]
Patch

[Chris Dumez]

Created attachment 446583 [details]
Patch

[Chris Dumez]

Created attachment 446628 [details]
Patch

[Darin Adler]

Comment on attachment 446628 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        This would align our behavior with Gecko and allow us to pass more tests in WPT.

What about specification?

> Source/WebCore/ChangeLog:21
> +        - The output will use the shortest possible string, omitting seconds or milliseconds when 0, as per
> +          https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#valid-normalised-local-date-and-time-string

Good, but not mentioned in the bug title.

> Source/WebCore/ChangeLog:27
> +        Fix bug where we would allow more than 3 digits for the millisecond part of the time (we
> +        would silently ignore follow-up digits instead of failing parsing). This is as per:

Good, but not mentioned in the bug title.

> Source/WebCore/ChangeLog:38
> +        The output will use the shortest possible string, omitting seconds or milliseconds when 0, as per
> +        https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#valid-normalised-local-date-and-time-string

Good, but not mentioned in the bug title.

> Source/WebCore/platform/DateComponents.cpp:483
>                          // Regardless of the number of digits, we only ever parse at most 3. All other
>                          // digits after that are ignored, but the buffer is incremented as if they were
>                          // all parsed.

This comment does not make sense any more and should be deleted.

[Chris Dumez]

Comment on attachment 446628 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        This would align our behavior with Gecko and allow us to pass more tests in WPT.
> 
> What about specification?

Specification allows a space or a 'T' (in line with WPT test):
https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#valid-local-date-and-time-string

Will clarify the change log.

>> Source/WebCore/ChangeLog:38
>> +        https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#valid-normalised-local-date-and-time-string
> 
> Good, but not mentioned in the bug title.

I guess I will need a more generic bug title.

>> Source/WebCore/platform/DateComponents.cpp:483
>>                          // all parsed.
> 
> This comment does not make sense any more and should be deleted.

OK.

[Chris Dumez]

Created attachment 446642 [details]
Patch

[Chris Dumez]

Created attachment 446746 [details]
Patch

[Darin Adler]

Comment on attachment 446746 [details]
Patch

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

> LayoutTests/platform/ios-wk2/imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-validity-valueMissing-expected.txt:87
> -PASS [INPUT in RADIO status] The checked attribute is false
> +FAIL [INPUT in RADIO status] The checked attribute is false assert_true: The validity.valueMissing should be true. expected true got false

Regression, not progression?

[Chris Dumez]

Comment on attachment 446746 [details]
Patch

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

>> LayoutTests/platform/ios-wk2/imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-validity-valueMissing-expected.txt:87
>> +FAIL [INPUT in RADIO status] The checked attribute is false assert_true: The validity.valueMissing should be true. expected true got false
> 
> Regression, not progression?

Weird, no such failure in the Mac output. I'll investigate.

Thanks for catching.

[Chris Dumez]

Created attachment 446771 [details]
Patch

[Chris Dumez]

It was just a bad baseline because I had a patch impacting the same test that landed super recently. No issue with the code change itself.

[EWS]

Committed r286869 (245100@main): <https://commits.webkit.org/245100@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446771 [details].

[Radar WebKit Bug Importer]

<rdar://problem/86335445>