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

10

155

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>
Operatic answered 3/5, 2011 at 15:10 Comment(6)
en.wikipedia.org/wiki/Unobtrusive_JavaScriptBondy
There's nothing wrong with the onlickc, but by making the href #, anyone who chooses to have javascript disabled will be stuck and unable to do anything. For some sites that's a good thing, but for simply opening a window, it's outright stupid to not provide a "real" link.Domella
Definitely read that link. It has very little to do with semantics, and more to do with ... all the stuff on that page :-)Incise
Try to open your link in a new tab, and you'll see an example of why it's wrong ...Matthewmatthews
That should be a <button>, because links are supposed to point to an actual resource. It's more semantic that way and is more clear for screen reader users.Ralaigh
Stackexchange also has great answers.softwareengineering.stackexchange.com/questions/86589/…Benedicto
L
181

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
Laboy answered 3/5, 2011 at 15:24 Comment(18)
You have listed the quite a few advantages, but what about the disadvantages? Debugging blue smoke?Florance
The wiki article has a one liner about criticisms, though it does link to a great article that is 75% advantages, and 25% disadvantages.Florance
@abelito: Not sure debugging is a disadvantage. If anything it is an advantage to debugging as you can step through your JavaScript within any browser debugging tool. Not sure you can do that as easy if the code is in-line an onClick. You also can write unit tests if you wish to against your scripts which is also very hard I'd assume if the code is inside an onClick and no logic is separated. I personally have no issue debugging unobtrusive JavaScript and the benefits in managing and testing JavaScript code are far to great to not to use it.Audley
@François Wahl: the main disadvantage is discoverability: looking at the HTML does not tell you which callbacks are attached to it, not even whther there are any.Laboy
@MichaelBorgwardt: I completly agree with you that it is a disadvantage. If code is well structured and organised I don't see it as a huge issue when working on a project or debugging someone elses code base. In general, while I agree with you, I don't see discoverability to be a good reason to write in-line script, sacraficing benefits such as code testability and the ability to separate out your function/behaviour code away from the DOM. I'm not saying in-line code is bad and won't work. It does and is absolutly fine, depending if you are interested in things like testability or not.Audley
What Michael said was the point I was trying to emphasize, though I agree cluttering the HTML is very undesireable. Nowadays, I try my best to keep the HTML clean and the blue smoke to a minimum by grouping all page-loading on(X) hooks in a centralized location in my javascript, clearly commented and separated away from other more general methods and functions.Florance
Good example. You could also make a link class and then style it to appear as a regular link on the page and bind an onclick event to all elements of the link class or one in particular, binding to its id.Glaze
@MichaelBorgwardt "You can add behaviour to a lot of HTML elements at once without code duplication"---Does this mean something along the lines of: adding something like onkeypress="someFilter();" on each textfield on a form individually, mixing it into the HTML rather than using jQuery, and using multiple selectors and doing it all at once on a script? Is my understanding of that bullet point correct?Joellajoelle
@Abdul: what you quoted describes an advantage of the code in my answer. Putting Javascript calls on each textfield individually would clearly be code duplication. I am not sure what you mean with "using multiple selectors and doing it all at once on a script".Laboy
@MichaelBorgwardt - by "using multiple selectors and doing it all at once on a script", I meant like $('#input1, #input2, #input3').each(function() { applySomeFilter() }); on each input that requires same filterJoellajoelle
@Abdul: Ah, I see. That's a rather fine point of design and should be decided on a case by case basis.Laboy
@abelito, I completely agree. Debugging becomes a challenge on big codebases with such approach. Especially when working mostly with the code you didn't write. So I prefer to write plain old inline JS inside of HTML attributes. For those who are from a React.js world it shouldn't be a big deal :)Behold
what about separation between html and javascript when using same id ("someLink" in this sexample) it connects strongly html and javascript, and mix the languages.Doff
@sarkiroka: well, you need a connection for it to work at all. And it definitely does not constitute "mixing the languages".Laboy
@MichaelBorgwardt, I see. but don't you think it would be better if the connection were outside from javascript or html? for example a json file describes this connection. after that the javascript contains only operations, and html contains only markup. and of course, the json contains link to a html fragement (for example with css3 selectors) and a link to javascript code fragement (for example a method name in an object, or a name of global function)Doff
Another point to this discussion, with novices - the onclick can more explicitly map causal relationships between the element interaction and the effect (function call) as well as reduce the cognitive load. Many lessons throw learners into the addEventListener which packs many more ideas inside a more difficult syntax. Furthermore, recent component based frameworks like Riot and React use the onclick attribute and is changing the notion of what separation should be. Transferring from HTML onclick to a component that supports this might be a more effective "step" process for learning.Mashe
What about Angular's (click)="..."? Is it bad too?Neary
@ImpulseTheFox: With Angular and similar client-side app frameworks, the design philosophy is quite different, there you don't have a separation of relatively fixed presentation (HTML) and reusable functionality (JavaScript code), instead you have separate small components of both HTML and Javascript, and the unit of reuse is that component.Laboy
A
43

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.

Aspirin answered 3/5, 2011 at 15:17 Comment(10)
Works without JavaScript?? It's jQuery. It needs Javascript enabled on the browser to work, right?Touchline
Yes, but the link can redirect to a page performing the action without JavaScript in this case.Retire
The link works without JavaScript. See how the link has a normal href attribute. If the user doesn't have JavaScript the link will still be a link and the user can still access the content.Incise
@Thomas Shields: No, he means that if you don't have Javascript, the link will still take you to /map/...Trevatrevah
@Thomas Shields no, without JavaScript, the browser will just follow the link to /map/ as that's the value of the href. The only difference is that it does not end up in a popup (in this case, as that's the functionality that JavaScript adds here)Herewith
JQuery is 31kb traffic for each pageload is too much,one onclick() isn't cost this)Operatic
It's not 31kB for each pageload, it's 31kB per site. If you use the Google CDN version, it's 31kB across all sites using it. In this trivial example, it's not worth it, but if you are writing a large site with 100 instances of a popup class, then this will pay for itself, both in download time as well as in maintainability.Aspirin
Rich, Google CDN don't so fast all the time I'we saw big problems with it.DNS and networks problems... :-(Operatic
Just use it like this: <script src="//ajax.googleapis.com/ajax/libs/jquery/1.5.1/jquery.min.js"></script> <script>window.jQuery || document.write('<script src="js/libs/jquery-1.5.1.min.js">\x3C/script>')</script> That falls back to a local version if the CDN doesn't work.Aspirin
Ah Rich! We all love you for this niftiest solution!Meteor
R
24

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.

Ricks answered 7/2, 2014 at 22:16 Comment(1)
if you aspire to write large, maintainable codebases use JS frameworks like Angular, Vue, React ... which recommend to bind event handlers inside their HTML templatesDefenestration
D
21

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.

Dislocate answered 3/5, 2011 at 15:20 Comment(3)
Even suggesting this way opens the user up to a bunch of problems with event handler collisions.Angrist
@Angrist just like an inline event handler does, hence why my answer says it's better to use addEventListener(), and why.Dislocate
Can someone explain this more? "code written this way goes through eval" ... I find nothing on the web about this. Are you saying there's some performance or security disadvantage to using inline Javascript?Domingo
I
15

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

Intellectualism answered 19/2, 2019 at 15:16 Comment(5)
hi Kamil, I am beginner at JavaScript and also faced confusion on the use of onclick, that is, please can you explain the disadvantages of onclick as the following: 1. You may only have one inline event assigned. 2.Inline events are stored as a property of the DOM element and, like all object properties, it can be overwritten. 3.This event will continue to fire even if you delete the onclick property.Agglomerate
Ad 1. Yes only one, however my experience (with angular) shows that this is enough in most situations - however if not then just use addEventListener. Ad 2. Yes they can be overwritten (however usually nobody do it) - where is the problem? Ad 3. Handler will be not fired after delete onclick attrib - proof hereDefenestration
React, Vue and Angular might look like they are using "inline event handlers" but they are not using an HTML attribute as described in the original question. They are using directives and under the hood they will be using addEventListener . (click) and onclick are definitely not equivalentZoogloea
@Zoogloea yes - but wrong habits persistsDefenestration
IMHO your onclick="someAction(3)" is an example of a "wrong habit". The someAction function is polluting the global namespaceZoogloea
P
12

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

})
Pulmotor answered 3/5, 2011 at 15:21 Comment(1)
Thanks for reminding me of progressive enhancement and so this still fits that paradigm as well: <a href="/map/" onclick="popup('/map/', 300, 300, 'map'); return false;">link</a>Hallel
B
8

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.

Bondy answered 3/5, 2011 at 15:22 Comment(3)
The Wikipedia page on the subject is over 4 years old. It isn't a new paradigm any more. :)Beeline
@David: It may not be "new", but there are people still not using it, so it's "new" to them.Bondy
So my concern here is that when I write this function and add it as an event listener, then there's a bug and someone else wants to fix it, in the HTML way they can inspect the button, see the function the fires, then copy and paste that into the browser console window and boom, takes you right to the function. With event listeners, I've got to search for the specific ID, which may have multiple instances across the same application.Archibald
E
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

Edelman answered 3/5, 2011 at 15:25 Comment(1)
I don't wanna create once more holy war, I'm trying to find true :\Operatic
L
2
  • 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.
Lully answered 7/11, 2018 at 23:5 Comment(0)
M
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, '&quot;')
  // 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>

enter image description here

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.

Machicolate answered 19/5, 2020 at 21:9 Comment(0)

© 2022 - 2024 — McMap. All rights reserved.