Bug 96714 – [Forms] multiple fields time input UI should save/restore its value even if it has an empty field.

• Component: Forms
• Status: RESOLVED FIXED

If user populates partial values into multiple fields time input UI, e.g. "--:12:-- AM". Back to a page navigation resets all fields empty unless all fields have value.

This behavior makes users to enter all fields again. So, we should save/restore all fields regardless field has value or not.

Created attachment 164056 [details]
Proof Of Concept

Comment on attachment 164056 [details]
Proof Of Concept

Could you review this change as architecture review?
Thanks in advance.

[Peter Beverloo (cr-android ews)]

Comment on attachment 164056 [details]
Proof Of Concept

Attachment 164056 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13848427

Created attachment 164073 [details]
Patch 1

Comment on attachment 164073 [details]
Patch 1

Could you review this patch?
Thanks in advance.

[Kent Tamura]

Comment on attachment 164073 [details]
Patch 1

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

You need to increment the version number in FormCotroller.cpp formStateSignature() because this is an incompatible change.

> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:146
> +        DateTimeNumericFieldElement::setValueAsInteger(hour12);

We need to do something for out-of-range values.
The hour12 value comes from a serialized state, and it might be corrupted.

> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:156
> +            DateTimeNumericFieldElement::setValueAsInteger(hour12 == 12 ? 12 : hour12 + 12);

ditto.

> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:158
> +            DateTimeNumericFieldElement::setValueAsInteger(hour12);

ditto.

> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:209
> +        setValueAsInteger(dateTimeFieldsState.millisecond());

ditto.

> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:247
> +        setValueAsInteger(dateTimeFieldsState.minute());

ditto.

> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:285
> +        setValueAsInteger(dateTimeFieldsState.second());

ditto.

[Kent Tamura]

Comment on attachment 164073 [details]
Patch 1

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

> Source/WebCore/html/TimeInputType.cpp:270
> +    DateTimeFieldsState fields = DateTimeFieldsState::restoreFormControlState(state);

nit: the variable name should be fieldsState or something.

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:414
> +void DateTimeEditElement::setValueAsDateTimeFieldsState(const DateTimeFieldsState& dateTimeFields, const DateComponents& dateForReadOnlyField)

ditto.  dateTiemFieldsState?

Created attachment 164082 [details]
Patch 2

Comment on attachment 164082 [details]
Patch 2

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Increment version number in formControlSignatre() in FormController.cpp
* Changes variable name to dateTimeFieldsState for DateTimeFieldsState
* DateTimeFieldElements.cpp: Checks incoming value from DateTimeFieldsState in setValueAsDateTimeFieldsState

[Kent Tamura]

Comment on attachment 164082 [details]
Patch 2

ok

Comment on attachment 164082 [details]
Patch 2

Clearing flags on attachment: 164082

Committed r128575: <http://trac.webkit.org/changeset/128575>

All reviewed patches have been landed.  Closing bug.