Closed Bug 935501 Opened 6 years ago Closed 6 years ago

Get mouse events working for <input type=number>'s spin up/down buttons

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla28
Tracking Status
firefox28 --- disabled
firefox29 --- verified

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file, 4 obsolete files)

We need <input type=number>'s spin up/down buttons to do the right thing when clicked on. This will be a little less than straightforward if we want the number to keep spinning if the user keeps a button depressed.
Attached patch patch (obsolete) — Splinter Review
Since you're looking at 935506 now...

The idea here is that mousing down on a spin button will immediately step up/down then, after half a second delay, will step again every 50 ms. I'm being pretty aggressive about stopping the timer. That said I do allow the mouse pointer to move from the up to the down button and visa versa while the mouse button is still depressed and it just changes the direction of spin without cancelling the spin. If the mouse leaves a button to a non-button I stop the repeat timer though.
Attachment #8336718 - Flags: review?(bugs)
Attached patch patch (obsolete) — Splinter Review
Per-IRC, here's a version that uses mouse capture instead of NS_MOUSE_EXIT_SYNTH.
Attachment #8336718 - Attachment is obsolete: true
Attachment #8336718 - Flags: review?(bugs)
Attachment #8337243 - Flags: review?(bugs)
Attached patch patch (obsolete) — Splinter Review
I've slightly changed how the code determines whether a pointer event is over one of the spin buttons to better fit with what I'm having to do to get native theming working.
Attachment #8337243 - Attachment is obsolete: true
Attachment #8337243 - Flags: review?(bugs)
Attachment #8337663 - Flags: review?(bugs)
this is about mouse events, not pointer events, right?
yes
Summary: Get pointer events working for <input type=number>'s spin up/down buttons → Get mouse events working for <input type=number>'s spin up/down buttons
Comment on attachment 8337663 [details] [diff] [review]
patch

I think this should use nsRepeatService so that we get consistent behavior with
scrollbars.


>+  if (mType == NS_FORM_INPUT_NUMBER &&
>+      mNumberControlSpinButtonTimer) {
>+    // If the timer is running the user has depressed the mouse on one of the
>+    // spin buttons. If the mouse exits the button we either want to reverse
>+    // the direction of spin if it has moved over the other button, or else we
>+    // want to end the spin. We do this here (rather than in PostHandleEvent)
>+    // because we don't want to let content preventDefault() the ending of the
>+    // spin.
s/the ending/to end/ or something?


>+    if (aVisitor.mEvent->message == NS_MOUSE_MOVE) {
>+      // Be agressive about stoping the timer:
aggressive 
stopping, I think


I'd like to see a new patch using repeat service.
Attachment #8337663 - Flags: review?(bugs) → review-
Attached patch patch (obsolete) — Splinter Review
(In reply to Olli Pettay [:smaug] from comment #6)
> I think this should use nsRepeatService

Ah, nice. Agreed!
Attachment #8337663 - Attachment is obsolete: true
Attachment #8338119 - Flags: review?(bugs)
Comment on attachment 8338119 [details] [diff] [review]
patch


Make sure the timer is stopped if nsHTMLInputElement dies while the time is active. Otherwise you have a sg:crit bug.

>+  if (mType == NS_FORM_INPUT_NUMBER) {
>+    if (mNumberControlSpinnerIsSpinning) {
>+      // If the timer is running the user has depressed the mouse on one of the
>+      // spin buttons. If the mouse exits the button we either want to reverse
>+      // the direction of spin if it has moved over the other button, or else
>+      // we want to end the spin. We do this here (rather than in
>+      // PostHandleEvent) because we don't want to let content preventDefault()
>+      // the end of the spin.
>+      if (aVisitor.mEvent->message == NS_MOUSE_MOVE) {
trusted event check here please.


>+            if (aVisitor.mEventStatus != nsEventStatus_eConsumeNoDefault) {
>+              // We didn't handle this to step up/down. Whateven this was, be
Whatever
>+              // aggressive about stopping the spin. (And don't set
>+              // nsEventStatus_eConsumeNoDefault after doing so, since that
>+              // might prevent, say, the context menu from opening.)
>+              StopNumberControlSpinnerSpin();
But this all is odd anyway. Content may dispatch mouseup/down to stop spinning?
(this if check doesn't have isTrusted check or anything)
Attachment #8338119 - Flags: review?(bugs) → review-
Attached patch patchSplinter Review
Attachment #8340353 - Flags: review?(bugs)
Attachment #8338119 - Attachment is obsolete: true
Comment on attachment 8340353 [details] [diff] [review]
patch

So we need still some tests for this.
Shouldn't be too hard to write.
Attachment #8340353 - Flags: review?(bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe4999863b7

I added some tests, although to test the spin we need to change the firing of 'input' events as discussed on IRC. I'll file a bug for that shortly and comment with the bug number here.
https://hg.mozilla.org/mozilla-central/rev/7fe4999863b7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Verified as fixed using the latest Aurora build 29.0a2 (20140304004003) under Win 7 64-bit, Ubuntu 32-bit and Mac OSX 10.9.
Status: RESOLVED → VERIFIED
Keywords: verifyme
QA Contact: petruta.rasa
You need to log in before you can comment on or make changes to this bug.