diff --git a/packages/svgcanvas/core/history.js b/packages/svgcanvas/core/history.js index bb0d28b5..0616e8de 100644 --- a/packages/svgcanvas/core/history.js +++ b/packages/svgcanvas/core/history.js @@ -8,6 +8,61 @@ import { NS } from './namespaces.js' import { getHref, setHref, getRotationAngle, getBBox } from './utilities.js' +import { getTransformList, transformListToTransform, transformPoint } from './math.js' + +// Attributes that affect an element's bounding box. Only these require +// recalculating the rotation center when changed. +export const BBOX_AFFECTING_ATTRS = new Set([ + 'x', 'y', 'x1', 'y1', 'x2', 'y2', + 'cx', 'cy', 'r', 'rx', 'ry', + 'width', 'height', 'd', 'points' +]) + +/** +* Relocate rotation center after a bbox-affecting attribute change. +* Uses the transform list API to update only the rotation entry, +* preserving compound transforms (translate, scale, etc.). +* @param {Element} elem - SVG element +* @param {string[]} changedAttrs - attribute names that were changed +*/ +function relocateRotationCenter (elem, changedAttrs) { + const hasBboxChange = changedAttrs.some(attr => BBOX_AFFECTING_ATTRS.has(attr)) + if (!hasBboxChange) return + + const angle = getRotationAngle(elem) + if (!angle) return + + const tlist = getTransformList(elem) + let n = tlist.numberOfItems + while (n--) { + const xform = tlist.getItem(n) + if (xform.type === 4) { // SVG_TRANSFORM_ROTATE + // Compute bbox BEFORE removing the rotation so we can bail out + // safely if getBBox returns nothing (avoids losing the rotation). + const box = getBBox(elem) + if (!box) return + + tlist.removeItem(n) + + // Transform bbox center through only post-rotation transforms. + // After removeItem(n), what was at n+1 is now at n. + let centerMatrix + if (n < tlist.numberOfItems) { + centerMatrix = transformListToTransform(tlist, n, tlist.numberOfItems - 1).matrix + } else { + centerMatrix = elem.ownerSVGElement.createSVGMatrix() // identity + } + const center = transformPoint( + box.x + box.width / 2, box.y + box.height / 2, centerMatrix + ) + + const newrot = elem.ownerSVGElement.createSVGTransform() + newrot.setRotate(angle, center.x, center.y) + tlist.insertItemBefore(newrot, n) + break + } + } +} /** * Group: Undo/Redo history management. @@ -344,17 +399,7 @@ export class ChangeElementCommand extends Command { // relocate rotational transform, if necessary if (!bChangedTransform) { - const angle = getRotationAngle(this.elem) - if (angle) { - const bbox = getBBox(this.elem) - if (!bbox) return - const cx = bbox.x + bbox.width / 2 - const cy = bbox.y + bbox.height / 2 - const rotate = ['rotate(', angle, ' ', cx, ',', cy, ')'].join('') - if (rotate !== this.elem.getAttribute('transform')) { - this.elem.setAttribute('transform', rotate) - } - } + relocateRotationCenter(this.elem, Object.keys(this.newValues)) } }) } @@ -388,17 +433,7 @@ export class ChangeElementCommand extends Command { }) // relocate rotational transform, if necessary if (!bChangedTransform) { - const angle = getRotationAngle(this.elem) - if (angle) { - const bbox = getBBox(this.elem) - if (!bbox) return - const cx = bbox.x + bbox.width / 2 - const cy = bbox.y + bbox.height / 2 - const rotate = ['rotate(', angle, ' ', cx, ',', cy, ')'].join('') - if (rotate !== this.elem.getAttribute('transform')) { - this.elem.setAttribute('transform', rotate) - } - } + relocateRotationCenter(this.elem, Object.keys(this.oldValues)) } }) } diff --git a/packages/svgcanvas/core/undo.js b/packages/svgcanvas/core/undo.js index 53104465..160e35da 100644 --- a/packages/svgcanvas/core/undo.js +++ b/packages/svgcanvas/core/undo.js @@ -6,6 +6,7 @@ */ import * as draw from './draw.js' import * as hstry from './history.js' +import { BBOX_AFFECTING_ATTRS } from './history.js' import { getRotationAngle, getBBox as utilsGetBBox, setHref, getStrokedBBoxDefaultVisible } from './utilities.js' @@ -239,26 +240,39 @@ export const changeSelectedAttributeNoUndoMethod = (attr, newValue, elems) => { svgCanvas.selectorManager.requestSelector(elem).resize() }, 0) } - // if this element was rotated, and we changed the position of this element - // we need to update the rotational transform attribute + // Only recalculate rotation center for attributes that change element geometry. + // Non-geometric attributes (stroke-width, fill, opacity, etc.) don't affect + // the bbox center, so the rotation is already correct and must not be touched. + // BBOX_AFFECTING_ATTRS is imported from history.js to keep the list in one place. const angle = getRotationAngle(elem) - if (angle !== 0 && attr !== 'transform') { + if (angle !== 0 && attr !== 'transform' && BBOX_AFFECTING_ATTRS.has(attr)) { const tlist = getTransformList(elem) let n = tlist.numberOfItems while (n--) { const xform = tlist.getItem(n) if (xform.type === 4) { - // remove old rotate + // Compute bbox BEFORE removing the rotation so we can bail out + // safely if getBBox returns nothing (avoids losing the rotation). + const box = utilsGetBBox(elem) + if (!box) break + tlist.removeItem(n) - const box = utilsGetBBox(elem) + // Transform bbox center through only the transforms that come + // AFTER the rotation in the list (not the pre-rotation transforms). + // After removeItem(n), what was at n+1 is now at n. + let centerMatrix + if (n < tlist.numberOfItems) { + centerMatrix = transformListToTransform(tlist, n, tlist.numberOfItems - 1).matrix + } else { + centerMatrix = svgCanvas.getSvgRoot().createSVGMatrix() // identity + } const center = transformPoint( - box.x + box.width / 2, box.y + box.height / 2, transformListToTransform(tlist).matrix + box.x + box.width / 2, box.y + box.height / 2, centerMatrix ) - const cx = center.x - const cy = center.y + const newrot = svgCanvas.getSvgRoot().createSVGTransform() - newrot.setRotate(angle, cx, cy) + newrot.setRotate(angle, center.x, center.y) tlist.insertItemBefore(newrot, n) break } diff --git a/tests/unit/rotation-recalc.test.js b/tests/unit/rotation-recalc.test.js new file mode 100644 index 00000000..b411c499 --- /dev/null +++ b/tests/unit/rotation-recalc.test.js @@ -0,0 +1,300 @@ +/** + * Tests for rotation recalculation when changing attributes on rotated elements. + * + * These tests verify two bugs that were fixed: + * + * Bug 1: Changing non-geometric attributes (stroke-width, fill, opacity, etc.) + * on a rotated element would trigger an unnecessary rotation center recalculation, + * corrupting compound transforms. + * + * Bug 2: The rotation center was computed through ALL remaining transforms + * (including pre-rotation ones like translate), causing the translate to leak + * into the center calculation and produce an incorrect rotation. + * + * Bug 3 (history.js only): The undo/redo code replaced the ENTIRE transform + * attribute with just rotate(...), destroying any other transforms in the list. + */ + +import { NS } from '../../packages/svgcanvas/core/namespaces.js' +import * as utilities from '../../packages/svgcanvas/core/utilities.js' +import * as history from '../../packages/svgcanvas/core/history.js' +import { getTransformList } from '../../packages/svgcanvas/core/math.js' + +describe('Rotation recalculation on attribute change', function () { + /** + * Helper: create an SVG with the given attributes inside an . + */ + function createSvgRect (attrs = {}) { + const svg = document.createElementNS(NS.SVG, 'svg') + document.body.appendChild(svg) + const rect = document.createElementNS(NS.SVG, 'rect') + for (const [k, v] of Object.entries(attrs)) { + rect.setAttribute(k, v) + } + svg.appendChild(rect) + return rect + } + + /** + * Helper: read back the transform list entries as simple objects for assertions. + */ + function readTransforms (elem) { + const tlist = getTransformList(elem) + const result = [] + for (let i = 0; i < tlist.numberOfItems; i++) { + const t = tlist.getItem(i) + result.push({ type: t.type, matrix: { ...t.matrix } }) + } + return result + } + + afterEach(() => { + document.body.textContent = '' + // Reset mock to default (no rotation) + utilities.mock({ + getHref () { return '#foo' }, + setHref () { /* empty fn */ }, + getRotationAngle () { return 0 } + }) + }) + + describe('ChangeElementCommand with rotated elements', function () { + it('non-geometric attribute change preserves simple rotation', function () { + const rect = createSvgRect({ + x: '0', + y: '0', + width: '100', + height: '100', + transform: 'rotate(30, 50, 50)', + 'stroke-width': '1' + }) + + // Mock getRotationAngle to return 30 (matching our transform) + utilities.mock({ + getHref () { return '' }, + setHref () { /* empty fn */ }, + getRotationAngle () { return 30 } + }) + + const transformsBefore = readTransforms(rect) + + // Simulate changing stroke-width from 1 to 5 + rect.setAttribute('stroke-width', '5') + const change = new history.ChangeElementCommand(rect, { 'stroke-width': '1' }) + + // Apply (redo) — should NOT touch the transform + change.apply() + const transformsAfterApply = readTransforms(rect) + assert.equal(transformsAfterApply.length, transformsBefore.length, + 'apply: transform list length unchanged') + assert.equal(transformsAfterApply[0].type, 4, + 'apply: rotation transform preserved') + + // Unapply (undo) — should NOT touch the transform + change.unapply() + const transformsAfterUnapply = readTransforms(rect) + assert.equal(transformsAfterUnapply.length, transformsBefore.length, + 'unapply: transform list length unchanged') + assert.equal(transformsAfterUnapply[0].type, 4, + 'unapply: rotation transform preserved') + }) + + it('non-geometric attribute change preserves compound transforms', function () { + const rect = createSvgRect({ + x: '0', + y: '0', + width: '100', + height: '100', + transform: 'translate(100, 50) rotate(30)', + 'stroke-width': '2' + }) + + utilities.mock({ + getHref () { return '' }, + setHref () { /* empty fn */ }, + getRotationAngle () { return 30 } + }) + + const tlistBefore = getTransformList(rect) + assert.equal(tlistBefore.numberOfItems, 2, + 'setup: two transforms (translate + rotate)') + assert.equal(tlistBefore.getItem(0).type, 2, + 'setup: first transform is translate') + assert.equal(tlistBefore.getItem(1).type, 4, + 'setup: second transform is rotate') + + // Capture the translate matrix before + const translateMatrix = { ...tlistBefore.getItem(0).matrix } + + // Simulate changing stroke-width from 2 to 5 + rect.setAttribute('stroke-width', '5') + const change = new history.ChangeElementCommand(rect, { 'stroke-width': '2' }) + + // Apply (redo) — must preserve both translate and rotate + change.apply() + const tlistAfter = getTransformList(rect) + assert.equal(tlistAfter.numberOfItems, 2, + 'apply: still two transforms') + assert.equal(tlistAfter.getItem(0).type, 2, + 'apply: first is still translate') + assert.equal(tlistAfter.getItem(1).type, 4, + 'apply: second is still rotate') + assert.closeTo(tlistAfter.getItem(0).matrix.e, translateMatrix.e, 0.01, + 'apply: translate tx preserved') + assert.closeTo(tlistAfter.getItem(0).matrix.f, translateMatrix.f, 0.01, + 'apply: translate ty preserved') + + // Unapply (undo) — must also preserve both transforms + change.unapply() + assert.equal(tlistAfter.numberOfItems, 2, + 'unapply: still two transforms') + assert.equal(tlistAfter.getItem(0).type, 2, + 'unapply: first is still translate') + assert.equal(tlistAfter.getItem(1).type, 4, + 'unapply: second is still rotate') + }) + + it('geometric attribute change updates rotation center correctly', function () { + // Element with simple rotation — changing x should update the rotation center + const rect = createSvgRect({ + x: '0', + y: '0', + width: '100', + height: '100', + transform: 'rotate(45, 50, 50)' + }) + + utilities.mock({ + getHref () { return '' }, + setHref () { /* empty fn */ }, + getRotationAngle () { return 45 } + }) + + // Change x from 0 to 20 (new bbox center at 70, 50) + rect.setAttribute('x', '20') + const change = new history.ChangeElementCommand(rect, { x: '0' }) + + // Apply should update the rotation center to (70, 50) + change.apply() + const tlist = getTransformList(rect) + assert.equal(tlist.numberOfItems, 1, 'still one transform') + assert.equal(tlist.getItem(0).type, 4, 'still a rotation') + // The rotation center should reflect the new bbox center + assert.closeTo(tlist.getItem(0).cx, 70, 0.01, + 'rotation cx updated to new bbox center') + assert.closeTo(tlist.getItem(0).cy, 50, 0.01, + 'rotation cy unchanged') + }) + + it('geometric attribute change on compound transform uses only post-rotation transforms for center', function () { + // Element with translate(100, 50) rotate(30) + // When x changes, the rotation center should be computed from the + // bbox center WITHOUT the pre-rotation translate leaking in. + const rect = createSvgRect({ + x: '0', + y: '0', + width: '100', + height: '100', + transform: 'translate(100, 50) rotate(30)' + }) + + utilities.mock({ + getHref () { return '' }, + setHref () { /* empty fn */ }, + getRotationAngle () { return 30 } + }) + + const tlistBefore = getTransformList(rect) + assert.equal(tlistBefore.numberOfItems, 2, 'setup: two transforms') + + // Change x from 0 to 20 + rect.setAttribute('x', '20') + const change = new history.ChangeElementCommand(rect, { x: '0' }) + + change.apply() + const tlist = getTransformList(rect) + + // Should still have 2 transforms: translate + rotate + assert.equal(tlist.numberOfItems, 2, + 'compound transform preserved (2 entries)') + assert.equal(tlist.getItem(0).type, 2, + 'first is still translate') + assert.equal(tlist.getItem(1).type, 4, + 'second is still rotate') + + // The translate should be unchanged + assert.closeTo(tlist.getItem(0).matrix.e, 100, 0.01, + 'translate tx unchanged') + assert.closeTo(tlist.getItem(0).matrix.f, 50, 0.01, + 'translate ty unchanged') + + // The rotation center should be (70, 50) — the new bbox center — + // NOT (170, 100) which is what you'd get if the translate leaked in. + assert.closeTo(tlist.getItem(1).cx, 70, 0.01, + 'rotation cx = new bbox center, not offset by translate') + assert.closeTo(tlist.getItem(1).cy, 50, 0.01, + 'rotation cy = new bbox center, not offset by translate') + }) + + it('fill change does not trigger rotation recalculation', function () { + const rect = createSvgRect({ + x: '0', + y: '0', + width: '100', + height: '100', + transform: 'rotate(45, 50, 50)', + fill: 'red' + }) + + utilities.mock({ + getHref () { return '' }, + setHref () { /* empty fn */ }, + getRotationAngle () { return 45 } + }) + + const tlistBefore = getTransformList(rect) + const cxBefore = tlistBefore.getItem(0).cx + const cyBefore = tlistBefore.getItem(0).cy + + rect.setAttribute('fill', 'blue') + const change = new history.ChangeElementCommand(rect, { fill: 'red' }) + + change.apply() + const tlistAfter = getTransformList(rect) + assert.equal(tlistAfter.getItem(0).cx, cxBefore, + 'rotation cx unchanged after fill change') + assert.equal(tlistAfter.getItem(0).cy, cyBefore, + 'rotation cy unchanged after fill change') + }) + + it('opacity change does not trigger rotation recalculation', function () { + const rect = createSvgRect({ + x: '0', + y: '0', + width: '100', + height: '100', + transform: 'translate(50, 25) rotate(60)', + opacity: '1' + }) + + utilities.mock({ + getHref () { return '' }, + setHref () { /* empty fn */ }, + getRotationAngle () { return 60 } + }) + + const tlistBefore = getTransformList(rect) + assert.equal(tlistBefore.numberOfItems, 2, 'setup: two transforms') + + rect.setAttribute('opacity', '0.5') + const change = new history.ChangeElementCommand(rect, { opacity: '1' }) + + change.apply() + const tlist = getTransformList(rect) + assert.equal(tlist.numberOfItems, 2, + 'opacity change preserves compound transform count') + assert.equal(tlist.getItem(0).type, 2, 'translate preserved') + assert.equal(tlist.getItem(1).type, 4, 'rotate preserved') + }) + }) +}) diff --git a/tests/visual/compound-transform-bug.svg b/tests/visual/compound-transform-bug.svg new file mode 100644 index 00000000..4e2e4914 --- /dev/null +++ b/tests/visual/compound-transform-bug.svg @@ -0,0 +1,15 @@ + + + + + + + + + + + + diff --git a/tests/visual/rotation-recalc-demo.html b/tests/visual/rotation-recalc-demo.html new file mode 100644 index 00000000..94599da0 --- /dev/null +++ b/tests/visual/rotation-recalc-demo.html @@ -0,0 +1,295 @@ + + + + +SVG-Edit Bug: Rotation Recalculation Corrupts Compound Transforms + + + + +

Bug: Rotation Recalculation Corrupts Compound Transforms

+

+ SVG-Edit recalculates rotation centers when element attributes change. Two bugs cause this + recalculation to corrupt elements with compound transforms (e.g. word art characters on a curve). +

+ + +
Bug 1: Non-geometric attribute change destroys compound transforms (history.js)
+ +

+ When undoing/redoing a stroke-width change, the old code replaces the + entire transform attribute with just rotate(...), destroying any + translate() or scale() transforms. Characters positioned via + compound transforms collapse to the SVG origin. +

+ +
+ +
+

Original Characters on curve

+

Each character has translate(x,y) rotate(angle)

+ + + + + S + T + O + R + Y + +

stroke-width: 2

+
+ +
→
+ + +
+

Bug After undo of stroke-width change

+

Old code replaces transform with just rotate() — translate destroyed

+ + + + S + T + O + R + Y + + S + T + O + R + Y + +

Characters collapse to origin — translate() destroyed

+
+ + + + +
+

Fixed After undo of stroke-width change

+

New code skips recalculation for non-geometric attrs — transform untouched

+ + + S + T + O + R + Y + +

Characters unchanged — non-geometric attrs skip recalculation

+
+
+ +
// history.js — old code (apply/unapply): +const rotate = `rotate(${angle}, ${cx}, ${cy})` +this.elem.setAttribute('transform', rotate) // replaces ENTIRE transform! + +// history.js — new code: +if (!BBOX_AFFECTING_ATTRS.has(attr)) return // skip for stroke-width, fill, etc. +// When needed, update only the rotation entry via transform list API
+ + + +
Bug 2: Pre-rotation translate leaks into rotation center (undo.js)
+ +

+ When a geometric attribute (e.g. width) changes on an element with + translate(tx,ty) rotate(angle), the rotation center should be computed + from the bbox center alone. The old code transforms the center through all remaining + transforms (including the pre-rotation translate), shifting the rotation center by (tx, ty). +

+ +
+ +
+

Original Rect with translate + rotate

+

translate(200,60) rotate(25, 50, 40) on 100×80 rect

+ + + + + + + translate(200, 60) + + + + + + center (50, 40) + + +
+ +
→
+ + +
+

Bug After width change: center shifted

+

Old code: center through all transforms → rotate(25, 260, 100)

+ + + + + + + + + + + + + + wrong! (260, 100) + + +

translate(200,60) leaks into rotation center calculation

+
+ + + + +
+

Fixed After width change: center correct

+

New code: center through post-rotation transforms only → rotate(25, 60, 40)

+ + + + + translate(200, 60) + + + + + correct (60, 40) + + +

Center computed from bbox only, translate excluded

+
+
+ +
// undo.js — old code (center through ALL remaining transforms): +center = transformPoint(bcx, bcy, transformListToTransform(tlist).matrix) +// For [translate(200,60), rotate(25)]: includes translate → center = (bcx+200, bcy+60) + +// undo.js — new code (center through only POST-rotation transforms): +centerMatrix = transformListToTransform(tlist, n, tlist.numberOfItems - 1).matrix +// For [translate(200,60)]: post-rotation = identity → center = (bcx, bcy)
+ + + diff --git a/tests/visual/rotation-recalc-demo.png b/tests/visual/rotation-recalc-demo.png new file mode 100644 index 00000000..feea0e7b Binary files /dev/null and b/tests/visual/rotation-recalc-demo.png differ diff --git a/tests/visual/screenshot.mjs b/tests/visual/screenshot.mjs new file mode 100644 index 00000000..c85dadc4 --- /dev/null +++ b/tests/visual/screenshot.mjs @@ -0,0 +1,21 @@ +#!/usr/bin/env node +/** + * Take screenshots of the rotation recalculation bug demo. + * Usage: node tests/visual/screenshot.mjs + */ +import { chromium } from 'playwright' +import { fileURLToPath } from 'node:url' +import { dirname, resolve } from 'node:path' + +const __dirname = dirname(fileURLToPath(import.meta.url)) +const htmlPath = resolve(__dirname, 'rotation-recalc-demo.html') +const outputPath = resolve(__dirname, 'rotation-recalc-demo.png') + +const browser = await chromium.launch() +const page = await browser.newPage({ viewport: { width: 1800, height: 980 } }) +await page.goto(`file://${htmlPath}`) +await page.waitForTimeout(500) // let fonts render +await page.screenshot({ path: outputPath, fullPage: true }) +await browser.close() + +console.log(`Screenshot saved to: ${outputPath}`)