Skip to main content

Search and Top Navigation

#8901 closed bug (fixed)

Opened December 11, 2012 04:45PM UTC

Closed December 27, 2012 04:06PM UTC

Spinner does not fire start/spin/stop events when calling stepUp()/Down(), pageUp()/Down() methods

Reported by: fedot Owned by: fedot
Priority: minor Milestone: 1.10.0
Component: ui.spinner Version: 1.9.2
Keywords: Cc:
Blocked by: Blocking:
Description

When you assign custom 'spin' event handler it is only called when spinner's own up and down buttons are clicked.

If you try and manipulate spinner via its methods like stepUp(), stepDown(), pageUp() and pageDown(), the 'spin' event handler is ignored.

Here is a test case:

http://jsfiddle.net/NCYZw/7/

I may be wrong, but could it be a typo in the following file on line #287 ?

https://github.com/jquery/jquery-ui/blob/1-9-stable/ui/jquery.ui.spinner.js

if ( !this.spinning || this._trigger( "spin", event, { value: value } ) !== false) {

Shouldn't there be && instead of | | ?

Attachments (0)
Change History (19)

Changed December 11, 2012 04:57PM UTC by scottgonzalez comment:1

component: ui.coreui.spinner
owner: → fedot
status: newpending

This is by design. It doesn't look like we documented why we made this choice, but perhaps it was so that programmatic steps can't be canceled. What are you doing that you need this behavior to change?

Changed December 11, 2012 05:04PM UTC by fedot comment:2

status: pendingnew

I want to have a custom list of possible spinner values.

The only way I found so far is to manipulate spinner's value using the 'spin' event.

This works absolutely fine if the spinner's own controls are used.

However I also require to "increase" or "decrease" spinner's value (i.e., assign next or previous allowed value) by some other events that happen on my page.

I was trying to avoid making it more complex and simply wanted to call standard stepUp()/stepDown() methods (which I hope would invoke already redefined logic handled by the 'spin' event).

Of course I can work around this if this is by design. Just thought it was suspicious.

Changed December 11, 2012 05:14PM UTC by scottgonzalez comment:3

status: newpending

I'm confused about how this would help. Wouldn't your increase and decrease methods fail if the next step isn't in your custom list? If I'm misunderstanding, could you provide a reduced example of what you're trying to accomplish and how the spin event would help? Thanks.

Changed December 12, 2012 11:15AM UTC by fedot comment:4

status: pendingnew

Please see the reduced example here: http://jsfiddle.net/pAXYr/1/

The buttons call stepUp()/Down() methods that do not invoke the 'spin' handler.

Compare that with the spinner's own up/down buttons which work as intended.

Changed December 12, 2012 01:10PM UTC by scottgonzalez comment:5

resolution: → wontfix
status: newclosed

This seems like a pretty big abuse of what spinner does; you should probably be using a select. If you really want to use a spinner, then just move all the spin logic into a separate function and call that from your buttons.

Changed December 12, 2012 01:52PM UTC by fedot comment:6

I already have done so with a separate function. I can't agree with your choice of words though. It isn't an "abuse". Even if we accepted for a moment that it was, it certainly is not a "pretty big" one.

If I had a <select> with hundreds of predefined numeric values instead, THAT would be "a pretty big abuse" (of end-user's brain and page's UI). Spinner is exactly what is logical in this case: it operates with integers only, the value can be changed in fine increments or really fast (by pressing holding the up/down button), there is no need to scroll some huge list of numbers, the control is self-explanatory: the up/down buttons indicate that the value increases or decreases and finally it just looks neater. Can you really imagine a <select> with a hundred of numeric values? This would look like something from 1994.

I was rather hoping that this discussion would come to another logical conclusion: we would agree to introduce new spinner option that will allow to specify a list of all possible values as an array. Seems our logic differs here too. If this as an "abuse" then I guess I should not even attempt raising a feature request...

Changed December 12, 2012 01:56PM UTC by fedot comment:7

BTW, going back to the original issue, I still fail to see how not raising the spin event when using the step/page methods is justified. Or to put it another way: what harm would it bring if it was raised?

The current spinner behaviour with regards to the issue described in this ticket is not documented, therefore, it can be implemented either way. If you change the current behaviour, you don't break any "promises". You are making it better for somebody who may need it.

Changed December 12, 2012 02:05PM UTC by scottgonzalez comment:8

Fair enough; your use case may not be a pretty big abuse of spinner, but your implementation is. I should have looked past the implementation. You should try using the incremental option instead of the spin event.

As I mentioned earlier, I believe the reason we did this was so that programmatic steps cannot be prevented.

Changed December 12, 2012 02:25PM UTC by fedot comment:9

Thank you, I will take a look at the 'incremental' option. Would it really be a lesser abuse if I changed it there? I think neither is an abuse (or both of them are -- depends how you look at it). What I would say, nevertheless, is that abuse starts where flexibility and extensibility ends (so how about that new option then? ;-) )

And on the other topic, if programmatic steps were possible to prevent, this would be bad... how? Shouldn't the choice be left to a programmer instead of being hardcoded?

Changed December 12, 2012 02:33PM UTC by scottgonzalez comment:10

Spinner has plenty of extension points. There are multiple ways to do what you want. Adding options is usually a last resort.

Changed December 12, 2012 03:32PM UTC by fedot comment:11

Fair enough. I fully agree with the last point about adding options as a last resort but only when the first part ("plenty of extension points" is true). Let's see.

There are in total 3 possible "entry points" where the logic could be changed: the 'spin' event, the 'start' event and the 'incremental' option - those are the only possibilities to trigger user function that could be used to manipulate the widget before it gets new value assigned.

I tried to use the suggested 'incremental' spinner option but failed to find a way to make it work (I'd like to be corrected) and here is why. When that 'incremental' event is fired its handler gets passed nothing but a 'number of spins' which is an absolute value. Without any reference to the event that originated spinning it is impossible to see which direction the spinning is going to go (because the spinner's new value has not been calculated yet). No luck here.

Unlike the 'incremental' option, the 'spin' event does offer you an object where the newly obtained but not yet assigned spinner value is ready and available (and all of that is nicely documented too). You can see which way the spinning is going and you can easily manipulate the final value. This way of doing things has been deemed abusive so we also discount it.

That leaves us with the 'start' event which does not offer the newly computed value so getting the spinning direction is harder (you need to check which button started the event). In addition, just like the 'spin' event, it does not fire when stepUp()/Down() methods are used. Therefore it is in no way better than the 'spin' event, in fact it is worse.

So, talking about "multiple ways of doing what I want". Not really. It seems there isn't any other way except what's already been suggested above: pointing 'spin' event to my own custom function that will mangle the values and then calling the same function instead of the built-in stepUp()/stepDown() methods. I would like to be corrected but while it may look like "spinner has plenty of extension points" the only possible way to achieve what I want is deemed abusive and there is no other way no matter how you spin it (pun intended).

Now I have also established that neither 'spin' nor 'start/stop' events fire when programmatic steps are used and yet, there is still no clarity on the main topic of this ticket: WHY is it bad to be able to prevent programmatic steps?

Changed December 12, 2012 03:40PM UTC by scottgonzalez comment:12

resolution: wontfix
status: closedreopened

I feel like we (the jQuery UI team) probably had a long discussion about this, but seeing as how there are no notes about this and I can't recall any discussions off the top of my head, I'm going to re-open this. Thanks for sticking with it :-)

I'd still like to find a better way to accomplish what you're trying to do though (hopefully without adding a new options).

Changed December 12, 2012 03:40PM UTC by scottgonzalez comment:13

status: reopenedopen
summary: Spinner does not fire 'spin' events when calling stepUp()/Down(), pageUp()/Down() methodsSpinner does not fire start/spin/stop events when calling stepUp()/Down(), pageUp()/Down() methods

Changed December 12, 2012 03:46PM UTC by fedot comment:14

Thanks ;-)

Changed December 12, 2012 03:48PM UTC by scottgonzalez comment:15

Userland solution: http://jsfiddle.net/pAXYr/1/

Changed December 12, 2012 03:50PM UTC by scottgonzalez comment:16

Nevermind, that fails for the same reason you mentioned of not being able to tell the direction you're moving.

Changed December 27, 2012 12:30PM UTC by jzaefferer comment:17

Scott, your last fiddle linked to the same fiddle fedot had above. Can't tell what you had in mind there.

I've looked for other places where calling methods triggers events (or not).

Seems likely that we modelled spinner after slider, but looking at other widgets, this seems like the wrong choice. Both slider and spinner should trigger the respective slider and spin events when changing the value programmatically. I don't see why programmatic events shouldn't be cancellable. It may seem like it encourages spreading user code across various places, but that doesn't have to be a bad thing.

Changed December 27, 2012 02:07PM UTC by scottgonzalez comment:18

This is what I meant to link to: http://jsfiddle.net/pAXYr/2/

But as I said right after that, it's a busted solution anyway.

Changed December 27, 2012 04:06PM UTC by Jörn Zaefferer comment:19

resolution: → fixed
status: openclosed

Spinner: Trigger start/spin/stop events when calling step/page methods. Fixes #8901 - Spinner does not fire start/spin/stop events when calling stepUp()/Down(), pageUp()/Down() methods

Changeset: 0d53fbfd0b7651652601b3b8577225ab753aab44