fix Safari bug with dropping element inside grid

[wurambo]

Description

This should fix #1684

In Safari, relatedTarget is null, causing the dragleave event to fire the dropout event early. To detect if we've actually dropped out, I've added a check to make sure the node has the _temporaryRemoved prop set. There was similar logic in place in the dropout event in v3 so this might have been the fix for that.

I've also tested that this is still working in Chrome and Edge, but was having issues trying to run the local build on Firefox

Current behavior

With fix

Checklist

• Created tests which fail without the change (if possible)
• All tests passing (yarn test)
• Extended the README / documentation, if necessary

[adumesny]

thanks for taking a stab. I'll have to take a look this weekend. I thought relatedTarget==null was the issue to work around, and !node._temporaryRemoved to return seem counter intuitive as that should be set during that leave event.... but will need to run through that code again.

[wurambo]

Yep, relatedTarget==null is the root cause of the issue. However, changing the check in the dragleave from

    if (this.el.contains(event.relatedTarget as HTMLElement)) return;

to

    if (!event.relatedTarget || this.el.contains(event.relatedTarget as HTMLElement)) return;

causes a bug where the grid it belongs to loses all drag and drop capabilities.

gif below of updated logic

I think in v3, there was logic that performed a similar check but the prop was named differently which gets set in the dropover event

      // jquery-ui bug. Must verify widget is being dropped out
      // check node variable that gets set when widget is out of grid
      if (!node._isOutOfGrid) {
        return;
      }

[adumesny]

please change

if (this.el.contains(event.relatedTarget as HTMLElement)) return;

to

// Note: Safari Mac has null relatedTarget which causes #1684 but handled elsewhere
if (event.relatedTarget && this.el.contains(event.relatedTarget as HTMLElement)) return;

assuming there isn't another field we could use there instead ????

and in V3 we had _isOutOfGrid but that ONLY used for this, whereas _temparyRemoved has a different meaning and used in the next call which itself checks for if (node._temporaryRemoved) return; so don't see how your code would do the right thing...

      // fix #1578 when dragging fast, we might get leave after other grid gets enter (which calls us to clean)
      // so skip this one if we're not the active grid really..
      if (!node.grid || node.grid === this) {
        this._leave(node, el, helper, true); // MATCH line 166
      }

in V3:

    .on(this.el, 'dropover', (event, el: GridItemHTMLElement) => {
      // ignore drop enter on ourself, and prevent parent from receiving event
      let node = el.gridstackNode;
      if (node && node.grid === this) {
        delete node._added; // reset this to track placeholder again in case we were over other grid #1484 (dropout doesn't always clear)
        return false;
      }

      // load any element attributes if we don't have a node
      if (!node) {
        node = this._readAttr(el);
      }

      // if the item came from another grid, let it know it was added here to removed duplicate shadow #393
      if (node.grid && node.grid !== this) {
        node._added = true;
      }

      // if not calculate the grid size based on element outer size
      let w = node.w || Math.round(el.offsetWidth / this.cellWidth()) || 1;
      let h = node.h || Math.round(el.offsetHeight / this.getCellHeight(true)) || 1;

      // copy the node original values (min/max/id/etc...) but override width/height/other flags which are this grid specific
      let newNode = this.engine.prepareNode({...node, ...{w, h, _added: false, _temporary: true}});
      newNode._isOutOfGrid = true;
      el.gridstackNode = newNode;
      el._gridstackNodeOrig = node;

      GridStackDD.get().on(el, 'drag', onDrag);
      return false; // prevent parent from receiving msg (which may be grid as well)
    })
    .on(this.el, 'dropout', (event, el: GridItemHTMLElement) => {
      let node = el.gridstackNode;
      if (!node) return;

      // clear any added flag now that we are leaving #1484
      delete node._added;

      // jquery-ui bug. Must verify widget is being dropped out
      // check node variable that gets set when widget is out of grid
      if (!node._isOutOfGrid) {
        return;
      }

and note that the comment said jquery-ui bug (which was the only plugging originally) yet your bug only happens in H5 or JQ as well?

[wurambo]

That change will not work - the reason why the dropout event is being called is because it's failing the conditional, it should be true and returning early but it's not. Since event.relatedTarget==null, if (event.relatedTarget will return false and not return early

// Note: Safari Mac has null relatedTarget which causes #1684 but handled elsewhere
if (event.relatedTarget && this.el.contains(event.relatedTarget as HTMLElement)) return;

  /** @internal called when the item is leaving our area, stop tracking if we had moving item */
  private _dragLeave(event: DragEvent): void {
    if (this.el.contains(event.relatedTarget as HTMLElement)) return;

    // In Safari, instead of returning early event.relatedTarget is null and causes the rest of the handler to run
    this._removeLeaveCallbacks();
    if (this.moving) {
      event.preventDefault();
      const ev = DDUtils.initEvent<DragEvent>(event, { target: this.el, type: 'dropout' });
      if (this.option.out) {
        this.option.out(ev, this._ui(DDManager.dragElement))
      }
      this.triggerEvent('dropout', ev);
    }
    delete this.moving;
  }

[wurambo]

The bug only happens in H5, JQ seems to be fine. It seems to be just an issue with the HTML5 dragleave event

[adumesny]

I didn't mean my suggestion was the fix, more a documenting the Safari bug where it matters.

worth checking if Safari sends any params we could use instead to fix at the right place. as I mentioned Safari HTML5 draggable is rather buggy vs other browsers I tried and why (and mobile) I may need to do a mouse pointer + touch event rather in a future release

[wurambo]

I didn't mean my suggestion was the fix, more a documenting the Safari bug where it matters.

Ah got it, will do - I'll document the bug in the handler

worth checking if Safari sends any params we could use instead to fix at the right place. as I mentioned Safari HTML5 draggable is rather buggy vs other browsers I tried and why (and mobile) I may need to do a mouse pointer + touch event rather in a future release

I think the only other params we could utilize is checking if the DragEvent occurred inside the Grid by comparing the DOMRect position. I've updated the PR to use that logic instead.

I realized that my earlier fix did not work as intended, it actually introduced another bug so definitely not the correct solution.

And agreed, I think a native mouse and touch event would provide better cross-browser compatibility. I didn't realize how buggy HTML5 draggable was even after being released for so long

[adumesny]

yes, thanks. I like that fix much better :)
(the other didn't look right)

[adumesny]

should be <=

[adumesny]

else if (this.el.contains(event.relatedTarget as HTMLElement)) return;
no reason to check with null value.