prevent event bubbling to support nested lists

[ValYouW]

Currently all mouse/touch events propagate up in the DOM making it impossible to support nested Slip lists (dragging the a nested list item will cause the parent item to be dragged as well). In this PR I have just called e.stopPropagation() in all mouse/touch events, also added a nested list example in the example page. Tested in Chrome/Edge and on Android. There is a codepen page with the changes here

Thanks.

[carter-thaxton]

Looks great! Thanks for including a demo example.

[carter-thaxton]

@kornelski I'll merge the PR, but you'll have to create a new release.

[ValYouW]

Thanks

[ValYouW]

@kornelski That makes perfect sense... but raise a bigger issue... my assumption was that whatever happens under Slip list is contained withing Slip only. If that was not the intention there might be users out there who actually rely on the events being propagated, and this change would break their app. So we can either:

1. Add option preventPropagation (or whatever name) which defaults to false
2. If all needed is to support nested lists we can check whether the event was originated by an element that "belongs" to "this" Slip (assume we can identify a Slip container)

Thoughts?

[kornelski]

No, containment is wrong. That's not how the platform works.

For example, if you have a popup menu, you want to capture events from anywhere to close the menu when user clicks outside your menu. But if slip "ate" all its events, it would feel like clicking it doesn't do anything.

It's typical that elements stop events they understand and use, and are transparent to everything else/unused.

[ValYouW]

In the popup example I would want it to get closed even when Slip understands and uses the event, which won't happen with stopPropagation. The best option would be to provide the user with the original event but not sure how easy it is to make that change now... In regards to support nested lists, I don't see a way currently to detect whether an element is a Slip element, any objections adding some class to the container element? That way slip could ignore events that belongs to a nested slip.

[kornelski]

Sorry, the popup example was wrong - that was a case for the capturing phase.

Still, I don't see the need to unconditionally stop propagation. Native form elements still expose clicks inside them.

AFAIK this implementation eats all mouse clicks, of all buttons. Doesn't it break text selection? Context menus?

For just nested lists to work you only need to stop where preventDefault is currently.

If in your application you can make list a black box, you can add your own click handlers on the container and kill events there. The override the other way is not possible, so always killing events is less useful than letting unused events bubble.

The code is deliberately designed to never need class on elements. See how setTarget/findTargetNode is used.

[ValYouW]

stopPropogation does not affect browser behavior, text selection and browser context menu works fine. In any case I agree that overall stopPropogation is not good, but for the same reasons a selective stopPropagation is not good either...

findTargetNode logic will not work for nested lists detection as it can check only against this.container and not someNestedList.container. Here is what I have in mind:

attach: function(container) {
  ...
  this.container.classList.add('slipjs');
  ...
}

detach: function() {
  ...
  this.container.classList.remove('slipjs');
  ...
}

shouldProcessEvent(e) {
  var targetNode = e.target;
  while (targetNode) {
    if (targetNode === this.container) { return true; }
    // Event "belongs" to a nested list
    if (targetNode.classList.contains('slipjs')) { return false; }
    targetNode = targetNode.parentNode;
  }
  return false;
},

onMouseDown: function(e) {
  if (!this.shouldProcessEvent(e)) {return;}
  ...
}

It seems to be working... Another option instead of using a class is to store all the Slipjs containers in some global var and test if targetNode is in the list and not equal to this.container.

BTW, adding stopPropagation where preventDefault is today is not working for nested list support...