_mouseDown method causing issue with child elements receiving event

[artknight]

Subject of the issue

An SPAN inside the .grid-stack-item element had a mouseDown event attached, however that was intercepted by the _mouseDown method.

Solution

I ended up adding span to the skipMouseDown array.

const skipMouseDown = ['input', 'textarea', 'button', 'select', 'option', 'span'];

Hope this helps!

[adumesny]

this isn't the right fix. we need to detect that an element (div, span, etc..) has mouse handling and let it handle instead... Not sure how to do that yet...
the above items clearly use mouse events, so no brainer. the others ?

[artknight]

@adumesny so it seems gridstack uses div elements for draggable fields. I would say if there were a way to extend that list from options that are passed in at initialization that would be a great help! I saw you left a comment in there with similar thoughts.. I think until there is a better solution ( that I cannot think of atm ) this would be the next best thing. imho

[adumesny]

maybe the child handling the event need to prevent it from propagating up (to the grid item), or marked it was handled so it can skip it there... that list extensible seem like a bandaid and wouldn't work in general (you have span, but no reason the grid itself isn't a span or any dom element.

[artknight]

@adumesny the problem is that for some reason gridstack gets the mousedown event before the child element. In my particular case I am using tomselect.js ( https://tom-select.js.org/ ) inside the gridstack field. However, when clicking on the dropdown option the click gets picked up by gridstack before it gets to tomselect, and thus the event is stopped from propagating.

[adumesny]

can you post a reproduceable example please ? can't fix it without one. ideally it would be plain html/js and not use any library I don't want to have to debug...

[artknight]

@adumesny here is the screenshot that shows that gridstack gets the event first and triggers a .blur() on the activeElement, which affects tomselect.

[adumesny]

I'm adding the event as bubble (not capture, unlike the move) so GS should get the event only if the child didn't consume it , not first crack at it.

https://github.com/gridstack/gridstack.js/blob/master/src/dd-draggable.ts#L88
https://github.com/gridstack/gridstack.js/blob/master/src/dd-draggable.ts#L152

this.dragEl.addEventListener('mousedown', this._mouseDown);
....
protected _mouseDown(e: MouseEvent): boolean {
...
    // document handler so we can continue receiving moves as the item is 'fixed' position, and capture=true so WE get a first crack
    document.addEventListener('mousemove', this._mouseMove, true); // true=capture, not bubble
    document.addEventListener('mouseup', this._mouseUp, true);
}

so don't think we get that event first... that lib need to consume that event instead. Again I need a reproduceable code showing the issue on our side.

[artknight]

@adumesny well, I understand your explanation! However, the screenshot above clearly shows the event getting picked up by GS first, and only then by tomselect. I am guessing that b/c the dropdown options are only getting added to the DOM on click they get handled in a different order.

I see a comment by you for the skipMouseDown param .....

// make sure we are not clicking on known object that handles mouseDown (TODO: make this extensible ?) #2054

Perhaps it is time to make the skipMouseDown param extensible? At least it would give me a way to updated it instead of making changes to the core js.

[adumesny]

your callstack doesn't show 'mousedown' handler for that lib at all - I would put a breakpoint there and in GS and see what happens... yes GS does call that lib onBlur() but that doesn't mean it didn't get the mouse down event first and failed to do the right thing.

[artknight]

of course it shows the mousedown handler! I believe that is the only way for it to be triggered. Since I have tested it thoroughly with all kinds of breakpoints, and deduced that it always goes through GS before it gets to tomselect.

If you are not interested in fixing it, at least please make it extensible :)

[adumesny]

we're not on the same page.... where does it show tomselect.onMouseDown() event handler (or whatever they would call it). all I see is their onBlur()
as I mentioned already, I need a reproduceable case to 'fix'. I don't have the bandwidth to spent this much on every 'bug' that comes in and debug where the issue really lies... done until you can do so.

[artknight]

@adumesny I think you are missing the point. The issue is that mouse events are processed by the event loop in a specific order. In this particular case you are capturing the mousedown event. According to the docs, the click event is triggered after the mousedown and mouseup events ( https://javascript.info/mouse-events-basics ). Therefore, the event gets captured by GS _mouseDown method and then the blur is triggered on the activeElement, which in this case is the tomselect dropdown option. .

//GS code on line 171
if (document.activeElement)
   document.activeElement.blur();

..Hope this makes sense...

The blur messes up tomselect into thinking the user changed his mind and closes prematurely.

[adumesny]

@artknight you origonally said (and I quote)
| An SPAN inside the .grid-stack-item element had a mouseDown event attached

so based on that all I can say is that span mouseDown handler should be called before GS gets to see down event (which you failed to show above) and should say it was handled insead of pasisng to GS. But now you say they only do click event, which is a very different thing...

@Gezdy ^^^ do you have suggestion since you're the one who added the blur() in Gezdy@5014538 for #2063

that lib cares about click event (down+up), and GS handles the down but messses it up.

[Gezdy]

Hello,I will try to check it asap, hope tomorrow. 

[Gezdy]

Hi @adumesny,

I did the previous correction as a developer and not as an architect, I kept the architecture.
For example, e.preventDefault() were already there avoiding blur events in the normal click event life cycle.
This is why I added a blur event call to keep focus on the active element.

The problem mentioned by @artknight is broader. The need is to not call "e.preventDefault()" in certain circumstances.

To resolve this, there are 2 different approaches:

1. (Architect) Review of mouse listeners: the cost can be high in completion of mouse events or partially refactoring
2. (Developer) Add a new option

I think the best approach is to add a new option as said by @artknight and as we see in comments in your code

if we declare a new option:

{
    skipMouseDownEls: ['selector 1', 'selector 2']
}

Then we can skip elements in _mouseDown (dd-draggable.ts) method like this:

if ((e.target as HTMLElement).closest( (this.option.skipMouseDownEls || []).join() )) return true;

This will resolve the @artknight problem.

[adumesny]

I would like the fix it the right way - may need to see what jquery-ui did and it if solved that problem instead of adding a workaround users will have to figure out. Yes, I added that comment about making the fixed list possibly expandable, but not sure that's the best approach...

[artknight]

I second @Gezdy's point, ...I actually really do like the idea of exposing the skipMouseDown param. That way it will give the developer the desired flexibility to ignore any child elements. Ultimately that is what I ended up doing anyways to make my app work, I just hate to having to modify the GS core lib for that.

[Gezdy]

I think there is a misunderstood.
Suggested "skipMouseDownEls" option is in addition to "skipMouseDown" parameter.

You keep the following part:

// make sure we are not clicking on known object that handles mouseDown (TODO: make this extensible ?) #2054
const skipMouseDown = ['input', 'textarea', 'button', 'select', 'option'];
const name = (e.target as HTMLElement).nodeName.toLowerCase();
if (skipMouseDown.find(skip => skip === name)) return true;

You only change this part:

// also check for content editable
//if ((e.target as HTMLElement).closest('[contenteditable="true"]')) return true;
let skipMouseDownEls = this.option.skipMouseDownEls || [];
skipMouseDownEls.push('[contenteditable="true"]');
if ((e.target as HTMLElement).closest( skipMouseDownEls.join() )) return true;

In that way, you let to users the possibility to skip elements without changing the core behaviour.
So, this is not a workaround in my opinion.

Hope this will help you in your analysis.

PS: I understand very well when you say "I would like the fix it the right way"

[artknight]

@adumesny any thoughts on what @Gezdy is suggesting?

[adumesny]

I don't have a reproduceable case to debug this, and would need to use the old jquery-ui code to see if they have it working there, but looking at their code they appear to have an option as well...

https://api.jqueryui.com/draggable/#option-cancel

  _mouseDown: function( event ) {
...
    // event.target.nodeName works around a bug in IE 8 with
    // disabled inputs (#7620)
    elIsCancel = ( typeof this.options.cancel === "string" && event.target.nodeName ?
    $( event.target ).closest( this.options.cancel ).length : false );
    if ( !btnIsLeft || elIsCancel || !this._mouseCapture( event ) ) {
      return true;
   }
..
  this.document
    .on( "mousemove." + this.widgetName, this._mouseMoveDelegate )
    .on( "mouseup." + this.widgetName, this._mouseUpDelegate );
  event.preventDefault();
  mouseHandled = true;
  return true;
}

so maybe it's ok to add as well. doesn't look like they are looking for a click() handler on the parent to skip our mousedown which would be the better fix IMO...

[adumesny]

fixed in next release. don't forget to donate if you find this lib useful!