Bug 42076 – Keyboard operations for <input type=number>

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

[Kent Tamura]

Keyboard operations for <input type=number>

[Kent Tamura]

Created attachment 61224 [details]
Patch

[Erik Arvidsson]

Just curious, this does not seem to cover non latin numbers (try Japanese for example).

Another common behavior of number inputs is that the increment increases if the user holds down the up/down key for a while, making it easier to make big changes.

[Kent Tamura]

(In reply to comment #2)
> Just curious, this does not seem to cover non latin numbers (try Japanese for example).

Right. The number type of HTML5 accepts only ASCII digits.
We should implement conversion from non-ASCII digits to HTML5 number in the future, and change the key input restriction too.

> Another common behavior of number inputs is that the increment increases if the user holds down the up/down key for a while, making it easier to make big changes.

It works with this patch.

[Darin Fisher (:fishd, Google)]

Comment on attachment 61224 [details]
Patch

WebCore/html/HTMLInputElement.cpp:2131
> +      }
nit: please add a new line before the "if (isTextField()..."

WebCore/html/HTMLInputElement.cpp:2119
> +      if (hasSpinButton() && evt->type() == eventNames().keydownEvent && evt->isKeyboardEvent()) {
nit: maybe it is better to put the isTextField case about the spin button
case since text fields are probably more common than spin buttons?

WebCore/html/HTMLInputElement.cpp:2121
> +          if (key == "Up") {
nit: how about using a temporary to avoid having to repeat the
work of calling those functions twice?

  int step = 0;
  if (key == "Up")
      step = 1;
  else if (key == "Down")
      step = -1;
  if (step != 0) {
      stepUpFromRenderer(step);
      evt->setDefaultHandled();
      return;
  }


WebCore/html/HTMLInputElement.cpp:2399
> +  static bool isNumberCharacter(UChar ch)
nit: normally better to put static helper functions at the top of the file.
that way the flow of HTMLInputElement methods is not interrupted by a non-
member function implementation.

i'm a little surprised that there isn't already a function like this
somewhere in webkit.  i didn't see it in ASCIICType.h :(]

R=me with these minor issues addressed.

[Kent Tamura]

Thank you for reviewing.

(In reply to comment #4)
> WebCore/html/HTMLInputElement.cpp:2131
> > +      }
> nit: please add a new line before the "if (isTextField()..."

Done.

> WebCore/html/HTMLInputElement.cpp:2119
> > +      if (hasSpinButton() && evt->type() == eventNames().keydownEvent && evt->isKeyboardEvent()) {
> nit: maybe it is better to put the isTextField case about the spin button
> case since text fields are probably more common than spin buttons?

Did not.  It's possible that non-textfield has spin buttons in the future.

> WebCore/html/HTMLInputElement.cpp:2121
> > +          if (key == "Up") {
> nit: how about using a temporary to avoid having to repeat the
> work of calling those functions twice?

Done.

> WebCore/html/HTMLInputElement.cpp:2399
> > +  static bool isNumberCharacter(UChar ch)
> nit: normally better to put static helper functions at the top of the file.
> that way the flow of HTMLInputElement methods is not interrupted by a non-
> member function implementation.

Moved it to the top.


Landed as http://trac.webkit.org/changeset/63586

[Alexey Proskuryakov]

+        - Reject characters other than + - 0-9 . e E

I think that at least this part of patch is wrong enough to be rolled out. Number formatting is different in different locales, and the implementation is not a step in the right direction.

Please roll this out, and let's discuss number validation separately.

+    return ch == '+' || ch == '-' || ch == '.' || ch == 'e' || ch == 'E'
+        || ch >= '0' && ch <= '9';

It's also formatted incorrectly, this line is not long enough to be wrapped.

+        if (key == "Up") {

Should we be checking for modifiers? What if the user presses Shift+Up to select to beginning, for example?

As far as I can tell, the behavior will be wrong.

WebCore/html/HTMLInputElement.cpp:2131
> +      }
nit: please add a new line before the "if (isTextField()..."

This style of comment is more precise, but I also find it very difficult to read.

[Kent Tamura]

(In reply to comment #6)
> +        - Reject characters other than + - 0-9 . e E
> 
> I think that at least this part of patch is wrong enough to be rolled out. Number formatting is different in different locales, and the implementation is not a step in the right direction.
> Please roll this out, and let's discuss number validation separately.

I think this patch is harmless for the current implementation of <input type=number>.  It doesn't accept localized numbers as a valid value, and we shouldn't submit localized numbers.

I filed Bug 42484 for localized number support.

> +        if (key == "Up") {
> 
> Should we be checking for modifiers? What if the user presses Shift+Up to select to beginning, for example?

Hmm, good point.  I have no idea what should happen for Shift+Up.

[Alexey Proskuryakov]

> I think this patch is harmless for the current implementation of <input type=number>.  It doesn't 
> accept localized numbers as a valid value, and we shouldn't submit localized numbers.

A correct implementation would use an entirely different approach, which is why I think that this is a step in a wrong direction, and should be rolled out. Someone could copy/paste or reuse this wrong code elsewhere.

This makes me unhappy that the support for input type=number isn't behind an ENABLE macro. An <input type=number> that I cannot even use "12345,80" in seems to be premature to expose in a shipping product.