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

[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 161399: Patch 1
https://bugs.webkit.org/attachment.cgi?id=161399&action=review

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


> Source/WebCore/ChangeLog:14
> +	   current implementation, it is hard to set focus to the last field
when
> +	   it gets focus, because we don't know how DateTimeEditElement gets

gets focus by Shift+Tab ?

> Source/WebCore/ChangeLog:19
> +

You should write more details of the behavior. e.g.
 - HTMLInputElement for type=time is not focusable, but has a focus ring by a
trick
 - behavior of HTMLInputElement::focus() and blur()
 - How 'focus' event and 'blur' event work (Also, this needs test!)

> Source/WebCore/ChangeLog:22
> +	   This patch also changes handling of Left/Right keys to aware text
> +	   direction ("dir" attribute"). Left/Right keys should move focus
> +	   physical right/left instead of logical right/left.

This should be done in a separated patch.

> Source/WebCore/html/TimeInputType.cpp:134
> +    m_timeInputType.element()->setFocus(false);
> +}
> +
> +void TimeInputType::DateTimeEditControlOwnerImpl::didFieldFocus()
> +{
> +    m_timeInputType.element()->setFocus(true);

I recommend adding comments about reasons why we need to call setFocus() and
why they are not focus() and blur().

> Source/WebCore/html/shadow/DateTimeEditElement.h:68
> +    virtual void blur() OVERRIDE FINAL;
>      virtual void defaultEventHandler(Event*) OVERRIDE;
>      void disabledStateChanged();
> +    virtual void focus(bool) OVERRIDE FINAL;

DateTimeEditElement is not focusable. So, overriding blur() and focus() is not
reasonable.
They should not be virtual, and we should rename them to blurFromField() and
focusOnTheFirstField() or something.

> Source/WebCore/html/shadow/DateTimeFieldElement.cpp:143
> +

Extra blank line