ColumnHider bug with array-based columns in IE #164

Closed
jason0x43 opened this Issue May 14, 2012 · 6 comments

Comments

Projects
None yet
1 participant
Contributor

jason0x43 commented May 14, 2012

There's a problem with the ColumnHider code in IE7/8 when using an array for a dgrid's columns property. At line 97 in dgrid/extensions/ColumnHider.js the code uses a for..in loop to iterate through the grid's columns. This works fine in Chrome, Safari, and Firefox, but it breaks for arrays in IE because IE will try to loop over all the object properties, not just the assigned indexes.

My first quick hack was to throw a check in the loop in ColumnHider to skip over obviously invalid columns (ones without a label property). My better solution was simply to use an object-based column definition instead of an array, but I don't really like that because 1) the dgrid wiki says an array should work, and 2) object properties are not technically ordered (although they are in most browsers, it seems).

Probably the most robust solution would be to always coerce column definitions into either arrays or objects whenever they're set, maybe in _setColumns.

@ghost

ghost commented May 24, 2012

I was unable to reproduce any sort of "break" when I tried, so I'm a bit curious as to what exactly you were experiencing. My only potential thought was that you may also be using some library that augments the Array prototype, but even then, that would affect more than just old IE.

That said, it's actually already possible to iterate over an array here instead of the columns hash, since column definitions always normalize into the subRows array of arrays as well. I'll be committing a change to make it iterate that instead.

ghost closed this in f1961f0 May 24, 2012

Contributor

jason0x43 commented May 24, 2012

I'll try to determine more precisely where the problem is coming from. The
only libraries I'm using are Dojo, dgrid+dependencies, and xdate.js. I
could imagine Dojo augmenting Array, but as you said, that would presumably
cause problems in other places.

On Thu, May 24, 2012 at 4:29 PM, SitePenKenFranqueiro <
reply@reply.github.com

wrote:

I was unable to reproduce any sort of "break" when I tried, so I'm a bit
curious as to what exactly you were experiencing. My only potential
thought was that you may also be using some library that augments the Array
prototype, but even then, that would affect more than just old IE.

That said, it's actually already possible to iterate over an array here
instead of the columns hash, since column definitions always normalize into
the subRows array of arrays as well. I'll be committing a change to make
it iterate that instead.


Reply to this email directly or view it on GitHub:
#164 (comment)

Jason

@ghost

ghost commented May 24, 2012

Dojo definitely does not augment any native prototypes. Either way, if you get the latest from dgrid master, hopefully your issue will be resolved.

Contributor

jason0x43 commented May 30, 2012

Thanks for the fix!

I played around with this a bit more just to make sure it wasn't something weird on the page I was working on. There is definitely a difference between how IE and Chrome handle for..in loops, and the ColumnHider code was sensitive to the difference.

You can reproduce the issue in the console on IE or Chrome with the code var a = ['a', 'b', 'c']; for (var i in a) { console.log(i); }.

In Chrome, Safari, and Firefox, you get:

0
1
2

In IE, on the other hand, you get:

LOG: 0
LOG: 1
LOG: 2
LOG: forEach
LOG: map
LOG: filter
LOG: reduce
LOG: indexOf

The ColumnHider code was assuming that everything in saw in the loop was a valid column definition, and it would choke when it tried to call col.label.replace on one of the functions.

@ghost

ghost commented May 30, 2012

FWIW, the above output certainly suggests that you're loading some additional library that's augmenting the Array prototype; if it's not affecting modern browsers, then perhaps it's using ES5 features to make the added methods non-enumerable when possible. IE9 has these methods available on Arrays natively and they won't be iterated in a for...in; IE < 9 doesn't have these methods natively at all.

There are several areas in Dojo where unprotected for...in loops are used, so your mileage may vary in general when used in combination with libraries that augment native prototypes.

Contributor

jason0x43 commented May 30, 2012

Good point. After more digging, I found the culprit--less.js. I've been
using less.js at runtime in the browser, and sure enough, it augments Array.

On Wed, May 30, 2012 at 8:36 AM, SitePenKenFranqueiro <
reply@reply.github.com

wrote:

FWIW, the above output certainly suggests that you're loading some
additional library that's augmenting the Array prototype; if it's not
affecting modern browsers, then perhaps it's using ES5 features to make the
added methods non-enumerable when possible. IE9 has these methods
available on Arrays natively and they won't be iterated in a for...in; IE <
9 doesn't have these methods natively at all.

There are several areas in Dojo where unprotected for...in loops are used,
so your mileage may vary in general when used in combination with libraries
that augment native prototypes.


Reply to this email directly or view it on GitHub:
#164 (comment)

Jason

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment