Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#16290 closed defect (fixed)

Setting value of Select widget to numeric value fails, sets default

Reported by: Pie21 Owned by: Douglas Hays
Priority: undecided Milestone: 1.9
Component: Dijit - Form Version: 1.8.1
Keywords: Cc:
Blocked By: Blocking:

Description

The Select widget does not select the correct option when given just a numeric ID that matches the value of one of the options in the Select list.

Looking at _FormSelectWidget._setValueAttr (line 389), the widget runs a filter over the available options and returns the one whose value exactly matches the new value, but only after the new value has been cast to a string. Since the option ID value is NOT a string (it's a number), there are no exact matches for the given ID (which has been cast to a string, since it's not an object), and thus newValue is set to {label: "", value: ""}.

A workaround for the time being is to set the Select value to the full option object. Given some numeric key you'd like to set to:

var myKey = 42;

... instead of setting the key directly (which will fail and set to the default first option):

mySelect.set("value", 42);

... filter out the matching option manually, and then call set to that:

mySelect.set(

"value", mySelect.options.filter(function(node) {return node.value === 42;})

);

This seems like a pretty easy fix, but I don't know why _setValueAttr was written the way it is in the first place, and how it seems to work for everyone else.

Version is actually 1.9.0dev, but the same apparent fault exists in 1.8.1. See line 408 https://github.com/dojo/dijit/blob/1.8.1/form/_FormSelectWidget.js#L408.

Attachments (1)

16290.patch (13.5 KB) - added by Douglas Hays 7 years ago.
possible implementation against [29946]

Download all attachments as: .zip

Change History (11)

comment:1 Changed 7 years ago by Pie21

Replace those 42s with myKey or whatever. And formatting.

comment:2 Changed 7 years ago by Pie21

I made a little module of monkey patches to import, so in case anyone is interested, a sufficient solution for me is the following:

    function patchNumericSelection() {
        // Monkey patch fix to numeric Select selection
        // http://bugs.dojotoolkit.org/ticket/16290
        var oldSetValueAttr = Select.prototype._setValueAttr;
        Select.prototype._setValueAttr = function (newValue) {
            if (typeof newValue === "number") {
                newValue = this.options.filter(function (node) {
                    return node.value === newValue;
                });
            }
            oldSetValueAttr.apply(this, arguments);
        };
    }

An alternative strategy would be to cast the existing option values to strings before comparing them with the new value that's been cast to a string, but this is an easy way to get the same effect without digging into the actual function code.

comment:3 Changed 7 years ago by bill

Dup of #12434. This would be nice to be fixed for 2.0. I don't know how to fix it before then without breaking backwards compatibility.

comment:4 Changed 7 years ago by Pie21

Ah yes, didn't see that. Makes some sense, I suppose, but there should at least be SOME method of setting a numeric value, even if it means more methods.

Also my monkey patch doesn't work if the Select is backed by a store that hasn't loaded yet (since the filter will return []). I made a simple improvement and just copied the store-loading handling from the widget:

    function patchNumericSelection() {
        // Monkey patch fix to numeric Select selection
        // http://bugs.dojotoolkit.org/ticket/16290
        var oldSetValueAttr = Select.prototype._setValueAttr;
        Select.prototype._setValueAttr = function (newValue) {
            // If loading, set as pending and wait
            if (this._loadingStore) {
                this._pendingValue = newValue;
            } else if (typeof newValue === "number") {
                newValue = this.options.filter(function (node) {
                    return node.value === newValue;
                });
            }
            oldSetValueAttr.apply(this, arguments);
        };
    }

If this won't be fixed, perhaps the patch is the best way to go for those of use who are using numeric stores and not needing to use removeOption() and the like.

comment:5 Changed 7 years ago by Douglas Hays

Will either of these approaches work (maintaining backward compat)?
1) allowNumericValue:true that disables widget processing by index
2) node.value within _FormSelectWidget is always converted to a String and then users can simple do the same: removeOption(String(42))

comment:6 Changed 7 years ago by bill

Sounds like (1) would definitely work, although (2) might break back-compat for some applications. Given the number of people that complain about this though I wouldn't mind either solution going in.

Changed 7 years ago by Douglas Hays

Attachment: 16290.patch added

possible implementation against [29946]

comment:7 Changed 7 years ago by Douglas Hays

Pie21, can you please apply the patch to see if it works for you.
set('value', ...) should work with a number, string, object or array now.
getOptions and removeOption still take indices, objects, or arrays but you can pass {value:42}.

comment:8 Changed 7 years ago by Douglas Hays

Resolution: fixed
Status: newclosed

In [29963]:

Fixes #16290. Deprecate getOptions(String) and removeOption(String) in favor of {value:StringOrNumber} parameters that can also be used for expanded searching capability like {selected:true, label:"New York"}. set('value', Number) works now but setting via indices is not supported (and didn't work before anyway). Added automated tests. !strict

comment:9 Changed 7 years ago by Douglas Hays

Milestone: tbd1.9

comment:10 Changed 7 years ago by bill

#16860 is a duplicate of this ticket.

Note: See TracTickets for help on using tickets.