Bug 45730

Created attachment 67514 [details] Patch

Attachment 67514 [details] did not build on mac: Build output: http://queues.webkit.org/results/4042003

Created attachment 67518 [details] Patch 2 (Fix Mac build)

+// Returns true if the input character can be used to represent a +// localized number. For example, this should return true for 0-9 . , + +// - for en-US locale. +bool isLocalizedNumberCharacter(UChar32); Should this depend on browser locale, or on page language?

(In reply to comment #4) > +// Returns true if the input character can be used to represent a > +// localized number. For example, this should return true for 0-9 . , + > +// - for en-US locale. > +bool isLocalizedNumberCharacter(UChar32); > > Should this depend on browser locale, or on page language? It should be the same as other functions in this file. So, it's browser locale. I'll update the comment.

Note that I'll add LocalizedNumberICU.cpp after this patch. Probably Qt needs LocalizedNumberQt.cpp, which uses QLocale, and Mac needs LocalizedNumberMac.mm for NSNumberFormatter?

Created attachment 70209 [details] Patch 2 (comment update, rebased)

Comment on attachment 70209 [details] Patch 2 (comment update, rebased) ok.

static bool isNumberCharacter(UChar ch) { - return ch == '+' || ch == '-' || ch == '.' || ch == 'e' || ch == 'E' - || (ch >= '0' && ch <= '9'); + return isLocalizedNumberCharacter(ch) || ch == '+' || ch == '-' || ch == '.' || ch == 'e' || ch == 'E' || (ch >= '0' && ch <= '9'); } This is confusing. Where does the definition come from? For example, a comment before isLocalizedNumberCharacter() says that it includes both period and comma for U.S. locale. But this only forces period. Why? +++ b/WebCore/rendering/RenderTextControlSingleLine.cpp @@ -693,7 +693,7 @@ void RenderTextControlSingleLine::updateFromElement() shouldUpdateValue = static_cast<HTMLTextFormControlElement*>(node())->supportsPlaceholder() || !static_cast<HTMLInputElement*>(node())->formControlValueMatchesRenderer(); } if (shouldUpdateValue) - setInnerTextValue(inputElement()->value()); + setInnerTextValue(inputElement()->visibleValue()); } What is this change expected to provide? It seems out of place in a bug that adds an API. ChangeLog comment doesn't explain the reason for this change. The concept of "visibleValue" in general seems unclear. How is one supposed to perform DOM manipulation if rendered value is different from DOM value? E.g. what if JS code want to select part of the number in the input field?

Comment on attachment 70209 [details] Patch 2 (comment update, rebased) Marking r- for the sake of ongoing discussion.

Created attachment 70515 [details] Patch rev.4

Thank you for the comments. (In reply to comment #9) > static bool isNumberCharacter(UChar ch) > { > - return ch == '+' || ch == '-' || ch == '.' || ch == 'e' || ch == 'E' > - || (ch >= '0' && ch <= '9'); > + return isLocalizedNumberCharacter(ch) || ch == '+' || ch == '-' || ch == '.' || ch == 'e' || ch == 'E' || (ch >= '0' && ch <= '9'); > } > > This is confusing. Where does the definition come from? For example, a comment before isLocalizedNumberCharacter() says that it includes both period and comma for U.S. locale. But this only forces period. Why? We try to parse a value in a localized number format, and try to parse it in the HTML5 format if the parsing in the localized number format fails. I added a comment about this to HTMLInputElement::sanitizeValue() and ChangeLog. > +++ b/WebCore/rendering/RenderTextControlSingleLine.cpp > @@ -693,7 +693,7 @@ void RenderTextControlSingleLine::updateFromElement() > shouldUpdateValue = static_cast<HTMLTextFormControlElement*>(node())->supportsPlaceholder() || !static_cast<HTMLInputElement*>(node())->formControlValueMatchesRenderer(); > } > if (shouldUpdateValue) > - setInnerTextValue(inputElement()->value()); > + setInnerTextValue(inputElement()->visibleValue()); > } > > What is this change expected to provide? It seems out of place in a bug that adds an API. ChangeLog comment doesn't explain the reason for this change. > > The concept of "visibleValue" in general seems unclear. How is one supposed to perform DOM manipulation if rendered value is different from DOM value? ok, I added an explanation of visibleValue() to ChangeLog. visibleValue() is a localized representation of a number, and value() (DOM manipulation) is the HTML5 number representation. > E.g. what if JS code want to select part of the number in the input field? We can't do it. HTML5 doesn't provide selection API for type=number.

Created attachment 72631 [details] Patch 5 (rebased)

Created attachment 73959 [details] Patch 6 (rebased)

Created attachment 75777 [details] Patch 7 (rebased)

Attachment 75777 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'HEAD']" exit_code: 128 error: git checkout-index: unable to write file LayoutTests/ChangeLog error: git checkout-index: unable to write file LayoutTests/fast/css/custom-font-xheight.html error: git checkout-index: unable to write file WebCore/ChangeLog fatal: Could not reset index file to revision 'HEAD'. If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 75777 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 75777 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 75777 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 75777 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 75777 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 75777 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2 Updating OpenSource Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061 Died at WebKitTools/Scripts/update-webkit line 132. If any of these errors are false positives, please file a bug against check-webkit-style.

Ping. ap, dglazkov, would you take a look at the patch please?

Created attachment 78100 [details] Patch 8 (rebased)

Comment on attachment 78100 [details] Patch 8 (rebased) ok.

Landed: http://trac.webkit.org/changeset/76661