0

Does anyone know why if you use document.body.innerHTML += "content"; the JavaScript on the page stops working? I have the following code:

document.addEventListener('contextmenu', function (e) {
    e.preventDefault();
    try {
        document.getElementById('menu').remove();
    } catch (x) {
        //
    }
    var newHtmlText = "<div id='menu'>";
    newHtmlText += "<div class='menu-item' id='menu-back' onclick='window.history.back();'>" +
        "<div class='fa fa-arrow-left icon'></div><div class='text'>Back</div><div class='clear'></div></div>";
    newHtmlText += "<div class='menu-item' id='menu-forward' onclick='window.history.forward();'>" +
        "<div class='fa fa-arrow-right icon'></div><div class='text'>Forward</div><div class='clear'></div></div>";
    newHtmlText += "<div class='menu-item' id='menu-reload' onclick='location.reload();'>" +
        "<div class='fa fa-repeat icon'></div><div class='text'>Reload</div><div class='clear'></div></div>";
    newHtmlText += "<hr />";
    newHtmlText += "<div class='menu-item' id='menu-home' onclick='location.href = \"/\";'>" +
        "<div class='fa fa-home icon'></div><div class='text'>Home</div><div class='clear'></div></div>";
    newHtmlText += "</div>";
    document.body.innerHTML += newHtmlText;
    var menu = document.getElementById('menu');
    menu.style.left = (e.clientX) + "px";
    menu.style.top = (e.clientY) + "px";
});

Every time I open the context menu the JavaScript stopped working. This is not the only time it has done this.

Makyen
  • 31,849
  • 12
  • 86
  • 121
Pete
  • 333
  • 1
  • 12
  • There's no `jquery` code here. Maybe you meant `javascript`. – abhishekkannojia Jul 20 '17 at 12:39
  • Instead of this document.body.innerHTML += elem; use $('body').append(elem); ... since you are using jQuery. – Shiladitya Jul 20 '17 at 12:40
  • It should be `document.body.innerHTML = elem`. You need to remove the `+` sign. – Milan Chheda Jul 20 '17 at 12:40
  • 1
    The other place I am using it is on a chrome extension, so I don't know if the user has jQuery or not. – Pete Jul 20 '17 at 12:40
  • If you're using it in a chrome extension I'd suggest you come up with a more unique id than "menu" – Khauri Jul 20 '17 at 12:41
  • @Pete Add this information: _" I am using it is on a chrome extension"_ in the post. – abhishekkannojia Jul 20 '17 at 12:42
  • @Pete It is not a matter of "what users have" but of "what YOU use or include". – ADreNaLiNe-DJ Jul 20 '17 at 12:42
  • Possible duplicate of [AngularJS breaks on innerHTML edit](https://stackoverflow.com/questions/36497066/angularjs-breaks-on-innerhtml-edit) – Rhumborl Jul 20 '17 at 12:46
  • BTW: You should not be using the variable name `elem` here. It implies that what it represents is an element. What it represents is HTML text. Thus, it should have a name like `newHtmlText`, or some other name that describes what it is, not something that describes what it definitely is *not*. – Makyen Jul 22 '17 at 17:38
  • As already mentioned, the IDs, classes, names, etc. which you use in an extension should *always* be non-generic. You are often adding elements and CSS to random pages. *All* such identifiers should have values which are unlikely to be duplicated in-page. This can be accomplished by adding a prefix unique to your extension to *every single one*. While this does not guarantee that you will have no collisions with in-page code, it makes it *much* less likely. This can be a pain in the rear, but it is part of what you should be doing in your extension to make it compatible across multiple pages. – Makyen Jul 22 '17 at 17:44
  • There are many ways to [represent multi-line strings](https://stackoverflow.com/questions/805107/creating-multiline-strings-in-javascript). The way that you have chosen is poor. It makes maintaining the HTML quite difficult. You should pick a methodology which allows you to write the HTML in a way that *looks* like HTML which as written in an *.html* file (e.g. indented with, usually, a single element per line). Doing so makes maintaining the HTML *much* easier. – Makyen Jul 22 '17 at 18:26

3 Answers3

5

Using x.innerHTML += y is equivalent to x.innerHTML = x.innerHTML + y;

This means that you are completely overwriting the old document with a new document - it may appear visually the same, but under the hood you've just nuked every single reference to everything.

If a bit of JavaScript elsewhere in the page used something like var container = document.getElementById('container');, in order to save a reference, well that reference is now gone.

If there are any event listeners bound to elements in the document, those are gone too because the elements were nuked and replaced with identical-looking ones.

If you want to add your context menu to the page, you should do something like:

var menu = document.createElement('div');
menu.id = 'menu';
menu.innerHTML = "Your menu HTML here";
document.body.appendChild(menu);

This will add the new element to the page without nuking the whole thing.

Niet the Dark Absol
  • 320,036
  • 81
  • 464
  • 592
2
document.body.innerHTML += "content";

Does three things:

  1. Reads the value of innerHTML
  2. Modifies that value
  3. Overwrites innerHTML with the new value

This deletes the page and then creates a new one.

Since script elements inserted with innerHTML are not executed, this kills the JS.

Don't append data using innerHTML. Generate DOM nodes (with createElement, createTextNode and friends) and then append them (with appendChild, insertBefore and so on).

Quentin
  • 914,110
  • 126
  • 1,211
  • 1,335
1

As has been explained by others, using:

document.body.innerHTML += newHtmlText;

deletes the entire page and recreates it. This causes:

  • All event listeners which were placed on elements to become disconnected from the DOM. If there is not some external reference to the DOM element or the listener function (e.g. a named function), the listener will be garbage collected (browser dependent).
  • Any saved references to elements will now reference elements which are no longer in the DOM. These DOM nodes, and their children, will not be garbage collected, as there is still a reference to them. The JavaScript using these references will not throw errors unless it tries to find references to associated DOM nodes to which the element is no longer connected (ancestor nodes, e.g. .parentNode). However, the JavaScript will be completely ineffective at manipulating the displayed DOM, as it will be using references to elements which are no longer in the DOM.
  • The entirety of the old DOM will be garbage collected (browser dependent), except for any elements, and their children, which have a reference saved elsewhere in JavaScript code.

This will almost certainly completely break most already existing JavaScript.

Use .insertAdjacentHTML()

The correct way to add HTML text, without disturbing the already existing contents, is to use element.insertAdjacentHTML(relativeLocation, HTMLtext). Using .insertAdjacentHTML(), you can add HTML text in four different locations relative to the referenced element. The value of relativeLocation can be:

  • 'beforebegin': Prior to the element
  • 'afterbegin': Just inside the beginning of element (like adding a new .firstChild, but you can add as many children (as much HTML text) as you desire).
  • 'beforeend': Prior to the end of the element (like .appendChild(), but you can add as many children (as much HTML text) as you desire).
  • 'afterend': Just after the element (like element.parentNode.insertBefore(newElement,element.nextSibling), but you can add as many children of the parentNode (as much HTML text) as you desire). Note: If you were inserting an element you could also use: element.insertAdjacentElement('afterend',newElement).

For what you desire to do, you would use:

document.body.insertAdjacentHTML('beforeend',newHtmlText);
Makyen
  • 31,849
  • 12
  • 86
  • 121