935506 - Make <input type=number> respond to the up/down arrow keys

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Jonathan Watt [:jwatt]]

We should make <input type=number> respond to the up/down/left/right/home/end keys as appropriate.

Comment 1

[Jonathan Watt [:jwatt]]

I should probably note that this is currently blocked on bug 930374. The story goes something like this:

I want to handle the up/down arrow keys in HTMLInputElement::PostHandleEvent to have them increase/decrease the value of the number control. Because <input type=number> has a nested <input type=text> that takes focus for the number control any keyboard events are targeted at that anon text control. On some platforms the up/down arrow keys are handled by text controls to send the cursor to the start/end of the control. As a result that code calls PreventDefault() here:

https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsTextEditorState.cpp?rev=371af5899d27&mark=871-872,877-878#838

As it happens it's not an issue that the text control handles the event. When the up/down arrow keys are pressed and cause the value of the number input to change we know that the text in the text control is going to change, and that's going to cause the cursor to be moved to the end of the text control. Since the handling of the key event by the text control is going to be cancelled out almost immediately, and never seen by the user, we can happily just go ahead and handle the event despite the fact that |aVisitor.mEventStatus == nsEventStatus_eConsumeNoDefault|...well, almost...

The issue is that we still need to support _script_ calling preventDefault() on the event, but because nsTextInputListener::HandleEvent only has access to the event and not aVisitor.mEventStatus it could only call PreventDefault(). As a result we can't distinguish between this internal code handling the event and script handling the event because |aVisitor.mEvent->mFlags.mDefaultPrevented| is true in both cases. :/

Anyway, bug 930374 should provide us with a way to make the distinction and proceed with this bug.

Comment 2

[Jonathan Watt [:jwatt]]

FWIW the bindings that handle the keys for the editor for the various platforms are here:

http://mxr.mozilla.org/mozilla-central/source/content/xbl/builtin/

Comment 3

[Jonathan Watt [:jwatt]]

Apparently smaug might be willing to take a patch to fix this bug, then a follow-up bug to fix script preventDefault() calls, so long as flipping the pref on for release is blocked by that follow-up. There shouldn't even be any need to ifdef the code for B2G/Fennec since the up/arrow keys are not something that you're likely to see there. Anyways, I'll attach what I have for this shortly after I've given it another check over.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch

What do you think?

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8337321 [details] [diff] [review]
patch


>   if (NS_SUCCEEDED(rv)) {
>-    if (nsEventStatus_eIgnore == aVisitor.mEventStatus) {
>+    WidgetKeyboardEvent* keyEvent = aVisitor.mEvent->AsKeyboardEvent();
>+    if (mType ==  NS_FORM_INPUT_NUMBER && keyEvent &&
>+        keyEvent->message == NS_KEY_PRESS &&
>+        (keyEvent->keyCode == NS_VK_UP || keyEvent->keyCode == NS_VK_DOWN) &&
>+        !(keyEvent->IsShift() || keyEvent->IsControl() ||
>+          keyEvent->IsAlt() || keyEvent->IsMeta() ||
>+          keyEvent->IsAltGraph() || keyEvent->IsFn() ||
>+          keyEvent->IsOS())) {
This is missing trustness check.
New code should handle it correctly. Add also some test that non-trusted events don't change the value of the form control.
(Would be also good to test other browsers)

With that, r=me

Comment 7

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02a1d87c5c6a

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to Olli Pettay [:smaug] from comment #6)
> New code should handle it correctly. Add also some test that non-trusted
> events don't change the value of the form control.

Done.

> (Would be also good to test other browsers)

I checked in Chrome, which also seemed to ignore events if they weren't trusted in this case.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/02a1d87c5c6a

Comment 10

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified as fixed using the latest Aurora build 29.0a2 (20140304004003) under Win 7 64-bit, Ubuntu 32-bit and Mac OSX 10.9.