Opened 11 years ago

Last modified 5 years ago

#3762 open bug

Slider: handles not restricted properly when set programmatically

Reported by: scottjehl Owned by:
Priority: major Milestone: none
Component: ui.slider Version: 1.6rc4
Keywords: Cc:
Blocked by: Blocking:

Description

When setting handle values via the 'values' option or method, the handles are not restricted from overlapping and passing one another.

For example: $(...).slider({values: [5,0], range: true}); This will result in a range that's not actually valid in using the slider, since the range will be negative. The second handle left of the first. The handle placement and getter value of the slider should be validated to [5,5] in this case. The option of course can be left as is.

The same goes for the values method. Both need to be restricted by the handle values.

Attachments (1)

3762.ui.slider.js.patch (1.4 KB) - added by andrew_ 11 years ago.
Proposed FIx

Download all attachments as: .zip

Change History (32)

comment:1 Changed 11 years ago by rdworth

Milestone: 1.8

comment:3 Changed 11 years ago by andrew_

Owner: set to andrew_
Status: newassigned

comment:4 Changed 11 years ago by andrew_

Proposed fix would be to enforce that values following one another in linear order are greater to or equal to a predecessor. ie. index:0 >= index:1 >= index:n.

comment:5 in reply to:  4 Changed 11 years ago by andrew_

Replying to andrew_:

Proposed fix would be to enforce that values following one another in linear order are greater to or equal to a predecessor. ie. index:0 >= index:1 >= index:n.

To specify, values would either;

  1. be adjusted accordingly
    • Invalid values would be replaced by the closest preceeding valid value
  2. the plugin would not generate a ui, awaiting valid values.

comment:6 Changed 11 years ago by Scott González

andrew_, go ahead with the proposed solution.

Each individual value should be adjusted according to the following rules:

  • if less than min, set to min
  • if less than previous handle value, set to previous handle value
  • if greater than max, set to max
  • if greater than next handle value, set to next handle value

Changed 11 years ago by andrew_

Attachment: 3762.ui.slider.js.patch added

Proposed FIx

comment:8 Changed 10 years ago by rdworth

Owner: changed from andrew_ to rdworth

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

Priority: criticalmajor

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

Milestone: 1.9.01.11.0

comment:11 Changed 7 years ago by mikesherov

Keywords: haspatch added
Owner: rdworth deleted
Status: assignedopen

comment:12 Changed 7 years ago by rooby

I can confirm this is still an issue.

See this example: http://jsfiddle.net/SvwTc/2/

The slider starts out with invalid values and strange things happen when you try to slide the slider.

Then you can use the button to see the values are able to be set back to their invalid state.

comment:13 Changed 7 years ago by lucas.ams

How can I assign myself as the owner of this ticket? I just fixed this bug.

By now, if range is true, only two handles are supported. So, here goes the solution:

_createRange: function() {
		var options = this.options,
			classes = "";
		
		if ( options.range ) {
			if ( options.range === true ) {
				if ( !options.values ) {
					options.values = [ this._valueMin(), this._valueMin() ];
				} else if ( options.values.length && options.values.length !== 2 ) {
					options.values = [ options.values[0], options.values[0] ];
				} else if ( $.isArray( options.values ) ) {
					options.values = options.values.slice(0);
				}
			}
	
			if(options.values[0] < this._valueMin()) {
				options.values[0] = this._valueMin();
			}
			if(options.values[1] > this._valueMax()) {
				options.values[1] = this._valueMax();
			}
			if(options.values[0] > options.values[1]) {
				options.values[1] = options.values[0]
			}
			
                          ... ... ...
Version 0, edited 7 years ago by lucas.ams (next)

comment:14 Changed 7 years ago by Scott González

@lucas.ams Please do not paste blocks of code into tickets. If you'd like to submit a fix, please sign our CLA and submit a pull request. Make sure to include a unit test showing that the patch actually does fix the problem. Thanks.

comment:15 Changed 7 years ago by lucas.ams

Sorry @scott.gonzalez, I'm new here! But how can I assign myself as the owner of this ticket? Can it be only after submit a pull request?

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

Tickets don't get assigned to members of the community. Just send the pull request.

comment:18 in reply to:  15 Changed 6 years ago by dekajp

Replying to lucas.ams:

Sorry @scott.gonzalez, I'm new here! But how can I assign myself as the owner of this ticket? Can it be only after submit a pull request?

is this still open ?

comment:19 in reply to:  16 ; Changed 6 years ago by dekajp

Replying to scott.gonzalez:

Tickets don't get assigned to members of the community. Just send the pull request.

Is it still open ?

comment:20 in reply to:  19 ; Changed 6 years ago by tj.vantoll

Replying to dekajp:

Replying to scott.gonzalez:

Tickets don't get assigned to members of the community. Just send the pull request.

Is it still open ?

Yes - https://github.com/jquery/jquery-ui/pull/1016

comment:21 in reply to:  20 Changed 6 years ago by dekajp

Replying to tj.vantoll:

Replying to dekajp:

Replying to scott.gonzalez:

Tickets don't get assigned to members of the community. Just send the pull request.

Is it still open ?

Yes - https://github.com/jquery/jquery-ui/pull/1016

@TJ pull 1016 is for ticket #9376 , this request is for Range validation . i think it's different.

comment:22 Changed 6 years ago by tj.vantoll

Keywords: haspatch removed
Summary: slider handles not restricted properly when set programmaticallySlider: handles not restricted properly when set programmatically

@dekajp Ah you're right, thanks. But to answer your original question http://jsfiddle.net/SvwTc/2/ shows that this is still a problem and this ticket is open.

comment:23 in reply to:  6 ; Changed 6 years ago by dekajp

Replying to scott.gonzalez:

andrew_, go ahead with the proposed solution.

Each individual value should be adjusted according to the following rules:

  • if less than min, set to min
  • if less than previous handle value, set to previous handle value
  • if greater than max, set to max
  • if greater than next handle value, set to next handle value

@scott @tj

Will this approach work ? ( if array size is more than 2 - slice it first 2 element) Sort the Array . compare the zero index with Min value if not equal to min value , replace it by min value . compare 1st index with Max value if not equal to max value ,replace it with max value.

comment:24 in reply to:  23 Changed 6 years ago by dekajp

Replying to dekajp:

Replying to scott.gonzalez:

andrew_, go ahead with the proposed solution.

Each individual value should be adjusted according to the following rules:

  • if less than min, set to min
  • if less than previous handle value, set to previous handle value
  • if greater than max, set to max
  • if greater than next handle value, set to next handle value

@scott @tj

Will this approach work ? ( if array size is more than 2 - slice it first 2 element) Sort the Array . compare the zero index with Min value if not equal to min value , replace it by min value . compare 1st index with Max value if not equal to max value ,replace it with max value.

@scott I think i see your point - you do not want "auto correct" - so that developer can see the problem and fix it , also the slider should behave normally.

  • [min=1,max=100][5,2] = [2,2]
  • [min=10,max=100][1,15] = [10,15]
  • [min=10,max=100][15,1] = [1,10] expected [10,10] [ Fails at no.4 - if greater than next handle value, set to next handle value ]
  • [min=1,max=20][5,25] = [5,20]
  • [min=1,max=20][25,5] = [5,5]
  • [min=10,max=20][25,5] =[5,10] expected [10,10] [ Fails at No. 2 - if less than previous handle value, set to previous handle value ]

i think we have to add this clause - when setting to next or previous handle value if newvalue is less than min set min or if more than max set max.

Last edited 6 years ago by dekajp (previous) (diff)

comment:25 Changed 6 years ago by dekajp

I feel we will be better with sort and then clean the values in one pass. we can avoid all these nested if conditions.

comment:26 Changed 6 years ago by Scott González

You can't avoid the logic by sorting, because you can set the value of an individual handle at any time. Surely you can agree that setting a single handle shouldn't modify other handles because of a sort.

comment:27 in reply to:  26 Changed 6 years ago by dekajp

Replying to scott.gonzalez:

You can't avoid the logic by sorting, because you can set the value of an individual handle at any time. Surely you can agree that setting a single handle shouldn't modify other handles because of a sort.

How about this ? in first pass let's treat individual values any handle value if is out of boundary [min-max] let's figure out the closest boundary value [min or max] and set it . in second pass then run your suggested 4 checks.

comment:28 Changed 6 years ago by Scott González

How about you just implement what I said? Why would we want two loops when one will do?

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

Milestone: 1.11.0none

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

I think the instructions above are confusing and potentially incorrect. So here's a more detailed set of rules:

Restricting the value of a handle based on other handles only applies with range: true and therefore also only applies when exactly two handles exist. When setting both values at the same time, either via .values( array ), .option( "values", array ), or initialization, the first handle can be any value between min and max and the second handle must be between the first handle and max. When setting a single handle, it is restricted by the min, max, and existing value of the other handle.

All other forms of sliders only restrict handle values based on min and max. This includes standard single handle sliders, min range, max range, and non-range multi-handle sliders.

Note: See TracTickets for help on using tickets.