Bug 42076 - Keyboard operations for <input type=number>
: Keyboard operations for <input type=number>
Status: RESOLVED FIXED
: WebKit
Forms
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 27968
  Show dependency treegraph
 
Reported: 2010-07-12 07:00 PST by
Modified: 2010-07-16 16:12 PST (History)


Attachments
Patch (7.75 KB, patch)
2010-07-12 07:04 PST, Kent Tamura
fishd: review+
fishd: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-07-12 07:00:15 PST
Keyboard operations for <input type=number>
------- Comment #1 From 2010-07-12 07:04:52 PST -------
Created an attachment (id=61224) [details]
Patch
------- Comment #2 From 2010-07-15 09:36:07 PST -------
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.
------- Comment #3 From 2010-07-15 16:35:08 PST -------
(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.
------- Comment #4 From 2010-07-16 10:48:31 PST -------
(From update of attachment 61224 [details])
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.
------- Comment #5 From 2010-07-16 14:55:44 PST -------
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
------- Comment #6 From 2010-07-16 14:59:52 PST -------
+        - 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.
------- Comment #7 From 2010-07-16 15:34:16 PST -------
(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.
------- Comment #8 From 2010-07-16 16:12:49 PST -------
> 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.