Files
svgedit/packages/svgcanvas/core/history.js
Geoff Youngs 173dd2607a Fix/rotation recalc compound transforms (#1083)
* Fix rotation recalculation corrupting compound transforms

Two bugs in the rotation center recalculation triggered by attribute
changes on rotated elements:

1. The recalculation fires for ALL attribute changes (stroke-width,
   fill, opacity, etc.) even though only geometric attributes (x, y,
   width, height, d, points, etc.) can change the bbox center. This
   causes unnecessary and incorrect rotation center updates.

2. In undo.js, the center is computed through ALL remaining transforms
   via transformListToTransform(tlist).matrix, but for compound
   transform lists like [translate(tx,ty), rotate(angle)], the
   pre-rotation translate leaks into the center calculation, producing
   rotate(angle, bcx+tx, bcy+ty) instead of the correct
   rotate(angle, bcx, bcy).

3. In history.js (undo/redo), the code replaces the ENTIRE transform
   attribute with just rotate(...), completely destroying any other
   transforms (translate, scale, etc.) in the list.

Fixes:
- Guard recalculation with a BBOX_AFFECTING_ATTRS set so non-geometric
  attribute changes skip the rotation block entirely.
- In undo.js, use transformListToTransform(tlist, n, max) to compute
  the center through only post-rotation transforms.
- In history.js, replace string-based transform replacement with
  transform list API that finds and updates only the rotation entry,
  preserving all other transforms. Extract shared logic into
  relocateRotationCenter() helper.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Add tests for rotation recalculation with compound transforms

Tests demonstrate and verify fixes for three bugs:

1. Non-geometric attributes (stroke-width, fill, opacity) no longer
   trigger rotation center recalculation — the transform list is
   left untouched.

2. Geometric attribute changes on elements with compound transforms
   (e.g. translate + rotate) compute the rotation center using only
   post-rotation transforms, preventing the translate from leaking
   into the center calculation.

3. ChangeElementCommand.apply/unapply preserve compound transform
   structure instead of replacing the entire transform attribute
   with just rotate(...).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Add visual demo and screenshot for rotation recalculation bug

Includes an HTML demo showing both bugs side-by-side:
- Bug 1: Non-geometric attribute changes (stroke-width) on compound
  transforms corrupt character positions (e.g., word art on curves)
- Bug 2: Rotation center computed through ALL transforms instead of
  only post-rotation transforms, causing translate to leak into center

Each panel shows Original / Buggy / Fixed comparisons with the actual
SVG transform math applied. A Playwright script generates screenshots.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Add sample SVG demonstrating compound transform corruption bug

A pinwheel of 6 colored rectangles using translate(200,200) rotate(N).
Loading this into SVG-Edit and changing stroke-width on the group
triggers the rotation recalculation bug on unfixed builds.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Fix standard linting: object properties on separate lines

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Centralize BBOX_AFFECTING_ATTRS and fix bbox-before-remove safety

Address two code review comments:

1. Export BBOX_AFFECTING_ATTRS from history.js and import it in undo.js
   so the attribute list is defined in one place.

2. Compute getBBox() before calling tlist.removeItem() so that a falsy
   bbox causes an early return/break without having already destroyed
   the rotation transform.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
2026-03-15 22:28:53 +01:00

675 lines
20 KiB
JavaScript

/* eslint-disable no-console */
/**
* For command history tracking and undo functionality.
* @module history
* @license MIT
* @copyright 2010 Jeff Schiller
*/
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.
*/
export const HistoryEventTypes = {
BEFORE_APPLY: 'before_apply',
AFTER_APPLY: 'after_apply',
BEFORE_UNAPPLY: 'before_unapply',
AFTER_UNAPPLY: 'after_unapply'
}
/**
* Base class for commands.
*/
export class Command {
/**
* @returns {string}
*/
getText () {
return this.text
}
/**
* @param {module:history.HistoryEventHandler} handler
* @param {callback} applyFunction
* @returns {void}
*/
apply (handler, applyFunction) {
handler && handler.handleHistoryEvent(HistoryEventTypes.BEFORE_APPLY, this)
applyFunction(handler)
handler && handler.handleHistoryEvent(HistoryEventTypes.AFTER_APPLY, this)
}
/**
* @param {module:history.HistoryEventHandler} handler
* @param {callback} unapplyFunction
* @returns {void}
*/
unapply (handler, unapplyFunction) {
handler && handler.handleHistoryEvent(HistoryEventTypes.BEFORE_UNAPPLY, this)
unapplyFunction()
handler && handler.handleHistoryEvent(HistoryEventTypes.AFTER_UNAPPLY, this)
}
/**
* @returns {Element[]} Array with element associated with this command
* This function needs to be surcharged if multiple elements are returned.
*/
elements () {
return [this.elem]
}
/**
* @returns {string} String with element associated with this command
*/
type () {
return this.constructor.name
}
}
// Todo: Figure out why the interface members aren't showing
// up (with or without modules applied), despite our apparently following
// http://usejsdoc.org/tags-interface.html#virtual-comments
/**
* An interface that all command objects must implement.
* @interface module:history.HistoryCommand
*/
/**
* Applies.
*
* @function module:history.HistoryCommand#apply
* @param {module:history.HistoryEventHandler} handler
* @fires module:history~Command#event:history
* @returns {void|true}
*/
/**
*
* Unapplies.
* @function module:history.HistoryCommand#unapply
* @param {module:history.HistoryEventHandler} handler
* @fires module:history~Command#event:history
* @returns {void|true}
*/
/**
* Returns the elements.
* @function module:history.HistoryCommand#elements
* @returns {Element[]}
*/
/**
* Gets the text.
* @function module:history.HistoryCommand#getText
* @returns {string}
*/
/**
* Gives the type.
* @function module:history.HistoryCommand.type
* @returns {string}
*/
/**
* @event module:history~Command#event:history
* @type {module:history.HistoryCommand}
*/
/**
* An interface for objects that will handle history events.
* @interface module:history.HistoryEventHandler
*/
/**
*
* @function module:history.HistoryEventHandler#handleHistoryEvent
* @param {string} eventType One of the HistoryEvent types
* @param {module:history~Command#event:history} command
* @listens module:history~Command#event:history
* @returns {void}
*
*/
/**
* History command for an element that had its DOM position changed.
* @implements {module:history.HistoryCommand}
*/
export class MoveElementCommand extends Command {
/**
* @param {Element} elem - The DOM element that was moved
* @param {Element} oldNextSibling - The element's next sibling before it was moved
* @param {Element} oldParent - The element's parent before it was moved
* @param {string} [text] - An optional string visible to user related to this change
*/
constructor (elem, oldNextSibling, oldParent, text) {
super()
this.elem = elem
this.text = text ? `Move ${elem.tagName} to ${text}` : `Move ${elem.tagName}`
this.oldNextSibling = oldNextSibling
this.oldParent = oldParent
this.newNextSibling = elem.nextSibling
this.newParent = elem.parentNode
}
/**
* Re-positions the element.
* @param {module:history.HistoryEventHandler} handler
* @fires module:history~Command#event:history
* @returns {void}
*/
apply (handler) {
super.apply(handler, () => {
const reference =
this.newNextSibling && this.newNextSibling.parentNode === this.newParent
? this.newNextSibling
: null
this.elem = this.newParent.insertBefore(this.elem, reference)
})
}
/**
* Positions the element back to its original location.
* @param {module:history.HistoryEventHandler} handler
* @fires module:history~Command#event:history
* @returns {void}
*/
unapply (handler) {
super.unapply(handler, () => {
const reference =
this.oldNextSibling && this.oldNextSibling.parentNode === this.oldParent
? this.oldNextSibling
: null
this.elem = this.oldParent.insertBefore(this.elem, reference)
})
}
}
/**
* History command for an element that was added to the DOM.
* @implements {module:history.HistoryCommand}
*/
export class InsertElementCommand extends Command {
/**
* @param {Element} elem - The newly added DOM element
* @param {string} text - An optional string visible to user related to this change
*/
constructor (elem, text) {
super()
this.elem = elem
this.text = text || `Create ${elem.tagName}`
this.parent = elem.parentNode
this.nextSibling = this.elem.nextSibling
}
/**
* Re-inserts the new element.
* @param {module:history.HistoryEventHandler} handler
* @fires module:history~Command#event:history
* @returns {void}
*/
apply (handler) {
super.apply(handler, () => {
const reference =
this.nextSibling && this.nextSibling.parentNode === this.parent
? this.nextSibling
: null
this.elem = this.parent.insertBefore(this.elem, reference)
})
}
/**
* Removes the element.
* @param {module:history.HistoryEventHandler} handler
* @fires module:history~Command#event:history
* @returns {void}
*/
unapply (handler) {
super.unapply(handler, () => {
this.parent = this.elem.parentNode
this.elem.remove()
})
}
}
/**
* History command for an element removed from the DOM.
* @implements {module:history.HistoryCommand}
*/
export class RemoveElementCommand extends Command {
/**
* @param {Element} elem - The removed DOM element
* @param {Node} oldNextSibling - The DOM element's nextSibling when it was in the DOM
* @param {Element} oldParent - The DOM element's parent
* @param {string} [text] - An optional string visible to user related to this change
*/
constructor (elem, oldNextSibling, oldParent, text) {
super()
this.elem = elem
this.text = text || `Delete ${elem.tagName}`
this.nextSibling = oldNextSibling
this.parent = oldParent
}
/**
* Re-removes the new element.
* @param {module:history.HistoryEventHandler} handler
* @fires module:history~Command#event:history
* @returns {void}
*/
apply (handler) {
super.apply(handler, () => {
this.parent = this.elem.parentNode
this.elem.remove()
})
}
/**
* Re-adds the new element.
* @param {module:history.HistoryEventHandler} handler
* @fires module:history~Command#event:history
* @returns {void}
*/
unapply (handler) {
super.unapply(handler, () => {
const reference =
this.nextSibling && this.nextSibling.parentNode === this.parent
? this.nextSibling
: null
this.parent.insertBefore(this.elem, reference) // Don't use `before` or `prepend` as `reference` may be `null`
})
}
}
/**
* @typedef {"#text"|"#href"|string} module:history.CommandAttributeName
*/
/**
* @typedef {PlainObject<module:history.CommandAttributeName, string>} module:history.CommandAttributes
*/
/**
* History command to make a change to an element.
* Usually an attribute change, but can also be textcontent.
* @implements {module:history.HistoryCommand}
*/
export class ChangeElementCommand extends Command {
/**
* @param {Element} elem - The DOM element that was changed
* @param {module:history.CommandAttributes} attrs - Attributes to be changed with the values they had *before* the change
* @param {string} text - An optional string visible to user related to this change
*/
constructor (elem, attrs, text) {
super()
this.elem = elem
this.text = text ? `Change ${elem.tagName} ${text}` : `Change ${elem.tagName}`
this.newValues = {}
this.oldValues = attrs
for (const attr in attrs) {
if (attr === '#text') {
this.newValues[attr] = (elem) ? elem.textContent : ''
} else if (attr === '#href') {
this.newValues[attr] = getHref(elem)
} else {
this.newValues[attr] = elem.getAttribute(attr)
}
}
}
/**
* Performs the stored change action.
* @param {module:history.HistoryEventHandler} handler
* @fires module:history~Command#event:history
* @returns {void}
*/
apply (handler) {
super.apply(handler, () => {
let bChangedTransform = false
Object.entries(this.newValues).forEach(([attr, value]) => {
const isNullishOrEmpty = value === null || value === undefined || value === ''
if (attr === '#text') {
this.elem.textContent = value === null || value === undefined ? '' : String(value)
} else if (attr === '#href') {
if (isNullishOrEmpty) {
this.elem.removeAttribute('href')
this.elem.removeAttributeNS(NS.XLINK, 'href')
} else {
setHref(this.elem, String(value))
}
} else if (isNullishOrEmpty) {
this.elem.setAttribute(attr, '')
this.elem.removeAttribute(attr)
} else {
this.elem.setAttribute(attr, value)
}
if (attr === 'transform') { bChangedTransform = true }
})
// relocate rotational transform, if necessary
if (!bChangedTransform) {
relocateRotationCenter(this.elem, Object.keys(this.newValues))
}
})
}
/**
* Reverses the stored change action.
* @param {module:history.HistoryEventHandler} handler
* @fires module:history~Command#event:history
* @returns {void}
*/
unapply (handler) {
super.unapply(handler, () => {
let bChangedTransform = false
Object.entries(this.oldValues).forEach(([attr, value]) => {
const isNullishOrEmpty = value === null || value === undefined || value === ''
if (attr === '#text') {
this.elem.textContent = value === null || value === undefined ? '' : String(value)
} else if (attr === '#href') {
if (isNullishOrEmpty) {
this.elem.removeAttribute('href')
this.elem.removeAttributeNS(NS.XLINK, 'href')
} else {
setHref(this.elem, String(value))
}
} else if (isNullishOrEmpty) {
this.elem.removeAttribute(attr)
} else {
this.elem.setAttribute(attr, value)
}
if (attr === 'transform') { bChangedTransform = true }
})
// relocate rotational transform, if necessary
if (!bChangedTransform) {
relocateRotationCenter(this.elem, Object.keys(this.oldValues))
}
})
}
}
// TODO: create a 'typing' command object that tracks changes in text
// if a new Typing command is created and the top command on the stack is also a Typing
// and they both affect the same element, then collapse the two commands into one
/**
* History command that can contain/execute multiple other commands.
* @implements {module:history.HistoryCommand}
*/
export class BatchCommand extends Command {
/**
* @param {string} [text] - An optional string visible to user related to this change
*/
constructor (text) {
super()
this.text = text || 'Batch Command'
this.stack = []
}
/**
* Runs "apply" on all subcommands.
* @param {module:history.HistoryEventHandler} handler
* @fires module:history~Command#event:history
* @returns {void}
*/
apply (handler) {
super.apply(handler, () => {
this.stack.forEach((stackItem) => {
console.assert(stackItem, 'stack item should not be null')
stackItem && stackItem.apply(handler)
})
})
}
/**
* Runs "unapply" on all subcommands.
* @param {module:history.HistoryEventHandler} handler
* @fires module:history~Command#event:history
* @returns {void}
*/
unapply (handler) {
super.unapply(handler, () => {
[...this.stack].reverse().forEach((stackItem) => {
console.assert(stackItem, 'stack item should not be null')
stackItem && stackItem.unapply(handler)
})
})
}
/**
* Iterate through all our subcommands.
* @returns {Element[]} All the elements we are changing
*/
elements () {
const elems = []
let cmd = this.stack.length
while (cmd--) {
if (!this.stack[cmd]) continue
const thisElems = this.stack[cmd].elements()
let elem = thisElems.length
while (elem--) {
if (!elems.includes(thisElems[elem])) { elems.push(thisElems[elem]) }
}
}
return elems
}
/**
* Adds a given command to the history stack.
* @param {Command} cmd - The undo command object to add
* @returns {void}
*/
addSubCommand (cmd) {
console.assert(cmd !== null, 'cmd should not be null')
this.stack.push(cmd)
}
/**
* @returns {boolean} Indicates whether or not the batch command is empty
*/
isEmpty () {
return !this.stack.length
}
}
/**
*
*/
export class UndoManager {
/**
* @param {module:history.HistoryEventHandler} historyEventHandler
*/
constructor (historyEventHandler) {
this.handler_ = historyEventHandler || null
this.undoStackPointer = 0
this.undoStack = []
// this is the stack that stores the original values, the elements and
// the attribute name for begin/finish
this.undoChangeStackPointer = -1
this.undoableChangeStack = []
}
/**
* Resets the undo stack, effectively clearing the undo/redo history.
* @returns {void}
*/
resetUndoStack () {
this.undoStack = []
this.undoStackPointer = 0
}
/**
* @returns {Integer} Current size of the undo history stack
*/
getUndoStackSize () {
return this.undoStackPointer
}
/**
* @returns {Integer} Current size of the redo history stack
*/
getRedoStackSize () {
return this.undoStack.length - this.undoStackPointer
}
/**
* @returns {string} String associated with the next undo command
*/
getNextUndoCommandText () {
return this.undoStackPointer > 0 ? this.undoStack[this.undoStackPointer - 1].getText() : ''
}
/**
* @returns {string} String associated with the next redo command
*/
getNextRedoCommandText () {
return this.undoStackPointer < this.undoStack.length ? this.undoStack[this.undoStackPointer].getText() : ''
}
/**
* Performs an undo step.
* @returns {void}
*/
undo () {
if (this.undoStackPointer > 0) {
const cmd = this.undoStack[--this.undoStackPointer]
cmd.unapply(this.handler_)
}
}
/**
* Performs a redo step.
* @returns {void}
*/
redo () {
if (this.undoStackPointer < this.undoStack.length && this.undoStack.length > 0) {
const cmd = this.undoStack[this.undoStackPointer++]
cmd.apply(this.handler_)
}
}
/**
* Adds a command object to the undo history stack.
* @param {Command} cmd - The command object to add
* @returns {void}
*/
addCommandToHistory (cmd) {
// TODO: we MUST compress consecutive text changes to the same element
// (right now each keystroke is saved as a separate command that includes the
// entire text contents of the text element)
// TODO: consider limiting the history that we store here (need to do some slicing)
// if our stack pointer is not at the end, then we have to remove
// all commands after the pointer and insert the new command
if (this.undoStackPointer < this.undoStack.length && this.undoStack.length > 0) {
this.undoStack = this.undoStack.splice(0, this.undoStackPointer)
}
this.undoStack.push(cmd)
this.undoStackPointer = this.undoStack.length
}
/**
* This function tells the canvas to remember the old values of the
* `attrName` attribute for each element sent in. The elements and values
* are stored on a stack, so the next call to `finishUndoableChange()` will
* pop the elements and old values off the stack, gets the current values
* from the DOM and uses all of these to construct the undo-able command.
* @param {string} attrName - The name of the attribute being changed
* @param {Element[]} elems - Array of DOM elements being changed
* @returns {void}
*/
beginUndoableChange (attrName, elems) {
const p = ++this.undoChangeStackPointer
let i = elems.length
const oldValues = new Array(i); const elements = new Array(i)
while (i--) {
const elem = elems[i]
if (!elem) { continue }
elements[i] = elem
oldValues[i] = elem.getAttribute(attrName)
}
this.undoableChangeStack[p] = {
attrName,
oldValues,
elements
}
}
/**
* This function returns a `BatchCommand` object which summarizes the
* change since `beginUndoableChange` was called. The command can then
* be added to the command history.
* @returns {BatchCommand} Batch command object with resulting changes
*/
finishUndoableChange () {
const p = this.undoChangeStackPointer--
const changeset = this.undoableChangeStack[p]
const { attrName } = changeset
const batchCmd = new BatchCommand(`Change ${attrName}`)
let i = changeset.elements.length
while (i--) {
const elem = changeset.elements[i]
if (!elem) { continue }
const changes = {}
changes[attrName] = changeset.oldValues[i]
if (changes[attrName] !== elem.getAttribute(attrName)) {
batchCmd.addSubCommand(new ChangeElementCommand(elem, changes, attrName))
}
}
this.undoableChangeStack[p] = null
return batchCmd
}
}