Using `return false` and/or `stopPropagation` on click events doesn't play nice with other libraries

[philipwalton]

Any library that binds click events to the document could potentially have functionality broken because jquery-ujs stops event propagation from [data-remote] clicks.

For example, most tooltip libraries are supposed to hide a shown tooltip when you click anywhere outside of the tooltip target element, but if your click outside happens to be on a link with a data-remote attribute, that event gets killed and the tooltip doesn't go away. Obviously this is a minor example, but it should help illustrate the conflict.

Since jquery-ujs is very high-level and abstract, I personally don't see any compelling reasons (other than perhaps historical) for it to be stopping event propagation.

Admittedly, making this change would likely break functionality on existing sites, so perhaps it could be set with a config option, so projects that rely on this behavior can still have it.

If this is a direction the jquery-ujs project is willing to go, I'd be happy to submit a pull request with the changes.

[JangoSteve]

This isn't actually true. All jquery-ujs bindings are delegated to the document, which is the root node. By the time any jquery-ujs bindings get called, the event has already propagated up to the root node, so it shouldn't interfere with any other click bindings.

[philipwalton]

Weird, you're right. Let me see if I can figure out what else is causing this conflict and report back.

On the same note, what is the reason for returning false if the event is already at the document? Why not just preventDefault()?

[JangoSteve]

That's a good question. To be honest, it's been a long time since I wrote that code, but if I remember correctly, we needed both preventDefault and stopPropagation for some reason, which is what return false essentially does, possibly some bug on IE or something.

I'm sure there should be some test in the test suite that fails on some browser without it, but if not, it's possibly we can remove it for now. It's possible it's since been fixed in jQuery. It's also remotely possible that it's just legacy code that never got changed.

[philipwalton]

Actually, it looks like there is still a problem.

Here's a reduced test case:

1. Click on the link and you won't see the alert.
2. Remove data-remote and you do see the alert
   http://jsfiddle.net/s45bu/

Here's the same case but with a version of jquery-ujs that uses e.preventDefault() instead of return false and it works fine:
http://jsfiddle.net/nrgKg/

I haven't looked into what jQuery is doing that's causing this problem, but it almost seems like stopPropagation on document is acting more like stopImmediatePropagation, which is strange.

Again, if you want, I'm willing to fix these issues, make sure the tests pass, and submit a pull request without the return false code.

[philipwalton]

I was pretty curious about why this was happening, so I did a bit more digging and discovered what jQuery is doing behind the scenes.

It appears that stopPropagation works differently on delegated event bindings than it does on non-delegated ones. Here's an example:

// assume the <a> element is inside the <p> element
$(document).on("click", "p", function() {
  alert("called!");
})
$(document).on("click", "a", function(e) {
  e.stopPropagation();
})

Based on the way stopPropagation works for non-delegated event bindings, you'd expect both handlers to run if you clicked the <a> element since both handlers are bound to document, but this isn't the case. jQuery is smart and actually figures out the order in which the delegated events handlers should be called based on where the elements that match the selectors appear in the DOM.

In addition, for delegated event bindings it applies stopPropagation based on real DOM placement. In other words, in the example above, jQuery knows that the <p> element is an ancestor of the <a> element so when stopPropagation is invoked in the handler bound on <a>, it ignores all handlers that are delegated to elements higher up in the DOM tree, even though they were originally bound to the same element.

Here is an example I put together to help illustrate. You can play with toggling the return false statements on the various handlers to see how the output changes.

For jquery-ujs, this means that any event handler bound to document (or any event handler bound to document and delegated to an element higher in the DOM tree than the UJS element) will not be called.

In my app, I have a link element that is registered as one of our custom Dropdown components. it also happens to have data-remote as an attribute. As a result, jquery-ujs is preventing the Dropdown initialization code from running when the link is clicked.

This could easily be remedied by using preventDefault instead of return false; though (as I said), I'm sure it would cause issues for countless sites that likely depend on the existing behavior without realizing it.

[scarfacedeb]

I vote +1 for using just preventDefault instead of return false, because now I can't trigger click handlers on the document.

My use case:

I'm opening popup and I want to close it when user clicks on any element outside of this popup.

$document.on( 'click.order', function(e){
      var $target = $(e.target);

  if ( e.target.id == 'order-close' || 
    !$target.closest('#order_container').length ){
    self.hide();
  }
});

It works well, except when I click on a link with [data-remote] attribute.
I think that when a click event bubbles up to the document, it reaches handler for [data-remote] first, which in turn prevents it from going further into my handler.

For now, I use an ugly workaround like this:

$services.on( 'click', 'a[data-remote]', function(){
  Order.hide();
});

---

I don't see any reason to include return false; instead of preventDefault in similar cases (e.g. css-tricks advises to use preventDefault), but I didn't test it with old and obscure browsers like IE6-7.

[jonkessler]

Is there any particular reason why this hasn't been changed? This is certainly an annoyance.

[aroberts]

I'm also hitting this as a blocking issue. Is there a list of browsers that tests need to pass in for pulls to get merged in?

[lucasmazza]

We don't have a strict list of browsers we support but if this change doesn't break anything on the latest major versions of browsers and some of latest IEs I think we are good to go. Feel free to send a Pull Request and we will check how this change affects our test suite 😄

[JangoSteve]

The browsers I always tested in, at the very least, were the latest official (not beta or preview) versions of:

• Chrome
• Firefox
• Safari
• Opera
• Internet Explorer

There aren't generally differences in JavaScript execution between the save browsers on different OS's, so I wouldn't be too concerned with that. If there are, someone will catch them and we can fix them.

Just FYI though, last I checked, there is still one individual test that fails on IE, and always has, and there's nothing we can do about it. So if you get one failing test on IE, make sure to run the suite both before and after your change, and make sure it's the same failing test.

[JangoSteve]

Oh, also, make sure the tests pass in each browser with each version of jQuery supported (which are linked at the top right of the test page layout).

And, I would very occasionally test in the next most recent versions of browsers if I had some reason to suspect a change might have an effect on that.

[JangoSteve]

Sorry for the multiple comments, but regarding this particular ticket, there were certain versions of browsers with certain versions of jQuery where certain behavior didn't work properly unless we actually did return false. It's entirely likely that time has solved this problem with the newer versions of browsers and jQuery.

[jonkessler]

@JangoSteve What's the word on this? It's been 5 months.