Opened 11 years ago

Closed 8 years ago

Last modified 2 years ago

#9138 closed defect (fixed)

FilteringSelect does not support options with values that contain commas

Reported by: hborders Owned by: dylan
Priority: low Milestone: 1.7
Component: Dijit - Form Version: 1.2.3
Keywords: Cc:
Blocked By: Blocking:

Description

I have a dijit.form.FilteringSelect? that contains options with commas (',') in their value. The FilteringSelect? fails to update its label or value when its attr method is called with a value containing a comma. Here is an example:

<select dojoType="dijit.form.FilteringSelect" id="withCommas">
  <option value="1,2" selected="selected">one or two</option>
  <option value="3">three</option>
</select>

When the page initially loads, the FilteringSelect? should have 'one or two' in its label, but it shows nothing. Further if I make the following call from firebug nothing happens either:

dijit.byId('withCommas').attr('value', '1,2');

I believe the problem lies with dojo.query's parsing of attribute-value selectors. It seems to separate queries that contain commas into separate individual queries regardless of where the comma occurs. The query parser should ignore commas within attribute-value literals.

Change History (11)

comment:1 Changed 11 years ago by Douglas Hays

Component: DijitCore
Owner: set to alex
Summary: FilteringSelect does not support options with values that contain commasdojo.query does not parse commas correctly

This appears to be a dojo.query bug. Consider the following snipit:

<select>
<option value=",">,</option>
</select>
<input type=button onclick=alert(dojo.query("option[value=',']")[0]) value="query option with ,">

dojo.query fails to find the option node on IE6, IE7, FF2, FF3, and Safari 3, but does find it on Safari 4.
I tested on the latest trunk = [17488].

comment:2 Changed 10 years ago by bill

Component: CoreQuery

comment:3 Changed 10 years ago by marcpeterson

I was able to get dojo.query to search for results with commas, but for some reason it doesn't like the _ComboBoxDataStore data store.

In dijit.form._ComboBoxDataStore.fetchItemByIdentity(), after the line

var item = dojo.query("option[value='" + args.identity + "']", this.root)[0];

You can add the following as a backup query (the original is still needed as it picks up integers, this doesn't)

if(!item){
	for(var i=0; i<this.root.length; i++){
		if(this.root[i].innerHTML == args.identity){
			item = this.root[i];
			break;
		}
	}
}

This looks at each of stored dom nodes and compares their contents to the search string. I'm not sure why it doesn't work on integers.

comment:4 Changed 10 years ago by wdoekes

I had to add the following to get it to work in both IE6 and FF3:

--- form/ComboBox.js	(revision 20788)
+++ form/ComboBox.js	(working copy)
@@ -1124,7 +1124,19 @@
 		//	|		{identity: "CA", onItem: function(item){...}
 		//
 		//		Call `onItem()` with the DOM node `<option value="CA">California</option>`
-		var item = dojo.query("option[value='" + args.identity + "']", this.root)[0];
+		var item = false;
+		try {
+			item = dojo.query("option[value='" + args.identity + "']", this.root)[0];
+		} catch(e) {}
+		if(!item || typeof(item.selectedIndex) !== undefined){ // FF3 can return a HTMLSelectWidget..
+			for(var i=0; i<this.root.length; i++){
+				if(this.root[i].innerHTML == args.identity){
+					item = this.root[i];
+					break;
+				}
+			}
+		}
+
 		args.onItem(item);
 	},
 	

IE6 threw an error on the query. Firefox3 returned a HTMLSelectElement widget instead of the HTMLOption.

Regards, Walter Doekes OSSO B.V.

comment:5 Changed 10 years ago by James Burke

Milestone: tbdfuture

comment:6 Changed 9 years ago by Chris Mitchell

Owner: changed from alex to dylan

please review/triage

comment:7 Changed 8 years ago by pechanec

I found this defect in 1.6.1 in Firefox 3 and 4 too. Would be great for me to be fixed.

comment:8 Changed 8 years ago by bill

It may be working better now, or at least on some browsers, since Kris has replaced the acme query engine w/a lightweight engine. Although would still fail if the app explicitly loaded the acme engine.

comment:9 Changed 8 years ago by ben hockey

Keywords: needsreview added
Priority: highlow

#7479 seems related and has a patch that claims to have dojo.query not parse the values for attributes.

Should this ticket be closed either as duplicate or wontfix?

comment:10 Changed 8 years ago by bill

Component: QueryDijit - Form
Keywords: needsreview removed
Milestone: future1.7
Resolution: fixed
Status: newclosed
Summary: dojo.query does not parse commas correctlyFilteringSelect does not support options with values that contain commas

Actually, the reported problem is fixed in 1.7, because FilteringSelect was refactored to not depend on dojo.query() so much, in my conversion for it to support dojo.store. As for the dojo.query() bug, let's assume that is reported in other tickets like the one you mentioned.

comment:11 Changed 2 years ago by wdoekes

Possible fix, in case you're still using 1.2:

--- static/dojo/1.2.x/dojo/_base/query.js	2017-09-15 13:13:13.860344455 +0200
+++ static/dojo/1.2.x/dojo/_base/query.js.patched	2017-09-15 13:13:42.980344981 +0200
@@ -993,7 +993,18 @@ dojo.require("dojo._base.NodeList");
 			// and return a dispatcher that will merge the parts when run
 
 			// var parts = query.split(", ");
-			var parts = query.split(/\s*,\s*/);
+			// var parts = query.split(/\s*,\s*/);
+			var parts = [];
+			var inquery = ',' + query;
+			var match = null;
+			do {
+				match = inquery.match(/,\s*((?:[^"',]+|"[^"]*"|'[^']*')+)\s*/);
+				if (match) {
+					parts.push(match[1]);
+					inquery = inquery.substring(match[0].length);
+				}
+			} while (match);
+
 			var tf = function(root){
 				var pindex = 0; // avoid array alloc for every invocation
 				var ret = [];

This should split form, options[value=','] into form and options[value=','] instead of splitting it into three components.

Regards, Walter Doekes OSSO B.V.

Note: See TracTickets for help on using tickets.