Incorrect layout on grid load in one column mode

adumesny · adumesny · commit 87e1b2839ffc · 2023-11-11T09:04:45.000-08:00
* fix #2527 * turned out to be much more complicated fix than expected. cleanup some of the code while debugging through it... * make sure we nodeBoundFix() before widthChanged is checked * cacheLayout() now checks fo existing node id so loading again doesn't mess things up
diff --git a/demo/serialization.html b/demo/serialization.html
@@ -12,47 +12,43 @@
 <body>
   <div class="container-fluid">
     <h1>Serialization demo</h1>
-    <a onClick="saveGrid()" class="btn btn-primary" href="#">Save</a>
-    <a onClick="loadGrid()" class="btn btn-primary" href="#">Load</a>
-    <a onClick="saveFullGrid()" class="btn btn-primary" href="#">Save Full</a>
-    <a onClick="loadFullGrid()" class="btn btn-primary" href="#">Load Full</a>
-    <a onClick="clearGrid()" class="btn btn-primary" href="#">Clear</a>
+    <a onclick="saveGrid()" class="btn btn-primary" href="#">Save</a>
+    <a onclick="loadGrid()" class="btn btn-primary" href="#">Load</a>
+    <a onclick="saveFullGrid()" class="btn btn-primary" href="#">Save Full</a>
+    <a onclick="loadFullGrid()" class="btn btn-primary" href="#">Load Full</a>
+    <a onclick="clearGrid()" class="btn btn-primary" href="#">Clear</a>
     <br/><br/>
-    <div contentEditable="true">Editable Div</div>
     <div id="gridCont"><div class="grid-stack"></div></div>
     <hr/>
-    <textarea id="saved-data" cols="100" rows="20" readonly="readonly"></textarea>
+    <textarea id="saved-data" style="width: 100%" cols="100" rows="20" readonly="readonly"></textarea>
   </div>
-
+  <script src="events.js"></script>
   <script type="text/javascript">
-    let grid = GridStack.init({
-      minRow: 1, // don't let it collapse when empty
-      cellHeight: '7rem',
-      draggable: { cancel: '.no-drag'} // example of additional custom elements to skip drag on
-    });
-    
-    grid.on('added removed change', function(e, items) {
-      if (!items) return;
-      let str = '';
-      items.forEach(function(item) { str += ' (x,y)=' + item.x + ',' + item.y; });
-      console.log(e.type + ' ' + items.length + ' items:' + str );
-    });
-
     let serializedData = [
-      {x: 0, y: 0, w: 2, h: 2, id: '0'},
-      {x: 3, y: 1, h: 3, id: '1', 
-      content: "<button onclick=\"alert('clicked!')\">Press me</button><div>text area</div><div><textarea></textarea></div><div>Input Field</div><input type='text'><div contentEditable=\"true\">Editable Div</div><div class=\"no-drag\">no drag</div>"},
-      {x: 4, y: 1, id: '2'},
-      {x: 2, y: 3, w: 3, id: '3'},
-      {x: 1, y: 3, id: '4'}
+      {x: 0, y: 0, w: 2, h: 2},
+      {x: 2, y: 1, w: 2, h: 3, 
+      content: `<button onclick=\"alert('clicked!')\">Press me</button><div>text area</div><div><textarea></textarea></div><div>Input Field</div><input type="text"><div contenteditable=\"true\">Editable Div</div><div class=\"no-drag\">no drag</div>`},
+      {x: 4, y: 1},
+      {x: 1, y: 3},
+      {x: 2, y: 3, w:3},
     ];
-    serializedData.forEach((n, i) =>
-      n.content = `<button onClick="removeWidget(this.parentElement.parentElement)">X</button><br> ${i}<br> ${n.content ? n.content : ''}`);
+    serializedData.forEach((n, i) => {
+      n.id = String(i);
+      n.content = `<button onclick="removeWidget(this.parentElement.parentElement)">X</button><br> ${i}<br> ${n.content ? n.content : ''}`;
+    });
     let serializedFull;
 
+    let grid = GridStack.init({
+      minRow: 1, // don't let it collapse when empty
+      cellHeight: 80,
+      float: true,
+      draggable: { cancel: '.no-drag'} // example of additional custom elements to skip drag on
+    }).load(serializedData);
+    addEvents(grid);
+
     // 2.x method - just saving list of widgets with content (default)
     function loadGrid() {
-      grid.load(serializedData, true); // update things
+      grid.load(serializedData);
     }
 
     // 2.x method
@@ -86,7 +82,7 @@ <h1>Serialization demo</h1>
       grid.removeWidget(el, false);
     }
 
-    loadGrid();
+    // setTimeout(() => loadGrid(), 1000); // TEST force a second load which should be no-op
   </script>
 </body>
 </html>
diff --git a/doc/CHANGES.md b/doc/CHANGES.md
@@ -107,6 +107,7 @@ Change log
 ## 9.5.0-dev (TBD)
 * fix [#2525](https://github.com/gridstack/gridstack.js/commit/2525) Fixed unhandled exception happening in _mouseMove handler
 * fix potential crash in doContentResize() if grid gets deleted by the time the delay happens
+* fix [#2527](https://github.com/gridstack/gridstack.js/issues/2527) Incorrect layout on grid load in one column mode
 
 ## 9.5.0 (2023-10-26)
 * feat [#1275](https://github.com/gridstack/gridstack.js/issues/1275) div scale support - Thank you [elmehdiamlou](https://github.com/elmehdiamlou) for implementing this teh right way (add scale to current code)
diff --git a/src/gridstack-engine.ts b/src/gridstack-engine.ts
@@ -346,7 +346,6 @@ export class GridStackEngine {
    * @param resizing if out of bound, resize down or move into the grid to fit ?
    */
   public prepareNode(node: GridStackNode, resizing?: boolean): GridStackNode {
-    node = node || {};
     node._id = node._id ?? GridStackEngine._idSeq++;
 
     // if we're missing position, have the grid position us automatically (before we set them to 0,0)
@@ -373,11 +372,12 @@ export class GridStackEngine {
     if (isNaN(node.w)) { node.w = defaults.w; }
     if (isNaN(node.h)) { node.h = defaults.h; }
 
-    return this.nodeBoundFix(node, resizing);
+    this.nodeBoundFix(node, resizing);
+    return node;
   }
 
   /** part2 of preparing a node to fit inside our grid - checks for x,y,w from grid dimensions */
-  public nodeBoundFix(node: GridStackNode, resizing?: boolean): GridStackNode {
+  public nodeBoundFix(node: GridStackNode, resizing?: boolean): GridStackEngine {
 
     let before = node._orig || Utils.copyPos({}, node);
 
@@ -436,7 +436,7 @@ export class GridStackEngine {
       node._dirty = true;
     }
 
-    return node;
+    return this;
   }
 
   /** returns a list of modified nodes from their original values */
@@ -520,7 +520,7 @@ export class GridStackEngine {
     if (dup) return dup; // prevent inserting twice! return it instead.
 
     // 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);
+    this._inColumnResize ? this.nodeBoundFix(node) : this.prepareNode(node);
     delete node._temporaryRemoved;
     delete node._removeDOM;
 
@@ -667,7 +667,7 @@ export class GridStackEngine {
     let resizing = (node.w !== o.w || node.h !== o.h);
     let nn: GridStackNode = Utils.copyPos({}, node, true); // get min/max out first, then opt positions next
     Utils.copyPos(nn, o);
-    nn = this.nodeBoundFix(nn, resizing);
+    this.nodeBoundFix(nn, resizing);
     Utils.copyPos(o, nn);
 
     if (!o.forceCollide && Utils.samePos(node, o)) return false;
@@ -926,7 +926,11 @@ export class GridStackEngine {
   public cacheLayout(nodes: GridStackNode[], column: number, clear = false): GridStackEngine {
     let copy: GridStackNode[] = [];
     nodes.forEach((n, i) => {
-      n._id = n._id ?? GridStackEngine._idSeq++; // make sure we have an id in case this is new layout, else re-use id already set
+      // make sure we have an id in case this is new layout, else re-use id already set
+      if (n._id === undefined) {
+        const existing = n.id ? this.nodes.find(n2 => n2.id === n.id) : undefined; // find existing node using users id
+        n._id = existing?._id ?? GridStackEngine._idSeq++;
+      }
       copy[i] = {x: n.x, y: n.y, w: n.w, _id: n._id} // only thing we change is x,y,w and id to find it back
     });
     this._layouts = clear ? [] : this._layouts || []; // use array to find larger quick
diff --git a/src/gridstack.ts b/src/gridstack.ts
@@ -666,7 +666,8 @@ export class GridStack {
 
     // if we're loading a layout into for example 1 column (_prevColumn is set only when going to 1) and items don't fit, make sure to save
     // the original wanted layout so we can scale back up correctly #1471
-    if (this._prevColumn && this._prevColumn !== this.opts.column && items.some(n => ((n.x || 0) + n.w) > (this.opts.column as number))) {
+    const column = this.opts.column as number;
+    if (this._prevColumn && this._prevColumn !== column && items.some(n => ((n.x || 0) + n.w) > column)) {
       this._ignoreLayoutsNodeChange = true; // skip layout update
       this.engine.cacheLayout(items, this._prevColumn, true);
     }
@@ -707,6 +708,7 @@ export class GridStack {
         // if item sizes to content, re-use the exiting height so it's a better guess at the final size 9same if width doesn't change)
         if (Utils.shouldSizeToContent(item)) w.h = item.h;
         // check if missing coord, in which case find next empty slot with new (or old if missing) sizes
+        this.engine.nodeBoundFix(w); // before widthChanged is checked below...
         if (w.autoPosition || w.x === undefined || w.y === undefined) {
           w.w = w.w || item.w;
           w.h = w.h || item.h;
@@ -718,7 +720,6 @@ export class GridStack {
         this.engine.nodes.push(item);
         if (Utils.samePos(item, w)) {
           this.moveNode(item, {...w, forceCollide: true});
-          Utils.copyPos(w, item, true);
         }
 
         this.update(item.el, w);
@@ -1219,6 +1220,7 @@ export class GridStack {
       let n = el?.gridstackNode;
       if (!n) return;
       let w = Utils.cloneDeep(opt); // make a copy we can modify in case they re-use it or multiple items
+      this.engine.nodeBoundFix(w);
       delete w.autoPosition;
       delete w.id;
 
@@ -1263,9 +1265,9 @@ export class GridStack {
       }
       Utils.sanitizeMinMax(n);
 
-      // finally move the widget
-      if (m !== undefined) this.moveNode(n, m);
-      if (changed) { // move will only update x,y,w,h so update the rest too
+      // finally move the widget and update attr
+      if (m) this.moveNode(n, m);
+      if (m || changed) {
         this._writeAttr(el, n);
       }
       if (ddChanged) {
