Opened 9 years ago

Closed 9 years ago

#11545 closed defect (invalid)

[patch] [cla] Select: 0 is not a valid option value

Reported by: denglerj Owned by: Douglas Hays
Priority: high Milestone: 1.7
Component: Dijit - Form Version: 1.5
Keywords: Cc:
Blocked By: Blocking:

Description

When database data for the option values for dijit.form.Select are integer values including 0, the option with the value 0 is not displayed in the selections and is therefore not selectable.

Attachments (4)

select.js.patch (676 bytes) - added by amathias 9 years ago.
Modified Select.js from dijit.form
_FormSelectWidget.js (19.6 KB) - added by amathias 9 years ago.
modified _FormSelectWidget.js from dijit.form
test_Select.html.patch (4.7 KB) - added by amathias 9 years ago.
modified test_Select.html programmatic example
_FormSelectWidget.js.patch (1.5 KB) - added by amathias 9 years ago.
modified _FormSelectWidget.js from dijit.form

Download all attachments as: .zip

Change History (17)

comment:1 Changed 9 years ago by bill

Summary: 0 is not a valid option value for dijit.form.SelectSelect: 0 is not a valid option value

Thanks. Actually I think it's messed up for all numeric values, as per this code:

getOptions: function(/* anything */ valueOrIdx){
	// summary:
	//		Returns a given option (or options).
	// valueOrIdx:
	//		If passed in as a string, that string is used to look up the option
	//		in the array of options - based on the value property.
	//		(See dijit.form.__SelectOption).
	//
	//		If passed in a number, then the option with the given index (0-based)
	//		within this select will be returned.

Changed 9 years ago by amathias

Attachment: select.js.patch added

Modified Select.js from dijit.form

Changed 9 years ago by amathias

Attachment: _FormSelectWidget.js added

modified _FormSelectWidget.js from dijit.form

Changed 9 years ago by amathias

Attachment: test_Select.html.patch added

modified test_Select.html programmatic example

comment:2 Changed 9 years ago by amathias

When creating a dijit.form.Select programmatically, setting an option's value to 0 is causing option.value to evaluate to false, even though the option.value exists and has a value of 0.

http://stackoverflow.com/questions/359494/javascript-vs-does-it-matter-which-equal-operator-i-use

Describes the root problem well.

There were 4 places, 1 in Select.js, 3 in _FormSelectWidget.js where I added a check to make sure that when object.value evaluations to false that the code is also checking whether the value has been set to 0.

A workaround for this problem is to set the value to "0" rather than 0 in your json object when you create the select.

One of the last examples in test_Select.html is a programmatic example for dijit.form.select. I modified this example to include values of 0 when creating the dijit.form.select.

Changed 9 years ago by amathias

Attachment: _FormSelectWidget.js.patch added

modified _FormSelectWidget.js from dijit.form

comment:3 Changed 9 years ago by amathias

_FormSelectWidget.js is really superfluous and can be ignored. The changes made are available in the attacked _formSelectWidget.js.patch file I attached.

comment:4 Changed 9 years ago by bill

Summary: Select: 0 is not a valid option value[patch] [cla] Select: 0 is not a valid option value

Hi, thanks for the patch.

We need to add automated tests for both value=0 and for separators (the <option type="separator"></option> thing in "Test 3" in test_Select.html).

Speaking of the test case, your patch seems strange to have multiple <option> tags w/the same value. Did I miss something?

It's troubling to need a special condition for 0... I'd like to just say

"value" in option

or as a second choice

option.value !== undefined

#9973 is a related ticket, and it's caused (partly?) because separators generate a SelectOption with value:"" rather than generating a SelectOption with no value attribute at all. (I'm talking about the dojo.query() code in _FormSelectWidget.js)

PS: like I said above it seems like there's a problem with all numeric values, not just 0. Maybe that's what #11215 is about.

comment:5 Changed 9 years ago by amathias

Hey thanks for the encouragement, I'm new to this process and this was the first ticket I've worked on.

Regarding the test case, I can remove one of the 0 values and attach a new test.

I can also write the automated test cases you mentioned.

#9973 is options with value : "" are generating menu seperators, not vice versa, but this is creating a problem that I a few paragraphs below.

This line of code in select.js :

if(!option.value ){ return new dijit.MenuSeparator?(); }

will create a menuSeparator if value : "" OR value: 0, which is why I add the check for value !== 0. At this point the loop in #11215 hasn't even been touched.

A non 0 specific solution to this particular IF statement might be converting option.value to a String before doing the check, but I dunno.

After fixing this if statement so that at least the values would be displayed, you run into a problem related to #11215.

#11215 creates a problem ONLY when the select is initially created or the first time you click on the select. This is because in both these instances, this code:

if(!dojo.isObject(i)){

evaluations to true.

The rest of the code:

291 i = i + ""; 292 } 293 if(typeof i === "string"){ 294 newValue[idx] = dojo.filter(opts, function(node){ 295 return node.value === i;

296 })[0]
{value: "", label: ""};

297 } 298 }, this);

ends up setting the value to "" and label to "".

The problem this loop causes is to break the selected : true attribute for numeric values because this code which executes next :

if(!this.multiple && (!newValue[0]
!newValue[0].value ) && opts.length){

newValue[0] = opts[0];

}

sets the display value to the first option value (instead of the option which is selected).

What is interesting is after the first time you click on the select, this line of code from #11215 :

if(!dojo.isObject(i)){

evaluates to false ! (i is now an object) Therefor i is never converted to string and label and value are not set to "".

When you start dealing with the 0 numeric values, you run into new issues, issues which my 3 modification to _FormSelectWidget fix. My guess is that the ticket submitter didn't get past the fact that 0 values are not displayed, because once you get them to display, you find out that when you select them, that value is not displayed as the 'selected' value.

This particular problem is actually NOT related to #11215 but instead related to the fact that option.value evaluates to false.

My patches above fix '0 not displaying as a value' and the following 'selecting an option with 0 as a value does not update as the selected value' (which I found after fixing the first issue).

Again, even if #11215 is fixed, the fact that when option.value == 0 evaluates to false, will still break the select.

Does the arguement for a non-zero solution change knowing that fixing #11215 will not fix the problem ?

If not, if we do not want a non-zero specific solution, we would need to modify the if statements somehow... maybe converting all the option values to strings at start is a solution ? Let me know what you think. This might be a viable solution for #11215 as well.

What has not been suggested is a fix for is the fact that #11215 causes numeric values with 'selected:true' to not be shown as the selected value (once you get them to display in the first place). What are the thoughts on fixing the loop in #11215 ?

Thanks for listening, I will work on the automated tests ! I hope whoever gets assigned this sends me an email, I would love to discuss ideas and help in any way possible.

comment:6 Changed 9 years ago by amathias

Edit:

Again, even if #11215 is fixed, the fact that when option.value == 0 evaluates to false, will still break the select.

Should be:

Again, even if #11215 is fixed, the fact that when option.value evaluates to false, will still break the select.

comment:7 Changed 9 years ago by amathias

#11687 looks like it might be similar as well.

comment:8 Changed 9 years ago by bill

Ah I didn't realize this was your first ticket, you did everything right with filing the CLA and attaching a patch file :-).

I agree #9973 is different, but I think it's related. Actually applying the fix in that ticket will fix this problem too. I'm not sure whether it's OK to just go with that fix (requiring type=separator) or not...

About #11215 OK I see how it's different, I guess we should handle one thing at a time, and fix this ticket and #9973 before attacking #11215.

comment:9 Changed 9 years ago by Douglas Hays

Owner: set to Douglas Hays

comment:10 Changed 9 years ago by Douglas Hays

Milestone: tbd1.7

comment:11 Changed 9 years ago by bill

Component: DijitDijit - Form

comment:12 Changed 9 years ago by Douglas Hays

Bill, your comment in #12434 would suggest that this ticket is invalid as well.

comment:13 Changed 9 years ago by Douglas Hays

Resolution: invalid
Status: newclosed

Bill has decided to keep numeric values reserved for indexing the options array.

Note: See TracTickets for help on using tickets.