Blog Home RSS Kelvin Jackson

Don't Nest Listeners, Even If They Aren't Link Tags!


I've decided to make a rule that if I lose more than a certain amount of time hunting down a particular bug, I'll write a blog post about it so that others (or future versions of me) have a chance at not having to lose that time if they run into the same issue. I'm not sure exactly what the threshold will be, but it should become clearer in the near future.

Today, I lost some time because I did something stupid with HTML. I had something that could be simplified down to essentially this (in React JSX, not pure HTML):

<div onClick={doSomething}> <a onClick={undoThatSomething}> Something in here </a> ... </div>

The a tag was positioned in such a way that it could easily be clicked without clicking directly on the div. Because the outer component was a div and not another a tag, I failed to notice that I was essentially nesting a link inside another link* until I had been poking around for a while, trying to figure out why the undoThatSomething() function wasn't doing its job.

A hacky solution to this is to call .stopPropagation() inside of undoThatSomething(e). I tried that as a way to get a preliminary version of the feature in question working, and it would have worked fine — except that that would have been terrible coding practice. Sometimes you can't avoid setting things up in an awkward way and then working around it, but cases where that's truly necessary are few and far between. We can do better.

I'm not going to go into too much detail about how I re-wrote this particular component, because in the end I got rid of both the doSomething() and undoThatSomething() functions in the process of lifting state out of this component in order to solve a different (albeit related) bug, but if that hadn't happened, the obvious solution here would have been to move the a tag out of the div — it had position: fixed already, so it wouldn't have required any CSS tinkering:

<div className="wrapper-to-make-react-happy"> <a onClick={undoThatSomething}>Something in here</a> <div onClick={doSomething}> ... </div> </div>

And there you have it.

In addition to being a cautionary tale about being careful when you code, this also provides yet another reason not to attach click listeners to div tags (or any tag that isn't already meant to be clickable). It's very tempting to do, and can make sense when you're running a quick test to see how your app would work if a particular event were clickable, but it's never a good idea in any project meant for production.

*By the way, if you aren't already keenly aware of this, it's worth knowing that browsers won't stop you from putting a link inside another link — they'll just "correct" the error to something that fits the HTML spec, by moving one link outside of the other or something like that. Don't nest a tags, kids — it's just not worth it.