Opened 5 years ago

Closed 5 years ago

#8901 closed bug (fixed)

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 | | ?

Change History (19)

comment:1 Changed 5 years ago by Scott González

Component: ui.coreui.spinner
Owner: set to 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?

comment:2 Changed 5 years ago by fedot

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.

comment:3 Changed 5 years ago by Scott González

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.

comment:4 Changed 5 years ago by fedot

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.

comment:5 Changed 5 years ago by Scott González

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.

comment:6 Changed 5 years ago by fedot

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...

comment:7 Changed 5 years ago by fedot

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.

comment:8 Changed 5 years ago by Scott González

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.

comment:9 Changed 5 years ago by fedot

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?

comment:10 Changed 5 years ago by Scott González

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

comment:11 Changed 5 years ago by fedot

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?

comment:12 Changed 5 years ago by Scott González

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).

comment:13 Changed 5 years ago by Scott González

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

comment:14 Changed 5 years ago by fedot

Thanks ;-)

comment:15 Changed 5 years ago by Scott González

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

comment:16 Changed 5 years ago by Scott González

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

comment:17 Changed 5 years ago by Jörn Zaefferer

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.

comment:18 Changed 5 years ago by Scott González

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.

comment:19 Changed 5 years ago by Jörn Zaefferer

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

Note: See TracTickets for help on using tickets.