Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#9102 closed defect (fixed)

dojox.form.RangeSlider shares default value object between instances

Reported by: Mark Wubben Owned by: dante
Priority: high Milestone: 1.4
Component: DojoX Form Version: 1.3.0
Keywords: Cc: Nathan Toone
Blocked By: Blocking:

Description

The value array for dojox.form.RangeSlider is declared on its prototype:

dojo.declare(
    "dojox.form._RangeSliderMixin",
    null,
{
    value: [0,100],

This means that each instance shares the same value object. In other words, you can't use more than one RangeSlider? on a page.

Seems like the default value object should instead be set in a postMixInProperties call:

    postMixInProperties: function() {
      this.inherited(arguments);
      
      this.value = [0, 100];
    },

though I'm not sure what the Dojo guidelines are on this.

Attachments (1)

rangeslide.patch (24.1 KB) - added by dante 11 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 11 years ago by dante

Milestone: tbd1.4

i think constructor: function(args){

this.value = args.value
[0,100];

}

will suffice. Looking at the file though I may be offended, and it needs a serious style cleanup (it uses spaces, first off)

Changed 11 years ago by dante

Attachment: rangeslide.patch added

comment:2 Changed 11 years ago by Mark Wubben

Works nicely!

I'm using another class to instantiate the sliders from a templated specification object ('sliderParams'). Because the slider simply reuses any value array passed to it, this still meant that I ended up sharing value arrays. The same would occur if I were to call attr('value', valueArr) on the slider. Given that the value array itself is not meant to be the value, but is merely a container of values, perhaps it makes sense to create a new array internally?

comment:3 Changed 11 years ago by bill

There's nothing wrong with all instances sharing the same value object. The also share the same postMixInProperties object, for example, and that works fine.

The only problem is that RangeSlider has some unusual code that modifies the contents of that shared object:

this.value.sort(function(a, b){
   return b-a; 
});

Note that calling new RangeSlider({value: [1,2]}); or similar command in markup creates a new array, rather than modifying the shared one.

comment:4 Changed 11 years ago by Mark Wubben

(With Peter's patch) It does not create a new array as such, but references the value array. So yes, it doesn't modify the array declared on the prototype, but it will share the value array if it isn't unique the moment it is passed to the constructor.

I would argue that the value array is a shorthand for valueA and valueB attributes, so it should be copied inside the RangeSlider? rather than referenced. I find the current behavior confusing, although I'm happy to copy the array myself, before passing it to the constructor.

comment:5 Changed 11 years ago by dante

Resolution: fixed
Status: newclosed

(In [17329]) [probably] fixes #9102 - lots of cleanups included, but the point of the ticket was the array values, which if I'm not mistaken was decided in this case should always be mapped. mark - can you grab this and double check I'm not insane? otherwise, please attach new patch which touches this post-massive-style-guideline patch

comment:6 Changed 11 years ago by bill

See also #9160. I think you've fixed the issue described in this ticket, but the fix needs to be redone so that RTL mode works. (IE, RTL mode was broken both before and after your patch.)

comment:7 Changed 11 years ago by Mark Wubben

@dante, value mapping works fine. Many thanks.

Note: See TracTickets for help on using tickets.