...
Created attachment 408017 [details] Patch
Comment on attachment 408017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408017&action=review > Source/WebCore/html/DateTimeFieldsState.h:39 > + enum AMPMValue { > + AMPMValueAM, > + AMPMValuePM, > + }; I think this would read cleaner as something like: enum class Meridiem { AM, PM };
Comment on attachment 408017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408017&action=review This is looking pretty great! Just a couple of minor comments as I continue to look. > Source/WebCore/html/DateTimeFieldsState.h:36 > + enum AMPMValue { Nit - in new code, we typically prefer 8-bit-wide enum classes. For instance, enum class AMPMValue : bool { … }; > Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.cpp:122 > + int index = m_typeAhead.handleEvent(&keyboardEvent, TypeAhead::MatchPrefix | TypeAhead::CycleFirstChar | TypeAhead::MatchIndex); Not specific to this patch or anything, but we should make `TypeAhead::handleEvent` return an `Optional<int>` at some point.
Comment on attachment 408017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408017&action=review r=me, nice work! > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:313 > + } else if (name == stepAttr && m_dateTimeEditElement) { > + updateInnerTextValue(); > } Style: no `{` `}` if the body is a single line > Source/WebCore/html/DateTimeFieldsState.h:48 > + Optional<AMPMValue> ampm; IMO it would be nicer to read as `meridiem` (e.g. `-webkit-datetime-edit-meridiem-field`) than as `ampm` > Source/WebCore/html/TimeInputType.cpp:121 > + if (!state.hour || !state.minute || !state.ampm) is it not possible (perhaps undesirable) to infer the `minute` and/or `meridiem` based on the `hour`? e.g. if the `hour` is `20`, then `minute` is `0` and `meridien` is `PM` > Source/WebCore/html/TimeInputType.cpp:140 > + auto millisecondsPerSecond = Decimal::fromDouble(msPerSecond); > + auto millisecondsPerMinute = Decimal::fromDouble(msPerMinute); <unrelated to this patch> would be neat if `Decimal` were `constexpr` :P > Source/WebCore/html/TimeInputType.cpp:153 > + layoutParameters.fallbackDateTimeFormat = "HH:mm:ss"; `_s` > Source/WebCore/html/TimeInputType.cpp:156 > + layoutParameters.fallbackDateTimeFormat = "HH:mm"; `_s` > Source/WebCore/html/shadow/DateTimeFieldElements.cpp:110 > + case 11: { NIT: none of these `case` actually need the `{` `}` since they don't declare any variables > Source/WebCore/html/shadow/DateTimeFieldElements.cpp:111 > + state.hour = value ? value : 12; <after reading below> Could we use `value ?: 12` here too? > Source/WebCore/html/shadow/DateTimeFieldElements.cpp:119 > + state.hour = (value % 12) ?: 12; `?:` 🤯 > Source/WebCore/html/shadow/DateTimeFieldElements.cpp:129 > + state.hour = value == 12 ? 12 : value % 12; Why not also do `(value % 12) ?: 12` here too? > Source/WebCore/html/shadow/DateTimeFieldElements.cpp:141 > + case 11: { ditto (:110)
(In reply to Devin Rousso from comment #4) > Comment on attachment 408017 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=408017&action=review > > r=me, nice work! Thanks for the review! I'm going to address one of the failing WPT tests in a separate patch before landing this one, as it should also apply to date inputs. > > Source/WebCore/html/TimeInputType.cpp:121 > > + if (!state.hour || !state.minute || !state.ampm) > > is it not possible (perhaps undesirable) to infer the `minute` and/or > `meridiem` based on the `hour`? e.g. if the `hour` is `20`, then `minute` > is `0` and `meridien` is `PM` DateTimeFieldsState will always have an `hour` between 1 and 12 (even if the user's locale has a different format, the value is normalized). If any one of these fields is empty, we return an empty string, matching the behavior of date/time inputs in other browsers.
Created attachment 408269 [details] Patch for landing
Committed r266779: <https://trac.webkit.org/changeset/266779> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408269 [details].
<rdar://problem/68571901>
Comment on attachment 408269 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=408269&action=review > Source/WebCore/html/shadow/DateTimeFieldElements.h:61 > + virtual void populateDateTimeFieldsState(DateTimeFieldsState&); > + virtual void setValueAsDate(const DateComponents&); These should be override instead of virtual. Also, why are the orders different? > Source/WebCore/html/shadow/DateTimeFieldElements.h:75 > + void populateDateTimeFieldsState(DateTimeFieldsState&); > + void setValueAsDate(const DateComponents&); Ditto. > Source/WebCore/html/shadow/DateTimeFieldElements.h:89 > + virtual void populateDateTimeFieldsState(DateTimeFieldsState&); > + virtual void setValueAsDate(const DateComponents&); Ditto. > Source/WebCore/html/shadow/DateTimeFieldElements.h:103 > + virtual void populateDateTimeFieldsState(DateTimeFieldsState&); > + virtual void setValueAsDate(const DateComponents&); Ditto.
Opened a new bug to address post-review comments: https://bugs.webkit.org/show_bug.cgi?id=216339.