React-grid-layout: Can anyone confirm that onResizeStop layout arg != onLayoutChange currentLayout arg?

[TheImpressionist]

I'm not quite sure whether this is a bug or intended. I can see it working both ways.

Anyways here's my issue:

...
  onLayoutChange = (layout: Array<Layout>): void => {
    // this argument doesn't match the one bellow
  }

  onResizeStop = (layout: Array<Layout>, oldItem: Layout, newItem: Layout, _, __, ___): void => {
    // layout here doesn't match the one above
  }

Basically what happens is when elements are next to each other, and you have multiples of them, and then you resize the one on the right, the ones on the left will be moved out of their original position even if they are not colliding:

(2 is all one item total length of 3 2's)
(All the others are standalone items)

1 | 2
3 | 2
4 | 2

// onDragStop happens, we reduce the height of the 2 item
// Next layout state

1 | 2
4 | 2
3 |

The ones on the left side get re-arranged as if they had to be due to collision or something.
However, if I do the layout control using onLayoutChange method all's well in the layout world.
I am using onResizeStart, onResize, onResizeStop for finer control, same with drag methods.

Just wanna know if this is intended or potentially a bug.

[TheImpressionist]

I'll look into the source code for the onDragStop and onLayoutChange. If it's a bug I'll submit a PR once I have the time.

[TheImpressionist]

Alright, as I understand this is intended. Considering the source code:

onDragStop(i: string, x: number, y: number, { e, node }: GridDragEvent) {
    const { oldDragItem } = this.state;
    let { layout } = this.state;
    const { cols, preventCollision } = this.props;
    const l = getLayoutItem(layout, i);
    if (!l) return;

    // Move the element here
    const isUserAction = true;
    layout = moveElement(
      layout,
      l,
      x,
      y,
      isUserAction,
      preventCollision,
      this.compactType(),
      cols
    );

    this.props.onDragStop(layout, oldDragItem, l, null, e, node);

    // Set state
    const newLayout = compact(layout, this.compactType(), cols);
    const { oldLayout } = this.state;
    this.setState({
      activeDrag: null,
      layout: newLayout,
      oldDragItem: null,
      oldLayout: null
    });

    this.onLayoutMaybeChanged(newLayout, oldLayout);

The layout gets compacted after the new layout gets generated. And as I understand this is the compaction that produces the correct state.
But wouldn't this be more logical @STRML?

  /**
   * When dragging stops, figure out which position the element is closest to and update its x and y.
   * @param  {String} i Index of the child.
   * @param {Number} x X position of the move
   * @param {Number} y Y position of the move
   * @param {Event} e The mousedown event
   * @param {Element} node The current dragging DOM element
   */
  onDragStop(i: string, x: number, y: number, { e, node }: GridDragEvent) {
    const { oldDragItem } = this.state;
    let { layout } = this.state;
    const { cols, preventCollision } = this.props;
    const l = getLayoutItem(layout, i);
    if (!l) return;

    // Move the element here
    const isUserAction = true;
    layout = moveElement(
      layout,
      l,
      x,
      y,
      isUserAction,
      preventCollision,
      this.compactType(),
      cols
    );


    // Set state
    const newLayout = compact(layout, this.compactType(), cols);
    const { oldLayout } = this.state;
    this.setState({
      activeDrag: null,
      layout: newLayout,
      oldDragItem: null,
      oldLayout: null
    });

    // Both now output the compacted layout
    this.props.onDragStop(newLayout, oldDragItem, l, null, e, node);
    this.onLayoutMaybeChanged(newLayout, oldLayout);
  }

I'm asking because when the breakpoint changes the onLayoutChange gets triggered and every now and then the items get re-ordered wrong without any drag, just the effect of the breakpoint change. That's a bit counter productive to keeping layouts in the same order as they were before. I tried preventing updates when the oldBreakpoint !== newBreakpoint and the onLayoutChange fires, but some of my users experience the issue of onLayoutChange still messing up their layout. Or I might be doing something wrong, which is not out of the picture.

Would just like to get a discussion going to understand how this is supposed to work.

[TheImpressionist]

Alright, by snooping around a little bit (barely, really), I found that the utils that are used in onResizeStop before calling onLayoutMaybeChanged are exported in the module.

So this is what I've done to have the layout get compacted using onDragStop method, without using onLayoutUpdate:

  private onResizeStop = (layout: Array<Layout>, oldItem: Layout, newItem: Layout, __, ___, ____): void => {
    if (!compareLayoutEntries(oldItem, newItem)) {
      const siblings = getXSiblings(newItem, layout);
      // Ignore, this is snap to X axis siblings 
      const snappedItem = snapToSiblingY(newItem, siblings, 10);
      const index = layout.findIndex(item => item.i === newItem.i);

      if (index < 0) {
        return;
      }

      // Ignore, assigning snapped item
      const newLayout = layout.map(entry => entry.i === snappedItem.i ? snappedItem : entry);
      // What I do here is I call the compact function exported in the utils
      // Provide the compaction type and the columns for the current breakpoint
      // Result: no messed layouts when using onDragStop.
      //              No need to use onLayoutChange when you want finer control
      this.props.updateLayoutEntry(RGL.utils.compact(newLayout, 'vertical', getDefaultColumns()[this.props.breakpoint]), this.props.breakpoint);
    }
  }

Perhaps the documentation should be updated about the utils being exported as well? The TypeScript typings don't mention that either, so I had to update my own typings to account for that. I'll probably do a PR on @types/react-grid-layout to have it account for the functions in the utils module.

Thank you for listening to my rants and train of thoughts.

Closing since a solution has been found. Feel free to let me know if you have any questions.

[TheImpressionist]

Re-opening this because the issue persists.

The output of onDragStop, the first argument, which is layout: Array<Layout> is absolutely wrong.
Moving a single element re-orders others seemingly at random, although no collision should have happened for that to take place. The final coordinates for the elements that weren't moved are all wrong. They have not been under collision with anything, nor had to be compacted.

I suspect compaction.

[TheImpressionist]

Running the utils compaction method doesn't solve this. It seems that it re-orders the elements thinking there was some sort of a collision even though there wasn't.

[TheImpressionist]

Right, I have this sorted out using another method, which is fairly ugly. But the issue persist if I don't use onLayoutChange and do custom control usingonDragStop. This method generates a wrong layout.

Not sure whether this is intentional or not, but seems like a design oversight.

[eddyw]

Moving a single element re-orders others seemingly at random, although no collision should have happened for that to take place. The final coordinates for the elements that weren't moved are all wrong. They have not been under collision with anything, nor had to be compacted.

I'm experiencing the same issue when updating a grid item h and there is, in fact, a collision. For me, when a grid item is not x: 0, it's moved down kind of like y => y + collisionItem.h + 1 (how it looks, I don't know what code does), so everything else after the collision is sometimes randomly placed somewhere else (along y)

Right, I have this sorted out using another method, which is fairly ugly. But the issue persist if I don't use onLayoutChange and do custom control usingonDragStop. This method generates a wrong layout.

Would you be able to share this fairly ugly approach? 🙈

[TheImpressionist]

@eddyw Sure, I'll do it a bit later. But it involves using the onLayoutChange and having extra update logic inside of the method servicing it when it should and shouldn't update.

I have found that onLayoutChange produces a more reliable result, but not all the time. Under larger load it reverts to randomly re-arranging the elements on drag or resize. I suspect the collision detection is tied to the performance somehow.

Looking into the source code I found there what looks like data mutations in a lot of places. This might produce the ugly side effect too. I wonder what's the design decision to mutate current state instead of producing new arrays with new objects each time. I suppose the reason is performance, but that's only a guess.

Once I get back to working on this I'll take a look and see if I could make a PR with a fix.

[TheImpressionist]

Alright, I've had some time to look into what might be wrong, and I'm honestly not sure anymore.

Now, this example is a bit laggy, because it has layouts within a layout. And there are a lot of components, which might be the cause of this, but regardless, I'd like to see if we could get this sorted out somehow.

Basically, when using onResizeStop and onDragStop exclusively to avoid the needless updates that onLayoutChange invokes, the layout is not what it should be. Elements re-arrange themselves as if colliding, though there are no collisions in the end. And this can be achieved with a pretty basic example.

Secondly, when using onLayoutChange and resizing components, they also rearrange themselves as if they were colliding. I'm not sure whether this is tied to the performance, because it mostly happens when the layout itself is complex, with a lot of components and nested layouts. (Though the nesting is only 1 level deep)

This is all with compactType="vertical", margin={[0, 0]}, and rowHeight={1}, but changing those settings has no impact, or none that I've found.

@STRML any ideas? Would you think this is performance based? I could not reproduce this given any of the simple examples.

[TheImpressionist]

Can confirm that it's an issue with compaction, or to be specific, collision detection:
By modifying the following function compactItem to take an additional paramter called resizeCall which is a boolean, I can do an early return before the collision detection function gets called. By doing that the output doesn't get mangled up.

export function compactItem(
  compareWith: Layout,
  l: LayoutItem,
  compactType: CompactType,
  cols: number,
  fullLayout: Layout,
  resizeCal: boolean
): LayoutItem {
  const compactV = compactType === "vertical";
  const compactH = compactType === "horizontal";
  if (compactV) {
    // Bottom 'y' possible is the bottom of the layout.
    // This allows you to do nice stuff like specify {y: Infinity}
    // This is here because the layout must be sorted in order to get the correct bottom `y`.
    l.y = Math.min(bottom(compareWith), l.y);
    // Move the element up as far as it can go without colliding.
    while (l.y > 0 && !getFirstCollision(compareWith, l)) {
      l.y--;
    }
  } else if (compactH) {
    l.y = Math.min(bottom(compareWith), l.y);
    // Move the element left as far as it can go without colliding.
    while (l.x > 0 && !getFirstCollision(compareWith, l)) {
      l.x--;
    }
  }

 // -------------- THE EARLY RETURN TO AVOID THE ODD COLLISION DETECTION ------
  if (resizeCall) {
    return l;
  }
  // -------------- END OF EARLY RETURN -----------------

  // Move it down, and keep moving it down if it's colliding.
  let collides;
  while ((collides = getFirstCollision(compareWith, l))) {
    if (compactH) {
      resolveCompactionCollision(fullLayout, l, collides.x + collides.w, "x");
    } else {
      resolveCompactionCollision(fullLayout, l, collides.y + collides.h, "y");
    }
    // Since we can't grow without bounds horizontally, if we've overflown, let's move it down and try again.
    if (compactH && l.x + l.w > cols) {
      l.x = cols - l.w;
      l.y++;
    }
  }
  return l;
}

I'll see if I can identify a more specific reason.

[TheImpressionist]

Looked through collision detection, and that's where the error arises from, though I cannot really grasp it why. It thinks that two members which are touching but are not colliding are colliding.

I tried to log the data, but everything is correct there, yet not in the final result.

[TheImpressionist]

Oof, finally. I found out two things, I've been using onResizeStop by mistake in some of the nested layouts, which produces a side effect in this case.

I'd like to propose a one thing though:

Call onResizeStop and onDragStop the same way that onLayoutMaybeChanged is being called.

Example:

  onResizeStop(i: string, w: number, h: number, { e, node }: GridResizeEvent) {
    const { layout, oldResizeItem } = this.state;
    const { cols } = this.props;
    var l = getLayoutItem(layout, i);


    // Set state
    const newLayout = compact(layout, compactType(this.props), cols);
    const { oldLayout } = this.state;
    this.setState({
      activeDrag: null,
      layout: newLayout,
      oldResizeItem: null,
      oldLayout: null
    });

    this.props.onResizeStop(newLayout, oldResizeItem, l, null, e, node);
    this.onLayoutMaybeChanged(newLayout, oldLayout);
  }

Example:

onDragStop

  onDragStop(i: string, x: number, y: number, { e, node }: GridDragEvent) {
    const { oldDragItem } = this.state;
    let { layout } = this.state;
    const { cols, preventCollision } = this.props;
    const l = getLayoutItem(layout, i);
    if (!l) return;

    // Move the element here
    const isUserAction = true;
    layout = moveElement(
      layout,
      l,
      x,
      y,
      isUserAction,
      preventCollision,
      compactType(this.props),
      cols
    );


    // Set state
    const newLayout = compact(layout, compactType(this.props), cols);
    const { oldLayout } = this.state;
    this.setState({
      activeDrag: null,
      layout: newLayout,
      oldDragItem: null,
      oldLayout: null
    });

    this.props.onDragStop(newLayout, oldDragItem, l, null, e, node);
    this.onLayoutMaybeChanged(newLayout, oldLayout);
  }

Unless there is a good reason the current implementation of onResizeStop calling this.props.onResizeStop before the layout has been normalized. I can hardly see any reason to not have it normalized for the onResizeStop call. But that's just me.

I mainly use onResizeStop and onDragStop because it's sure to fire once after an action was completed. However, onLayoutChanged fires too many times, impacting the performance greatly. It fires at the start of the render, on any collision fixes, and sometimes seemingly just at random, that's why I wanna avoid using that method.

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this issue will be closed in 7 days

[marcel-k]

bump (has anybody looked into this yet?)

[eddyw]

@marcel-k personally, I gave up on this project 🙈
Plenty of reported issues (and bugs, including mine) just go unanswered, marked as stale, and just closed 🤷‍♂

If it's of any help, I reported a bug here: https://github.com/STRML/react-grid-layout/issues/1045
And there is code sandbox where has a pretty naive implementation of RGL (rendering). Using that code as base I wrote my own implementation using react-dnd. Code is private but I was hoping to eventually make it open source.