Bug 229773 – [iOS] Simplify date picker logic for datetime-local inputs

• Component: Forms
• Status: RESOLVED FIXED

[Aditya Keerthi]

Share more code between datetime-local inputs and other input types in the date picker logic.

Remove unnecessary time zone conversions.

[Aditya Keerthi]

Created attachment 437071 [details]
Patch

[Darin Adler]

Comment on attachment 437071 [details]
Patch

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

> Source/WebKit/UIProcess/ios/forms/WKDateTimeInputControl.mm:563
> +        NSString *timeString = [[value componentsSeparatedByString:@"T"] objectAtIndex:1];

This will throw an uncaught Objective-C exception and crash if there is no "T" at all in the string or if the only "T" is the last character.

> Source/WebKit/UIProcess/ios/forms/WKDateTimeInputControl.mm:564
> +        NSString *sanitizedTimeString = [timeString substringToIndex:[kTimeFormatString length]];

This will throw an uncaught Objective-C exception and crash if timeString is shorter than [kTimeFormatString length].

Same problem with the existing code above; not sure why this isn’t a problem in practice.

> Source/WebKit/UIProcess/ios/forms/WKDateTimeInputControl.mm:596
> +    RetainPtr<NSDateFormatter> dateFormatter = [self dateFormatterForPicker];

Should consider just using auto here. But also, why use a local variable at all?

[Aditya Keerthi]

Created attachment 437092 [details]
Patch

[Aditya Keerthi]

(In reply to Darin Adler from comment #2)
> Comment on attachment 437071 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437071&action=review
> 
> > Source/WebKit/UIProcess/ios/forms/WKDateTimeInputControl.mm:563
> > +        NSString *timeString = [[value componentsSeparatedByString:@"T"] objectAtIndex:1];
> 
> This will throw an uncaught Objective-C exception and crash if there is no
> "T" at all in the string or if the only "T" is the last character.
> 
> > Source/WebKit/UIProcess/ios/forms/WKDateTimeInputControl.mm:564
> > +        NSString *sanitizedTimeString = [timeString substringToIndex:[kTimeFormatString length]];
> 
> This will throw an uncaught Objective-C exception and crash if timeString is
> shorter than [kTimeFormatString length].
> 
> Same problem with the existing code above; not sure why this isn’t a problem
> in practice.

The value attribute of a time input will always be empty or contain a prefix of the format "HH:mm". Similarly, the value attribute of a datetime-local input will either be empty of contain a prefix of the format "yyyy-MM-dd'T'HH:mm". The value is passed in to this method is always non-empty, hence we're guaranteed those formats by the HTML spec.

I've added an ASSERT([value length]) to make the non-empty case clear. Please let me know if you think addition assertions would be helpful.
 
> > Source/WebKit/UIProcess/ios/forms/WKDateTimeInputControl.mm:596
> > +    RetainPtr<NSDateFormatter> dateFormatter = [self dateFormatterForPicker];
> 
> Should consider just using auto here. But also, why use a local variable at
> all?

Removed the local variable.

[Aditya Keerthi]

Created attachment 437093 [details]
Patch

[Darin Adler]

Comment on attachment 437071 [details]
Patch

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

>>> Source/WebKit/UIProcess/ios/forms/WKDateTimeInputControl.mm:563
>>> +        NSString *timeString = [[value componentsSeparatedByString:@"T"] objectAtIndex:1];
>> 
>> This will throw an uncaught Objective-C exception and crash if there is no "T" at all in the string or if the only "T" is the last character.
> 
> The value attribute of a time input will always be empty or contain a prefix of the format "HH:mm". Similarly, the value attribute of a datetime-local input will either be empty of contain a prefix of the format "yyyy-MM-dd'T'HH:mm". The value is passed in to this method is always non-empty, hence we're guaranteed those formats by the HTML spec.
> 
> I've added an ASSERT([value length]) to make the non-empty case clear. Please let me know if you think addition assertions would be helpful.

I take your for for it that these values will be well formed.

I don’t understand when you say it is guaranteed "by the HTML spec"; surely it’s not the HTML specification that provides those guarantees in practice! The guarantee must come from where the string comes from, and how it constructs it.

[Aditya Keerthi]

(In reply to Darin Adler from comment #6)
> Comment on attachment 437071 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437071&action=review
> 
> >>> Source/WebKit/UIProcess/ios/forms/WKDateTimeInputControl.mm:563
> >>> +        NSString *timeString = [[value componentsSeparatedByString:@"T"] objectAtIndex:1];
> >> 
> >> This will throw an uncaught Objective-C exception and crash if there is no "T" at all in the string or if the only "T" is the last character.
> > 
> > The value attribute of a time input will always be empty or contain a prefix of the format "HH:mm". Similarly, the value attribute of a datetime-local input will either be empty of contain a prefix of the format "yyyy-MM-dd'T'HH:mm". The value is passed in to this method is always non-empty, hence we're guaranteed those formats by the HTML spec.
> > 
> > I've added an ASSERT([value length]) to make the non-empty case clear. Please let me know if you think addition assertions would be helpful.
> 
> I take your for for it that these values will be well formed.
> 
> I don’t understand when you say it is guaranteed "by the HTML spec"; surely
> it’s not the HTML specification that provides those guarantees in practice!
> The guarantee must come from where the string comes from, and how it
> constructs it.

Apologies for the poor explanation.

The string comes from HTMLInputElement::value(), which, in compliance with the specification,  returns either an empty string or the appropriately formatted strings described earlier.

[Darin Adler]

Comment on attachment 437093 [details]
Patch

Since this relies on the format of the string, I think we should consider comments in the code that guarantees the format, pointing here at code that depends on the format, in case we are changing it in the future.

[Aditya Keerthi]

Created attachment 437194 [details]
Patch for landing

[Aditya Keerthi]

(In reply to Darin Adler from comment #8)
> Comment on attachment 437093 [details]
> Patch
> 
> Since this relies on the format of the string, I think we should consider
> comments in the code that guarantees the format, pointing here at code that
> depends on the format, in case we are changing it in the future.

Due to the level of indirection involved, I’m not sure where to put such a comment. 

The value is guaranteed by HTMLInputElement::sanitizeValue -> BaseDateAndTimeInputType::typeMismatchFor -> BaseDateAndTimeInputType::parseToDateComponents -> DateComponents::fromParsing* -> DateComponents::parse*.

Additionally, it feels strange to point to UIProcess code from inside WebCore.

I did notice we were missing some links to the specification at the lowest level, so I’ve added those. But I understand that doesn’t address your concern.

Happy to address in a follow-up if you have any recommendations, or would prefer a more defensive style to avoid relying on the format.

[Darin Adler]

(In reply to Aditya Keerthi from comment #10)
> Additionally, it feels strange to point to UIProcess code from inside
> WebCore.

This is exactly why we need the comment. Someone modifying WebCore because of an update to a specification would never dream that there is code depending on this in WebKit. Comments need not obey architecture boundaries and layering, which is why they are so useful!

[EWS]

Committed r281955 (241262@main): <https://commits.webkit.org/241262@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/82694572>

[Aditya Keerthi]

(In reply to Darin Adler from comment #11)
> (In reply to Aditya Keerthi from comment #10)
> > Additionally, it feels strange to point to UIProcess code from inside
> > WebCore.
> 
> This is exactly why we need the comment. Someone modifying WebCore because
> of an update to a specification would never dream that there is code
> depending on this in WebKit. Comments need not obey architecture boundaries
> and layering, which is why they are so useful!

It's also worth noting that it's highly unlikely (certainly undesirable) the format/spec changes, since it would cause compatibility issues on websites.

[Aditya Keerthi]

(In reply to Aditya Keerthi from comment #14)
> (In reply to Darin Adler from comment #11)
> > (In reply to Aditya Keerthi from comment #10)
> > > Additionally, it feels strange to point to UIProcess code from inside
> > > WebCore.
> > 
> > This is exactly why we need the comment. Someone modifying WebCore because
> > of an update to a specification would never dream that there is code
> > depending on this in WebKit. Comments need not obey architecture boundaries
> > and layering, which is why they are so useful!
> 
> It's also worth noting that it's highly unlikely (certainly undesirable) the
> format/spec changes, since it would cause compatibility issues on websites.

That said, how does adding the comment "Any changes to the following parsing logic should be made with caution, as date pickers in the UIProcess depend on the existing format." to each of the parsing methods in DateComponents.cpp sound?

[Darin Adler]

(In reply to Aditya Keerthi from comment #14)
> It's also worth noting that it's highly unlikely (certainly undesirable) the
> format/spec changes, since it would cause compatibility issues on websites.

OK, if that’s right, the comment isn’t needed.