Merge pull request #1894 from adumesny/master

adumesny · web-flow · commit 2e18461228e7 · 2021-12-21T08:07:09.000-08:00
fix #1785 switching column overlap
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`	`}`