WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
105501
Fix typing zero into multiple field input
https://bugs.webkit.org/show_bug.cgi?id=105501
Summary
Fix typing zero into multiple field input
Keishi Hattori
Reported
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.
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Keishi Hattori
Comment 1
2012-12-19 23:25:23 PST
Created
attachment 180285
[details]
Patch
yosin
Comment 2
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.
WebKit Review Bot
Comment 3
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
Keishi Hattori
Comment 4
2012-12-20 01:41:01 PST
Created
attachment 180300
[details]
Patch
Keishi Hattori
Comment 5
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
yosin
Comment 6
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().
Keishi Hattori
Comment 7
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().
Kent Tamura
Comment 8
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();
Keishi Hattori
Comment 9
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.
Keishi Hattori
Comment 10
2012-12-20 20:23:55 PST
Created
attachment 180467
[details]
Patch
Kent Tamura
Comment 11
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.
Kent Tamura
Comment 12
2012-12-20 20:46:40 PST
Comment on
attachment 180467
[details]
Patch ok
WebKit Review Bot
Comment 13
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
>
WebKit Review Bot
Comment 14
2012-12-21 04:43:33 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug