Summary: | iOS: setting 'defaultValue' of input type=date from script does not cause UI update | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Stephen McGruer <smcgruer> | ||||
Component: | Forms | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | ajuma, cdumez, commit-queue, darin, jonlee, simon.fraser, webkit-bug-importer, wenson_hsieh | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | iPhone / iPad | ||||||
OS: | iOS 9.3 | ||||||
Attachments: |
|
Description
Stephen McGruer
2018-05-25 09:01:08 PDT
Created attachment 341301 [details]
Patch
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? (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 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.
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? (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+. 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 on attachment 341301 [details] Patch Clearing flags on attachment: 341301 Committed r232289: <https://trac.webkit.org/changeset/232289> All reviewed patches have been landed. Closing bug. |