Fix #885: Escape column IDs in CSS selectors like we do grid IDs

Also avoid calling `put-selector/put` in areas sensitive to this for grid IDs, as it currently can't handle some characters and doesn't understand escaping. Special characters in column IDs are replaced with hyphens rather than escaped - Grid already behaved this way, but updates were necessary to extensions for consistency.
dojo · Apr 15, 2014 · 4b2bf76 · 4b2bf76
1 parent 414da95
commit 4b2bf76
Show file tree

Hide file tree

Showing 7 changed files with 67 additions and 37 deletions.
diff --git a/ColumnSet.js b/ColumnSet.js
@@ -187,7 +187,8 @@ function(kernel, declare, lang, Deferred, listen, aspect, query, has, miscUtil,
 			// summary:
 			//		Dynamically creates a stylesheet rule to alter a columnset's style.
 
-			var rule = this.addCssRule("#" + miscUtil.escapeCssIdentifier(this.domNode.id) + " .dgrid-column-set-" + colsetId, css);
+			var rule = this.addCssRule("#" + miscUtil.escapeCssIdentifier(this.domNode.id) +
+				" .dgrid-column-set-" + miscUtil.escapeCssIdentifier(colsetId, "-"), css);
 			this._positionScrollers();
 			return rule;
 		},

diff --git a/Grid.js b/Grid.js
@@ -1,13 +1,19 @@
 define(["dojo/_base/kernel", "dojo/_base/declare", "dojo/on", "dojo/has", "put-selector/put", "./List", "./util/misc", "dojo/_base/sniff"],
 function(kernel, declare, listen, has, put, List, miscUtil){
 	var contentBoxSizing = has("ie") < 8 && !has("quirks");
-	var invalidClassChars = /[^\._a-zA-Z0-9-]/g;
+
 	function appendIfNode(parent, subNode){
 		if(subNode && subNode.nodeType){
 			parent.appendChild(subNode);
 		}
 	}
 
+	function replaceInvalidChars(str) {
+		// Replaces invalid characters for a CSS identifier with hyphen,
+		// as dgrid does for field names / column IDs when adding classes.
+		return miscUtil.escapeCssIdentifier(str, "-");
+	}
+
 	var Grid = declare(List, {
 		columns: null,
 		// cellNavigation: Boolean
@@ -105,7 +111,9 @@ function(kernel, declare, listen, has, put, List, miscUtil){
 					column = subRow[i];
 					id = column.id;
 
-					extraClasses = column.field ? ".field-" + column.field : "";
+					extraClasses = column.field ?
+						".field-" + replaceInvalidChars(column.field) :
+						"";
 					className = typeof column.className === "function" ?
 						column.className(object) : column.className;
 					if(className){
@@ -114,10 +122,9 @@ function(kernel, declare, listen, has, put, List, miscUtil){
 
 					cell = put(tag + (
 							".dgrid-cell.dgrid-cell-padding" +
-							(id ? ".dgrid-column-" + id : "") +
+							(id ? ".dgrid-column-" + replaceInvalidChars(id) : "") +
 							extraClasses.replace(/ +/g, ".")
-						).replace(invalidClassChars,"-") +
-						"[role=" + (tag === "th" ? "columnheader" : "gridcell") + "]");
+						) + "[role=" + (tag === "th" ? "columnheader" : "gridcell") + "]");
 					cell.columnId = id;
 					if(contentBoxSizing){
 						// The browser (IE7-) does not support box-sizing: border-box, so we emulate it with a padding div
@@ -367,7 +374,7 @@ function(kernel, declare, listen, has, put, List, miscUtil){
 			//		Dynamically creates a stylesheet rule to alter a column's style.
 
 			return this.addCssRule("#" + miscUtil.escapeCssIdentifier(this.domNode.id) +
-				" .dgrid-column-" + colId, css);
+				" .dgrid-column-" + replaceInvalidChars(colId), css);
 		},
 
 		/*=====

diff --git a/extensions/ColumnHider.js b/extensions/ColumnHider.js
@@ -83,7 +83,11 @@ function(declare, has, listen, miscUtil, put, i18n){
 
 		_renderHiderMenuEntry: function(col){
 			var id = col.id,
-				div, checkId, checkbox;
+				replacedId = miscUtil.escapeCssIdentifier(id, "-"),
+				div,
+				checkId,
+				checkbox,
+				label;
 
 			if(col.hidden){
 				// Hide the column (reset first to avoid short-circuiting logic)
@@ -97,12 +101,17 @@ function(declare, has, listen, miscUtil, put, i18n){
 
 			// Create the checkbox and label for each column selector.
 			div = put("div.dgrid-hider-menu-row");
-			checkId = this.domNode.id + "-hider-menu-check-" + id;
+			checkId = this.domNode.id + "-hider-menu-check-" + replacedId;
 
-			this._columnHiderCheckboxes[id] = checkbox = put(div, "input#" + checkId +
-					".dgrid-hider-menu-check.hider-menu-check-" + id + "[type=checkbox]");
-			put(div, "label.dgrid-hider-menu-label.hider-menu-label" + id +
-				"[" + forAttr + "=" + checkId + "]", col.label || col.field || "");
+			// put-selector can't handle invalid selector characters, and the
+			// ID could have some, so add it directly
+			checkbox = this._columnHiderCheckboxes[id] =
+				put(div, "input.dgrid-hider-menu-check.hider-menu-check-" + replacedId + "[type=checkbox]");
+			checkbox.id = checkId;
+
+			label = put(div, "label.dgrid-hider-menu-label.hider-menu-label-" + replacedId +
+				"[" + forAttr + "=" + checkId + "]",
+				col.label || col.field || "");
 
 			put(this.hiderMenuNode, div);
 
@@ -137,8 +146,9 @@ function(declare, has, listen, miscUtil, put, i18n){
 
 				// Create the column list, with checkboxes.
 				hiderMenuNode = this.hiderMenuNode =
-					put("div#dgrid-hider-menu-" + this.id +
-						".dgrid-hider-menu[role=dialog][aria-label=" + this.i18nColumnHider.popupLabel + "]");
+					put("div.dgrid-hider-menu[role=dialog][aria-label=" +
+						this.i18nColumnHider.popupLabel + "]");
+				hiderMenuNode.id = this.id + "-hider-menu";
 
 				this._listeners.push(listen(hiderMenuNode, "keyup", function (e) {
 					var charOrCode = e.charCode || e.keyCode;
@@ -281,7 +291,8 @@ function(declare, has, listen, miscUtil, put, i18n){
 			}
 
 			this._columnHiderRules[id] =
-				miscUtil.addCssRule(selectorPrefix + id, "display: none;");
+				miscUtil.addCssRule(selectorPrefix + miscUtil.escapeCssIdentifier(id, "-"),
+					"display: none;");
 
 			if(has("ie") === 8 && !has("quirks")){
 				tableRule = miscUtil.addCssRule(".dgrid-row-table", "display: inline-table;");

diff --git a/extensions/ColumnResizer.js b/extensions/ColumnResizer.js
@@ -105,8 +105,9 @@ function resizeColumnWidth(grid, colId, width, parentType, doResize){
 			rule.set("width", width);
 		}else{
 			// Use miscUtil function directly, since we clean these up ourselves anyway
-			rule = miscUtil.addCssRule(
-				"#" + miscUtil.escapeCssIdentifier(grid.domNode.id) + " .dgrid-column-" + colId, "width: " + width + ";");
+			rule = miscUtil.addCssRule("#" + miscUtil.escapeCssIdentifier(grid.domNode.id) +
+				" .dgrid-column-" + miscUtil.escapeCssIdentifier(colId, "-"),
+				"width: " + width + ";");
 		}
 
 		// keep a reference for future removal
@@ -226,8 +227,9 @@ return declare(null, {
 			if((rule = this._oldColumnSizes[colId])){
 				rule.set("width", column.width + "px");
 			}else{
-				rule = miscUtil.addCssRule(
-					"#" + this.domNode.id + " .dgrid-column-" + colId, "width: " + column.width + "px;");
+				rule = miscUtil.addCssRule("#" + miscUtil.escapeCssIdentifier(this.domNode.id) +
+					" .dgrid-column-" + miscUtil.escapeCssIdentifier(colId, "-"),
+					"width: " + column.width + "px;");
 			}
 			this._columnSizes[colId] = rule;
 		}
@@ -254,7 +256,8 @@ return declare(null, {
 			var colNode = colNodes[i],
 				id = colNode.columnId,
 				col = grid.columns[id],
-				childNodes = colNode.childNodes;
+				childNodes = colNode.childNodes,
+				resizeHandle;
 
 			if(!col || col.resizable === false){ continue; }
 
@@ -266,8 +269,9 @@ return declare(null, {
 				put(headerTextNode, childNodes[0]);
 			}
 
-			put(colNode, headerTextNode, "div.dgrid-resize-handle.resizeNode-"+id).columnId =
-				assoc && assoc[id] || id;
+			resizeHandle = put(colNode, headerTextNode, "div.dgrid-resize-handle.resizeNode-" +
+				miscUtil.escapeCssIdentifier(id, "-"));
+			resizeHandle.columnId = assoc && assoc[id] || id;
 		}
 
 		if(!grid.mouseMoveListen){
@@ -311,7 +315,8 @@ return declare(null, {
 		dom.setSelectable(this.domNode, false);
 		this._startX = this._getResizeMouseLocation(e); //position of the target
 
-		this._targetCell = query(".dgrid-column-" + target.columnId, this.headerNode)[0];
+		this._targetCell = query(".dgrid-column-" + miscUtil.escapeCssIdentifier(target.columnId, "-"),
+			this.headerNode)[0];
 
 		// Show resizerNode after initializing its x position
 		this._updateResizerPosition(e);
@@ -369,7 +374,8 @@ return declare(null, {
 			obj = this._getResizedColumnWidths(),//get current total column widths before resize
 			totalWidth = obj.totalWidth,
 			lastCol = obj.lastColId,
-			lastColWidth = query(".dgrid-column-"+lastCol, this.headerNode)[0].offsetWidth;
+			lastColWidth = query(".dgrid-column-" + miscUtil.escapeCssIdentifier(lastCol, "-"),
+				this.headerNode)[0].offsetWidth;
 
 		if(newWidth < this.minWidth){
 			//enforce minimum widths

diff --git a/test/extensions/ColumnHider.html b/test/extensions/ColumnHider.html
@@ -52,11 +52,12 @@
 						col5: "Column 5"
 					},
 					altColumns = [
-						{ field: "col2", label: "Col2" },
+						// Specify IDs with special chars to test replacement
+						{ id: "col:2", field: "col2", label: "Col2" },
 						// test setting unhidable *and* hidden (i.e. not in menu, not displayed)
-						{ field: "col4", label: "Col4", sortable: false, hidden: true, unhidable: true },
-						{ field: "col1", label: "Col1" },
-						{ field: "col5" } // no label, to test fallback to field
+						{ id: "col:4", field: "col4", label: "Col4", sortable: false, hidden: true, unhidable: true },
+						{ id: "col:1", field: "col1", label: "Col1" },
+						{ id: "col:5", field: "col5" } // no label, to test fallback to field
 					],
 					body = document.body,
 					rtl = location.search.indexOf("rtl") !== -1,

diff --git a/test/extensions/ColumnResizer.html b/test/extensions/ColumnResizer.html
@@ -95,12 +95,13 @@
 					};
 					columns1 = lang.clone(columns);
 
-					// columns2 tests using an array of columns with explicit IDs
+					// columns2 tests using an array of columns with explicit IDs,
+					// including special characters to test replacement
 					columns2 = [
-						{ id: "col2", field: "col2", label: "Column 2" },
-						{ id: "col4", field: "col4", label: "Column 4", sortable: false },
-						{ id: "col1", field: "col1", label: "Column 1" },
-						{ id: "col5", field: "col5", label: "Column 5" }
+						{ id: "col:2", field: "col2", label: "Column 2" },
+						{ id: "col:4", field: "col4", label: "Column 4", sortable: false },
+						{ id: "col:1", field: "col1", label: "Column 1" },
+						{ id: "col:5", field: "col5", label: "Column 5" }
 					];
 
 					var ResizeGrid = declare([Grid, Selection, ColumnResizer]);

diff --git a/util/misc.js b/util/misc.js
@@ -148,14 +148,17 @@ define(["put-selector/put"], function(put){
 			};
 		},
 
-		escapeCssIdentifier: function(id){
+		escapeCssIdentifier: function(id, replace){
 			// summary:
-			//		Escapes normally-invalid characters in a CSS identifier (such as .);
+			//		Escapes normally-invalid characters in a CSS identifier (such as . or :);
 			//		see http://www.w3.org/TR/CSS2/syndata.html#value-def-identifier
 			// id: String
 			//		CSS identifier (e.g. tag name, class, or id) to be escaped
+			// replace: String?
+			//		If specified, indicates that invalid characters should be
+			//		replaced by the given string rather than being escaped
 
-			return id.replace(invalidCssChars, "\\$1");
+			return id.replace(invalidCssChars, replace || "\\$1");
 		}
 	};
 	return util;