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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented
FileSize
tabledrag_iterator.patch1.34 KB

Oops, snuck some extra changes in there.

ksenzee’s picture

ksenzee
she/her
Location Seattle area
CreditAttribution: ksenzee commented
FileSize
800968_tabledrag-for_2.patch551 bytes

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

ksenzee’s picture

ksenzee
she/her
Location Seattle area
CreditAttribution: ksenzee 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 CreditAttribution: Damien Tournoud commented
Status: Needs review » Reviewed & tested by the community

Yep. Better Javascript!

Dries’s picture

Dries CreditAttribution: 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 CreditAttribution: Big Z commented
Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Patch (to be ported)
FileSize
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 CreditAttribution: ptaff commented

subscribe

mstrelan’s picture

mstrelan CreditAttribution: mstrelan commented

+1 for backport to D6

Albert Volkman’s picture

Albert Volkman CreditAttribution: Albert Volkman commented
Status: Patch (to be ported) » Needs review
FileSize
tabledrag_iterator-800968-d6-10.patch578 bytes

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

budda’s picture

budda
Primary language English
Location Cheshire
CreditAttribution: budda 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
CreditAttribution: Gábor Hojtsy 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 CreditAttribution: 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