Created attachment 407602 [details] Patch

Created attachment 407603 [details] Patch

Created attachment 407607 [details] Patch

Comment on attachment 407603 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407603&action=review > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:257 > + if (!m_dateTimeEditElement) { > + InputType::handleFocusEvent(oldFocusedNode, direction); > + } else if (direction == FocusDirectionBackward) { WebKit coding style doesn’t use braces in a single-line if statement body like this. Or could do this in early return style instead of using else. > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:259 > + if (auto* page = element()->document().page()) > + page->focusController().advanceFocus(direction, 0); I’m confused about this code. Why do we need this here explicitly and don’t instead call through to some other code that does this? This doesn’t seem like behavior that’s specific to this particular element and input type. > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.h:67 > + void editControlValueChanged() final; This is a confusing function name. It sounds like something that would return a boolean to tell you if the edit control value changed. This is the reason we use names like "didBlur". This one could use a name like "didChangeValue" to be less ambiguous. > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.h:80 > + void handleFocusEvent(Node* oldFocusedNode, FocusDirection) override; I’d think this should be final, not override. > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.h:84 > + bool hasCustomFocusLogic() const override; I’d think this should be final, not override. > Source/WebCore/html/DateTimeFieldsState.h:33 > +class DateTimeFieldsState { Can this just be a struct containing three optional data members instead of a class with lots of functions? > Source/WebCore/html/DateTimeFieldsState.h:35 > + static const unsigned emptyValue; Why? Can we use Optional<unsigned> instead for clarity? Or if the reason we don’t want to use Optional is that we want to save space, then could we use uint8_t instead of uint32_t? > Source/WebCore/html/DateTimeFieldsState.h:41 > + unsigned dayOfMonth() const { return m_dayOfMonth; } > + unsigned month() const { return m_month; } > + unsigned year() const { return m_year; } Should these assert that the values are not "emptyValue"? > Source/WebCore/html/DateTimeFieldsState.h:49 > + void setDayOfMonth(unsigned dayOfMonth) { m_dayOfMonth = dayOfMonth; } > + void setMonth(unsigned month) { m_month = month; } > + void setYear(unsigned year) { m_year = year; } Are callers allowed to pass "emptyValue"? > Source/WebCore/html/DateTimeFieldsState.h:54 > + unsigned m_year; > + unsigned m_month; > + unsigned m_dayOfMonth; I suggest initializing the values here, not in a constructor. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:164 > + for (size_t fieldIndex = 0; fieldIndex < m_fields.size(); ++fieldIndex) { > + if (m_fields[fieldIndex].ptr() == &field) > + return fieldIndex; > + } > + return invalidFieldIndex; I suggest using Vector::findMatching instead of writing the loop. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:208 > + bool notifyOwner = true; > + for (auto& field : m_fields) { > + if (field.ptr() == newFocusedElement.get()) { > + notifyOwner = false; > + break; > + } > + } This would be nicer if it used Vector::contains (or probably needs to use Vector::findMatching because there is no Vector::containsMatching yet). > Source/WebCore/html/shadow/DateTimeEditElement.cpp:224 > + for (size_t fieldIndex = startIndex; fieldIndex < m_fields.size(); ++fieldIndex) { Seems like fieldIndex is a really long name for such a super-local thing. Typically we use "I" for things like this, one of the few exceptions to "use words for variable names" style in WebKit. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:240 > + const size_t startFieldIndex = fieldIndexOf(field); The const here doesn’t add much. We normally don’t use const just because a local variable isn’t changed, especially in such a short function. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:248 > + const size_t startFieldIndex = fieldIndexOf(field); Ditto. > Source/WebCore/html/shadow/DateTimeEditElement.h:75 > + static constexpr size_t invalidFieldIndex = static_cast<size_t>(-1); Can we use an Optional<size_t> instead of using a special value to express "not found"? Or you could use the same notFound that Vector uses for its find operation (I plan to change that to an Optional at some point). > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:87 > + if (key == "Left") { > + if (m_fieldOwner && m_fieldOwner->focusOnPreviousField(*this)) { > + keyboardEvent.setDefaultHandled(); > + return; > + } > + } I suggest using && here unless you want to move the return out of the inner if statement. > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:94 > + if (key == "Right") { > + if (m_fieldOwner && m_fieldOwner->focusOnNextField(*this)) { > + keyboardEvent.setDefaultHandled(); > + return; > + } > + } Ditto. > Source/WebCore/html/shadow/DateTimeFieldElement.h:43 > + enum EventBehavior { DispatchNoEvent, DispatchInputAndChangeEvent }; I suggest renaming to DispatchInputAndChangeEvents. Also would be good to base this enumeration on bool. enum EventBehavior : bool { DispatchNoEvents, DispatchInputAndChangeEvents }; > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:43 > + return std::min(std::max(value, minimum), maximum); Could use std::clamp.

Created attachment 407616 [details] Patch for landing

(In reply to Darin Adler from comment #4) > Comment on attachment 407603 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=407603&action=review > > > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:257 > > + if (!m_dateTimeEditElement) { > > + InputType::handleFocusEvent(oldFocusedNode, direction); > > + } else if (direction == FocusDirectionBackward) { > > WebKit coding style doesn’t use braces in a single-line if statement body > like this. Or could do this in early return style instead of using else. Changed to an early return for readability. > > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:259 > > + if (auto* page = element()->document().page()) > > + page->focusController().advanceFocus(direction, 0); > > I’m confused about this code. Why do we need this here explicitly and don’t > instead call through to some other code that does this? This doesn’t seem > like behavior that’s specific to this particular element and input type. I've added a comment to explain, but will also explain here. Consider the following list of focusable elements, with their focusable children: - <input type=text id="before"> - <input type=date> - <datetime-month-field> - <datetime-day-field> - <datetime-year-field> - <input type=text id="after"> Suppose the <datetime-month-field> has focus. Upon a Shift+Tab, the <input type=date> "receives focus" (as it is the previous focusable element), and the handleFocusEvent method is called with FocusDirectionBackward. However, backwards focus from <datetime-month-field> should focus the "before" element. Consequently, an additional call to advanceFocus is made. Without the added logic, two Shift+Tabs would be required to focus the "before" element if <datetime-month-field> is focused. > > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.h:67 > > + void editControlValueChanged() final; > > This is a confusing function name. It sounds like something that would > return a boolean to tell you if the edit control value changed. This is the > reason we use names like "didBlur". This one could use a name like > "didChangeValue" to be less ambiguous. Renamed to "didChangeValueFromControl". > > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.h:80 > > + void handleFocusEvent(Node* oldFocusedNode, FocusDirection) override; > > I’d think this should be final, not override. Changed to final. > > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.h:84 > > + bool hasCustomFocusLogic() const override; > > I’d think this should be final, not override. Changed to final. > > Source/WebCore/html/DateTimeFieldsState.h:33 > > +class DateTimeFieldsState { > > Can this just be a struct containing three optional data members instead of > a class with lots of functions? Changed to a struct with optional data members. Note that this struct will grow in the near future, as support for more date/time inputs is added. > > Source/WebCore/html/DateTimeFieldsState.h:35 > > + static const unsigned emptyValue; > > Why? Can we use Optional<unsigned> instead for clarity? Or if the reason we > don’t want to use Optional is that we want to save space, then could we use > uint8_t instead of uint32_t? The members are now Optional<unsigned>. > > Source/WebCore/html/DateTimeFieldsState.h:41 > > + unsigned dayOfMonth() const { return m_dayOfMonth; } > > + unsigned month() const { return m_month; } > > + unsigned year() const { return m_year; } > > Should these assert that the values are not "emptyValue"? They probably should have, but I've removed these methods with the change to a struct of Optionals. > > Source/WebCore/html/DateTimeFieldsState.h:49 > > + void setDayOfMonth(unsigned dayOfMonth) { m_dayOfMonth = dayOfMonth; } > > + void setMonth(unsigned month) { m_month = month; } > > + void setYear(unsigned year) { m_year = year; } > > Are callers allowed to pass "emptyValue"? They weren't. However, the methods have now been removed with the change to a struct of Optionals. > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:164 > > + for (size_t fieldIndex = 0; fieldIndex < m_fields.size(); ++fieldIndex) { > > + if (m_fields[fieldIndex].ptr() == &field) > > + return fieldIndex; > > + } > > + return invalidFieldIndex; > > I suggest using Vector::findMatching instead of writing the loop. Done. > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:208 > > + bool notifyOwner = true; > > + for (auto& field : m_fields) { > > + if (field.ptr() == newFocusedElement.get()) { > > + notifyOwner = false; > > + break; > > + } > > + } > > This would be nicer if it used Vector::contains (or probably needs to use > Vector::findMatching because there is no Vector::containsMatching yet). Used Vector::findMatching. > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:224 > > + for (size_t fieldIndex = startIndex; fieldIndex < m_fields.size(); ++fieldIndex) { > > Seems like fieldIndex is a really long name for such a super-local thing. > Typically we use "I" for things like this, one of the few exceptions to "use > words for variable names" style in WebKit. Used "i". > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:240 > > + const size_t startFieldIndex = fieldIndexOf(field); > > The const here doesn’t add much. We normally don’t use const just because a > local variable isn’t changed, especially in such a short function. Removed. > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:248 > > + const size_t startFieldIndex = fieldIndexOf(field); > > Ditto. Removed. > > Source/WebCore/html/shadow/DateTimeEditElement.h:75 > > + static constexpr size_t invalidFieldIndex = static_cast<size_t>(-1); > > Can we use an Optional<size_t> instead of using a special value to express > "not found"? Or you could use the same notFound that Vector uses for its > find operation (I plan to change that to an Optional at some point). I used notFound, since this was only returned when using findMatching. > > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:87 > > + if (key == "Left") { > > + if (m_fieldOwner && m_fieldOwner->focusOnPreviousField(*this)) { > > + keyboardEvent.setDefaultHandled(); > > + return; > > + } > > + } > > I suggest using && here unless you want to move the return out of the inner > if statement. Done. > > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:94 > > + if (key == "Right") { > > + if (m_fieldOwner && m_fieldOwner->focusOnNextField(*this)) { > > + keyboardEvent.setDefaultHandled(); > > + return; > > + } > > + } > > Ditto. Done. > > Source/WebCore/html/shadow/DateTimeFieldElement.h:43 > > + enum EventBehavior { DispatchNoEvent, DispatchInputAndChangeEvent }; > > I suggest renaming to DispatchInputAndChangeEvents. Also would be good to > base this enumeration on bool. > > enum EventBehavior : bool { DispatchNoEvents, > DispatchInputAndChangeEvents }; Done. > > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:43 > > + return std::min(std::max(value, minimum), maximum); > > Could use std::clamp. Done.

Committed r266396: <https://trac.webkit.org/changeset/266396> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407616 [details].

<rdar://problem/68145864>