Bug 105501 - Fix typing zero into multiple field input
Summary: Fix typing zero into multiple field input
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-19 23:00 PST by Keishi Hattori
Modified: 2012-12-21 04:43 PST (History)
5 users (show)

See Also:


Attachments
Patch (11.13 KB, patch)
2012-12-19 23:25 PST, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (18.50 KB, patch)
2012-12-20 01:41 PST, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (18.45 KB, patch)
2012-12-20 20:23 PST, Keishi Hattori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 2012-12-19 23:00:04 PST
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.
Comment 1 Keishi Hattori 2012-12-19 23:25:23 PST
Created attachment 180285 [details]
Patch
Comment 2 yosin 2012-12-19 23:38:07 PST
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.
Comment 3 WebKit Review Bot 2012-12-20 00:09:32 PST
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
Comment 4 Keishi Hattori 2012-12-20 01:41:01 PST
Created attachment 180300 [details]
Patch
Comment 5 Keishi Hattori 2012-12-20 01:43:57 PST
(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
Comment 6 yosin 2012-12-20 02:06:08 PST
(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().
Comment 7 Keishi Hattori 2012-12-20 03:35:59 PST
(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().
Comment 8 Kent Tamura 2012-12-20 07:36:43 PST
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();
Comment 9 Keishi Hattori 2012-12-20 18:18:48 PST
(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.
Comment 10 Keishi Hattori 2012-12-20 20:23:55 PST
Created attachment 180467 [details]
Patch
Comment 11 Kent Tamura 2012-12-20 20:46:23 PST
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.
Comment 12 Kent Tamura 2012-12-20 20:46:40 PST
Comment on attachment 180467 [details]
Patch

ok
Comment 13 WebKit Review Bot 2012-12-21 04:43:29 PST
Comment on attachment 180467 [details]
Patch

Clearing flags on attachment: 180467

Committed r138365: <http://trac.webkit.org/changeset/138365>
Comment 14 WebKit Review Bot 2012-12-21 04:43:33 PST
All reviewed patches have been landed.  Closing bug.