Bug 95168 – [Forms] Shift+Tab should focus the last field of multiple fields time input UI

• Component: Forms
• Status: RESOLVED FIXED

Current implementation sets focus on the first field even if moving focus by Shift+Tab.
This behavior is counterintuitive.

Created attachment 161128 [details]
Work In Progress

Created attachment 161399 [details]
Patch 1

Comment on attachment 161399 [details]
Patch 1

Could you review this patch?
Thanks in advance.

[Kent Tamura]

Comment on attachment 161399 [details]
Patch 1

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

Created attachment 161418 [details]
Patch 2

Created attachment 161420 [details]
Patch 3

Comment on attachment 161420 [details]
Patch 3

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Remove Left/Right keys motion in RTL.
* Rename DateTimeEditElement::blur()/folcus() to blurByOwner()/focusByOwner()
* Add comments to TImeInputType::blur()/focus()
* Add a test file time-multiple-fields-blur-and-focus-events.html

[Kent Tamura]

Comment on attachment 161420 [details]
Patch 3

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.

[Kent Tamura]

(In reply to comment #8)
> (From update of attachment 161420 [details])
> 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

We had better move the function to DateTimeNumericFieldElement in another patch.

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 161420 [details] [details])
> > 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
> 
> We had better move the function to DateTimeNumericFieldElement in another patch.

We have two option:
1. Having DateTimeFieldElement::focusOnNextField - current implementation
  PROS: Derived classes don't aware existence of DateTimeFieldOwner class.
  CONS: We need to add functions to DateTimeFieldElement when we have new functions in DateTimeFieldOwner class.
2. Having DateTimeFieldElement::fieldOwner() then DateTimeNumericField calls DateTimeFieldOwner::focusOnNext() via fieldOwner() function.
  PROS: Derived classes can use DateTimeFieldOwner functions without changing DateTimeFieldElement class
  CONS: We may have same code sequence, checking fieldOwner != nullptr then call fieldOwner()->function, in number of places.

[Kent Tamura]

(In reply to comment #10)
> 1. Having DateTimeFieldElement::focusOnNextField - current implementation
>   PROS: Derived classes don't aware existence of DateTimeFieldOwner class.

I see. If you think the current structure is simpler, you don't need to move focusOnNextField().

Also, DateTimeFieldElement::defaultEventHandler() calls m_fieldOwner->focusOnNextField(*this) in the patch. It would be a justification of DateTimeFieldElement::focusOnNextField() if defaultEventHandler() used it.

(In reply to comment #8)
> (From update of attachment 161420 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161420&action=review
> 
> > Source/WebCore/html/shadow/DateTimeEditElement.cpp:383
> > +    if (DateTimeFieldElement* field = focusField()) {
> > +        field->defaultEventHandler(event);
> > +        if (event->defaultHandled())
> > +            return;
> >      }
> 
> Do we need this code?

Yes, for dispatching event via "input" element, e.g. input.dispatchEvent(...).

Created attachment 161438 [details]
Patch 4

Created attachment 161439 [details]
Patch 5

Comment on attachment 161439 [details]
Patch 5

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Rename focusXXX functions/variables to focusedXXX.
* Add new test time-multiple-fields-blur-and-focus-events.html
* Update ChangeLog

[Kent Tamura]

Comment on attachment 161439 [details]
Patch 5

View in context: https://bugs.webkit.org/attachment.cgi?id=161439&action=review

> Source/WebCore/ChangeLog:49
> +        (WebCore::DateTimeEditElement::didFieldBlur): Added for calling DateTimeEditControlOwner::didFieldBlur().
> +        (WebCore::DateTimeEditElement::didFieldFocus): Added for calling DateTimeEditControlOwner::didFieldFocus().

Need to update the comments.

> Source/WebCore/ChangeLog:65
> +        (EditControlOwner): Added didFieldBlur() and didFieldFocus() function declarations.

Need to update the comment.
Also, you should mention removed functions and their reasons.

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

"focus from field" sounds strange.  It should be didFocusOnField.

> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-blur-and-focus-events.html:50
> +    shouldBeEqualToString('keyDown("\t"); state()', 'blur=0 focus=1');
> +    shouldBeEqualToString('keyDown("\t"); state()', 'blur=0 focus=1');
> +    shouldBeEqualToString('keyDown("\t"); state()', 'blur=0 focus=1');

Oh, I thought focus and blur were dispatched in a case of focus change between internal fields.  This result is nice.

Created attachment 161600 [details]
Patch 6

Comment on attachment 161600 [details]
Patch 6

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Rename didFocusFromXXX to didFocusOnXXX
* Mention removed functions in ChangeLog

[Kent Tamura]

Comment on attachment 161600 [details]
Patch 6

View in context: https://bugs.webkit.org/attachment.cgi?id=161600&action=review

> Source/WebCore/ChangeLog:88
> +        (WebCore::DateTimeNumericFieldElement::didBlur): Added for resetting type-ahead timer.
> +        (WebCore::DateTimeNumericFieldElement::didFocus): Added for resetting type-ahead timer.

Why do you need to reset it? Is this a behavior change?

> Source/WebCore/html/TimeInputType.cpp:-145
> -void TimeInputType::DateTimeEditControlOwnerImpl::focusAndSelectEditControlOwner()
> +bool TimeInputType::hasCustomFocusLogic() const
>  {
> -    RefPtr<HTMLInputElement> input(m_timeInputType.element());
> -    input->focus();
> -    input->select();

Removing focus() and select() looks a behavior change.  But ChangeLog doesn't mention it.

(In reply to comment #19)
> (From update of attachment 161600 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161600&action=review
> 
> > Source/WebCore/ChangeLog:88
> > +        (WebCore::DateTimeNumericFieldElement::didBlur): Added for resetting type-ahead timer.
> > +        (WebCore::DateTimeNumericFieldElement::didFocus): Added for resetting type-ahead timer.
> 
> Why do you need to reset it? Is this a behavior change?
These changes prevent accumulating digits on following scenario:

1. Type "1" at hour field
2. Type Tab to move minute field
3. Type Shift+Tab to move hour field
4. Type "2" at hour field.

When above sequence occurred in typeahead timeout, 1 sec. We expect the hour field to "2". Without these changes, the hour field is "12". This may not be happened by human action. I found this during writing layout test.

Created attachment 161605 [details]
Patch 7

Comment on attachment 161605 [details]
Patch 7

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Mention reason of removal of DateTimeEditControlOwnerImpl::focusAndSelectEditControlOwner

[Kent Tamura]

(In reply to comment #20)
> > Why do you need to reset it? Is this a behavior change?
> These changes prevent accumulating digits on following scenario:
> 
> 1. Type "1" at hour field
> 2. Type Tab to move minute field
> 3. Type Shift+Tab to move hour field
> 4. Type "2" at hour field.
> 
> When above sequence occurred in typeahead timeout, 1 sec. We expect the hour field to "2". Without these changes, the hour field is "12". This may not be happened by human action. I found this during writing layout test.

Does this issue exist in the current code? If so, we should not fix it in this patch.

Created attachment 161614 [details]
Patch 8

(In reply to comment #23)
> (In reply to comment #20)
> > > Why do you need to reset it? Is this a behavior change?
> > These changes prevent accumulating digits on following scenario:
> > 
> > 1. Type "1" at hour field
> > 2. Type Tab to move minute field
> > 3. Type Shift+Tab to move hour field
> > 4. Type "2" at hour field.
> > 
> > When above sequence occurred in typeahead timeout, 1 sec. We expect the hour field to "2". Without these changes, the hour field is "12". This may not be happened by human action. I found this during writing layout test.
> 
> Does this issue exist in the current code? If so, we should not fix it in this patch.

Yes, this issue exists on the current code. I'll file another bug.

Comment on attachment 161614 [details]
Patch 8

Could you review this patch?
Thanks in advance.

= Change since the last patch =
* Remove changes of DateTimeNumericFieldElement => will be in another patch

[Kent Tamura]

Comment on attachment 161614 [details]
Patch 8

View in context: https://bugs.webkit.org/attachment.cgi?id=161614&action=review

> Source/WebCore/ChangeLog:38
> +        (WebCore::TimeInputType::DateTimeEditControlOwnerImpl::focusAndSelectEditControlOwner): Removed. In this patch, settting focus to field is handled by DateTimeEditElement.

How about input->select() ? It had no effect, right?

Created attachment 161615 [details]
Patch 9

Comment on attachment 161615 [details]
Patch 9

Clearing flags on attachment: 161615

Committed r127226: <http://trac.webkit.org/changeset/127226>

All reviewed patches have been landed.  Closing bug.