Bug 105501 – Fix typing zero into multiple field input

• Component: Forms
• Status: RESOLVED FIXED

[Keishi Hattori]

Typing zero into certain fields for multiple field input will turn it into 1.
Typing zero in 12 hour field will set 12 can move to the next field so you can't type 09 to enter 9 o clock.

[Keishi Hattori]

Created attachment 180285 [details]
Patch

Comment on attachment 180285 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +

Could you write new behavior what this patch does?

> Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:136
> +    if (charCode < '0' || charCode > '9')

We want to handle localized digit. We have been using convertFromLocalizedNumber().

> Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:-169
> -    m_lastDigitCharTime = 0;

Can we remove this reset timer statement? Please try "1" + Backspace + "7". It should be "17".

> Source/WebCore/html/shadow/DateTimeNumericFieldElement.h:99
> +    mutable StringBuilder m_typeAheadBuffer;

It seems we don't need to have "mutable" for m_typeAheadBuffer.

[WebKit Review Bot]

Comment on attachment 180285 [details]
Patch

Attachment 180285 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15411869

New failing tests:
fast/forms/month-multiple-fields/month-multiple-fields-keyboard-events.html
fast/forms/time-multiple-fields/time-multiple-fields-keyboard-events.html
fast/forms/week-multiple-fields/week-multiple-fields-ax-value-changed-notification.html
fast/forms/month-multiple-fields/month-multiple-fields-ax-value-changed-notification.html
fast/forms/time-multiple-fields/time-multiple-fields-ax-value-changed-notification.html
fast/forms/datetimelocal-multiple-fields/datetimelocal-multiple-fields-keyboard-events.html
fast/forms/date-multiple-fields/date-multiple-fields-keyboard-events.html
fast/forms/week-multiple-fields/week-multiple-fields-keyboard-events.html

[Keishi Hattori]

Created attachment 180300 [details]
Patch

[Keishi Hattori]

(In reply to comment #2)
> (From update of attachment 180285 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180285&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +
> 
> Could you write new behavior what this patch does?
Done.
> > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:136
> > +    if (charCode < '0' || charCode > '9')
> 
> We want to handle localized digit. We have been using convertFromLocalizedNumber().
Done.
> > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:-169
> > -    m_lastDigitCharTime = 0;
> 
> Can we remove this reset timer statement? Please try "1" + Backspace + "7". It should be "17".
Fixed.
> > Source/WebCore/html/shadow/DateTimeNumericFieldElement.h:99
> > +    mutable StringBuilder m_typeAheadBuffer;
> 
> It seems we don't need to have "mutable" for m_typeAheadBuffer.
StringBuilder::toString is not a const and DateTimeNumericFieldElement::visibleValue() is const

(In reply to comment #5)
> (In reply to comment #2)
> > > Source/WebCore/html/shadow/DateTimeNumericFieldElement.h:99
> > > +    mutable StringBuilder m_typeAheadBuffer;
> > 
> > It seems we don't need to have "mutable" for m_typeAheadBuffer.
> StringBuilder::toString is not a const and DateTimeNumericFieldElement::visibleValue() is const

If so, this function should not be "const" and name could be visualValueAndResetTypeAhead().

If we have StringBuilder::toStringWithoutReseting() or so, we can split this function into visbleValue() and resetTypeAhead().

[Keishi Hattori]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #2)
> > > > Source/WebCore/html/shadow/DateTimeNumericFieldElement.h:99
> > > > +    mutable StringBuilder m_typeAheadBuffer;
> > > 
> > > It seems we don't need to have "mutable" for m_typeAheadBuffer.
> > StringBuilder::toString is not a const and DateTimeNumericFieldElement::visibleValue() is const
> 
> If so, this function should not be "const" and name could be visualValueAndResetTypeAhead().
> 
> If we have StringBuilder::toStringWithoutReseting() or so, we can split this function into visbleValue() and resetTypeAhead().

visibleValue() isn't reseting the type ahead buffer. I think StringBuilder is just reallocating a new buffer when we call toString().

[Kent Tamura]

Comment on attachment 180300 [details]
Patch

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

> Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:113
> +    int value = typeAheadValue();
> +    m_typeAheadBuffer.clear();
> +    if (value >= 0)
> +        setValueAsInteger(value, DispatchEvent);

If the field is for 1-12h and m_typeAheadBuffer is "0", this code will set "0".  I don't think it's a correct behavior.

Dispatching events at this timing looks dangerous.  But I think it's ok because an Event object for this 'blur' event has a RefPtr for this.

> Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:225
> +    if (m_typeAheadBuffer.length() > 0) {
> +        bool ok = false;
> +        return m_typeAheadBuffer.toString().toInt(&ok);

.length() never be negative.  It should be
  if (m_typeAheadBuffer.length())

The variable 'ok' is not read.  It should be omitted.
  return m_typeAheadBuffer.toString().toInt();

[Keishi Hattori]

(In reply to comment #8)
> (From update of attachment 180300 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180300&action=review
> 
> > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:113
> > +    int value = typeAheadValue();
> > +    m_typeAheadBuffer.clear();
> > +    if (value >= 0)
> > +        setValueAsInteger(value, DispatchEvent);
> 
> If the field is for 1-12h and m_typeAheadBuffer is "0", this code will set "0".  I don't think it's a correct behavior.
This was my intension. Typing "0" shows "00" but doesn't set the value. When blur occurs, setValueAsInteger(0) is called which will set "12" for a 1-12h field and "01" for a month field.

> Dispatching events at this timing looks dangerous.  But I think it's ok because an Event object for this 'blur' event has a RefPtr for this.
I'm not sure but I think its ok.
> > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:225
> > +    if (m_typeAheadBuffer.length() > 0) {
> > +        bool ok = false;
> > +        return m_typeAheadBuffer.toString().toInt(&ok);
> 
> .length() never be negative.  It should be
>   if (m_typeAheadBuffer.length())
Fixed.
> The variable 'ok' is not read.  It should be omitted.
>   return m_typeAheadBuffer.toString().toInt();
Fixed.

[Keishi Hattori]

Created attachment 180467 [details]
Patch

[Kent Tamura]

Comment on attachment 180300 [details]
Patch

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

>>> Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:113
>>> +        setValueAsInteger(value, DispatchEvent);
>> 
>> If the field is for 1-12h and m_typeAheadBuffer is "0", this code will set "0".  I don't think it's a correct behavior.
>> 
>> Dispatching events at this timing looks dangerous.  But I think it's ok because an Event object for this 'blur' event has a RefPtr for this.
> 
> This was my intension. Typing "0" shows "00" but doesn't set the value. When blur occurs, setValueAsInteger(0) is called which will set "12" for a 1-12h field and "01" for a month field.

ok, this behavior is not the main issue of this bug and is compatible with the current behavior.  Let's discuss this later.

[Kent Tamura]

Comment on attachment 180467 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 180467 [details]
Patch

Clearing flags on attachment: 180467

Committed r138365: <http://trac.webkit.org/changeset/138365>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.