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
Patch (18.50 KB, patch)
2012-12-20 01:41 PST, Keishi Hattori
no flags
Patch (18.45 KB, patch)
2012-12-20 20:23 PST, Keishi Hattori
no flags
Keishi Hattori
Comment 1 2012-12-19 23:25:23 PST
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
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
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.