2

I'm writing a simple text-base game in my free time. It's called TARDIS flight (Doctor Who!) and I'm working on the main menu.

So I'm using a function, addMainMenuListeners, to add all the event listeners with addEventListener, after I set the innerHTML of the main menu.
Everything works fine, until the point where I go back to the main menu from one of the submenus. Then, I found that the buttons don't work anymore.
I'm calling addMainMenuListeners after I set the innerHTML, but even though I do it, and I do it in the console, and I check, there is no event.

Code:
In my main javascript file:

function addMainMenuListeners()
{
    if($("start")) $("start").addEventListener("click", startGame);
    if($("instructions")) $("instructions").addEventListener("click", instructions);
    if($("settings")) $("settings").addEventListener("click", settings);
    if($("back")) $("back").addEventListener("click", resetMainMenu);
    if($("volume")) $("volume").addEventListener("change", function(){saveAndChangeVolume($("volume").value);});
}

function instructions()
{
    $("mainmenu").innerHTML = "<h1>Instructions</h1><p>Fly your TARDIS through the time vortex using the console.  Make sure that you use the correct materialization codes.  Try to keep the time-space coordinates close to the values of the ones given.  AND DON'T CREATE PARADOXES, WHATEVER YOU DO!</p><button id='back'>Back</button>";
    addMainMenuListeners();
    return true;
}

function settings()
{
    $("mainmenu").innerHTML = "<h1>Volume</h1><span>1</span><input type='range' id='volume' min='1' max='100' /><span>100</span><br><button id='back'>Back</button>";
    addMainMenuListeners();
    loadVolume();
    return true;
}

function resetMainMenu()
{
    $("mainmenu").innerHTML = "<h1>TARDIS Flight</h1><button id='start'>Start!</button><button id='instructions'>Instructions</button><button id='settings'>Settings</button>";
    addMainMenuListeners();
    return true;
}



And in my HTML file:

<div id="mainmenu">
<h1>TARDIS Flight</h1>
<button id="start">Start!</button>
<button id="instructions">Instructions</button>
<button id="settings">Settings</button>
</div>

If you need any clarification, just ask.

EDIT: Evidentally, nobody got what I meant. I was readding the event listeners after doing the innerHTML, as you can see from the code. I simply cannot see the event being added, the function is firing but not adding the event.
Also, I am using a custom $ function, just a return document.getElementById(id) sort of function.

Atutouato
  • 375
  • 4
  • 13
  • You appear have jQuery added to the page, why are you not using any of the jQuery functions? `.text()`, `.on()`, etc? – Kyle Muir Dec 11 '13 at 03:08
  • 1
    I made my own `$` function. It's just a `return document.getElementById(id)` function. Sorry I didn't clarify. – Atutouato Dec 11 '13 at 03:10
  • Or is `$()` defined as something other than a jQuery-like library? I think @KyleMuir has the correct assumption. It looks like you're mixing vanilla JS DOM properties with jQuery objects. – Sukima Dec 11 '13 at 03:11
  • What do you mean, that is the issue here? – Atutouato Dec 11 '13 at 03:13
  • adding on to @KyleMuir you should use jquery, and add a # before the mainmenu. # is for id and . is for class. – iCezz Dec 11 '13 at 03:13
  • 1
    ***Folks, OP is not using jQuery. It doesn't look like jQuery. OP confirmed it above that it's not jQuery. If it was jQuery, it wouldn't have worked at all.*** – Blue Skies Dec 11 '13 at 03:15
  • @iCezz, I am doing fine with my own `$` function. I would prefer not to use jQuery. – Atutouato Dec 11 '13 at 03:16
  • 1
    @Atutouato if I was you, I would have multiple divs with my "stages" of my TARDIS flight in it and show\hide those when things are clicked rather than using `.innerHTML` which is destroying your event handlers. – Kyle Muir Dec 11 '13 at 03:18
  • can you even try to alert 'ok' when click on any button? because most likely your calling of element is wrong thus unable to add the handler correctly. – iCezz Dec 11 '13 at 03:19
  • What is meant by *"until the point where I go back to the main menu from one of the submenus"* – Blue Skies Dec 11 '13 at 03:19
  • @KyleMuir, that's a good idea. I'll try that. – Atutouato Dec 11 '13 at 03:23
  • ...and you say that you can't "see" the event listeners. Are you asking specifically about display in the console? – Blue Skies Dec 11 '13 at 03:23
  • @BlueSkies, what I mean by that is that I click on the "Settings" button, then the submenu appears, and then I click on the "Back" button. Then it doesn't do anything when I click on any of the main menu buttons. – Atutouato Dec 11 '13 at 03:24
  • @BlueSkies, it does not do the event, and I do not see it in the chrome console. – Atutouato Dec 11 '13 at 03:24
  • Nothing wrong with making your own `$()` it's just confusing for others reading it. Either convert to `getElementById` in the SO question or use a different name. – Sukima Dec 11 '13 at 03:30
  • use bind() in jquery check [this][1] and [this][2] for more detail. [1]: http://stackoverflow.com/questions/2398099/jquery-equivalent-of-javascripts-addeventlistener-method [2]: http://stackoverflow.com/questions/7025437/addeventlistener-in-jquery – F.C.Hsiao Dec 11 '13 at 03:30
  • @Sukima: OP doesn't need to change the variable name. – Blue Skies Dec 11 '13 at 03:30
  • @Atutouato: Do you get any error messages in the console? I can't see why the handlers wouldn't be bound again... though that sort of DOM destruction isn't a great idea IMO. – Blue Skies Dec 11 '13 at 03:31
  • 1
    Is it possible that when the `addMainMenuListeners` is called that the elements from the `innerHTML` statement haven't been added to the DOM yet? Is innerHTML synchronous? Does your `$()` method do any caching? – Sukima Dec 11 '13 at 03:31
  • 1
    @player889, I'm not using jQuery!!! – Atutouato Dec 11 '13 at 03:31
  • @Sukima, my $ method does use caching from a suggestion on a previous question for another project. And no, `addMainMenuListeners` is called after the `innerHTML` setting. – Atutouato Dec 11 '13 at 03:33
  • 2
    I think maybe this is your problem: http://stackoverflow.com/questions/16776606/innerhtml-cant-be-trusted-does-not-always-execute-synchronously – Sukima Dec 11 '13 at 03:34
  • Atutouato: If you've cached the elements, then @Sukima is probably right. If the `$` has them cached by ID, then it would return the previous one that you destroyed. And no, don't do that sort of caching. – Blue Skies Dec 11 '13 at 03:35
  • @Sukima: Hm... I'll try using `setTimeout`s and see if that helps. – Atutouato Dec 11 '13 at 03:36
  • @BlueSkies, good idea. Goodbye, caching. – Atutouato Dec 11 '13 at 03:36
  • @BlueSkies, disabling caching worked. – Atutouato Dec 11 '13 at 03:40
  • And @Sukima, `setTimeout`s worked too. – Atutouato Dec 11 '13 at 03:40
  • Just to be clear, it can sometimes be a good idea to cache locally in a variable to avoid repeated calls like in your `addMainMenuListeners` function. But I wouldn't do it as a global cache via the `$` function. That's just asking for memory leaks. ;-) – Blue Skies Dec 11 '13 at 03:41
  • Please post your $ function for future reference, since the caching in that function turned out to be the problem. (That $function might benefit from an optional second argument to flag that the requested id's cache should be updated). @Sukima: nice catch (without knowing his $function)!!!!! – GitaarLAB Dec 11 '13 at 03:42
  • ...also heed [Kyle's advice above](http://stackoverflow.com/users/2469255/kyle-muir) and don't destroy and rebuld your elements like that. – Blue Skies Dec 11 '13 at 03:46
  • 1
    @GitaarLAB, good idea. I'll do that. – Atutouato Dec 11 '13 at 03:48
  • possible duplicate of [addEventListener gone after appending innerHTML](http://stackoverflow.com/questions/2684956/addeventlistener-gone-after-appending-innerhtml) – rlemon Dec 11 '13 at 04:04

3 Answers3

5

Check to see if your $() uses any caching. If it caches old references to elements then when innerHTML is set the $("id") will return a reference to an invalid reference.

[edit] The references are more likely valid even though they are no longer visible in the HTML DOM. So modifying the detached elements works but it doesn't do any good since they are detached from the DOM.

Sukima
  • 9,965
  • 3
  • 46
  • 60
  • @Sukima, then fix it (instead of changing your answer to start with 'this answer is incorrect'. (I just deleted my comment because I started doubting myself, but just confirmed by the comments that I was right, You DID find the caching-problem, so let your answer reflect that, whilst the asker is updating his question to include his @ Function – GitaarLAB Dec 11 '13 at 03:56
  • 2
    You might want to delete all the [cowbell](http://vimeo.com/51038971) (aka incorrect previous answer) – GitaarLAB Dec 11 '13 at 04:02
  • You say: 'If it caches old references to elements then when innerHTML is set the $("id") will return a reference to an *invalid reference*.' I did some testing out of curiosity and (at least in FF) it turns out that if you have some 'caching' to elements, one can still access those elements after a `parentNode.removeChild` or full `parentNode.innerHTML` rewrite via the cached reference (so the element is not gone, just no longer in html visible to the user), so the properties were set but to the wrong elm. See this fiddle (that includes my proposed refresh-id flag): http://jsfiddle.net/G28Lu/ – GitaarLAB Dec 11 '13 at 12:07
1

Sukima psychic ability's catched the main problem: your custom $ function (to replace document.getElementById) used a caching mechanism.

After some testing (out of personal curiosity) it turned out that as long as you have some reference to an element, the element is still accessible, even after elm.parentNode.removeChild or a full elm.parentNode.innerHTML rewrite (at least, in FF).

In other words: the events WERE added, every time, just to the wrong/old/previous elements instead of the new ones. Otherwise there would also have been errors that the elements didn't exist and thus didn't have an addEventListener method..

See this crude test-fiddle for proof: http://jsfiddle.net/G28Lu/


I toyed around with a $ function (as you haven't posted yours yet) and gave it an 'refresh'-flag to go with the optional cache mechanism:

var $=function(/*cache_enabled*/c){  // getElementById with optional caching & refresh
   return c ?( c={}, function(s,r){return (!r && c[s]) || (c[s]=document.getElementById(s));} 
           ):( function(s){return document.getElementById(s);} );
}(true); // or nothing or 0 etc to disable the cache

Note: dynamically created functions are slow (for some reason) so it has 2 functions so the browser can run them optimized (and the other should be cleared by the garbage collector).

To request a fresh just add a flag that evaluates to true as second argument, like: $('elm_id', 1)

Then I modified your addMainMenuListeners function a little to first test for the existence of a fresh getElementById and then add the eventListener via the freshly updated cached reference (so, essentially I changed nothing in the flow of your routine).

formatted to emphasize what changed and how it works

function addMainMenuListeners(){
   $(   'start'    , 1) && $(   'start'    ).addEventListener('click', startGame);
   $('instructions', 1) && $('instructions').addEventListener('click', instructions);
   $(  'settings'  , 1) && $(  'settings'  ).addEventListener('click', settings);
   $(   'back'     , 1) && $(    'back'    ).addEventListener('click', resetMainMenu);
   $(  'volume'    , 1) && $(   'volume'   ).addEventListener('change', function(){ 
                                               saveAndChangeVolume($('volume').value); 
                                             });
}

Finally, putting above 2 functions and the rest of your functions/html into this fiddle rendered a working result: http://jsfiddle.net/xHUGu/ !
Note: I had to substitute a dummy function startGame otherwise there would be a fatal error. The missing volume-functions were not critical.

I would like to point out that this is not really the way to go with your interface, there would be a lot of work you could save both yourself and the browser. You might want to look into div's (containing your subsections of html) and toggle them so there is only one visible. Like tabs. (Hint for google-query).

Credit still goes to Sukima ('the force is strong in this one'), but I felt it was a good idea to share the correct explanation to your problem (with proof) and not to waste the work that was done anyway (out of curiosity).

Hope this helps!

Community
  • 1
  • 1
GitaarLAB
  • 14,536
  • 11
  • 60
  • 80
  • 1
    This is an incredibly detailed response. Though I have fixed the problem and finished the game, this is the type of answer that I feel people should always give. – Atutouato Dec 13 '13 at 03:29
  • You are very welcome, txh for the upvote! (I'm curious to your game hehe). – GitaarLAB Dec 13 '13 at 03:36
0

Disabling caching on the $ function worked. It was referencing to a destroyed HTML element, and that's why it didn't work. Also, setTimeouts helped for reliability in the case that innerHTML didn't execute in time.

Atutouato
  • 375
  • 4
  • 13