Closed Bug 946390 Opened 6 years ago Closed 6 years ago

Allow content to preventDefault key events targeting <input type=number>

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox29 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file, 1 obsolete file)

We handle the up/down arrow keys for <input type=number> 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 going through the <input type=number> are targeted at that anon text control.

The complication is that on some platforms the up/down arrow keys are handled by text controls to send the cursor to the start/end of the control. The anon text control is no exception. The code that does that ends up causing PreventDefault() to be called 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 happily just go ahead and handle the event in HTMLInputElement::PostHandleEvent despite the fact that |aVisitor.mEventStatus == nsEventStatus_eConsumeNoDefault|...well, almost happily...

The wrinkle is that we still need to allow _script_ to call preventDefault() on the event and have that work, but because nsTextInputListener::HandleEvent only has access to the event (and not aVisitor.mEventStatus) it could only call PreventDefault(). We can't currently distinguish between internal code calling PreventDefault and script calling preventDefault, so the result is that we currently ignore preventDefault calls made by script for up/down arrow key events.

Bug 930374 should provide us with a way to make the distinction between script and internal code calling preventDefault. Once that is fixed we should make sure that we don't ignore script calls.
[The code to handle the up/down arrow keys was introduced in bug 935506.]
Attached patch patch (obsolete) — Splinter Review
Attachment #8343168 - Flags: review?(bugs)
Attached patch patchSplinter Review
Attachment #8343168 - Attachment is obsolete: true
Attachment #8343168 - Flags: review?(bugs)
Attachment #8343169 - Flags: review?(bugs)
Comment on attachment 8343169 [details] [diff] [review]
patch


>-      // XXX we still need to allow script to call preventDefault() on the
>-      // event, but right now we can't tell the difference between the editor
>-      // on script doing that (bug 930374).
>-      StepNumberControlForUserEvent(keyEvent->keyCode == NS_VK_UP ? 1 : -1);
>-      aVisitor.mEventStatus = nsEventStatus_eConsumeNoDefault;
>+      if (!aVisitor.mEvent->mFlags.mDefaultPreventedByContent) {
>+        StepNumberControlForUserEvent(keyEvent->keyCode == NS_VK_UP ? 1 : -1);
>+        aVisitor.mEventStatus = nsEventStatus_eConsumeNoDefault;
>+      }

mDefaultPreventedByContent is not the thing bug 930374 adds.
You want something like
!mDefaultPrevented  || mDefaultPreventedByDefaultHandler
Attachment #8343169 - Flags: review?(bugs) → review-
Comment on attachment 8343169 [details] [diff] [review]
patch

This should be now fine since bug 930374 was change to use mDefaultPreventedByContent.


But I don't understand how the test passed before
Attachment #8343169 - Flags: review- → review+
I wasn't sure about the status of Masayuki's patch, so I don't think I tested this.
https://hg.mozilla.org/mozilla-central/rev/e9b06b13344d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Changing target milestone to Firefox 29 since <input type=number> is now pref'd off for Firefox 28.
Target Milestone: mozilla28 → mozilla29
You need to log in before you can comment on or make changes to this bug.