fix #1785 switching column overlap

adumesny · adumesny · commit 2d4d2c84cf19 · 2021-12-12T15:46:46.000-08:00
* guard against 0 column
* making changes in a layout, propagates the differences to larger layout (deleting smaller ones)
causing potential negative (x,y) cached values.
We always used nodeBoundFix() to prevent that when adding the nodes back, but due to saving _orig value after (instead of early) and combination of _dirty, we didn't always updating collision in the DOM
* also changed updateNodeWidths() to always scale things down live (regardless of cached layout to make it smoother) but use the largest 12 column layout to scale things up if not already cached.
* more efficient in updating _dirty only nodes attributes from last values.
* added test file showing issue.
diff --git a/demo/column.html b/demo/column.html
@@ -72,8 +72,10 @@ <h1>column() grid demo (fix cellHeight)</h1>
       {x: 0, y: 4, w: 12}
     ];
     let count = 0;
-    items.forEach(n => n.content = '<button onClick="grid.removeWidget(this.parentNode.parentNode)">X</button><br>' + count++ + (n.text ? n.text : ''));
-    grid.load(items);
+    items.forEach(n => {
+      n.content = '<button onClick="grid.removeWidget(this.parentNode.parentNode)">X</button><br>' + count++ + (n.text ? n.text : '');
+      grid.addWidget(n); // DOM order will be 0,1,2,3,4,5,6 vs column1 = 0,1,4,3,2,5,6
+    });
 
     function addWidget() {
       let n = items[count] || {
@@ -91,6 +93,9 @@ <h1>column() grid demo (fix cellHeight)</h1>
       text.innerHTML = n;
     }
     function setOneColumn(dom) {
+      if (grid.opts.column === 1 && grid.opts.oneColumnModeDomSort !== dom) {
+        column(12); // go ack to 12 before going to new column1 version
+      }
       grid.opts.oneColumnModeDomSort = dom;
       grid.column(1, layout);
       text.innerHTML = dom ? '1 DOM' : '1';
@@ -106,7 +111,7 @@ <h1>column() grid demo (fix cellHeight)</h1>
     function setLayout(name) {
       layout = name === 'custom' ? this.columnLayout : name;
     }
-
+    // setOneColumn(true); // test dom order
   </script>
 </body>
 </html>
diff --git a/doc/CHANGES.md b/doc/CHANGES.md
@@ -5,6 +5,7 @@ Change log
 <!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE -->
 **Table of Contents**  *generated with [DocToc](http://doctoc.herokuapp.com/)*
 
+- [4.3.1-dev TBD](#431-dev-tbd)
 - [4.3.1 (2021-10-18)](#431-2021-10-18)
 - [4.3.0 (2021-10-15)](#430-2021-10-15)
 - [4.2.7 (2021-9-12)](#427-2021-9-12)
@@ -64,6 +65,7 @@ Change log
 <!-- END doctoc generated TOC please keep comment here to allow auto update -->
 
 ## 4.3.1-dev TBD
+* fix [#1785](https://github.com/gridstack/gridstack.js/issue/1785) overlapping items when switching column() and making edits. Thank you [@radovanobal](https://github.com/radovanobal) for sponsoring it.
 * add [#1887](https://github.com/gridstack/gridstack.js/pull/1887) support for IE (new es5 folder) by [@SmileLifeIven](https://github.com/SmileLifeIven)
 
 ## 4.3.1 (2021-10-18)
diff --git a/spec/e2e/html/1785_column_many_switch.html b/spec/e2e/html/1785_column_many_switch.html
@@ -0,0 +1,76 @@
+<!DOCTYPE html>
+<html lang="en">
+<head>
+  <meta charset="utf-8">
+  <meta http-equiv="X-UA-Compatible" content="IE=edge">
+  <meta name="viewport" content="width=device-width, initial-scale=1">
+  <title>Changing Column a lot</title>
+
+  <link rel="stylesheet" href="../../../demo/demo.css"/>
+  <link rel="stylesheet" href="../../../dist/gridstack-extra.min.css"/>
+  <script src="../../../dist/gridstack-h5.js"></script>
+
+</head>
+<body>
+  <div class="container-fluid">
+    <h1>Changing Column 5 x 12 causing overlap https://github.com/gridstack/gridstack.js/issues/1785</h1>
+    <div>
+      <a class="btn btn-primary" onClick="next()" href="#">Next</a>
+      <a class="btn btn-primary" onclick="auto()" href="#">Auto</a>
+    </div>
+
+    <div class="grid-stack"></div>
+  </div>
+  <script src="events.js"></script>
+  <script type="text/javascript">
+    let count = 0;
+    let grid;
+    let items = [ 
+      // simplest case data would show the issue at 7 column (overlapping 0 & 2)
+      // {x: 0, y: 0},
+      // {x: 2, y: 0, w: 3},
+      // {x: 0, y: 1, w: 2},
+      // {x: 3, y: 1, h: 2},
+      // {x: 6, y: 6, w: 3, h: 2}
+    ];
+    items.forEach(item => item.content = String(count++));
+
+    function addNewWidget() {
+      var node = items[count] || {
+        x: Math.round(12 * Math.random()),
+        y: Math.round(5 * Math.random()),
+        w: Math.round(1 + 3 * Math.random()),
+        h: Math.round(1 + 3 * Math.random())
+      };
+      node.content = String(count++);
+      if (grid) {
+        grid.addWidget(node);
+      } else {
+        items.push(node);
+      }
+    };
+
+    grid = GridStack.init({cellHeight: 50, margin: 5}).load(items);
+    let column = 1;
+    for (i=0; i<7; i++) addNewWidget(); // load 7 random items
+
+    function next() {
+      setColumn(column++);
+      if (column > 12) { column = 1; }
+    }
+    function auto() {
+      for (let i = 1; i <= 12; i++) {
+        setColumn(i);
+      }
+    }
+    function setColumn(n) {
+      console.log("gridding at", n);
+      grid.column(n);
+      grid.compact();
+    }
+    for (let i = 1; i <= 5; i++) {
+      auto();
+    }
+  </script>
+</body>
+</html>
diff --git a/src/gridstack-engine.ts b/src/gridstack-engine.ts
@@ -34,10 +34,10 @@ export class GridStackEngine {
   private _float: boolean;
   /** @internal */
   private _prevFloat: boolean;
-  /** @internal */
+  /** @internal cached layouts of difference column count so we can restore ack (eg 12 -> 1 -> 12) */
   private _layouts?: Layout[][]; // maps column # to array of values nodes
-  /** @internal */
-  private _ignoreLayoutsNodeChange: boolean;
+  /** @internal true while we are resizing widgets during column resize to skip certain parts */
+  private _inColumnResize: boolean;
   /** @internal true if we have some items locked */
   private _hasLocked: boolean;
   /** @internal unique global internal _id counter NOT starting at 0 */
@@ -273,6 +273,7 @@ export class GridStackEngine {
 
   /** @internal called to top gravity pack the items back OR revert back to original Y positions when floating */
   private _packNodes(): GridStackEngine {
+    if (this.batchMode) { return; }
     this._sortNodes(); // first to last
 
     if (this.float) {
@@ -346,6 +347,8 @@ export class GridStackEngine {
   /** part2 of preparing a node to fit inside our grid - checks  for x,y from grid dimensions */
   public nodeBoundFix(node: GridStackNode, resizing?: boolean): GridStackNode {
 
+    let before = node._orig || Utils.copyPos({}, node);
+
     if (node.maxW) { node.w = Math.min(node.w, node.maxW); }
     if (node.maxH) { node.h = Math.min(node.h, node.maxH); }
     if (node.minW && node.minW <= this.column) { node.w = Math.max(node.w, node.minW); }
@@ -354,7 +357,8 @@ export class GridStackEngine {
     if (node.w > this.column) {
       // if user loaded a larger than allowed widget for current # of columns,
       // remember it's full width so we can restore back (1 -> 12 column) #1655
-      if (this.column < 12) {
+      // IFF we're not in the middle of column resizing!
+      if (this.column < 12 && !this._inColumnResize) {
         node.w = Math.min(12, node.w);
         this.cacheOneLayout(node, 12);
       }
@@ -391,6 +395,10 @@ export class GridStackEngine {
       }
     }
 
+    if (!Utils.samePos(node, before)) {
+      node._dirty = true;
+    }
+
     return node;
   }
 
@@ -446,10 +454,11 @@ export class GridStackEngine {
 
   /** call to add the given node to our list, fixing collision and re-packing */
   public addNode(node: GridStackNode, triggerAddEvent = false): GridStackNode {
-    let dup: GridStackNode;
-    if (dup = this.nodes.find(n => n._id === node._id)) return dup; // prevent inserting twice! return it instead.
+    let dup = this.nodes.find(n => n._id === node._id);
+    if (dup) return dup; // prevent inserting twice! return it instead.
 
-    node = this.prepareNode(node);
+    // skip prepareNode if we're in middle of column resize (not new) but do check for bounds!
+    node = this._inColumnResize ? this.nodeBoundFix(node) : this.prepareNode(node);
     delete node._temporaryRemoved;
     delete node._removeDOM;
 
@@ -473,11 +482,10 @@ export class GridStackEngine {
     }
 
     this.nodes.push(node);
-    triggerAddEvent && this.addedNodes.push(node);
+    if (triggerAddEvent) { this.addedNodes.push(node); }
 
     this._fixCollisions(node);
-    this._packNodes()
-      ._notify();
+    if (!this.batchMode) { this._packNodes()._notify(); }
     return node;
   }
 
@@ -696,7 +704,7 @@ export class GridStackEngine {
 
   /** @internal called whenever a node is added or moved - updates the cached layouts */
   public layoutsNodesChange(nodes: GridStackNode[]): GridStackEngine {
-    if (!this._layouts || this._ignoreLayoutsNodeChange) return this;
+    if (!this._layouts || this._inColumnResize) return this;
     // remove smaller layouts - we will re-generate those on the fly... larger ones need to update
     this._layouts.forEach((layout, column) => {
       if (!layout || column === this.column) return this;
@@ -705,12 +713,12 @@ export class GridStackEngine {
       }
       else {
         // we save the original x,y,w (h isn't cached) to see what actually changed to propagate better.
-        // Note: we don't need to check against out of bound scaling/moving as that will be done when using those cache values.
+        // NOTE: we don't need to check against out of bound scaling/moving as that will be done when using those cache values. #1785
+        let ratio = column / this.column;
         nodes.forEach(node => {
           if (!node._orig) return; // didn't change (newly added ?)
           let n = layout.find(l => l._id === node._id);
           if (!n) return; // no cache for new nodes. Will use those values.
-          let ratio = column / this.column;
           // Y changed, push down same amount
           // TODO: detect doing item 'swaps' will help instead of move (especially in 1 column mode)
           if (node.y !== node._orig.y) {
@@ -736,55 +744,60 @@ export class GridStackEngine {
    * Note we store previous layouts (especially original ones) to make it possible to go
    * from say 12 -> 1 -> 12 and get back to where we were.
    *
-   * @param oldColumn previous number of columns
+   * @param prevColumn previous number of columns
    * @param column  new column number
    * @param nodes different sorted list (ex: DOM order) instead of current list
    * @param layout specify the type of re-layout that will happen (position, size, etc...).
    * Note: items will never be outside of the current column boundaries. default (moveScale). Ignored for 1 column
    */
-  public updateNodeWidths(oldColumn: number, column: number, nodes: GridStackNode[], layout: ColumnOptions = 'moveScale'): GridStackEngine {
-    if (!this.nodes.length || oldColumn === column) return this;
+  public updateNodeWidths(prevColumn: number, column: number, nodes: GridStackNode[], layout: ColumnOptions = 'moveScale'): GridStackEngine {
+    if (!this.nodes.length || !column || prevColumn === column) return this;
 
     // cache the current layout in case they want to go back (like 12 -> 1 -> 12) as it requires original data
-    this.cacheLayout(this.nodes, oldColumn);
+    this.cacheLayout(this.nodes, prevColumn);
+    this.batchUpdate(); // do this EARLY as it will call saveInitial() so we can detect where we started for _dirty and collision
+    let newNodes: GridStackNode[] = [];
 
     // if we're going to 1 column and using DOM order rather than default sorting, then generate that layout
-    if (column === 1 && nodes && nodes.length) {
+    let domOrder = false;
+    if (column === 1 && nodes?.length) {
+      domOrder = true;
       let top = 0;
       nodes.forEach(n => {
         n.x = 0;
         n.w = 1;
         n.y = Math.max(n.y, top);
         top = n.y + n.h;
       });
+      newNodes = nodes;
+      nodes = [];
     } else {
-      nodes = Utils.sort(this.nodes, -1, oldColumn); // current column reverse sorting so we can insert last to front (limit collision)
-    }
-
-    // see if we have cached previous layout.
-    let cacheNodes = this._layouts[column] || [];
-    // if not AND we are going up in size start with the largest layout as down-scaling is more accurate
-    let lastIndex = this._layouts.length - 1;
-    if (cacheNodes.length === 0 && column > oldColumn && column < lastIndex) {
-      cacheNodes = this._layouts[lastIndex] || [];
-      if (cacheNodes.length) {
-        // pretend we came from that larger column by assigning those values as starting point
-        oldColumn = lastIndex;
-        cacheNodes.forEach(cacheNode => {
-          let j = nodes.findIndex(n => n._id === cacheNode._id);
-          if (j !== -1) {
+      nodes = Utils.sort(this.nodes, -1, prevColumn); // current column reverse sorting so we can insert last to front (limit collision)
+    }
+
+    // see if we have cached previous layout IFF we are going up in size (restore) otherwise always
+    // generate next size down from where we are (looks more natural as you gradually size down).
+    let cacheNodes: Layout[] = [];
+    if (column > prevColumn) {
+      cacheNodes = this._layouts[column] || [];
+      // ...if not, start with the largest layout (if not already there) as down-scaling is more accurate
+      // by pretending we came from that larger column by assigning those values as starting point
+      let lastIndex = this._layouts.length - 1;
+      if (!cacheNodes.length && prevColumn !== lastIndex && this._layouts[lastIndex]?.length) {
+        prevColumn = lastIndex;
+        this._layouts[lastIndex].forEach(cacheNode => {
+          let n = nodes.find(n => n._id === cacheNode._id);
+          if (n) {
             // still current, use cache info positions
-            nodes[j].x = cacheNode.x;
-            nodes[j].y = cacheNode.y;
-            nodes[j].w = cacheNode.w;
+            n.x = cacheNode.x;
+            n.y = cacheNode.y;
+            n.w = cacheNode.w;
           }
         });
-        cacheNodes = []; // we still don't have new column cached data... will generate from larger one.
       }
     }
 
     // if we found cache re-use those nodes that are still current
-    let newNodes: GridStackNode[] = [];
     cacheNodes.forEach(cacheNode => {
       let j = nodes.findIndex(n => n._id === cacheNode._id);
       if (j !== -1) {
@@ -799,14 +812,15 @@ export class GridStackEngine {
     // ...and add any extra non-cached ones
     if (nodes.length) {
       if (typeof layout === 'function') {
-        layout(column, oldColumn, newNodes, nodes);
-      } else {
-        let ratio = column / oldColumn;
+        layout(column, prevColumn, newNodes, nodes);
+      } else if (!domOrder) {
+        let ratio = column / prevColumn;
         let move = (layout === 'move' || layout === 'moveScale');
         let scale = (layout === 'scale' || layout === 'moveScale');
         nodes.forEach(node => {
+          // NOTE: x + w could be outside of the grid, but addNode() below will handle that
           node.x = (column === 1 ? 0 : (move ? Math.round(node.x * ratio) : Math.min(node.x, column - 1)));
-          node.w = ((column === 1 || oldColumn === 1) ? 1 :
+          node.w = ((column === 1 || prevColumn === 1) ? 1 :
             scale ? (Math.round(node.w * ratio) || 1) : (Math.min(node.w, column)));
           newNodes.push(node);
         });
@@ -816,15 +830,14 @@ export class GridStackEngine {
 
     // finally re-layout them in reverse order (to get correct placement)
     newNodes = Utils.sort(newNodes, -1, column);
-    this._ignoreLayoutsNodeChange = true;
-    this.batchUpdate();
-    this.nodes = []; // pretend we have no nodes to start with (we use same structures) to simplify layout
+    this._inColumnResize = true; // prevent cache update
+    this.nodes = []; // pretend we have no nodes to start with (add() will use same structures) to simplify layout
     newNodes.forEach(node => {
       this.addNode(node, false); // 'false' for add event trigger
-      node._dirty = true; // force attr update
-    }, this);
+      delete node._orig; // make sure the commit doesn't try to restore things back to original
+    });
     this.commit();
-    delete this._ignoreLayoutsNodeChange;
+    delete this._inColumnResize;
     return this;
   }
 
diff --git a/src/gridstack.ts b/src/gridstack.ts
@@ -668,7 +668,7 @@ export class GridStack {
    * Note: items will never be outside of the current column boundaries. default (moveScale). Ignored for 1 column
    */
   public column(column: number, layout: ColumnOptions = 'moveScale'): GridStack {
-    if (this.opts.column === column) return this;
+    if (column < 1 || this.opts.column === column) return this;
     let oldColumn = this.opts.column;
 
     // if we go into 1 column mode (which happens if we're sized less than minW unless disableOneColumnMode is on)
diff --git a/src/types.ts b/src/types.ts
@@ -201,7 +201,7 @@ export interface GridStackMoveOpts extends GridStackPosition {
   pack?: boolean;
   /** true if we are calling this recursively to prevent simple swap or coverage collision - default false*/
   nested?: boolean;
-  /* vars to calculate other cells coordinates */
+  /** vars to calculate other cells coordinates */
   cellWidth?: number;
   cellHeight?: number;
   marginTop?: number;
diff --git a/src/utils.ts b/src/utils.ts
@@ -219,7 +219,7 @@ export class Utils {
     return true;
   }
 
-  /* copies over b size & position (GridStackPosition), and possibly min/max as well */
+  /** copies over b size & position (GridStackPosition), and possibly min/max as well */
   static copyPos(a: GridStackWidget, b: GridStackWidget, minMax = false): GridStackWidget {
     a.x = b.x;
     a.y = b.y;
@@ -233,7 +233,7 @@ export class Utils {
     return a;
   }
 
-  /* true if a and b has same size & position */
+  /** true if a and b has same size & position */
   static samePos(a: GridStackPosition, b: GridStackPosition): boolean {
     return a && b && a.x === b.x && a.y === b.y && a.w === b.w && a.h === b.h;
   }

Original file line number	Diff line number	Diff line change
`@@ -219,7 +219,7 @@ export class Utils {`
`219`	`219`	`return true;`
`220`	`220`	`}`
`221`	`221`
`222`		`- /* copies over b size & position (GridStackPosition), and possibly min/max as well */`
	`222`	`+ /** copies over b size & position (GridStackPosition), and possibly min/max as well */`
`223`	`223`	`static copyPos(a: GridStackWidget, b: GridStackWidget, minMax = false): GridStackWidget {`
`224`	`224`	`a.x = b.x;`
`225`	`225`	`a.y = b.y;`
`@@ -233,7 +233,7 @@ export class Utils {`
`233`	`233`	`return a;`
`234`	`234`	`}`
`235`	`235`
`236`		`- /* true if a and b has same size & position */`
	`236`	`+ /** true if a and b has same size & position */`
`237`	`237`	`static samePos(a: GridStackPosition, b: GridStackPosition): boolean {`
`238`	`238`	`return a && b && a.x === b.x && a.y === b.y && a.w === b.w && a.h === b.h;`
`239`	`239`	`}`