[webkit-reviews] review denied: [Bug 95168] [Forms] Shift+Tab should focus the last field of multiple fields time input UI : [Attachment 161420] Patch 3

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied yosin at chromium.org's request for
review:
Bug 95168: [Forms] Shift+Tab should focus the last field of multiple fields
time input UI
https://bugs.webkit.org/show_bug.cgi?id=95168

Attachment 161420: Patch 3
https://bugs.webkit.org/attachment.cgi?id=161420&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=161420&action=review


> Source/WebCore/ChangeLog:72
> +	   (WebCore::DateTimeFieldElement::focusOnNextField): Added for moving
focus to next field for DateTimeNumbericFieldElment

"Added" looks incorrect.  This already exists.
This function is used only by DateTimeNumericFieldElement. We

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:-237
> -void DateTimeEditElement::focusAndSelectSpinButtonOwner()

Don't change the function order.  It makes review harder.

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:275
> +size_t DateTimeEditElement::focusFieldIndex() const

focusedFieldIndex() is a better name because 'focus' looks a verb.

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:284
> +DateTimeFieldElement* DateTimeEditElement::focusField() const

ditto.	focusedField() is better

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:383
> +    if (DateTimeFieldElement* field = focusField()) {
> +	   field->defaultEventHandler(event);
> +	   if (event->defaultHandled())
> +	       return;
>      }

Do we need this code?

> Source/WebCore/html/shadow/DateTimeEditElement.h:68
> +    void focusByOwner(bool);

The argument should be removed.

> Source/WebCore/html/shadow/DateTimeFieldElement.cpp:85
> +	   if (m_fieldOwner) {

nit: we prefer early-return.

> Source/WebCore/html/shadow/DateTimeFieldElement.cpp:93
> +	   if (m_fieldOwner) {

ditto.

> Source/WebCore/html/shadow/DateTimeFieldElement.h:54
> +	   virtual void didFieldBlur() = 0;
> +	   virtual void didFieldFocus() = 0;

should be fieldDidBlur() and fieldDidFocus(), or didBlurFromField() and
didFocusOnField() because 'did' should prepend to a verb.

>
LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-blur-and-focus
-events.html:18
> +

Need to check what happens if testInput has focus and TAB is pressed.