Summary: | Fix typing zero into multiple field input | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keishi Hattori <keishi> | ||||||||
Component: | Forms | Assignee: | Keishi Hattori <keishi> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | dglazkov, ojan.autocc, tkent, webkit.review.bot, yosin | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Keishi Hattori
2012-12-19 23:00:04 PST
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. 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 Created attachment 180300 [details]
Patch
(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(). (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 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(); (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. Created attachment 180467 [details]
Patch
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 on attachment 180467 [details]
Patch
ok
Comment on attachment 180467 [details] Patch Clearing flags on attachment: 180467 Committed r138365: <http://trac.webkit.org/changeset/138365> All reviewed patches have been landed. Closing bug. |