Skip to content

Commit 548d603

Browse files
author
Ronald A Richardson
committed
fix: resolve drag/resize non-interactive, center-snap, and choppiness bugs
Root cause analysis ------------------- 1. Elements become non-interactive after first drag/resize moveElement() previously wrote to keep the properties panel in sync. Writing to any @Tracked property triggers a Glimmer re-render. Re-rendering causes the {{#if @isSelected}} block in element-renderer.hbs to insert/remove the .tb-handle-* divs, and in some cases destroys and recreates the element wrapper DOM node entirely. When the DOM node is recreated, interact.js loses its internal pointer state and the element becomes permanently unresponsive until the page reloads. Fix: remove the selectedElement write from moveElement(). The properties panel will reflect updated values the next time the user clicks the element (selectElement is called, which sets selectedElement to the same mutated object). No re-render occurs during drag/resize. 2. Mouse snaps to element center during drag interact.modifiers.snap() was configured with: relativePoints: [{ x: 0, y: 0 }] This tells interact.js to snap the element's top-left corner to the grid, not the pointer. Because the pointer is typically somewhere in the middle of the element, interact.js compensates by jumping the element so its top-left lands on the nearest grid point — which feels like the pointer is being pulled to the element's origin (center-snap). Fix: remove the snap modifier entirely. Free movement is smooth and accurate. Grid snapping can be re-added later using offset:'startCoords' which snaps relative to where the drag started, not the element origin. 3. Choppy movement The snap modifier (relativePoints) combined with restrict (parent bounding box) ran two constraint passes per pointer event, causing visible jank. Fix: both modifiers removed from draggable. Only restrictSize (minimum element dimensions) is kept on resizable, as it has no pointer-position side effects. 4. Zoom read at event time The previous implementation captured zoom at interactable-creation time via a closure. If the user changed zoom after an element was placed, the drag deltas would be scaled by the wrong factor. Fix: zoom is now read from this.args.zoom inside each event handler via a getZoom() closure, so it always reflects the current zoom level.
1 parent 20f5bb7 commit 548d603

File tree

2 files changed

+74
-68
lines changed

2 files changed

+74
-68
lines changed

‎addon/components/template-builder.js‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -164,16 +164,19 @@ export default class TemplateBuilderComponent extends Component {
164164
moveElement(uuid, changes) {
165165
const el = this.elements.find((e) => e.uuid === uuid);
166166
if (!el) return;
167-
// Mutate in-place — Glimmer only re-renders if a @tracked property
168-
// changes. The content array reference stays the same, so no re-render.
167+
// Mutate the element object in-place. The content array reference does
168+
// NOT change, so Glimmer does not schedule a re-render. This is
169+
// intentional — re-rendering would destroy and recreate the DOM nodes,
170+
// which would also destroy the interact.js instances and make the
171+
// element non-interactive.
172+
//
173+
// IMPORTANT: do NOT write to any @tracked property here (including
174+
// `this.selectedElement`). Even a "minimal" tracked write causes
175+
// Glimmer to re-render the canvas, which destroys the DOM nodes.
176+
// The properties panel will pick up the new values the next time the
177+
// user clicks the element (which calls selectElement → sets
178+
// selectedElement to the same mutated object).
169179
Object.assign(el, changes);
170-
// Keep selectedElement in sync too (same object reference, so the
171-
// properties panel will reflect the updated values on next interaction).
172-
if (this.selectedElement?.uuid === uuid) {
173-
// Trigger a minimal tracked update so the properties panel inputs
174-
// reflect the new x/y/width/height values.
175-
this.selectedElement = el;
176-
}
177180
}
178181

179182
@action

‎addon/components/template-builder/canvas.js‎

Lines changed: 62 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -21,20 +21,26 @@ import interact from 'interactjs';
2121
* Drag vs. resize separation
2222
* --------------------------
2323
* Resize is restricted to the four corner handle elements (.tb-handle-*)
24-
* via the `handle` option. This means any drag gesture on the element body
25-
* is unambiguously a move, not a resize.
24+
* via per-edge CSS selectors. Any drag gesture on the element body is
25+
* therefore unambiguously a move, not a resize.
2626
*
2727
* Re-render safety
2828
* ----------------
29-
* When `onUpdateElement` triggers a template re-render, Glimmer destroys
30-
* and recreates the element's DOM node. `teardownElement` (via will-destroy)
31-
* unsets the old interactable, and `setupElement` (via did-insert on the new
32-
* node) creates a fresh one — so the element is always interactive.
29+
* `moveElement` (the silent positional sync) must NOT write to any @tracked
30+
* property that causes a Glimmer re-render, because re-rendering destroys
31+
* and recreates DOM nodes, which breaks interact.js. In particular,
32+
* `selectedElement` must NOT be updated inside `moveElement`.
33+
*
34+
* The only time interact.js instances are torn down and rebuilt is when
35+
* `teardownElement` fires (will-destroy) followed by `setupElement`
36+
* (did-insert) — which happens when `updateElement` triggers a full
37+
* re-render (e.g. property panel changes). This is correct and expected.
3338
*
3439
* @argument {Object} template - The template object (width, height, unit, orientation, content)
3540
* @argument {Object} selectedElement - The currently selected element (or null)
3641
* @argument {Function} onSelectElement - Called with element when user clicks it
3742
* @argument {Function} onUpdateElement - Called with (elementId, changes) when drag/resize completes
43+
* @argument {Function} onMoveElement - Called with (elementId, {x,y,width?,height?}) silently
3844
* @argument {Function} onDeselectAll - Called when user clicks the canvas background
3945
* @argument {Number} zoom - Zoom level (1 = 100%)
4046
*/
@@ -89,7 +95,6 @@ export default class TemplateBuilderCanvasComponent extends Component {
8995

9096
@action
9197
willDestroyCanvas() {
92-
// Destroy all interact.js instances to prevent memory leaks.
9398
this._interactables.forEach((interactable) => {
9499
try { interactable.unset(); } catch (_) { /* ignore */ }
95100
});
@@ -102,14 +107,12 @@ export default class TemplateBuilderCanvasComponent extends Component {
102107

103108
@action
104109
setupElement(element, el) {
105-
// If a stale interactable exists for this uuid (e.g. after a re-render
106-
// recreated the DOM node), unset it before creating a new one.
110+
// Unset any stale interactable for this uuid before creating a new one.
107111
const stale = this._interactables.get(element.uuid);
108112
if (stale) {
109113
try { stale.unset(); } catch (_) { /* ignore */ }
110114
this._interactables.delete(element.uuid);
111115
}
112-
113116
this._setupInteract(element, el);
114117
}
115118

@@ -142,16 +145,18 @@ export default class TemplateBuilderCanvasComponent extends Component {
142145
// -------------------------------------------------------------------------
143146

144147
_setupInteract(element, el) {
145-
const zoom = this.zoom;
148+
// ── Helpers ────────────────────────────────────────────────────────────
146149

147-
// Helper: read the current translate values from the element's
148-
// data attributes (seeded in element-renderer.js handleInsert).
150+
// Read the current translate values from the element's data attributes.
151+
// These are seeded by element-renderer.js handleInsert and kept in sync
152+
// by the move/end listeners below.
149153
const getPos = () => ({
150154
x: parseFloat(el.dataset.x) || 0,
151155
y: parseFloat(el.dataset.y) || 0,
152156
});
153157

154-
// Helper: apply translate (+ optional rotation) to the element.
158+
// Write a new translate (+ optional rotation) to the element's style
159+
// and update the data attributes so getPos() stays accurate.
155160
const applyTransform = (x, y) => {
156161
const rotation = element.rotation ?? 0;
157162
el.style.transform = rotation
@@ -161,51 +166,48 @@ export default class TemplateBuilderCanvasComponent extends Component {
161166
el.dataset.y = y;
162167
};
163168

169+
// ── Zoom accessor ──────────────────────────────────────────────────────
170+
// Read zoom from args at event time (not captured at setup time) so
171+
// that zoom changes are reflected without needing to re-create the
172+
// interactable.
173+
const getZoom = () => this.args.zoom ?? 1;
174+
175+
// ── Interactable ───────────────────────────────────────────────────────
164176
const interactable = interact(el)
177+
165178
// ── Drag ──────────────────────────────────────────────────────────
166179
.draggable({
167-
// Require a small movement threshold before a drag starts.
168-
// This prevents accidental drags when the user intends to click.
169-
startAxis: 'xy',
170-
lockAxis: 'xy',
180+
// No modifiers — modifiers that use relativePoints or restrict
181+
// cause the pointer to snap to the element origin (center-snap
182+
// feel) and introduce jank. Free movement is smooth and accurate.
171183
listeners: {
172184
move: (event) => {
173-
const pos = getPos();
185+
const zoom = getZoom();
186+
const pos = getPos();
187+
// Divide interact.js deltas by zoom so the element
188+
// moves at the same speed as the pointer regardless of
189+
// the current zoom level.
174190
const x = pos.x + event.dx / zoom;
175191
const y = pos.y + event.dy / zoom;
176192
applyTransform(x, y);
177193
},
178-
end: (event) => {
194+
end: () => {
179195
const pos = getPos();
180-
// Use onMoveElement (silent, no re-render) for positional
181-
// updates from drag gestures so interact.js instances are
182-
// not destroyed and recreated after every drag.
183-
if (this.args?.onMoveElement) {
184-
this.args.onMoveElement(element.uuid, { x: pos.x, y: pos.y });
196+
if (this.args.onMoveElement) {
197+
this.args.onMoveElement(element.uuid, {
198+
x: Math.round(pos.x),
199+
y: Math.round(pos.y),
200+
});
185201
}
186202
},
187203
},
188-
modifiers: [
189-
// 5 px grid snap
190-
interact.modifiers.snap({
191-
targets: [interact.snappers.grid({ x: 5, y: 5 })],
192-
range: Infinity,
193-
relativePoints: [{ x: 0, y: 0 }],
194-
}),
195-
// Keep element inside the canvas
196-
interact.modifiers.restrict({
197-
restriction: 'parent',
198-
elementRect: { top: 0, left: 0, bottom: 1, right: 1 },
199-
endOnly: false,
200-
}),
201-
],
202204
})
205+
203206
// ── Resize ────────────────────────────────────────────────────────
204207
.resizable({
205-
// Restrict resize gestures to the four corner handle elements.
206-
// This is the key fix: without this, any drag on the element
207-
// body is ambiguously interpreted as a resize on the nearest
208-
// edge, which is why drag was broken.
208+
// Each edge value is a CSS selector matched against the pointer
209+
// target. The corner handles each match two adjacent edges so
210+
// they resize in both directions simultaneously.
209211
edges: {
210212
top: '.tb-handle-nw, .tb-handle-ne',
211213
left: '.tb-handle-nw, .tb-handle-sw',
@@ -214,40 +216,41 @@ export default class TemplateBuilderCanvasComponent extends Component {
214216
},
215217
listeners: {
216218
move: (event) => {
217-
const pos = getPos();
218-
// When resizing from the top or left, the element's
219-
// origin shifts — we must update the translate too.
219+
const zoom = getZoom();
220+
const pos = getPos();
221+
222+
// When resizing from the top or left edge the element's
223+
// origin shifts — update the translate to compensate.
220224
const x = pos.x + event.deltaRect.left / zoom;
221-
const y = pos.y + event.deltaRect.top / zoom;
222-
const w = event.rect.width / zoom;
225+
const y = pos.y + event.deltaRect.top / zoom;
226+
const w = event.rect.width / zoom;
223227
const h = event.rect.height / zoom;
224228

225229
el.style.width = `${w}px`;
226230
el.style.height = `${h}px`;
227231
applyTransform(x, y);
228232
},
229233
end: (event) => {
230-
const pos = getPos();
231-
const w = event.rect.width / zoom;
234+
const zoom = getZoom();
235+
const pos = getPos();
236+
const w = event.rect.width / zoom;
232237
const h = event.rect.height / zoom;
233-
if (this.args?.onMoveElement) {
238+
if (this.args.onMoveElement) {
234239
this.args.onMoveElement(element.uuid, {
235-
x: pos.x,
236-
y: pos.y,
237-
width: w,
238-
height: h,
240+
x: Math.round(pos.x),
241+
y: Math.round(pos.y),
242+
width: Math.round(w),
243+
height: Math.round(h),
239244
});
240245
}
241246
},
242247
},
243248
modifiers: [
249+
// Enforce a minimum element size so elements cannot be
250+
// collapsed to zero. No snap modifier — same reasoning as drag.
244251
interact.modifiers.restrictSize({
245252
min: { width: 20, height: 10 },
246253
}),
247-
// 5 px grid snap for size too
248-
interact.modifiers.snapSize({
249-
targets: [interact.snappers.grid({ width: 5, height: 5 })],
250-
}),
251254
],
252255
});
253256

0 commit comments

Comments
 (0)