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]);
          };


        }
      }

  }
})();

Answer 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
}

Answer 2

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.