Skip to content

Commit

Permalink
Adjusting CSS rule insertion to escape periods in IDs
Browse files Browse the repository at this point in the history
It turns out, periods in IDs have been valid in HTML since 4.01 (at least):
http://www.w3.org/TR/html401/types.html#h-6.2

In the event that a user creates a dgrid with a period in its ID, such
as "foo.bar", the injected CSS rule creates a selector of "#foo.bar",
which winds up parsing as an element with ID of "foo" and a class of
"bar".

Fortunately, CSS2 allows for backslashes:
http://www.w3.org/TR/CSS2/syndata.html#value-def-identifier

As such, this pull request includes modification to four primary areas
where the grid DOM node ID is being used for rule creation. I create a
simple regex:

	var allPeriods = /\./g;

Then in the injection, it's as simple as replacing it:

	someSelector = "#" + this.domNode.id.replace(allPeriods, "\\.") + whatever;

I've tested these changes and they do not seem to impact any of the
related tests in any negative ways. I've also augmented the
ColumnResizer tests to include a grid with a period in its ID, as that's
the most readily apparent example of this bug that I'd ran into.

Tests performed in the following:

* Chrome 24, OS X
* Firefox 18, OS X
* IE8, Windows 7

The resizing works in all of them.
  • Loading branch information
brianarn committed Jan 25, 2013
1 parent 42234d0 commit d59cb28
Show file tree
Hide file tree
Showing 5 changed files with 19 additions and 8 deletions.
3 changes: 2 additions & 1 deletion ColumnSet.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ function(kernel, declare, Deferred, listen, aspect, query, has, put, hasClass, G
});

var colsetidAttr = "data-dgrid-column-set-id";
var allPeriods = /\./g;

hasClass("safari", "ie-7");

Expand Down Expand Up @@ -217,7 +218,7 @@ function(kernel, declare, Deferred, listen, aspect, query, has, put, hasClass, G
// summary:
// Dynamically creates a stylesheet rule to alter a columnset's style.

var rule = this.addCssRule("#" + this.domNode.id + " .dgrid-column-set-" + colsetId, css);
var rule = this.addCssRule("#" + this.domNode.id.replace(allPeriods, "\\.") + " .dgrid-column-set-" + colsetId, css);
positionScrollers(this);
return rule;
},
Expand Down
11 changes: 6 additions & 5 deletions Grid.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
define(["dojo/_base/kernel", "dojo/_base/declare", "dojo/on", "dojo/has", "put-selector/put", "./List", "dojo/_base/sniff"],
function(kernel, declare, listen, has, put, List){
var contentBoxSizing = has("ie") < 8 && !has("quirks");
var invalidClassChars = /[^\._a-zA-Z0-9-]/g;
var invalidClassChars = /[^\._a-zA-Z0-9-]/g;
var allPeriods = /\./g;
function appendIfNode(parent, subNode){
if(subNode && subNode.nodeType){
parent.appendChild(subNode);
Expand Down Expand Up @@ -56,7 +57,7 @@ function(kernel, declare, listen, has, put, List){
if(!element && typeof columnId != "undefined"){
var row = this.row(target),
rowElement = row.element;
if(rowElement){
if(rowElement){
var elements = rowElement.getElementsByTagName("td");
for(var i = 0; i < elements.length; i++){
if(elements[i].columnId == columnId){
Expand Down Expand Up @@ -155,7 +156,7 @@ function(kernel, declare, listen, has, put, List){
if(column.formatter){
td.innerHTML = column.formatter(data);
}else if(column.renderCell){
// A column can provide a renderCell method to do its own DOM manipulation,
// A column can provide a renderCell method to do its own DOM manipulation,
// event handling, etc.
appendIfNode(td, column.renderCell(object, data, td, options));
}else if(data != null){
Expand Down Expand Up @@ -222,7 +223,7 @@ function(kernel, declare, listen, has, put, List){
!sort.descending
}];

// Emit an event with the new sort
// Emit an event with the new sort
eventObj = {
bubbles: true,
cancelable: true,
Expand Down Expand Up @@ -347,7 +348,7 @@ function(kernel, declare, listen, has, put, List){
// summary:
// Dynamically creates a stylesheet rule to alter a column's style.

return this.addCssRule("#" + this.domNode.id + " .dgrid-column-" + colId, css);
return this.addCssRule("#" + this.domNode.id.replace(allPeriods, "\\.") + " .dgrid-column-" + colId, css);
},

/*=====
Expand Down
3 changes: 2 additions & 1 deletion extensions/ColumnHider.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ function(declare, has, listen, miscUtil, put){

var activeGrid, // references grid for which the menu is currently open
bodyListener, // references pausable event handler for body mousedown
allPeriods = /\./g, // regex to clean identifiers for CSS rule addition
// Need to handle old IE specially for checkbox listener and for attribute.
hasIE = has("ie"),
hasIEQuirks = hasIE && has("quirks"),
Expand Down Expand Up @@ -215,7 +216,7 @@ function(declare, has, listen, miscUtil, put){
// Hides the column indicated by the given id.

// Use miscUtil function directly, since we clean these up ourselves anyway
var selectorPrefix = "#" + this.domNode.id + " .dgrid-column-",
var selectorPrefix = "#" + this.domNode.id.replace(allPeriods, "\\.") + " .dgrid-column-",
next, rules, i; // used in IE8 code path

if(has("ie") === 8 && !has("quirks")){
Expand Down
3 changes: 2 additions & 1 deletion extensions/ColumnResizer.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ define(["dojo/_base/declare", "dojo/on", "dojo/query", "dojo/_base/lang", "dojo/
function(declare, listen, query, lang, dom, geom, has, miscUtil, put){

var hasPointFromNode = has("touch") && webkitConvertPointFromNodeToPage;
var allPeriods = /\./g;

function addRowSpan(table, span, startRow, column, id){
// loop through the rows of the table and add this column's id to
Expand Down Expand Up @@ -102,7 +103,7 @@ function resizeColumnWidth(grid, colId, width, parentType){
}else{
// Use miscUtil function directly, since we clean these up ourselves anyway
rule = miscUtil.addCssRule(
"#" + grid.domNode.id + " .dgrid-column-" + colId, "width: " + width + ";");
"#" + grid.domNode.id.replace(allPeriods, "\\.") + " .dgrid-column-" + colId, "width: " + width + ";");
}

// keep a reference for future removal
Expand Down
7 changes: 7 additions & 0 deletions test/extensions/ColumnResizer.html
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,11 @@
store: testStore,
columns: columns1
}, "grid");
window.gridPeriod = new ResizeGrid({
id: "gridPeriod",
store: testStore,
columns: columns1
}, "grid.period");
window.gridWide = new ResizeGrid({
store: testStore,
columns: lang.clone(columns)
Expand Down Expand Up @@ -161,6 +166,8 @@ <h2>A basic grid with column resizing</h2>
<button onclick="grid.set('adjustLastColumn', true);">Enable</button>
<button onclick="grid.set('adjustLastColumn', false);">Disable</button>
</div>
<h2>A basic grid with a period in its ID, to ensure injected size rules work</h2>
<div id="grid.period"></div>
<h2>Another grid w/ columns whose width initially exceed the table's width,
placed within a relatively-positioned element</h2>
<div class="testrelative">
Expand Down

0 comments on commit d59cb28

Please sign in to comment.