Bug 216188 – [macOS] Add editability to input type=time

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

[Aditya Keerthi]

...

[Aditya Keerthi]

Created attachment 408017 [details]
Patch

[Sam Weinig]

Comment on attachment 408017 [details]
Patch

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

> Source/WebCore/html/DateTimeFieldsState.h:39
> +    enum AMPMValue {
> +        AMPMValueAM,
> +        AMPMValuePM,
> +    };

I think this would read cleaner as something like:

enum class Meridiem {
    AM,
    PM
};

[Wenson Hsieh]

Comment on attachment 408017 [details]
Patch

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

This is looking pretty great! Just a couple of minor comments as I continue to look.

> Source/WebCore/html/DateTimeFieldsState.h:36
> +    enum AMPMValue {

Nit - in new code, we typically prefer 8-bit-wide enum classes. For instance,

enum class AMPMValue : bool {
    …
};

> Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.cpp:122
> +    int index = m_typeAhead.handleEvent(&keyboardEvent, TypeAhead::MatchPrefix | TypeAhead::CycleFirstChar | TypeAhead::MatchIndex);

Not specific to this patch or anything, but we should make `TypeAhead::handleEvent` return an `Optional<int>` at some point.

[Devin Rousso]

Comment on attachment 408017 [details]
Patch

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

r=me, nice work!

> Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:313
> +    } else if (name == stepAttr && m_dateTimeEditElement) {
> +        updateInnerTextValue();
>      }

Style: no `{` `}` if the body is a single line

> Source/WebCore/html/DateTimeFieldsState.h:48
> +    Optional<AMPMValue> ampm;

IMO it would be nicer to read as `meridiem` (e.g. `-webkit-datetime-edit-meridiem-field`) than as `ampm`

> Source/WebCore/html/TimeInputType.cpp:121
> +    if (!state.hour || !state.minute || !state.ampm)

is it not possible (perhaps undesirable) to infer the `minute` and/or `meridiem` based on the `hour`?  e.g. if the `hour` is `20`, then `minute` is `0` and `meridien` is `PM`

> Source/WebCore/html/TimeInputType.cpp:140
> +    auto millisecondsPerSecond = Decimal::fromDouble(msPerSecond);
> +    auto millisecondsPerMinute = Decimal::fromDouble(msPerMinute);

<unrelated to this patch> would be neat if `Decimal` were `constexpr` :P

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

`_s`

> Source/WebCore/html/TimeInputType.cpp:156
> +        layoutParameters.fallbackDateTimeFormat = "HH:mm";

`_s`

> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:110
> +    case 11: {

NIT: none of these `case` actually need the `{` `}` since they don't declare any variables

> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:111
> +        state.hour = value ? value : 12;

<after reading below> Could we use `value ?: 12` here too?

> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:119
> +        state.hour = (value % 12) ?: 12;

`?:` 🤯

> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:129
> +        state.hour = value == 12 ? 12 : value % 12;

Why not also do `(value % 12) ?: 12` here too?

> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:141
> +    case 11: {

ditto (:110)

[Aditya Keerthi]

(In reply to Devin Rousso from comment #4)
> Comment on attachment 408017 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408017&action=review
> 
> r=me, nice work!

Thanks for the review! I'm going to address one of the failing WPT tests in a separate patch before landing this one, as it should also apply to date inputs.

> > Source/WebCore/html/TimeInputType.cpp:121
> > +    if (!state.hour || !state.minute || !state.ampm)
> 
> is it not possible (perhaps undesirable) to infer the `minute` and/or
> `meridiem` based on the `hour`?  e.g. if the `hour` is `20`, then `minute`
> is `0` and `meridien` is `PM`

DateTimeFieldsState will always have an `hour` between 1 and 12 (even if the user's locale has a different format, the value is normalized). If any one of these fields is empty, we return an empty string, matching the behavior of date/time inputs in other browsers.

[Aditya Keerthi]

Created attachment 408269 [details]
Patch for landing

[EWS]

Committed r266779: <https://trac.webkit.org/changeset/266779>

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

[Radar WebKit Bug Importer]

<rdar://problem/68571901>

[Ryosuke Niwa]

Comment on attachment 408269 [details]
Patch for landing

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

> Source/WebCore/html/shadow/DateTimeFieldElements.h:61
> +    virtual void populateDateTimeFieldsState(DateTimeFieldsState&);
> +    virtual void setValueAsDate(const DateComponents&);

These should be override instead of virtual.
Also, why are the orders different?

> Source/WebCore/html/shadow/DateTimeFieldElements.h:75
> +    void populateDateTimeFieldsState(DateTimeFieldsState&);
> +    void setValueAsDate(const DateComponents&);

Ditto.

> Source/WebCore/html/shadow/DateTimeFieldElements.h:89
> +    virtual void populateDateTimeFieldsState(DateTimeFieldsState&);
> +    virtual void setValueAsDate(const DateComponents&);

Ditto.

> Source/WebCore/html/shadow/DateTimeFieldElements.h:103
> +    virtual void populateDateTimeFieldsState(DateTimeFieldsState&);
> +    virtual void setValueAsDate(const DateComponents&);

Ditto.

[Aditya Keerthi]

Opened a new bug to address post-review comments: https://bugs.webkit.org/show_bug.cgi?id=216339.