Bug 45730 – API to support localized numbers for <input type=number>

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

API to support localized numbers for <input type=number>

[Kent Tamura]

Created attachment 67514 [details]
Patch

[Eric Seidel (no email)]

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

[Kent Tamura]

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

[Alexey Proskuryakov]

+// 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?

[Kent Tamura]

(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.

[Kent Tamura]

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?

[Kent Tamura]

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

[Dimitri Glazkov (Google)]

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

ok.

[Alexey Proskuryakov]

 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?

[Alexey Proskuryakov]

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

Marking r- for the sake of ongoing discussion.

[Kent Tamura]

Created attachment 70515 [details]
Patch rev.4

[Kent Tamura]

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.

[Kent Tamura]

Created attachment 72631 [details]
Patch 5 (rebased)

[Kent Tamura]

Created attachment 73959 [details]
Patch 6 (rebased)

[Kent Tamura]

Created attachment 75777 [details]
Patch 7 (rebased)

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[Kent Tamura]

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

[Kent Tamura]

Created attachment 78100 [details]
Patch 8 (rebased)

[Dimitri Glazkov (Google)]

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

ok.

[Kent Tamura]

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