calling load() allows overlapping widgets

* fix gridstack#2492 * make sure we use existing items and not reset to start from scratch so we do collision at each step.
adumesny · Oct 14, 2023 · 79ee8d8 · 79ee8d8
1 parent 27735fe
commit 79ee8d8
Show file tree

Hide file tree

Showing 5 changed files with 51 additions and 11 deletions.
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/)*
 
+- [9.3.0-dev (TBD)](#930-dev-tbd)
 - [9.3.0 (2023-09-30)](#930-2023-09-30)
 - [9.2.2 (2023-09-27)](#922-2023-09-27)
 - [9.2.1 (2023-09-20)](#921-2023-09-20)
@@ -101,6 +102,9 @@ Change log
 
 <!-- END doctoc generated TOC please keep comment here to allow auto update -->
 
+## 9.3.0-dev (TBD)
+* fix [#2492](https://github.com/gridstack/gridstack.js/issues/2492) calling load() allows overlapping widgets
+
 ## 9.3.0 (2023-09-30)
 * fix [#1275](https://github.com/gridstack/gridstack.js/issues/1275) div scale support - Thank you [VincentMolinie](https://github.com/VincentMolinie) for implementing this
 

diff --git a/spec/e2e/html/2492_load_twice.html b/spec/e2e/html/2492_load_twice.html
@@ -0,0 +1,32 @@
+<!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>load() twice bug</title>
+
+  <link rel="stylesheet" href="../../../demo/demo.css" />
+  <script src="../../../dist/gridstack-all.js"></script>
+
+</head>
+<body>
+  <div class="container-fluid">
+    <h1>load twice bug</h1>
+    <p>this load() twice in a row with overlapping content and floating item</p>
+    <div class="grid-stack"></div>
+  </div>
+  <script type="text/javascript">
+    let items = [
+      {x: 0, y: 0, w:2, content: '0 wide'},
+      {x: 1, y: 0, content: '1 over'},
+      {x: 2, y: 1, content: '2 float'},
+    ];
+    let count = 0;
+    items.forEach(n => n.id = String(count++)); // TEST loading with ids
+    let grid = GridStack.init({disableOneColumnMode: true, cellHeight: 70, margin: 5}).load(items)
+    items.forEach(n => n.content += '*')
+    grid.load(items);
+  </script>
+</body>
+</html>
diff --git a/src/gridstack-engine.ts b/src/gridstack-engine.ts
@@ -655,7 +655,7 @@ export class GridStackEngine {
   public moveNode(node: GridStackNode, o: GridStackMoveOpts): boolean {
     if (!node || /*node.locked ||*/ !o) return false;
     let wasUndefinedPack: boolean;
-    if (o.pack === undefined) {
+    if (o.pack === undefined && !this.batchMode) {
       wasUndefinedPack = o.pack = true;
     }
 

diff --git a/src/gridstack.ts b/src/gridstack.ts
@@ -657,10 +657,10 @@ export class GridStack {
    * @example
    * see http://gridstackjs.com/demo/serialization.html
    */
-  public load(layout: GridStackWidget[], addRemove: boolean | AddRemoveFcn = GridStack.addRemoveCB || true): GridStack {
+  public load(items: GridStackWidget[], addRemove: boolean | AddRemoveFcn = GridStack.addRemoveCB || true): GridStack {
     // if passed list has coordinates, use them (insert from end to beginning for conflict resolution) else force widget same order
-    const haveCoord = layout.some(w => w.x !== undefined || w.y !== undefined);
-    let items = haveCoord ? Utils.sort(layout, -1, this._prevColumn || this.getColumn()) : layout;
+    const haveCoord = items.some(w => w.x !== undefined || w.y !== undefined);
+    if (haveCoord) items = Utils.sort(items, -1, this._prevColumn || this.getColumn());
     this._insertNotAppend = haveCoord; // if we create in reverse order...
 
     // 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
@@ -681,7 +681,8 @@ export class GridStack {
     if (addRemove) {
       let copyNodes = [...this.engine.nodes]; // don't loop through array you modify
       copyNodes.forEach(n => {
-        let item = items.find(w => n.id === w.id);
+        if (!n.id) return;
+        let item = Utils.find(items, n.id);
         if (!item) {
           if (GridStack.addRemoveCB)
             GridStack.addRemoveCB(this.el, n, false, false);
@@ -691,19 +692,16 @@ export class GridStack {
       });
     }
 
-    // now add/update the widgets - starting with an empty list to reduce collision and add no-coord ones at next available spot
-    let copyNodes = this.engine.nodes;
-    this.engine.nodes = [];
+    // now add/update the widgets
     items.forEach(w => {
-      let item = (w.id !== undefined) ? copyNodes.find(n => n.id === w.id) : undefined;
+      let item = Utils.find(this.engine.nodes, w.id);
       if (item) {
         // check if missing coord, in which case find next empty slot with new (or old if missing) sizes
         if (w.autoPosition || w.x === undefined || w.y === undefined) {
           w.w = w.w || item.w;
           w.h = w.h || item.h;
           this.engine.findEmptyPosition(w);
         }
-        this.engine.nodes.push(item); // now back to current list...
         this.update(item.el, w);
         if (w.subGridOpts?.children) { // update any sub grid as well
           let sub = item.el.querySelector('.grid-stack') as GridHTMLElement;
@@ -1202,6 +1200,7 @@ export class GridStack {
       if (!n) return;
       let w = Utils.cloneDeep(opt); // make a copy we can modify in case they re-use it or multiple items
       delete w.autoPosition;
+      delete w.id;
 
       // move/resize widget if anything changed
       let keys = ['x', 'y', 'w', 'h'];
@@ -1244,7 +1243,7 @@ export class GridStack {
       Utils.sanitizeMinMax(n);
 
       // finally move the widget
-      if (m) this.moveNode(n, m);
+      if (m !== undefined) this.moveNode(n, m);
       if (changed) { // move will only update x,y,w,h so update the rest too
         this._writeAttr(el, n);
       }

diff --git a/src/utils.ts b/src/utils.ts
@@ -147,6 +147,11 @@ export class Utils {
       return nodes.sort((b, a) => ((b.x ?? 1000) + (b.y ?? 1000) * column)-((a.x ?? 1000) + (a.y ?? 1000) * column));
   }
 
+  /** find an item by id */
+  static find(nodes: GridStackNode[], id: string): GridStackNode | undefined {
+    return id ? nodes.find(n => n.id === id) : undefined;
+  }
+
   /**
    * creates a style sheet with style id under given parent
    * @param id will set the 'gs-style-id' attribute to that id