Why is using onClick() in HTML a bad practice?

Question

I have heard many times that using JavaScript events, such as onClick(), in HTML is a bad practice, because it's not good for semantics. I would like to know what the downsides are and how to fix the following code?

<a href="#" onclick="popup('/map/', 300, 300, 'map'); return false;">link</a>

Answer 1

I don't know what you mean with bad practice. Maybe if you create HTML and JS files by hand or with a template engine it is bad practice. But it is not always a bad practice, because it has significant downsides.

• JavaScript and HTML are normally strongly coupled. If you change one, you often have to change the other. Having both in the same proximity increases the cohesion, which is normally good.
• Separating JavaScript from HTML makes your code complicated with no benefit. It only pays off if there exists something like an abstraction layer. But we are talking here about HTML and JS. This is both the representation layer of your application. That is the lowest layer. You don't want to implement your business logic as html events. You don't want to pollute your business logic with html events. The html events are nothing else than craftsmen for your DOM.
• Separating JavaScript from HTML requires extra mechanisms like IDs and Classes and DOM navigation. This is extra code with no benefit.
• In general in programming you should try to do things as local as possible. Having global functions is normally a bad idea. That means if in an HTML page you have one element that should have an event to another element, you should define that event handler as local as possible. That would be the DOM tree up where both elements come together. That is hard to achieve in HTML, because HTML/JS has no such scoping. But making the conclusion that everything and all events should be global would be wrong. This is how React works. But that is not the only way. And that is not free from problems.
• Mixing of languages is not a problem. It is a strength. HTML and JS are designed to be used together. If you think, they don't work well together, you should live with this and not change your complete structure.
• Statements like you are more flexible, "you can add behaviour to a lot of HTML elements at once" are not helpful. This are hypothetic situations. There are situations where it could be good for example to "add behaviour to a lot of HTML elements at once" and there are situations where this is not ever expected. With the argument, it is more flexible and it enables new features you can justify everything.
• Nobody creates HTML by hand. If you have a tool or a library or a framework, it actually does not matter where you write your JS code. What matters is that you as a developer have a good overview of everything. And that is established by the mechanism that creates your HTML and not by the HTML representation.
• There are indeed good reasons why you should separate JS from HTML (see the other answers). If these matter for you, you should do this separation. But this is not a general recommendation. If you for example use Server Based Rendering and create HTML completely programmatically with a library like PyHiccup, it may not be so much of concern where you create your JS code. No human ever reads your HTML files.

Example

I try to pick out an arbitrary example. On that page it is recommended to prefer in React the following code

class MyButton extends React.Component {
  handleClick() {
    console.log('Button clicked!');
  }

  render() {
    return (
      <button onClick={this.handleClick}>
        Click me
      </button>
    );
  }
} 

over this version:

<button onClick={() => console.log('Button clicked!')}>
  Click me
</button>

But there are numerous downsides of this recommendation.

• The code is 4 times longer. There are studies that demonstrate that the lines of code have an impact to the number of bugs. Fewer lines of code result in fewer bugs even if the lines contain the same functionality and if they are more dense and more complicated (see here).
• If you quickly want to introduce an event, you have more code to write and more work.
• A log message is normally something, which you quickly want to add into the code for debugging purposes. You don't want to change the structure of the code for that. Debugging or testing should not induce such damage.
• The code uses much more complicated language features. The author or the reader have to joggle with more stuff in mind. That distracts him from the real problem.
• Apparently the code requires a more complicated structure. In this arbitrary example the code has a OOP structure. That decision alone comes with pros and cons. It is not good, that the simple convention to avoid inline events forces us to make other so huge decisions that, normally would not be required or which normally could also be made later. Maybe if you already use a framework like React and all these decisions have already been made, then the recommendation to avoid inline events is appropriate if this fits into the overall picture of your development approach. But there are so many frameworks, and they are all different. A general recommendation to avoid inline events cannot be made.
• The code is harder to test, because we have to instantiate the class. The code with the inline event can nearly (if it was not a React example) be tested directly in any browser.

Answer 2

Two more reasons not to use inline handlers:

They can require tedious quote escaping issues

Given an arbitrary string, if you want to be able to construct an inline handler that calls a function with that string, for the general solution, you'll have to escape the attribute delimiters (with the associated HTML entity), and you'll have to escape the delimiter used for the string inside the attribute, like the following:

const str = prompt('What string to display on click?', 'foo\'"bar');
const escapedStr = str
  // since the attribute value is going to be using " delimiters,
  // replace "s with their corresponding HTML entity:
  .replace(/"/g, '"')
  // since the string literal inside the attribute is going to delimited with 's,
  // escape 's:
  .replace(/'/g, "\\'");
  
document.body.insertAdjacentHTML(
  'beforeend',
  '<button onclick="alert(\'' + escapedStr + '\')">click</button>'
);

That's incredibly ugly. From the above example, if you didn't replace the 's, a SyntaxError would result, because alert('foo'"bar') is not valid syntax. If you didn't replace the "s, then the browser would interpret it as an end to the onclick attribute (delimited with "s above), which would also be incorrect.

If one habitually uses inline handlers, one would have to make sure to remember do something similar to the above (and do it right) every time, which is tedious and hard to understand at a glance. Better to avoid inline handlers entirely so that the arbitrary string can be used in a simple closure:

const str = prompt('What string to display on click?', 'foo\'"bar');
const button = document.body.appendChild(document.createElement('button'));
button.textContent = 'click';
button.onclick = () => alert(str);

Isn't that so much nicer?

---

The scope chain of an inline handler is extremely peculiar

What do you think the following code will log?

let disabled = true;

<form>
  <button onclick="console.log(disabled);">click</button>
</form>

Try it, run the snippet. It's probably not what you were expecting. Why does it produce what it does? Because inline handlers run inside with blocks. The above code is inside three with blocks: one for the document, one for the <form>, and one for the <button>:

let disabled = true;

<form>
  <button onclick="console.log(disabled);">click</button>
</form>

Since disabled is a property of the button, referencing disabled inside the inline handler refers to the button's property, not the outer disabled variable. This is quite counter-intuitive. with has many problems: it can be the source of confusing bugs and significantly slows down code. It isn't even permitted at all in strict mode. But with inline handlers, you're forced to run the code through withs - and not just through one with, but through multiple nested withs. It's crazy.

with should never be used in code. Because inline handlers implicitly require with along with all its confusing behavior, inline handlers should be avoided as well.

Answer 3

Revision

Unobtrusive JavaScript approach was good in the PAST - especially events handler bind in HTML was considered as bad practice (mainly because onclick events run in the global scope and may cause unexpected error what was mention by YiddishNinja)

However...

Currently it seems that this approach is a little outdated and needs some update. If someone want to be professional frontend developper and write large and complicated apps then he need to use frameworks like Angular, Vue.js, etc... However that frameworks usually use (or allow to use) HTML-templates where event handlers are bind in html-template code directly and this is very handy, clear and effective - e.g. in angular template usually people write:

<button (click)="someAction()">Click Me</button> 

In raw js/html the equivalent of this will be

<button onclick="someAction()">Click Me</button>

The difference is that in raw js onclick event is run in the global scope - but the frameworks provide encapsulation.

So where is the problem?

The problem is when novice programmer who always heard that html-onclick is bad and who always use btn.addEventListener("onclick", ... ) wants to use some framework with templates (addEventListener also have drawbacks - if we update DOM in dynamic way using innerHTML= (which is pretty fast) - then we loose events handlers bind in that way). Then he will face something like bad-habits or wrong-approach to framework usage - and he will use framework in very bad way - because he will focus mainly on js-part and no on template-part (and produce unclear and hard to maintain code). To change this habits he will loose a lot of time (and probably he will need some luck and teacher).

So in my opinion, based on experience with my students, better would be for them if they use html-handlers-bind at the beginning. As I say it is true that handlers are call in global scope but a this stage students usually create small applications which are easy to control. To write bigger applications they choose some frameworks.

So what to do?

We can UPDATE the Unobtrusive JavaScript approach and allow bind event handlers (eventually with simple parameters) in html (but only bind handler - not put logic into onclick like in OP quesiton). So in my opinion in raw js/html this should be allowed

<button onclick="someAction(3)">Click Me</button>

or

function popup(num,str,event) {
   let re=new RegExp(str);  
   // ... 
   event.preventDefault();
   console.log("link was clicked");
}

<a href="https://example.com" onclick="popup(300,'map',event)">link</a>

But below examples should NOT be allowed

<button onclick="console.log('xx'); someAction(); return true">Click Me</button>

<a href="#" onclick="popup('/map/', 300, 300, 'map'); return false;">link</a>

The reality changes, our point of view should too

Answer 4

• onclick events run in the global scope and may cause unexpected error.
• Adding onclick events to many DOM elements will slow down the
  performance and efficiency.

Answer 5

If you are using jQuery then:

HTML:

 <a id="openMap" href="/map/">link</a>

JS:

$(document).ready(function() {
    $("#openMap").click(function(){
        popup('/map/', 300, 300, 'map');
        return false;
    });
});

This has the benefit of still working without JS, or if the user middle clicks the link.

It also means that I could handle generic popups by rewriting again to:

HTML:

 <a class="popup" href="/map/">link</a>

JS:

$(document).ready(function() {
    $(".popup").click(function(){
        popup($(this).attr("href"), 300, 300, 'map');
        return false;
    });
});

This would let you add a popup to any link by just giving it the popup class.

This idea could be extended even further like so:

HTML:

 <a class="popup" data-width="300" data-height="300" href="/map/">link</a>

JS:

$(document).ready(function() {
    $(".popup").click(function(){
        popup($(this).attr("href"), $(this).data('width'), $(this).data('height'), 'map');
        return false;
    });
});

I can now use the same bit of code for lots of popups on my whole site without having to write loads of onclick stuff! Yay for reusability!

It also means that if later on I decide that popups are bad practice, (which they are!) and that I want to replace them with a lightbox style modal window, I can change:

popup($(this).attr("href"), $(this).data('width'), $(this).data('height'), 'map');

to

myAmazingModalWindow($(this).attr("href"), $(this).data('width'), $(this).data('height'), 'map');

and all my popups on my whole site are now working totally differently. I could even do feature detection to decide what to do on a popup, or store a users preference to allow them or not. With the inline onclick, this requires a huge copy and pasting effort.

Answer 6

With very large JavaScript applications, programmers are using more encapsulation of code to avoid polluting the global scope. And to make a function available to the onClick action in an HTML element, it has to be in the global scope.

You may have seen JS files that look like this...

(function(){
    ...[some code]
}());

These are Immediately Invoked Function Expressions (IIFEs) and any function declared within them will only exist within their internal scope.

If you declare function doSomething(){} within an IIFE, then make doSomething() an element's onClick action in your HTML page, you'll get an error.

If, on the other hand, you create an eventListener for that element within that IIFE and call doSomething() when the listener detects a click event, you're good because the listener and doSomething() share the IIFE's scope.

For little web apps with a minimal amount of code, it doesn't matter. But if you aspire to write large, maintainable codebases, onclick="" is a habit that you should work to avoid.

Answer 7

Your question will trigger discussion I suppose. The general idea is that it's good to separate behavior and structure. Furthermore, afaik, an inline click handler has to be evalled to 'become' a real javascript function. And it's pretty old fashioned, allbeit that that's a pretty shaky argument. Ah, well, read some about it @quirksmode.org

Answer 8

You're probably talking about unobtrusive Javascript, which would look like this:

<a href="#" id="someLink">link</a>

with the logic in a central javascript file looking something like this:

$('#someLink').click(function(){
    popup('/map/', 300, 300, 'map'); 
    return false;
});

The advantages are

• behaviour (Javascript) is separated from presentation (HTML)
• no mixing of languages
• you're using a javascript framework like jQuery that can handle most cross-browser issues for you
• You can add behaviour to a lot of HTML elements at once without code duplication

Answer 9

It's a new paradigm called "Unobtrusive JavaScript". The current "web standard" says to separate functionality and presentation.

It's not really a "bad practice", it's just that most new standards want you to use event listeners instead of in-lining JavaScript.

Also, this may just be a personal thing, but I think it's much easier to read when you use event listeners, especially if you have more than 1 JavaScript statement you want to run.

Answer 10

There are a few reasons:

1. I find it aids maintenence to separate markup, i.e. the HTML and client-side scripts. For example, jQuery makes it easy to add event handlers programatically.

2. The example you give would be broken in any user agent that doesn't support javascript, or has javascript turned off. The concept of progressive enhancement would encourage a simple hyperlink to /map/ for user agents without javascript, then adding a click handler prgramatically for user agents that support javascript.

For example:

Markup:

<a id="example" href="/map/">link</a>

Javascript:

$(document).ready(function(){

    $("#example").click(function(){
        popup('/map/', 300, 300, 'map');
        return false;
    });

})

Answer 11

It's not good for several reasons:

• it mixes code and markup
• code written this way goes through eval
• and runs in the global scope

The simplest thing would be to add a name attribute to your <a> element, then you could do:

document.myelement.onclick = function() {
    window.popup('/map/', 300, 300, 'map');
    return false;
};

although modern best practise would be to use an id instead of a name, and use addEventListener() instead of using onclick since that allows you to bind multiple functions to a single event.