This is not showing up in core as an error, but if the Array prototype has been added to, this will cause the iterating code to run over properties (and methods) of the Array, as opposed to just the elements. It's bad form in JavaScript to use the for...in construct when working with arrays (should only be used for objects).

CommentFileSizeAuthor
#10 tabledrag_iterator-800968-d6-10.patch578 bytesalbert volkman
#7 tabledrag-for-800968-7.patch.txt511 bytesBig Z
#2 800968_tabledrag-for_2.patch551 bytesksenzee
#1 tabledrag_iterator.patch1.34 KBJacobSingh
tabledrag_iterator.patch3.44 KBJacobSingh

Comments

JacobSingh’s picture

JacobSingh commented
StatusFileSize
new tabledrag_iterator.patch1.34 KB

Oops, snuck some extra changes in there.

ksenzee’s picture

ksenzee
she/her
commented
StatusFileSize
new 800968_tabledrag-for_2.patch551 bytes

A simpler fix is just to construct the for loop correctly.

ksenzee’s picture

ksenzee
she/her
commented
Title: tabledrag.js should be using $().each instead of for...in when iterating over an array. » tabledrag.js should not use for...in to iterate over an array

Guess I should retitle the issue, too.

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community

Yep. Better Javascript!

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Big Z’s picture

Big Z commented
Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Patch (to be ported)
StatusFileSize
new tabledrag-for-800968-7.patch.txt511 bytes

Could this please be back ported to the 6.x branch. Patch attached.

Many thanks!

ptaff’s picture

ptaff commented

subscribe

mstrelan’s picture

mstrelan commented

+1 for backport to D6

albert volkman’s picture

albert volkman commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new tabledrag_iterator-800968-d6-10.patch578 bytes

Re-posting patch from #7. No credit please!

budda’s picture

budda
Primary language English
Location Cheshire
commented
Component: block.module » base system
Priority: Minor » Major
Status: Needs review » Reviewed & tested by the community

Super, perfect fix for my ageing Drupal 6 site.
At present its preventing admin forms from working in Chrome so a bit more than a 'minor' issue I think.
The bug is also not just in the block.module - it also affects the taxonomy admin, webform field designer, CCK field editor etc.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Looks good. Thanks, committed, pushed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

j0rd’s picture

j0rd commented

I usually check the patches before I update my Drupal and I ran across this issue and fix.

I would recommend taking it one step further and look at reducing the number of gratuitous .length calls with the hopes of improving the speed of tabledrag.js further. From my simple test (profiling patch included in issue), it could reduce the number of calls to .length from ~30,000+ to roughly 450 for a single drag.

Take a look
#1571814: tableDrag.js, reduce the number of calls of .length from 33478 to 455