...
Created attachment 405979 [details] Patch
Created attachment 405980 [details] Patch
Created attachment 406019 [details] Patch
Comment on attachment 406019 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406019&action=review > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:165 > +#if PLATFORM(MAC) > + if (!m_dateTimeEditElement) > + return; > + > + DateTimeEditElement::LayoutParameters layoutParameters(element()->locale()); > + setupLayoutParameters(layoutParameters); > + > + if (!DateTimeFormatValidator().validateFormat(layoutParameters.dateTimeFormat, *this)) > + layoutParameters.dateTimeFormat = layoutParameters.fallbackDateTimeFormat; > + > + auto date = parseToDateComponents(element()->value()); > + if (!date) > + m_dateTimeEditElement->setEmptyValue(layoutParameters); > + else > + m_dateTimeEditElement->setValueAsDate(layoutParameters, *date); > +#else We should try to figure out a way to do this without adding PLATFORM(MAC) checks in here. Ideally, this would be something that can exist on all platforms, controlled at runtime, and enabled for Mac. At the very least, we should figure out how to make this theme based.
(In reply to Sam Weinig from comment #4) > Comment on attachment 406019 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=406019&action=review > > > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:165 > > +#if PLATFORM(MAC) > > + if (!m_dateTimeEditElement) > > + return; > > + > > + DateTimeEditElement::LayoutParameters layoutParameters(element()->locale()); > > + setupLayoutParameters(layoutParameters); > > + > > + if (!DateTimeFormatValidator().validateFormat(layoutParameters.dateTimeFormat, *this)) > > + layoutParameters.dateTimeFormat = layoutParameters.fallbackDateTimeFormat; > > + > > + auto date = parseToDateComponents(element()->value()); > > + if (!date) > > + m_dateTimeEditElement->setEmptyValue(layoutParameters); > > + else > > + m_dateTimeEditElement->setValueAsDate(layoutParameters, *date); > > +#else > > We should try to figure out a way to do this without adding PLATFORM(MAC) > checks in here. Ideally, this would be something that can exist on all > platforms, controlled at runtime, and enabled for Mac. > > At the very least, we should figure out how to make this theme based. What do you think of replacing the PLATFORM(MAC) check here with a RuntimeEnabledFeatures check? This method is the only one that will require such a check – and the other PLATFORM(MAC) checks can be removed.
(In reply to Aditya Keerthi from comment #5) > (In reply to Sam Weinig from comment #4) > > Comment on attachment 406019 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=406019&action=review > > > > > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:165 > > > +#if PLATFORM(MAC) > > > + if (!m_dateTimeEditElement) > > > + return; > > > + > > > + DateTimeEditElement::LayoutParameters layoutParameters(element()->locale()); > > > + setupLayoutParameters(layoutParameters); > > > + > > > + if (!DateTimeFormatValidator().validateFormat(layoutParameters.dateTimeFormat, *this)) > > > + layoutParameters.dateTimeFormat = layoutParameters.fallbackDateTimeFormat; > > > + > > > + auto date = parseToDateComponents(element()->value()); > > > + if (!date) > > > + m_dateTimeEditElement->setEmptyValue(layoutParameters); > > > + else > > > + m_dateTimeEditElement->setValueAsDate(layoutParameters, *date); > > > +#else > > > > We should try to figure out a way to do this without adding PLATFORM(MAC) > > checks in here. Ideally, this would be something that can exist on all > > platforms, controlled at runtime, and enabled for Mac. > > > > At the very least, we should figure out how to make this theme based. > > What do you think of replacing the PLATFORM(MAC) check here with a > RuntimeEnabledFeatures check? This method is the only one that will require > such a check – and the other PLATFORM(MAC) checks can be removed. What would the name of check be? Why does this need to be conditionalized at all?
(In reply to Sam Weinig from comment #6) > (In reply to Aditya Keerthi from comment #5) > > (In reply to Sam Weinig from comment #4) > > > Comment on attachment 406019 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=406019&action=review > > > > > > > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:165 > > > > +#if PLATFORM(MAC) > > > > + if (!m_dateTimeEditElement) > > > > + return; > > > > + > > > > + DateTimeEditElement::LayoutParameters layoutParameters(element()->locale()); > > > > + setupLayoutParameters(layoutParameters); > > > > + > > > > + if (!DateTimeFormatValidator().validateFormat(layoutParameters.dateTimeFormat, *this)) > > > > + layoutParameters.dateTimeFormat = layoutParameters.fallbackDateTimeFormat; > > > > + > > > > + auto date = parseToDateComponents(element()->value()); > > > > + if (!date) > > > > + m_dateTimeEditElement->setEmptyValue(layoutParameters); > > > > + else > > > > + m_dateTimeEditElement->setValueAsDate(layoutParameters, *date); > > > > +#else > > > > > > We should try to figure out a way to do this without adding PLATFORM(MAC) > > > checks in here. Ideally, this would be something that can exist on all > > > platforms, controlled at runtime, and enabled for Mac. > > > > > > At the very least, we should figure out how to make this theme based. > > > > What do you think of replacing the PLATFORM(MAC) check here with a > > RuntimeEnabledFeatures check? This method is the only one that will require > > such a check – and the other PLATFORM(MAC) checks can be removed. > > What would the name of check be? Why does this need to be conditionalized at > all? (but yes, that does seem like the right direction, though I would Settings instead of RuntimeEnabledFeatures. RuntimeEnabledFeatures should only be used as a last resort if you don't have access to the Page or Document. RuntimeEnabledFeatures is a global singleton, so does not allow for process sharing, which will no doubt bite us soon enough).
(In reply to Sam Weinig from comment #6) > (In reply to Aditya Keerthi from comment #5) > > (In reply to Sam Weinig from comment #4) > > > Comment on attachment 406019 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=406019&action=review > > > > > > > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:165 > > > > +#if PLATFORM(MAC) > > > > + if (!m_dateTimeEditElement) > > > > + return; > > > > + > > > > + DateTimeEditElement::LayoutParameters layoutParameters(element()->locale()); > > > > + setupLayoutParameters(layoutParameters); > > > > + > > > > + if (!DateTimeFormatValidator().validateFormat(layoutParameters.dateTimeFormat, *this)) > > > > + layoutParameters.dateTimeFormat = layoutParameters.fallbackDateTimeFormat; > > > > + > > > > + auto date = parseToDateComponents(element()->value()); > > > > + if (!date) > > > > + m_dateTimeEditElement->setEmptyValue(layoutParameters); > > > > + else > > > > + m_dateTimeEditElement->setValueAsDate(layoutParameters, *date); > > > > +#else > > > > > > We should try to figure out a way to do this without adding PLATFORM(MAC) > > > checks in here. Ideally, this would be something that can exist on all > > > platforms, controlled at runtime, and enabled for Mac. > > > > > > At the very least, we should figure out how to make this theme based. > > > > What do you think of replacing the PLATFORM(MAC) check here with a > > RuntimeEnabledFeatures check? This method is the only one that will require > > such a check – and the other PLATFORM(MAC) checks can be removed. > > What would the name of check be? This functionality was formerly referred to as INPUT_MULTIPLE_FIELDS_UI, so maybe DateTimeMultipleFieldsUI? > Why does this need to be conditionalized at all? There are two reasons why this should be conditionalized. 1. The feature is under active development and leaves date/time inputs in a partially completed state. 2. The feature does not make sense given current iOS behaviour. On iOS, focusing on date/time inputs presents a picker in a fullscreen overlay, and individual editable fields would be inaccessible. Furthermore, iOS date/time controls have a very different appearance. That said, we could probably eventually enable this feature on iOS and make additional (platform-specific) stylesheet changes to preserve appearance, even if the functionality is inaccessible.
(In reply to Aditya Keerthi from comment #8) > (In reply to Sam Weinig from comment #6) > > (In reply to Aditya Keerthi from comment #5) > > > (In reply to Sam Weinig from comment #4) > > > > Comment on attachment 406019 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=406019&action=review > > > > > > > > > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:165 > > > > > +#if PLATFORM(MAC) > > > > > + if (!m_dateTimeEditElement) > > > > > + return; > > > > > + > > > > > + DateTimeEditElement::LayoutParameters layoutParameters(element()->locale()); > > > > > + setupLayoutParameters(layoutParameters); > > > > > + > > > > > + if (!DateTimeFormatValidator().validateFormat(layoutParameters.dateTimeFormat, *this)) > > > > > + layoutParameters.dateTimeFormat = layoutParameters.fallbackDateTimeFormat; > > > > > + > > > > > + auto date = parseToDateComponents(element()->value()); > > > > > + if (!date) > > > > > + m_dateTimeEditElement->setEmptyValue(layoutParameters); > > > > > + else > > > > > + m_dateTimeEditElement->setValueAsDate(layoutParameters, *date); > > > > > +#else > > > > > > > > We should try to figure out a way to do this without adding PLATFORM(MAC) > > > > checks in here. Ideally, this would be something that can exist on all > > > > platforms, controlled at runtime, and enabled for Mac. > > > > > > > > At the very least, we should figure out how to make this theme based. > > > > > > What do you think of replacing the PLATFORM(MAC) check here with a > > > RuntimeEnabledFeatures check? This method is the only one that will require > > > such a check – and the other PLATFORM(MAC) checks can be removed. > > > > What would the name of check be? > > This functionality was formerly referred to as INPUT_MULTIPLE_FIELDS_UI, so > maybe DateTimeMultipleFieldsUI? DateTimeMultipleFieldsUI doesn't really tell me what this is. I think we need more iteration on the name. > > > Why does this need to be conditionalized at all? > > There are two reasons why this should be conditionalized. > > 1. The feature is under active development and leaves date/time inputs in a > partially completed state. > > 2. The feature does not make sense given current iOS behaviour. On iOS, > focusing on date/time inputs presents a picker in a fullscreen overlay, and > individual editable fields would be inaccessible. Furthermore, iOS date/time > controls have a very different appearance. That said, we could probably > eventually enable this feature on iOS and make additional > (platform-specific) stylesheet changes to preserve appearance, even if the > functionality is inaccessible. If we intend to keep this difference, it should be clear in the code and in the name of the conditionals. Rather than thinking about this as iOS vs. macOS, try to think about this as different behaviors. If you ever find yourself using a PLATFORM() check, really ask yourself if this is the best way to make this check. It probably isn't.
Created attachment 406094 [details] Patch
(In reply to Sam Weinig from comment #9) > (In reply to Aditya Keerthi from comment #8) > > (In reply to Sam Weinig from comment #6) > > > (In reply to Aditya Keerthi from comment #5) > > > > (In reply to Sam Weinig from comment #4) > > > > > Comment on attachment 406019 [details] > > > > > Patch > > > > > > > > > > View in context: > > > > > https://bugs.webkit.org/attachment.cgi?id=406019&action=review > > > > > > > > > > > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:165 > > > > > > +#if PLATFORM(MAC) > > > > > > + if (!m_dateTimeEditElement) > > > > > > + return; > > > > > > + > > > > > > + DateTimeEditElement::LayoutParameters layoutParameters(element()->locale()); > > > > > > + setupLayoutParameters(layoutParameters); > > > > > > + > > > > > > + if (!DateTimeFormatValidator().validateFormat(layoutParameters.dateTimeFormat, *this)) > > > > > > + layoutParameters.dateTimeFormat = layoutParameters.fallbackDateTimeFormat; > > > > > > + > > > > > > + auto date = parseToDateComponents(element()->value()); > > > > > > + if (!date) > > > > > > + m_dateTimeEditElement->setEmptyValue(layoutParameters); > > > > > > + else > > > > > > + m_dateTimeEditElement->setValueAsDate(layoutParameters, *date); > > > > > > +#else > > > > > > > > > > We should try to figure out a way to do this without adding PLATFORM(MAC) > > > > > checks in here. Ideally, this would be something that can exist on all > > > > > platforms, controlled at runtime, and enabled for Mac. > > > > > > > > > > At the very least, we should figure out how to make this theme based. > > > > > > > > What do you think of replacing the PLATFORM(MAC) check here with a > > > > RuntimeEnabledFeatures check? This method is the only one that will require > > > > such a check – and the other PLATFORM(MAC) checks can be removed. > > > > > > What would the name of check be? > > > > This functionality was formerly referred to as INPUT_MULTIPLE_FIELDS_UI, so > > maybe DateTimeMultipleFieldsUI? > > DateTimeMultipleFieldsUI doesn't really tell me what this is. I think we > need more iteration on the name. I named the preference DateTimeInputsEditableComponentsEnabled.
Comment on attachment 406094 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406094&action=review This is looking way better, thanks! > Source/WebCore/css/html.css:490 > +input::-webkit-datetime-edit { > + display: inline-block; > + overflow: hidden; > +} Are these pseudo elements accessible by the main document (I think they are, but I can't recall completely). If so, can we add some tests for them? > Source/WebCore/css/html.css:506 > + background-color: -apple-system-control-accent; Is there a non-apple specific value we can use here so that this can work for all ports? > Source/WebCore/html/MonthInputType.cpp:140 > + No need for this newline. > Source/WebCore/html/TimeInputType.cpp:116 > + No need for this newline. > Source/WebCore/html/WeekInputType.cpp:93 > + No need for this newline. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:81 > + const int countForAbbreviatedMonth = 3; > + const int countForFullMonth = 4; > + const int countForNarrowMonth = 5; These should be constexpr.
Created attachment 406203 [details] Patch
(In reply to Sam Weinig from comment #12) > Comment on attachment 406094 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=406094&action=review > > This is looking way better, thanks! > > > Source/WebCore/css/html.css:490 > > +input::-webkit-datetime-edit { > > + display: inline-block; > > + overflow: hidden; > > +} > > Are these pseudo elements accessible by the main document (I think they are, > but I can't recall completely). If so, can we add some tests for them? Added a test. > > Source/WebCore/css/html.css:506 > > + background-color: -apple-system-control-accent; > > Is there a non-apple specific value we can use here so that this can work > for all ports? I modified the stylesheet to use "background-color: highlight;" on non-Cocoa ports. There is no concept of an accent color on other ports. I kept "background-color: -apple-system-control-accent;" on Cocoa-ports to match the appearance of system date controls.
<rdar://problem/66879949>
Comment on attachment 406203 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406203&action=review > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:171 > + auto date = parseToDateComponents(element()->value()); > + if (!date) > + m_dateTimeEditElement->setEmptyValue(layoutParameters); > + else > + m_dateTimeEditElement->setValueAsDate(layoutParameters, *date); Nit - might be a bit cleaner as: if (auto date = parseToDateComponents(element()->value())) m_dateTimeEditElement->setValueAsDate(layoutParameters, *date); else m_dateTimeEditElement->setEmptyValue(layoutParameters); > Source/WebCore/html/DateInputType.cpp:94 > + return results.contains(DateTimeFormatValidationResults::HasYear) > + && results.contains(DateTimeFormatValidationResults::HasMonth) > + && results.contains(DateTimeFormatValidationResults::HasDay); Nit - might be cleaner using OptionSet::containsAll.
Created attachment 407112 [details] Patch
Created attachment 407178 [details] current beaviour after upgrading to ios 14 beta in ionic4 application Just wanted to know is this issue same as the attached screenshot. (we are not able to edit the time) and wanted to know if the fix is available or should we wait for the next rollout.
(In reply to bhargavsivapuram802 from comment #18) > Created attachment 407178 [details] > current beaviour after upgrading to ios 14 beta in ionic4 application > > Just wanted to know is this issue same as the attached screenshot. > (we are not able to edit the time) and wanted to know if the fix is > available or should we wait for the next rollout. This bug is for macOS work. Please ensure that you are on the latest beta, and if the issue persists, please file a new bug with reproduction steps.
Comment on attachment 407112 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407112&action=review Overall this is looking good! Mainly lots of NITs :) > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:53 > + void visitField(DateTimeFormat::FieldType, int) final; > + void visitLiteral(const String&) final { } NIT: why not make the entire class `final`? > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:66 > + break; Style: I personally like adding newlines after `break` and `return` inside long `switch`, but it's up to you :) > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:146 > + if (element()->document().page() && element()->document().page()->settings().dateTimeInputsEditableComponentsEnabled()) { Any reason not to just use `element()->document().settings().dateTimeInputsEditableComponentsEnabled()`? > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:147 > + m_dateTimeEditElement = DateTimeEditElement::create(element()->document(), *this); NIT: you could make local variables for some of these "getters" to avoid repeated invocations ``` auto& element = &this->element(); auto& document = element.document(); ``` > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:160 > + m_dateTimeEditElement = nullptr; [for my own knowledge] Does this actually remove `DateTimeEditElement` from `element()`? Or is that not the point of this function? > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:167 > + RefPtr<Node> node = element()->userAgentShadowRoot()->firstChild(); Does this need to be `RefPtr<Node>`, or can it just be `auto`? > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:269 > + return element()->computeInheritedLanguage(); Should this also `ASSERT(element());`? > Source/WebCore/html/DateInputType.h:52 > + bool isValidFormat(OptionSet<DateTimeFormatValidationResults>) const override; > + void setupLayoutParameters(DateTimeEditElement::LayoutParameters&) const override; should these be `final`? > Source/WebCore/html/DateTimeLocalInputType.cpp:104 > + return results.contains(DateTimeFormatValidationResults::HasYear) > + && results.contains(DateTimeFormatValidationResults::HasMonth) > + && results.contains(DateTimeFormatValidationResults::HasDay) > + && results.contains(DateTimeFormatValidationResults::HasHour) > + && results.contains(DateTimeFormatValidationResults::HasMinute); `containsAll`? > Source/WebCore/html/DateTimeLocalInputType.h:54 > + bool isValidFormat(OptionSet<DateTimeFormatValidationResults>) const override; > + void setupLayoutParameters(DateTimeEditElement::LayoutParameters&) const override; should these be `final`? > Source/WebCore/html/MonthInputType.cpp:139 > + return results.contains(DateTimeFormatValidationResults::HasYear) > + && results.contains(DateTimeFormatValidationResults::HasMonth); `containsAll`? > Source/WebCore/html/MonthInputType.h:57 > + bool isValidFormat(OptionSet<DateTimeFormatValidationResults>) const override; > + void setupLayoutParameters(DateTimeEditElement::LayoutParameters&) const override; should these be `final`? > Source/WebCore/html/TimeInputType.cpp:115 > + return results.contains(DateTimeFormatValidationResults::HasHour) > + && results.contains(DateTimeFormatValidationResults::HasMinute); `containsAll`? > Source/WebCore/html/TimeInputType.h:54 > + bool isValidFormat(OptionSet<DateTimeFormatValidationResults>) const override; > + void setupLayoutParameters(DateTimeEditElement::LayoutParameters&) const override; should these be `final`? > Source/WebCore/html/WeekInputType.cpp:92 > + return results.contains(DateTimeFormatValidationResults::HasYear) > + && results.contains(DateTimeFormatValidationResults::HasWeek); `containsAll`? > Source/WebCore/html/WeekInputType.h:53 > + bool isValidFormat(OptionSet<DateTimeFormatValidationResults>) const override; > + void setupLayoutParameters(DateTimeEditElement::LayoutParameters&) const override; should these be `final`? > Source/WebCore/html/shadow/DateTimeEditElement.cpp:59 > + void visitField(DateTimeFormat::FieldType, int) final; > + void visitLiteral(const String&) final; NIT: why not make the entire class `final`? > Source/WebCore/html/shadow/DateTimeEditElement.cpp:104 > + default: { NIT: the `{ }` isn't needed here > Source/WebCore/html/shadow/DateTimeEditElement.cpp:134 > +DateTimeEditElement::EditControlOwner::~EditControlOwner() > +{ > +} ` = default;`? > Source/WebCore/html/shadow/DateTimeEditElement.cpp:146 > +DateTimeEditElement::~DateTimeEditElement() > +{ > +} ` = default;`? > Source/WebCore/html/shadow/DateTimeEditElement.cpp:150 > + ASSERT(firstChild()); If we know that `firstChild()` is valid, can we return an `Element&` instead? > Source/WebCore/html/shadow/DateTimeEditElement.cpp:187 > + for (Node* childNode = fieldsWrapper->firstChild(); childNode; childNode = fieldsWrapper->firstChild()) { Why not use `while (auto* childNode = fieldsWrapper->firstChild())`? > Source/WebCore/html/shadow/DateTimeEditElement.cpp:223 > + if (!m_editControlOwner) > + return emptyString(); > + return m_editControlOwner->valueForEditControl(); NIT: you've done this as a ternary in other places; any reason not to here as well? > Source/WebCore/html/shadow/DateTimeEditElement.h:70 > + static const size_t invalidFieldIndex = static_cast<size_t>(-1); this doesn't appear to be used > Source/WebCore/html/shadow/DateTimeEditElement.h:72 > + // Datetime can be represented by at most 8 fields: day-of-week? timezone? > Source/WebCore/html/shadow/DateTimeEditElement.h:81 > + static const int maximumNumberOfFields = 8; NIT: `constexpr`? > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:47 > +DateTimeFieldElement::FieldOwner::~FieldOwner() > +{ > +} ` = default;`? > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:76 > + const String newVisibleValue = visibleValue(); Why is this `const`? > Source/WebCore/html/shadow/DateTimeFieldElements.cpp:72 > + setValueAsInteger(date.month() + 1); NIT: I'd add a comment here explaining why the `+ 1` is needed. > Source/WebCore/html/shadow/DateTimeFieldElements.h:46 > + void setValueAsDate(const DateComponents&) final; NIT: if a class is `final` you don't need `final` on methods. > Source/WebCore/html/shadow/DateTimeFieldElements.h:59 > + void setValueAsDate(const DateComponents&) final; ditto (:46) > Source/WebCore/html/shadow/DateTimeFieldElements.h:72 > + void setValueAsDate(const DateComponents&) final; ditto (:46) > Source/WebCore/html/shadow/DateTimeFieldElements.h:85 > + void setValueAsDate(const DateComponents&) final; ditto (:46) > Source/WebCore/html/shadow/DateTimeNumericFieldElement.h:46 > + void setValueAsInteger(int) override; NIT: should this be `final`? > Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.cpp:56 > + , m_selectedIndex(-1) NIT: `invalidIndex`? > Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.cpp:58 > + ASSERT(!symbols.isEmpty()); NIT: I'd check `m_symbols` as that's what's ultimately used by other member functions. > Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.h:47 > + static const int invalidIndex = -1; NIT: `constexpr`?
Created attachment 407267 [details] Patch
(In reply to Devin Rousso from comment #20) > Comment on attachment 407112 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=407112&action=review > > Overall this is looking good! Mainly lots of NITs :) > > > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:146 > > + if (element()->document().page() && element()->document().page()->settings().dateTimeInputsEditableComponentsEnabled()) { > > Any reason not to just use > `element()->document().settings().dateTimeInputsEditableComponentsEnabled()`? I didn't realize settings() was accessible from document(). Thanks! > > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:160 > > + m_dateTimeEditElement = nullptr; > > [for my own knowledge] Does this actually remove `DateTimeEditElement` from > `element()`? Or is that not the point of this function? This assignment does not. However, the function should remove `DateTimeEditElement` from `element()`. I added a missing call to `InputType::destroyShadowSubtree()`. > > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:167 > > + RefPtr<Node> node = element()->userAgentShadowRoot()->firstChild(); > > Does this need to be `RefPtr<Node>`, or can it just be `auto`? I think `auto` should be fine. > > Source/WebCore/html/shadow/DateTimeEditElement.h:72 > > + // Datetime can be represented by at most 8 fields: > > day-of-week? timezone? Day-of-week is not displayed in any of the five existing date/time controls (date, datetime-local, month, week, time). Timezone would have been necessary if `<input type=datetime>` wasn't removed from the spec. However, it is not needed for datetime-local. > > Source/WebCore/html/shadow/DateTimeNumericFieldElement.h:46 > > + void setValueAsInteger(int) override; > > NIT: should this be `final`? Modified to `final` in this patch. This is going to change in a forthcoming patch though.
Comment on attachment 407267 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407267&action=review > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:156 > + auto& element = *(this->element()); Nit - no need for parentheses around `this->element()`. > Source/WebCore/html/shadow/DateTimeEditElement.h:89 > + EditControlOwner* m_editControlOwner; Should m_editControlOwner ever be null? If not, this should be a EditControlOwner&. > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:73 > + Text& textNode = downcast<Text>(*firstChild()); Nit - auto& > Source/WebCore/html/shadow/DateTimeFieldElement.h:64 > + FieldOwner* m_fieldOwner; Same as m_editControlOwner — should this be a reference?
Comment on attachment 407267 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407267&action=review >> Source/WebCore/html/shadow/DateTimeEditElement.h:89 >> + EditControlOwner* m_editControlOwner; > > Should m_editControlOwner ever be null? If not, this should be a EditControlOwner&. (In case this does turn out that this should be a raw pointer, we should initialize it to { nullptr };) > Source/WebCore/html/shadow/DateTimeNumericFieldElement.h:58 > + int m_value; > + bool m_hasValue; Nit - I think we prefer to specify initial values here, instead of in the constructors. > Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.h:58 > + int m_selectedIndex; (Ditto w.r.t. the initial value)
Created attachment 407499 [details] Patch
(In reply to Wenson Hsieh from comment #23) > Comment on attachment 407267 [details] > Patch > > > Source/WebCore/html/shadow/DateTimeEditElement.h:89 > > + EditControlOwner* m_editControlOwner; > > Should m_editControlOwner ever be null? If not, this should be a > EditControlOwner&. SpinButtonElement uses a similar design as this patch and states that it can outlive its owner during event handling. To avoid a use-after-free I've updated the code to use WeakPtr.
Comment on attachment 407499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407499&action=review r=me, nice work! One general comment is that there's a lot of places where you could use `auto` (or `auto&`). Up to you whether to keep the explicitly written type or not, but I personally like to use `auto` for most things that are obvious from reading surrounding code :) > Source/WebCore/html/shadow/DateTimeEditElement.cpp:81 > + constexpr int countForAbbreviatedMonth = 3; > + constexpr int countForFullMonth = 4; > + constexpr int countForNarrowMonth = 5; NIT: should we move these inside the `case` for months? > Source/WebCore/html/shadow/DateTimeEditElement.cpp:108 > + return; NIT: why not replace the `break`s above with `return`? > Source/WebCore/html/shadow/DateTimeEditElement.cpp:135 > + , m_editControlOwner(makeWeakPtr(&editControlOwner)) I don't think you need the `&`. You should be able to `makeWeakPtr` with a reference. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:204 > + for (size_t fieldIndex = 0; fieldIndex < m_fields.size(); ++fieldIndex) > + m_fields[fieldIndex]->setValueAsDate(date); any reason not to use a range-for? ``` for (auto& field : m_fields) ``` > Source/WebCore/html/shadow/DateTimeEditElement.cpp:211 > + for (size_t fieldIndex = 0; fieldIndex < m_fields.size(); ++fieldIndex) > + m_fields[fieldIndex]->setEmptyValue(); ditto (:203) > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:49 > + , m_fieldOwner(makeWeakPtr(&fieldOwner)) I don't think you need the `&`. You should be able to `makeWeakPtr` with a reference. > Source/WebCore/html/shadow/DateTimeFieldElement.h:66 > + WeakPtr<FieldOwner> m_fieldOwner; Is it possible/expected for `m_fieldOwner` to be destroyed before this object is destroyed? If not, can we just make this a `FieldOwner&` instead? > Source/WebCore/html/shadow/DateTimeFieldElements.cpp:73 > + setValueAsInteger(date.month() + 1); Does this get "transformed" back to 0-based after the user edits it? If not, should it? > Source/WebCore/html/shadow/DateTimeNumericFieldElement.h:40 > + DateTimeNumericFieldElement(Document&, FieldOwner&, const String&); NIT: the `const String&` should also have parameter name here as it's not clear what it's used for > Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.cpp:44 > + for (unsigned index = 0; index < symbols.size(); ++index) > + maximumLength = std::max(maximumLength, numGraphemeClusters(symbols[index])); any reason not to use a range-for? ``` for (auto& symbol : symbols) ``` > Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.cpp:48 > + for (unsigned length = 0; length < maximumLength; ++length) > + builder.append('-'); NIT: can we use `pad` to simplify this?
Created attachment 407503 [details] Patch for landing
(In reply to Devin Rousso from comment #27) > Comment on attachment 407499 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=407499&action=review > > r=me, nice work! Thanks for the review! > > Source/WebCore/html/shadow/DateTimeFieldElement.h:66 > > + WeakPtr<FieldOwner> m_fieldOwner; > > Is it possible/expected for `m_fieldOwner` to be destroyed before this > object is destroyed? If not, can we just make this a `FieldOwner&` instead? I'm not certain that `m_fieldOwner` cannot outlive this object. To be safe, I've kept it as a WeakPtr. > > Source/WebCore/html/shadow/DateTimeFieldElements.cpp:73 > > + setValueAsInteger(date.month() + 1); > > Does this get "transformed" back to 0-based after the user edits it? If > not, should it? We don't need to worry about it being transformed back to 0-based, since getting the full value returns a string of the form "yyyy-MM-dd" (not a DateComponents). In cases where a DateComponents is needed, the "yyyy-MM-dd" string is parsed accordingly.
Committed r266351: <https://trac.webkit.org/changeset/266351> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407503 [details].