Bug 216311

Created attachment 408328 [details]
Patch

Comment on attachment 408328 [details]
Patch

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

> Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:229
> +    return date.second()

NIT: I wonder if these all would be better as early returns so we avoid `createStepRange` and `Decimal::fromDouble` if we do have the "fast" path of `date.second()` 🤔

> Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:239
> +    return date.millisecond()

ditto (:229)

> Source/WebCore/html/DateTimeFieldsState.h:44
> +            return { };

NIT: i prefer `WTF::nullopt` personally

> Source/WebCore/html/DateTimeLocalInputType.cpp:101
> -    return results.containsAll({ DateTimeFormatValidationResults::HasYear, DateTimeFormatValidationResults::HasMonth, DateTimeFormatValidationResults::HasDay, DateTimeFormatValidationResults::HasHour, DateTimeFormatValidationResults::HasMinute });
> +    return results.containsAll({ DateTimeFormatValidationResults::HasYear, DateTimeFormatValidationResults::HasMonth, DateTimeFormatValidationResults::HasDay, DateTimeFormatValidationResults::HasHour, DateTimeFormatValidationResults::HasMinute, DateTimeFormatValidationResults::HasMeridiem });

Is this an "oops" from a prior patch or something new?

> Source/WebCore/html/DateTimeLocalInputType.cpp:110
> +    auto hourMinuteString = makeString(pad('0', 2, *state.hour23()), ':', pad('0', 2, *state.minute));

Should this respect the preference of whether or not to use 24hr time?

Also, I wonder if you could use any of the `Locale` methods instead of hardcoding raw strings below?

> Source/WebCore/html/DateTimeLocalInputType.cpp:127
> +        layoutParameters.fallbackDateTimeFormat = "yyyy-MM-dd'T'HH:mm:ss"_s;

Should this include milliseconds if `layoutParameters.shouldHaveMillisecondField`?  I think that would be `"yyyy-MM-dd'T'HH:mm:ss.SSS`.

> Source/WebCore/html/DateTimeLocalInputType.cpp:130
> +        layoutParameters.fallbackDateTimeFormat = "yyyy-MM-dd'T'HH:mm"_s;

ditto (:110)

> Source/WebCore/html/TimeInputType.cpp:141
>          layoutParameters.fallbackDateTimeFormat = "HH:mm:ss"_s;

ditto (DateTimeLocalInputType.cpp:127)

> Source/WebKit/UIProcess/mac/WebDateTimePickerMac.mm:245
> +- (NSString *)dateFormatStringForType:(NSString *)type andValue:(NSString *)value

NIT: do we normally add `and`?

> Source/WebKit/UIProcess/mac/WebDateTimePickerMac.mm:249
> +        // Add two additional characters for the string delimiters in 'T'.
> +        auto valueLengthForFormat = value.length + 2;

Can you explain this a bit more?  Are the `'` not included in the `DateTimeChooserParameters`?

Also, I don't think we use `auto` for ObjC types 🤔

> LayoutTests/fast/forms/datetimelocal/datetimelocal-editable-components/datetimelocal-editable-components-focus-and-blur-events.html:15
> +input::-webkit-datetime-edit-year-field {

Can you combine all these rules into one?

> LayoutTests/fast/forms/datetimelocal/datetimelocal-editable-components/datetimelocal-editable-components-mouse-events.html:15
> +input::-webkit-datetime-edit-year-field {

ditto

Comment on attachment 408328 [details]
Patch

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

>> Source/WebKit/UIProcess/mac/WebDateTimePickerMac.mm:249
>> +        auto valueLengthForFormat = value.length + 2;
> 
> Can you explain this a bit more?  Are the `'` not included in the `DateTimeChooserParameters`?
> 
> Also, I don't think we use `auto` for ObjC types 🤔

It’s mostly a stylistic choice, but we can use auto for ObjC types, especially in cases where the type is apparent (e.g. `auto foo = [Foo foo];`).

(That said, in this particular case it’s just a POD rather than an ObjC type).

Created attachment 408374 [details]
Patch

(In reply to Devin Rousso from comment #2)
> Comment on attachment 408328 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408328&action=review
> 
> > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:229
> > +    return date.second()
> 
> NIT: I wonder if these all would be better as early returns so we avoid
> `createStepRange` and `Decimal::fromDouble` if we do have the "fast" path of
> `date.second()` 🤔

Good idea! Updated.

> > Source/WebCore/html/DateTimeFieldsState.h:44
> > +            return { };
> 
> NIT: i prefer `WTF::nullopt` personally

Changed to use WTF::nullopt.

> > Source/WebCore/html/DateTimeLocalInputType.cpp:101
> > -    return results.containsAll({ DateTimeFormatValidationResults::HasYear, DateTimeFormatValidationResults::HasMonth, DateTimeFormatValidationResults::HasDay, DateTimeFormatValidationResults::HasHour, DateTimeFormatValidationResults::HasMinute });
> > +    return results.containsAll({ DateTimeFormatValidationResults::HasYear, DateTimeFormatValidationResults::HasMonth, DateTimeFormatValidationResults::HasDay, DateTimeFormatValidationResults::HasHour, DateTimeFormatValidationResults::HasMinute, DateTimeFormatValidationResults::HasMeridiem });
> 
> Is this an "oops" from a prior patch or something new?

This was an oversight from a previous patch (where this code was never actually executed).
 
> > Source/WebCore/html/DateTimeLocalInputType.cpp:110
> > +    auto hourMinuteString = makeString(pad('0', 2, *state.hour23()), ':', pad('0', 2, *state.minute));
> 
> Should this respect the preference of whether or not to use 24hr time?

No, this method is only used to format the value of the fields into a valid HTML date/time value, which always uses 24-hour (00-23) time.
 
> Also, I wonder if you could use any of the `Locale` methods instead of
> hardcoding raw strings below?

These are fallback values in case the user's current locale does not have all the necessary fields. To avoid hardcoding, I would need to allocate an en-US locale to obtain the fallback strings.
 
> > Source/WebCore/html/DateTimeLocalInputType.cpp:127
> > +        layoutParameters.fallbackDateTimeFormat = "yyyy-MM-dd'T'HH:mm:ss"_s;
> 
> Should this include milliseconds if
> `layoutParameters.shouldHaveMillisecondField`?  I think that would be
> `"yyyy-MM-dd'T'HH:mm:ss.SSS`.

Not needed as the builder checks layoutParameters.shouldHaveMillisecondField when building the seconds field. It's done that way because none of the NSDateFormatterStyles include a millisecond field.  

> > Source/WebKit/UIProcess/mac/WebDateTimePickerMac.mm:245
> > +- (NSString *)dateFormatStringForType:(NSString *)type andValue:(NSString *)value
> 
> NIT: do we normally add `and`?

Removed the "and". Following https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingMethods.html.
 
> > Source/WebKit/UIProcess/mac/WebDateTimePickerMac.mm:249
> > +        // Add two additional characters for the string delimiters in 'T'.
> > +        auto valueLengthForFormat = value.length + 2;
> 
> Can you explain this a bit more?  Are the `'` not included in the
> `DateTimeChooserParameters`?

The value is DateTimeChooserParameters is a valid HTML date, and does not include the delimiters (example value: 2020-09-10T06:35). However, the format string needs to include delimiters for the literal 'T', and we will always have a two character difference.

> > LayoutTests/fast/forms/datetimelocal/datetimelocal-editable-components/datetimelocal-editable-components-focus-and-blur-events.html:15
> > +input::-webkit-datetime-edit-year-field {
> 
> Can you combine all these rules into one?

Done.

Comment on attachment 408374 [details]
Patch

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

r=me, thanks for answering my questions :)

also, nice tests!

> LayoutTests/platform/mac-wk2/imported/w3c/web-platform-tests/html/semantics/forms/the-form-element/form-elements-filter-expected.txt:6
> -                             
> +                              
>                                                  
> -                                                            
> +                                                              

o.0 ???

> LayoutTests/platform/mac-wk2/imported/w3c/web-platform-tests/html/semantics/selectors/pseudo-classes/inrange-outofrange-expected.txt:8
> -                                                    
> +                                                     

o.0 ???

(In reply to Devin Rousso from comment #6)
> Comment on attachment 408374 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408374&action=review
> 
> r=me, thanks for answering my questions :)
> 
> also, nice tests!

Thanks for the review!

> > LayoutTests/platform/mac-wk2/imported/w3c/web-platform-tests/html/semantics/forms/the-form-element/form-elements-filter-expected.txt:6
> > -                             
> > +                              
> >                                                  
> > -                                                            
> > +                                                              
> 
> o.0 ???

I think this is due to the larger size of the datetime-local input pushing the some of the content (other form controls) onto a new line.

Committed r266830: <https://trac.webkit.org/changeset/266830>

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

<rdar://problem/68647451>