From 3251ae1cab77e75d025a3741585f601a723b0a5f Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Wed, 19 Aug 2026 10:44:41 +0100 Subject: [PATCH] fix(sorter): use model-space index when filtered children precede drop target MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getChildrenDim skips children whose DOM element does not match the itemSel (e.g. comment nodes rendered as nodeType=8). findPosition returns an index into the filtered dims array, which was used directly as the model-space insert index — off by however many children were skipped earlier in the list. Fix: stamp each Dimension with its unfiltered position (indexEl) in getChildrenDim, then translate in getDropPosition via `(dimension.indexEl ?? dimensionIndex)`. Also guards getDropPosition against the crash that occurred when all children were filtered (childrenDimensions.length === 0) but the parent still had model children. Fixes drops being off-by-N when MSO conditional comment siblings exist (e.g. content produced by grapesjs-mjml). --- packages/core/src/utils/sorter/Dimension.ts | 4 + .../utils/sorter/DropLocationDeterminer.ts | 13 +- .../specs/utils/sorter/sorterDropIndex.ts | 230 ++++++++++++++++++ 3 files changed, 243 insertions(+), 4 deletions(-) create mode 100644 packages/core/test/specs/utils/sorter/sorterDropIndex.ts diff --git a/packages/core/src/utils/sorter/Dimension.ts b/packages/core/src/utils/sorter/Dimension.ts index dc8b7e284..9309ec904 100644 --- a/packages/core/src/utils/sorter/Dimension.ts +++ b/packages/core/src/utils/sorter/Dimension.ts @@ -12,6 +12,8 @@ export default class Dimension { public width: number; public offsets: ReturnType; public dir?: boolean; + /** Index of this child in the *unfiltered* children list (model-space). Set by getChildrenDim. */ + public indexEl?: number; /** * Initializes the DimensionCalculator with the given initial dimensions. @@ -34,6 +36,7 @@ export default class Dimension { this.width = initialDimensions.width; this.offsets = initialDimensions.offsets; this.dir = initialDimensions.dir; + this.indexEl = initialDimensions.indexEl; } /** @@ -111,6 +114,7 @@ export default class Dimension { width: this.width, offsets: { ...this.offsets }, // Shallow copy of offsets dir: this.dir, + indexEl: this.indexEl, }); } diff --git a/packages/core/src/utils/sorter/DropLocationDeterminer.ts b/packages/core/src/utils/sorter/DropLocationDeterminer.ts index 8308529d0..d65e41fa3 100644 --- a/packages/core/src/utils/sorter/DropLocationDeterminer.ts +++ b/packages/core/src/utils/sorter/DropLocationDeterminer.ts @@ -252,10 +252,14 @@ export class DropLocationDeterminer> ext let placeholderDimensions = nodeDimensions.clone(), index = 0, placement = 'inside' as Placement; - if (nodeHasChildren) { - ({ index, placement } = findPosition(childrenDimensions, mouseX, mouseY)); - placeholderDimensions = childrenDimensions[index].clone(); - index = index + (placement == 'after' ? 1 : 0); + if (nodeHasChildren && childrenDimensions.length > 0) { + const { index: dimensionIndex, placement: foundPlacement } = findPosition(childrenDimensions, mouseX, mouseY); + placement = foundPlacement; + const dimension = childrenDimensions[dimensionIndex]; + placeholderDimensions = dimension.clone(); + // Use the unfiltered (model-space) index so skipped children (e.g. comment + // nodes) don't cause an off-by-N insertion error. + index = (dimension.indexEl ?? dimensionIndex) + (placement == 'after' ? 1 : 0); } return { @@ -492,6 +496,7 @@ export class DropLocationDeterminer> ext const dim = this.getDim(el); dim.dir = this.getDirection(el, targetElement); + dim.indexEl = i; dims.push(dim); }); diff --git a/packages/core/test/specs/utils/sorter/sorterDropIndex.ts b/packages/core/test/specs/utils/sorter/sorterDropIndex.ts new file mode 100644 index 000000000..b49f971f2 --- /dev/null +++ b/packages/core/test/specs/utils/sorter/sorterDropIndex.ts @@ -0,0 +1,230 @@ +import { DropLocationDeterminer } from '../../../../src/utils/sorter/DropLocationDeterminer'; +import { SortableTreeNode } from '../../../../src/utils/sorter/SortableTreeNode'; +import Dimension from '../../../../src/utils/sorter/Dimension'; +import { DragDirection, DragSource } from '../../../../src/utils/sorter/types'; +import { setupTestEditor } from '../../../common'; +import Editor from '../../../../src/editor'; + +// Minimal concrete SortableTreeNode for test use only. +// The treeClass constructor signature requires (model, dragSource?); we stash +// the element and children on the instance after construction via factory helpers. +class TestNode extends SortableTreeNode { + public _element: HTMLElement | undefined; + public _children: TestNode[] = []; + public _parent: TestNode | null = null; + + constructor(model: any = {}, dragSource: DragSource = {}) { + super(model, dragSource); + } + + static create(element?: HTMLElement, children: TestNode[] = []): TestNode { + const node = new TestNode(); + node._element = element; + node._children = children; + node._children.forEach((c) => (c._parent = node)); + return node; + } + + get element() { + return this._element; + } + get view() { + return undefined; + } + getChildren() { + return this._children.length ? this._children : null; + } + getParent() { + return this._parent; + } + addChildAt(node: TestNode, index: number) { + return node; + } + removeChildAt(_index: number) {} + indexOfChild(node: TestNode) { + return this._children.indexOf(node); + } + canMove() { + return true; + } + equals(other?: TestNode): other is TestNode { + return other === this; + } +} + +function makeDeterminer(em: any, itemSel = '.gjs-comp') { + const container = document.createElement('div'); + return new DropLocationDeterminer({ + em, + treeClass: TestNode, + containerContext: { + container, + itemSel, + document, + }, + positionOptions: { canvasRelative: false, relative: false, windowMargin: 0 }, + dragDirection: DragDirection.Vertical, + eventHandlers: {}, + }); +} + +function makeDim(top: number, height: number, indexEl?: number): Dimension { + const d = new Dimension({ top, left: 0, height, width: 200, offsets: {} as any, dir: true }); + d.indexEl = indexEl; + return d; +} + +describe('Sorter drop-index offset (comment-node skew)', () => { + let editor: Editor; + let fixtures: HTMLElement; + + beforeEach(() => { + ({ editor, fixtures } = setupTestEditor({ withCanvas: true })); + }); + + afterEach(() => { + jest.restoreAllMocks(); + editor.destroy(); + }); + + describe('Dimension', () => { + test('clone preserves indexEl', () => { + const d = makeDim(0, 100, 3); + const cloned = d.clone(); + expect(cloned.indexEl).toBe(3); + }); + + test('clone without indexEl keeps it undefined', () => { + const d = makeDim(0, 100); + expect(d.clone().indexEl).toBeUndefined(); + }); + }); + + describe('getChildrenDim', () => { + test('stamps indexEl with unfiltered child position, skipping null-element children', () => { + const determiner = makeDeterminer(editor.getModel()); + const parentEl = document.createElement('div'); + + // Children: [no-el, div, no-el, div, div] — indices 0,1,2,3,4 in model + const div1 = document.createElement('div'); + const div2 = document.createElement('div'); + const div3 = document.createElement('div'); + div1.className = 'gjs-comp'; + div2.className = 'gjs-comp'; + div3.className = 'gjs-comp'; + + const parent = TestNode.create(parentEl, [ + TestNode.create(undefined), + TestNode.create(div1), + TestNode.create(undefined), + TestNode.create(div2), + TestNode.create(div3), + ]); + + jest + .spyOn(determiner as any, 'getDim') + .mockImplementation((el: unknown) => makeDim(el === div1 ? 0 : el === div2 ? 100 : 200, 100)); + jest.spyOn(determiner as any, 'getDirection').mockReturnValue(true); + + const dims: Dimension[] = (determiner as any).getChildrenDim(parent); + + expect(dims).toHaveLength(3); + expect(dims[0].indexEl).toBe(1); + expect(dims[1].indexEl).toBe(3); + expect(dims[2].indexEl).toBe(4); + }); + }); + + describe('getDropPosition', () => { + test('returns model-space index when earlier children are filtered out', () => { + // Scenario: parent [skip, div, skip, div, div] + // Mouse after the last div → should land at model index 5 (after index 4) + const determiner = makeDeterminer(editor.getModel()); + const parentEl = document.createElement('div'); + + const parent = TestNode.create(parentEl, [ + TestNode.create(undefined), + TestNode.create(document.createElement('div')), + TestNode.create(undefined), + TestNode.create(document.createElement('div')), + TestNode.create(document.createElement('div')), + ]); + + const parentDim = makeDim(0, 300); + parent.nodeDimensions = parentDim; + // Pre-supply childrenDimensions with correct indexEl so getChildrenDim is bypassed + parent.childrenDimensions = [makeDim(0, 100, 1), makeDim(100, 100, 3), makeDim(200, 100, 4)]; + + // y=280 is inside the last child (top=200, height=100) and past its center + const result = (determiner as any).getDropPosition(parent, 100, 280); + + expect(result.placement).toBe('after'); + // Without fix: index would be 2+1=3; with fix: (4 ?? 2)+1=5 + expect(result.index).toBe(5); + }); + + test('returns correct index when drop is before a middle child', () => { + const determiner = makeDeterminer(editor.getModel()); + const parentEl = document.createElement('div'); + + const parent = TestNode.create(parentEl, [ + TestNode.create(undefined), + TestNode.create(document.createElement('div')), + TestNode.create(undefined), + TestNode.create(document.createElement('div')), + TestNode.create(document.createElement('div')), + ]); + + parent.nodeDimensions = makeDim(0, 300); + // Pre-supply childrenDimensions: skip items 0 and 2 + parent.childrenDimensions = [makeDim(0, 100, 1), makeDim(100, 100, 3), makeDim(200, 100, 4)]; + + // y=40 is in first child (top=0, height=100) before its center (50) + const result = (determiner as any).getDropPosition(parent, 100, 40); + + expect(result.placement).toBe('before'); + // dimensionIndex=0, indexEl=1, placement='before' → index = 1+0 = 1 + expect(result.index).toBe(1); + }); + + test('does not crash when all children lack measurable elements', () => { + // Second bug: parent with only un-measurable children → childrenDimensions=[] + const determiner = makeDeterminer(editor.getModel()); + const parentEl = document.createElement('div'); + + const parent = TestNode.create(parentEl, [TestNode.create(undefined), TestNode.create(undefined)]); + + parent.nodeDimensions = makeDim(0, 200); + parent.childrenDimensions = []; // all filtered out + + // Should not throw and should return index=0, placement='inside' + let result: any; + expect(() => { + result = (determiner as any).getDropPosition(parent, 100, 100); + }).not.toThrow(); + + expect(result.index).toBe(0); + expect(result.placement).toBe('inside'); + }); + + test('clean children (no skipped nodes) are unaffected', () => { + // Regression: if no children are filtered, indexEl == dimensionIndex and result is identical + const determiner = makeDeterminer(editor.getModel()); + const parentEl = document.createElement('div'); + + const parent = TestNode.create(parentEl, [ + TestNode.create(document.createElement('div')), + TestNode.create(document.createElement('div')), + TestNode.create(document.createElement('div')), + ]); + + parent.nodeDimensions = makeDim(0, 300); + parent.childrenDimensions = [makeDim(0, 100, 0), makeDim(100, 100, 1), makeDim(200, 100, 2)]; + + // y=280 → after last child (index 2) → 2+1=3 + const result = (determiner as any).getDropPosition(parent, 100, 280); + expect(result.placement).toBe('after'); + expect(result.index).toBe(3); + }); + }); +});