4

i have the following javascript below after i finish an ajax query

all of my images have name="pic"

<script type="text/javascript">
 function done() {
     var e = document.getElementsByName("pic");
     alert(e.length);
     for (var i = 0; i < e.length; i++) {
         cvi_instant.add(e[i], { shadow: 75, shade: 10 });
     }
 }

my goal is to apply an image border around using this library:

http://www.netzgesta.de/instant/

the problem is that for some reason this works but it only seem to apply to every other picture instead of every one. any clue why the code above would skip every other element??

EDIT: I added an alert in the loop and it does correctly go 0, 1,2,3,4,5,6 . . 

     for (var i = 0; i < e.length; i++)
     {
         alert(i);
         cvi_instant.add(e[i], { shadow: 75, shade: 10 });
     }
Improve this question
3
  • 1
    I'm going to make an extravagant (and possibly wildly inaccurate) claim. It's because that code (cvi_instant.add) is also using a variable named 'i'. Try calling your variable 'k'. Commented Sep 22, 2009 at 0:20
  • 1
    This should be inaccurate. "i" is declared with "var" so it's not global, and numbers are not passed by reference. However, if there was another "i" declared globally there might be a problem. Commented Sep 22, 2009 at 0:41
  • 1
    Try alerting "i" before the loop (e.g., with the e.length) and tell us what it is. Also, try firebug extension for firefox so you can actually debug and step through the code. This will make things clearer for you. Commented Sep 22, 2009 at 0:47

4 Answers 4

Reset to default
7

it only seem to apply to every other picture instead of every one

That's a classic sign of destructive iteration.

Consider what happens if, as I'm guessing, the function cvi_instant.add replaces the element named pic with some other element or elements.

getElementsByName returns a ‘live’ NodeList: it is kept up to date every time you make a change to the DOM. So if it had five elements before, after your call to cvi_instant.add it now contains only four: the first node is gone and nodes 1–4 have moved down to positions 0–3.

Now you go around the loop again. i++, so we're looking at element 1. But element 1 is now what was originally element 2! We skipped the original element 1, and we will continue skipping every other element until we reach the end of the (now half as long) list.

Altering a list at the same time as iterating it causes this kind of problem. If the process inside the iteration actually adds elements to the list you can even get an infinite loop!

The quick fix is to iterate the loop backwards. Now you do the last element first, leaving all the other elements in their original positions and causing no skipping:

 var e= document.getElementsByName("pic");
 for (var i= e.length; i-->0;) {
     cvi_instant.add(e[i], { shadow: 75, shade: 10 });
 }

Another simple solution if you know you're always going to be removing the element from the list on each call is:

 var e= document.getElementsByName("pic");
 while (e.length>0) {
     cvi_instant.add(e[0], { shadow: 75, shade: 10 });
 }

The most general solution is needed when your loop body can do anything to the list, such as inserting new elements named pic at the start of the document or removing other elements from the middle. It is slightly slower but always safe to make a static copy of the list to work from:

 function Array_fromList(l) {
     var a= [];
     for (var i= 0; i<l.length; i++)
         a.push(l[i]);
     return a;
 }

 var e= Array_fromList(document.getElementsByName("pic"));
 for (var i= 0; i<e.length; i++) {
     cvi_instant.add(e[i], { shadow: 75, shade: 10 });
 }
Improve this answer
Sign up to request clarification or add additional context in comments.

8 Comments

thanks for your comments . . do you have any suggestion for a fix here?
iterating backwards is an interesting "solution" - I rather like it!
I wouldn't have expected that the array returned from "getElementsByName" could be modified elsewhere. That's pretty dumb if they're not returning a new, unique array object,
bobince - you are a genius . . iterating backwards did the trick . .
Yeah, it's the same NodeList implementation as eg. document.links or document.body.childNodes, which you kind of expect to be ‘live’, applied consistently throughout the DOM. It's the misleading get in the method name that seems to imply you might be getting a static result list.
|
1

My guess is that cvi_instant.add() is doing some incrementing or iteration on the values passed to it. Try doing this instead - it's easier, and I believe it will fix your problem:

function done() {
  var e = document.getElementsByName('pic');
  for (pic in e) { cvs_instant.add(pic, { shadow: 75, shade: 10 }); }
}
Improve this answer

4 Comments

when i changed it to this . . the javascript function never ends and none of my pictures get the update.. any ideas?
"for/in" loop over an array might cause problems if you've added methods to the Array prototype (e.g., .bind from prototype.js). "pic" would end up being a function sometimes.
Note that I spelled "cvi_instant" wrong in my sample :) Can you provide us with the body of "cvi_instant" so we can see better what's going on?
what do you suggest . changing the "name" ??
1

Hi I came across the same problem. My script was skipping every other element. I finally solved it by simply changing the variable name from i to k in my loop. My guess is therefor that the variable i is used by getElementsByTagName internally to keep track of where it is in the live nodelist and is leaking out to the programmers interface somehow. So its a bug! :-)

Improve this answer

Comments

0

-- EDIT:

All of what I claim below appears to be totally wrong. I leave this here as a point for anyone who thought the same :) I tested in FF3. I would love to claim that I saw this behaviour once, in IE, but maybe it was many years ago (come to think of it, it was probably 7 years ago). My memory is probably bad :)

-- OLD:

To slightly expand on my wild guess, if it turns out to be accurate:

From memory, if you don't declare a variable ('var ...') it'll use one from somewhere else.

Thus, without testing, this code:

for(var k = 0; k < 2; k++){
    f();
    alert("k: " + k);
}

function f () {
  k++;
}

Should show the same behaviour. I think TML's solution is quite nice, from a 'defensive coding' point of view, it my analysis turns out to be correct.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.