fix(compact): Fix rearrangement of grid during collision cascade

Fixes react-grid-layout#739 Re-applies a fix erroneously removed from react-grid-layout#650 in react-grid-layout@1550308 Thanks to @torkelo
mfoonirlee · Mar 6, 2018 · eaaed5a · eaaed5a
1 parent fec2a95
commit eaaed5a
Show file tree

Hide file tree

Showing 2 changed files with 92 additions and 94 deletions.
diff --git a/lib/utils.js b/lib/utils.js
@@ -190,10 +190,13 @@ function resolveCompactionCollision(
   // Go through each item we collide with.
   for (let i = itemIndex + 1; i < layout.length; i++) {
     const otherItem = layout[i];
-
     // Ignore static items
     if (otherItem.static) continue;
 
+    // Optimization: we can break early if we know we're past this el
+    // We can do this b/c it's a sorted layout
+    if (otherItem.y > (item.y + item.h)) break;
+
     if (collides(item, otherItem)) {
       resolveCompactionCollision(
         layout,
@@ -342,25 +345,25 @@ export function getStatics(layout: Layout): Array<LayoutItem> {
 export function moveElement(
   layout: Layout,
   l: LayoutItem,
-  x: number,
-  y: number,
+  x: ?number,
+  y: ?number,
   isUserAction: ?boolean,
   preventCollision: ?boolean,
   compactType: CompactType,
-  cols: number
+  cols: number,
 ): Layout {
   if (l.static) return layout;
-  log(`Moving element ${l.i} to [${x},${y}] from [${l.x},${l.y}]`);
 
   // Short-circuit if nothing to do.
   if (l.y === y && l.x === x) return layout;
 
+  log(`Moving element ${l.i} to [${String(x)},${String(y)}] from [${l.x},${l.y}]`);
   const oldX = l.x;
   const oldY = l.y;
 
   // This is quite a bit faster than extending the object
-  l.x = x;
-  l.y = y;
+  if (typeof x === 'number') l.x = x;
+  if (typeof y === 'number') l.y = y;
   l.moved = true;
 
   // If this collides with anything, move it.
@@ -369,9 +372,9 @@ export function moveElement(
   // nearest collision.
   let sorted = sortLayoutItems(layout, compactType);
   const movingUp =
-    compactType === "vertical"
-      ? oldY >= y
-      : compactType === "horizontal" ? oldX >= x : false;
+    compactType === "vertical" && typeof y === 'number' ? oldY >= y
+    : compactType === "horizontal" && typeof x === 'number' ? oldX >= x
+    : false;
   if (movingUp) sorted = sorted.reverse();
   const collisions = getAllCollisions(sorted, l);
 
@@ -451,7 +454,7 @@ export function moveElementAwayFromCollision(
     // Make a mock item so we don't modify the item here, only modify in moveElement.
     const fakeItem: LayoutItem = {
       x: compactH ? Math.max(collidesWith.x - itemToMove.w, 0) : itemToMove.x,
-      y: !compactH ? Math.max(collidesWith.y - itemToMove.h, 0) : itemToMove.y,
+      y: compactV ? Math.max(collidesWith.y - itemToMove.h, 0) : itemToMove.y,
       w: itemToMove.w,
       h: itemToMove.h,
       i: "-1"
@@ -467,8 +470,8 @@ export function moveElementAwayFromCollision(
       return moveElement(
         layout,
         itemToMove,
-        fakeItem.x,
-        fakeItem.y,
+        compactH ? fakeItem.x : undefined,
+        compactV ? fakeItem.y : undefined,
         isUserAction,
         preventCollision,
         compactType,
@@ -480,8 +483,8 @@ export function moveElementAwayFromCollision(
   return moveElement(
     layout,
     itemToMove,
-    compactH ? collidesWith.x + collidesWith.w : itemToMove.x,
-    compactV ? collidesWith.y + collidesWith.h : itemToMove.y,
+    compactH ? itemToMove.x + 1 : undefined,
+    compactV ? itemToMove.y + 1 : undefined,
     isUserAction,
     preventCollision,
     compactType,

diff --git a/test/spec/utils-test.js b/test/spec/utils-test.js
@@ -33,6 +33,18 @@ function stripObject(obj) {
 function assertDeepEqualStrip(obj1, obj2) {
   assert.deepEqual(stripArray(obj1), stripArray(obj2));
 }
+
+function assertSubset(arr1, arr2) {
+  // strip a before comparing to b
+  const arr1Stripped = arr1.map((el, i) => {
+    const other = arr2[i];
+    return Object.keys(el).reduce((memo, key) => {
+      if (key in other) memo[key] = el[key];
+      return memo;
+    }, {})
+  });
+  assert.deepEqual(arr1Stripped, arr2);
+}
 //
 // Specs
 //
@@ -106,6 +118,13 @@ describe("validateLayout", () => {
 });
 
 describe("moveElement", () => {
+  function compactAndMove(layout, layoutItem, x, y, isUserAction, preventCollision, compactType, cols) {
+    return compact(
+      moveElement(layout, layoutItem, x, y, isUserAction, preventCollision, compactType, cols),
+      compactType, cols
+    );
+  }
+
   it("Does not change layout when colliding on no rearrangement mode", () => {
     const layout = [
       { i: "1", x: 0, y: 1, w: 1, h: 1, moved: false },
@@ -116,12 +135,9 @@ describe("moveElement", () => {
       moveElement(
         layout,
         layoutItem,
-        1,
-        2, // x, y
-        true,
-        true, // isUserAction, preventCollision
-        null,
-        2
+        1, 2, // x, y
+        true, true, // isUserAction, preventCollision
+        null, 2, // compactType, cols
       ),
       [
         { i: "1", x: 0, y: 1, w: 1, h: 1, moved: false },
@@ -140,12 +156,9 @@ describe("moveElement", () => {
       moveElement(
         layout,
         layoutItem,
-        1,
-        0, // x, y
-        true,
-        false, // isUserAction, preventCollision
-        "vertical",
-        2 // compactType, cols
+        1, 0, // x, y
+        true, false, // isUserAction, preventCollision
+        "vertical", 2, // compactType, cols
       ),
       [
         { i: "1", x: 1, y: 0, w: 1, h: 1, moved: true },
@@ -156,56 +169,50 @@ describe("moveElement", () => {
 
   it("Moves elements out of the way without causing panel jumps when compaction is vertical", () => {
     const layout = [
-      { x: 0, y: 0, w: 1, h: 10, moved: false, i: "A" },
-      { x: 0, y: 10, w: 1, h: 1, moved: false, i: "B" },
-      { x: 0, y: 11, w: 1, h: 1, moved: false, i: "C" }
+      { x: 0, y: 0, w: 1, h: 10, i: "A" },
+      { x: 0, y: 10, w: 1, h: 1, i: "B" },
+      { x: 0, y: 11, w: 1, h: 1, i: "C" }
     ];
     // move A down slightly so it collides with C; can cause C to jump above B.
     // We instead want B to jump above A (it has the room)
     const itemA = layout[0];
-    assert.deepEqual(
-      moveElement(
+    assertSubset(
+      compactAndMove(
         layout,
         itemA,
-        0,
-        1, // x, y
-        true,
-        false, // isUserAction, preventCollision
-        "vertical",
-        10 // compactType, cols
+        0, 1, // x, y
+        true, false, // isUserAction, preventCollision
+        "vertical", 10 // compactType, cols
       ),
       [
-        { x: 0, y: 1, w: 1, h: 10, moved: true, i: "A" },
-        { x: 0, y: 0, w: 1, h: 1, moved: true, i: "B" },
-        { x: 0, y: 11, w: 1, h: 1, moved: false, i: "C" }
+        { x: 0, y: 1, w: 1, h: 10, i: "A" },
+        { x: 0, y: 0, w: 1, h: 1, i: "B" },
+        { x: 0, y: 11, w: 1, h: 1,  i: "C" }
       ]
     );
   });
 
   it("Calculates the correct collision when moving large object far", () => {
     const layout = [
-      { x: 0, y: 0, w: 1, h: 10, moved: false, i: "A" },
-      { x: 0, y: 10, w: 1, h: 1, moved: false, i: "B" },
-      { x: 0, y: 11, w: 1, h: 1, moved: false, i: "C" }
+      { x: 0, y: 0, w: 1, h: 10, i: "A" },
+      { x: 0, y: 10, w: 1, h: 1, i: "B" },
+      { x: 0, y: 11, w: 1, h: 1, i: "C" }
     ];
     // Move A down by 2. This should move B above, but since we don't compact in between,
     // C should move below.
     const itemA = layout[0];
-    assert.deepEqual(
+    assertSubset(
       moveElement(
         layout,
         itemA,
-        0,
-        2, // x, y
-        true,
-        false, // isUserAction, preventCollision
-        "vertical",
-        10 // compactType, cols
+        0, 2, // x, y
+        true, false, // isUserAction, preventCollision
+        "vertical", 10, // compactType, cols
       ),
       [
-        { x: 0, y: 2, w: 1, h: 10, moved: true, i: "A" },
-        { x: 0, y: 1, w: 1, h: 1, moved: true, i: "B" },
-        { x: 0, y: 12, w: 1, h: 1, moved: true, i: "C" }
+        { x: 0, y: 2, w: 1, h: 10, i: "A" },
+        { x: 0, y: 1, w: 1, h: 1, i: "B" },
+        { x: 0, y: 12, w: 1, h: 1, i: "C" }
       ]
     );
   });
@@ -223,12 +230,9 @@ describe("moveElement", () => {
       moveElement(
         layout,
         itemA,
-        1,
-        0, // x, y
-        true,
-        false, // isUserAction, preventCollision
-        "vertical",
-        2 // compactType, cols
+        1, 0, // x, y
+        true, false, // isUserAction, preventCollision
+        "vertical", 2, // compactType, cols
       ),
       [
         { x: 1, y: 0, w: 1, h: 1, i: "A", moved: true },
@@ -240,9 +244,9 @@ describe("moveElement", () => {
 
   it("Moves elements out of the way without causing panel jumps when compaction is horizontal", () => {
     const layout = [
-      { y: 0, x: 0, h: 1, w: 10, moved: false, i: "A" },
-      { y: 0, x: 11, h: 1, w: 1, moved: false, i: "B" },
-      { y: 0, x: 12, h: 1, w: 1, moved: false, i: "C" }
+      { y: 0, x: 0, h: 1, w: 10, i: "A" },
+      { y: 0, x: 11, h: 1, w: 1, i: "B" },
+      { y: 0, x: 12, h: 1, w: 1, i: "C" }
     ];
     // move A over slightly so it collides with C; can cause C to jump left of B
     // this test will check that that does not happen
@@ -251,17 +255,14 @@ describe("moveElement", () => {
       moveElement(
         layout,
         itemA,
-        2,
-        0, // x, y
-        true,
-        false, // isUserAction, preventCollision
-        "horizontal",
-        10 // compactType, cols
+        2, 0, // x, y
+        true, false, // isUserAction, preventCollision
+        "horizontal", 10, // compactType, cols
       ),
       [
         { y: 0, x: 2, h: 1, w: 10, moved: true, i: "A" },
         { y: 0, x: 1, h: 1, w: 1, moved: true, i: "B" },
-        { y: 0, x: 12, h: 1, w: 1, moved: false, i: "C" }
+        { y: 0, x: 12, h: 1, w: 1, i: "C" }
       ]
     );
   });
@@ -278,27 +279,24 @@ describe("moveElement", () => {
     // move B left slightly so it collides with A; can cause C to jump above A
     // this test will check that that does not happen
     const itemB = layout[1];
-    assert.deepEqual(
-      moveElement(
+    assertSubset(
+      compactAndMove(
         layout,
         itemB,
-        1,
-        0, // x, y
-        true,
-        false, // isUserAction, preventCollision
-        "vertical",
-        4 // compactType, cols
+        1, 0, // x, y
+        true, false, // isUserAction, preventCollision
+        "vertical", 4, // compactType, cols
       ),
       [
-        { x: 0, y: 1, w: 2, h: 1, i: "A", moved: true },
-        { x: 1, y: 0, w: 2, h: 1, i: "B", moved: true },
-        { x: 0, y: 2, w: 1, h: 1, i: "C", moved: true },
-        { x: 1, y: 2, w: 3, h: 1, i: "D", moved: true }
+        { x: 0, y: 1, w: 2, h: 1, i: "A" },
+        { x: 1, y: 0, w: 2, h: 1, i: "B" },
+        { x: 0, y: 2, w: 1, h: 1, i: "C" },
+        { x: 1, y: 2, w: 3, h: 1, i: "D" }
       ]
     );
   });
 
-  it.only("Moves one element to another should cause moving down panels, vert compact, example 2", () => {
+  it("Moves one element to another should cause moving down panels, vert compact, example 2", () => {
     // | A |
     // |B|C|
     //   | |
@@ -311,21 +309,18 @@ describe("moveElement", () => {
     ];
     // Move C up.
     const itemB = layout[2];
-    assert.deepEqual(
-      moveElement(
+    assertSubset(
+      compactAndMove(
         layout,
         itemB,
-        1,
-        0, // x, y
-        true,
-        false, // isUserAction, preventCollision
-        "vertical",
-        4 // compactType, cols
+        1, 0, // x, y
+        true, false, // isUserAction, preventCollision
+        "vertical", 4, // compactType, cols
       ),
       [
-        { x: 0, y: 2, w: 2, h: 1, i: "A", moved: true },
-        { x: 0, y: 3, w: 1, h: 1, i: "B", moved: true },
-        { x: 1, y: 0, w: 1, h: 2, i: "C", moved: true }
+        { x: 0, y: 2, w: 2, h: 1, i: "A" },
+        { x: 0, y: 3, w: 1, h: 1, i: "B" },
+        { x: 1, y: 0, w: 1, h: 2, i: "C" }
       ]
     );
   });