Bug 185982 - iOS: setting 'defaultValue' of input type=date from script does not cause UI update
Summary: iOS: setting 'defaultValue' of input type=date from script does not cause UI ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: WebKit Nightly Build
Hardware: iPhone / iPad iOS 9.3
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-25 09:01 PDT by Stephen McGruer
Modified: 2018-05-30 06:36 PDT (History)
8 users (show)

See Also:


Attachments
Patch (5.86 KB, patch)
2018-05-25 10:49 PDT, Stephen McGruer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen McGruer 2018-05-25 09:01:08 PDT
What steps will reproduce the problem?
(1) Create an input type=date
(2) From script, set the 'defaultValue' to a value
(3) Observe the input element's rendered UI

What is the expected result?

The UI should update to show the default value (since there was no previous selection). This happens on desktop Chrome.

What happens instead?

The UI doesn't update; the date picker is blank. However if you select it, the date picker widget appears and shows that the default value is set.


See test case http://output.jsbin.com/vocunov 

Note that this comes from a React bug that they found and worked around, sadly without informing browser vendors at the time :( - https://github.com/facebook/react/issues/7233

We recently fixed this in Chromium Android (http://crbug.com/838898), and I have a similar patch (to be uploaded shortly) which fixes it in WebKit iOS.
Comment 1 Stephen McGruer 2018-05-25 10:49:21 PDT
Created attachment 341301 [details]
Patch
Comment 2 Stephen McGruer 2018-05-25 10:50:03 PDT
First version of the patch has been added to this bug. I am not sure how to mark a LayoutTest as needing to be run on iOS - is that automatic in WebKit?
Comment 3 Chris Dumez 2018-05-25 10:53:13 PDT
(In reply to Stephen McGruer from comment #2)
> First version of the patch has been added to this bug. I am not sure how to
> mark a LayoutTest as needing to be run on iOS - is that automatic in WebKit?

if the iOS-sim bubble becomes green, then your new test is passing on iOS.
Comment 4 Darin Adler 2018-05-28 20:27:02 PDT
Comment on attachment 341301 [details]
Patch

Nice to fix this.

Patch looks OK but not ideal.

There’s already a general purpose InputType::attributeChanged function, so there is no need to add the valueAttributeChanged function. I fact, we should probably consider removing the other InputType xxxAttributeChanged functions for the various specific attributes. They are redundant, bloat the HTMLInputElement::parseAttribute function and the InputType base class unnecessarily, and cause additional unneeded work for other input types.

Note that the same issue comes up with plain text input fields; for those, TextFieldInputType::attributeChanged handles it. It’s a bit untidy to do almost the same thing for the same reason for the date/time fields, but write the code differently and separately. I assume this comes about because the programmers doing the date/time code weren’t aware of the already-existing text field code. The updateAppearance function has the same purpose as the updateInnerTextValue function, but uses a different name.

From code inspection, I notice a similar but distinct problem if the lang attribute of the element is changed, or the lang attribute of an ancestor element if it’s the ancestor that happens to determine the computed language (nearest ancestor with a lang attribute). BaseDateAndTimeInputType::localizeValue will format the value based on the locale, which depends on the computed language value, and nothing will cause BaseChooserOnlyDateAndTimeInputType::updateAppearance to be called when the locale changes.

Another similar but distinct problem can occur if the document’s content language is changed by adding a <meta> element to the document with an "http-equiv" attribute in it and there are no "lang" attributes.

Would be nice if we handled all these cases properly.
Comment 5 Stephen McGruer 2018-05-29 10:15:53 PDT
Thank you for the quick and thorough review Darin. Before I respond to the majority of your review, can I check - you marked the patch as 'review+', are you happy for this version of the patch to be landed and your comments left as future work (probably for others working in this area, not myself)? Or would you prefer if we attempted to resolve your comments in this same patch?
Comment 6 Darin Adler 2018-05-29 21:40:23 PDT
(In reply to Stephen McGruer from comment #5)
> Thank you for the quick and thorough review Darin. Before I respond to the
> majority of your review, can I check - you marked the patch as 'review+',
> are you happy for this version of the patch to be landed and your comments
> left as future work (probably for others working in this area, not myself)?
> Or would you prefer if we attempted to resolve your comments in this same
> patch?

Either way is OK with me. That’s why I said review+.
Comment 7 Darin Adler 2018-05-29 21:41:52 PDT
I think the issues with language are going to be both challenging to fix efficiently and also challenging to test in a platform-independent way.
Comment 8 WebKit Commit Bot 2018-05-30 06:35:55 PDT
Comment on attachment 341301 [details]
Patch

Clearing flags on attachment: 341301

Committed r232289: <https://trac.webkit.org/changeset/232289>
Comment 9 WebKit Commit Bot 2018-05-30 06:35:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2018-05-30 06:36:20 PDT
<rdar://problem/40648857>