views:

936

answers:

2

Hi,
I use code like this to dynamically create some link-based controls:

<script type="text/javascript">
    function BuildControls (id, ...)
    {  
        var div = document.getElementById (id);  
        /* some stuff with var sp=document.createElement('span'); */
        var a = document.createElement ('a');
        a.innerHTML = on ? 'Cancel' : captions_do[i];
        a.className = class_do;
        a.style.display = on2 ? '' : 'none';
        a.id = 'iAccept'+id+'_'+i+'Control';
        div.appendChild (sp);
        div.innerHTML+='<br />';
        div.appendChild (a);
        a.onclick = function () {alert ('wtf')};
        div.innerHTML+='<br />';
    }
</script>
...
<div id="someid"></div>  
<script type="text/javascript">  
    BuildControls ('someid', ...);
</script>

So when I click on the link, it does nothing. If I call a.onclick() explicitly, it works. What is wrong?

A: 

Your code as posted works fine in IE7 and Firefox - there must be something in the /* ... */ that's breaking it.

Greg
I've updated the code, removing some ...s. Is there something wrong with it?
stanch
hm... I tried to remove the <br> adding thing. It helps. Thank you.
stanch
@stanch: mixing `innerHTML` and direct DOM manipulation is generally a bad idea - only do it if you know what you're doing ;)
Christoph
+1  A: 

div.innerHTML+='<br />';

That is syntactical sugar for:

div.innerHTML= div.innerHTML+'<br />';

Which makes it clearer what is happening: you just serialised the entire contents of the div to HTML, added a tiny string, and parsed the results back into HTML. Even if it worked flawlessly, that'd be really inefficient.

When you serialise an element, you lose any non-attribute-based information attached to it, including JavaScript properties, event handlers and listeners. Say goodbye to your onclick.

Never use “innerHTML+=”. It is always a mistake.

An alternative is to set innerHTML on a standalone div not attached to the document, then append its childNodes one-by-one to the element where you really want it.

But for something this simple, you should just be using the standard DOM methods:

div.appendChild(document.createElement('br'));
bobince
I've already found this out, thank you. Seems that you answer is the most perfect.
stanch