JavaScript: Attach onclick to a link inside "while" loop

Question

I'm having trouble with attaching an onclick to each link object inside a loop, when you click the link, it seems to always return data relevant to the first item in the loop, regardless what was clicked. Where as i need each link clicked to have the relevant href to that link

In the example below, regardless what link was clicked, the console.log would always show "http://example.com/link1/"

HTML Example

  <li>
    <h2><a class="t" href="http://example.com/link1/"><i>Link 1 text</i></a></h2>
    <div class="panel" >Content</div>
  </li>

  <li>
    <h2><a class="t" href="http://example.com/link2/"><i>Link 2 text</i></a></h2>
    <div class="panel" >Content</div>
  </li>

  <li>
    <h2><a class="t" href="http://example.com/link3/"><i>Link 3 text</i></a></h2>
    <div class="panel" >Content</div>
  </li>

</ul>

JavaScript:

(function(){
  var theh2s = document.getElementById("panelList").getElementsByTagName("h2"), 
  i = theh2s.length;

  while (i--) {

      var getTheLinks = theh2s[i].getElementsByTagName("a");

      if (getTheLinks){
        if (getTheLinks[0].href){

          console.log(getTheLinks[0].href);

          getTheLinks[0].onclick = function() {
            console.log(getTheLinks[0].href);
            _gaq.push(['_trackEvent', 'Homepage', 'AB Click - Test B', getTheLinks[0].href]);
          };


        }
      }

  }
})();

possible duplicate of [Assign click handlers in for loop](http://stackoverflow.com/questions/4091765/assign-click-handlers-in-for-loop) — Pointy, Nov 14 '13 at 15:29

score 1 · Accepted Answer · 2013-11-15T05:53:50.813

1

The problem is that when a click occured, getTheLinks has already been set to the last h2 in the list. To prevent each loop to override the previous one, you have to ~~use a closure~~ create a new context using this pattern : (function(i){...})(i).

As Felix Kling mentionned, a closure actually is the source of the problem. The following article could enlighten you about this concept. There is a paragraph concerning this common pitfall you just have encountered : https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Closures.

(function () {
    var theh2s = document.getElementById("panelList").getElementsByTagName("h2"),
        i = theh2s.length;
    while (i--) {
        (function (i) {
            var getTheLinks = theh2s[i].getElementsByTagName("a");
            if (getTheLinks) {
                if (getTheLinks[0].href) {
                    console.log(getTheLinks[0].href);
                    getTheLinks[0].onclick = function () {
                        console.log(getTheLinks[0].href);
                        _gaq.push(['_trackEvent', 'Homepage', 'AB Click - Test B', getTheLinks[0].href]);
                    };
                }
            }
        })(i);
    }
})();

I'm not familiar with JSLint. If you need to be JSLint valid, I guess you'll have to move the function definition outside the loop like this :

while (i--) {
    f(i)
}
function f(i) {
    // same as above
}

edited Nov 15 '13 at 05:53

answered Nov 14 '13 at 15:37

Thanks this worked. Can you explain why this was different? I understand that i was actioning an object not assigning a variable as you have done – acasperw Nov 14 '13 at 15:41
*"To prevent each loop to override the previous one, you have to use a closure"*. This part of the explanation is not correct. The click handler is already a closure. In fact, closures are the source of the problem. So adding another closure cannot be the solution. The reason why this works is that a new scope is created, by executing a function. That function doesn't have to be a closure though (technically every function is a closure in JS, but I hope you get my point). – Felix Kling Nov 14 '13 at 15:43
@FelixKling Thanks for clarifying, I probably misuse the term, and my bad english also doesn't help :D So, I guess I should say "you have to create a new context"? – Nov 14 '13 at 15:43
Yeah, that sounds better :) – Felix Kling Nov 14 '13 at 15:56
Hi Both, running this JS Lint i receive the error "Don't make functions within a loop." – acasperw Nov 14 '13 at 16:01
It might have been easier to define the function outside the loop, and secondly to pass in the event which would give you e.currentTarget. From that you would know which element had been clicked and thus you would be operating from information specific to the event, NOT specific to the current state of the closure within which the function was defined. Remember a closure inherits the state of it's enclosing closure, and this is what killed you here. – mangr3n Nov 14 '13 at 16:15
@acasperw I've added a note about JSLint, but I'm unable do more about that, sorry :/ – Nov 14 '13 at 16:20
@acasperw: You are getting the error for exactly the problem you are experiencing. Unfortunately JSLint doesn't seem to be smart enough to detect that you are actually using an IIFE to cope with that problem. A solution is to define the function outside the loop or use JSHint and disable the warning ;) – Felix Kling Nov 14 '13 at 22:16

score 1 · Answer 2 · answered Nov 14 '13 at 16:21

I'll go ahead and demonstrate my preferred solution to this problem

(function(){
  function processLinkClick(e) {
    var link = e.currentTarget;
    console.log(link.href);
    _gaq.push(['_trackEvent', 'Homepage', 'AB Click - Test B', link.href]);
  }

  var theh2s = document.getElementById("panelList").getElementsByTagName("h2"), 
  i = theh2s.length;

  while (i--) {

      var getTheLinks = theh2s[i].getElementsByTagName("a");

      if (getTheLinks){
          if (getTheLinks[0].href){

              console.log(getTheLinks[0].href);

              getTheLinks[0].onclick = processLinkClick;


          }
      }

   }
})();

... and yes that function could be inlined, I just broke it out to be more clear.

Remember event functions receive an event object, and that should (if possible) be the only thing you use to establish context for processing the event. In this case the context is, "link that was clicked", the event answers that question, not the enclosing context. I would argue from a software design point of view that using the event is going to be a cleaner solution and easier to maintain in the long run.

+1 Using `this` instead of `e.currentTarget` also works right? — , Nov 14 '13 at 16:26
Technically, I believe it does, I'm forgetting off the top of my head, why I stopped doing that. If I recall it's because I usually use something like dojo.hitch or $.click and at one time I saw issues that confused me about the "this" context for my attached function. dojo.hitch also allowed you to specify the "this" context for the hitched, or attached function. So, if I can I use the event. — mangr3n, Nov 14 '13 at 16:30

JavaScript: Attach onclick to a link inside "while" loop

2 Answers2