Bug 96714 - [Forms] multiple fields time input UI should save/restore its value even if it has an empty field.
Summary: [Forms] multiple fields time input UI should save/restore its value even if i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yosin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-13 18:09 PDT by yosin
Modified: 2012-09-14 03:08 PDT (History)
5 users (show)

See Also:


Attachments
Proof Of Concept (43.99 KB, patch)
2012-09-13 23:40 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 1 (40.31 KB, patch)
2012-09-14 01:59 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 2 (42.02 KB, patch)
2012-09-14 03:00 PDT, yosin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yosin 2012-09-13 18:09:45 PDT
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.
Comment 1 yosin 2012-09-13 23:40:37 PDT
Created attachment 164056 [details]
Proof Of Concept
Comment 2 yosin 2012-09-13 23:41:48 PDT
Comment on attachment 164056 [details]
Proof Of Concept

Could you review this change as architecture review?
Thanks in advance.
Comment 3 Peter Beverloo (cr-android ews) 2012-09-14 00:19:33 PDT
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
Comment 4 yosin 2012-09-14 01:59:02 PDT
Created attachment 164073 [details]
Patch 1
Comment 5 yosin 2012-09-14 02:01:13 PDT
Comment on attachment 164073 [details]
Patch 1

Could you review this patch?
Thanks in advance.
Comment 6 Kent Tamura 2012-09-14 02:27:04 PDT
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.
Comment 7 Kent Tamura 2012-09-14 02:28:54 PDT
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?
Comment 8 yosin 2012-09-14 03:00:44 PDT
Created attachment 164082 [details]
Patch 2
Comment 9 yosin 2012-09-14 03:03:09 PDT
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
Comment 10 Kent Tamura 2012-09-14 03:07:19 PDT
Comment on attachment 164082 [details]
Patch 2

ok
Comment 11 yosin 2012-09-14 03:08:38 PDT
Comment on attachment 164082 [details]
Patch 2

Clearing flags on attachment: 164082

Committed r128575: <http://trac.webkit.org/changeset/128575>
Comment 12 yosin 2012-09-14 03:08:42 PDT
All reviewed patches have been landed.  Closing bug.