Files
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

309 lines
12 KiB
JavaScript

/**
* Tools for undo.
* @module undo
* @license MIT
* @copyright 2011 Jeff Schiller
*/
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'
import {
isGecko
} from '../common/browser.js'
import {
transformPoint, transformListToTransform, getTransformList
} from './math.js'
const {
UndoManager, HistoryEventTypes
} = hstry
let svgCanvas = null
/**
* @function module:undo.init
* @param {module:undo.undoContext} undoContext
* @returns {void}
*/
export const init = (canvas) => {
svgCanvas = canvas
canvas.undoMgr = getUndoManager()
}
export const getUndoManager = () => {
return new UndoManager({
/**
* @param {string} eventType One of the HistoryEvent types
* @param {module:history.HistoryCommand} cmd Fulfills the HistoryCommand interface
* @fires module:undo.SvgCanvas#event:changed
* @returns {void}
*/
handleHistoryEvent (eventType, cmd) {
const EventTypes = HistoryEventTypes
// TODO: handle setBlurOffsets.
if (eventType === EventTypes.BEFORE_UNAPPLY || eventType === EventTypes.BEFORE_APPLY) {
svgCanvas.clearSelection()
} else if (eventType === EventTypes.AFTER_APPLY || eventType === EventTypes.AFTER_UNAPPLY) {
const cmdType = cmd.type()
const isApply = (eventType === EventTypes.AFTER_APPLY)
if (cmdType === 'ChangeElementCommand' && cmd.elem === svgCanvas.getSvgContent()) {
const values = isApply ? cmd.newValues : cmd.oldValues
if (values.width !== null && values.width !== undefined && values.width !== '') {
const newContentW = Number(values.width)
if (Number.isFinite(newContentW) && newContentW > 0) {
svgCanvas.contentW = newContentW
}
}
if (values.height !== null && values.height !== undefined && values.height !== '') {
const newContentH = Number(values.height)
if (Number.isFinite(newContentH) && newContentH > 0) {
svgCanvas.contentH = newContentH
}
}
}
const elems = cmd.elements()
svgCanvas.pathActions.clear()
svgCanvas.call('changed', elems)
if (cmdType === 'MoveElementCommand') {
const parent = isApply ? cmd.newParent : cmd.oldParent
if (parent === svgCanvas.getSvgContent()) {
draw.identifyLayers()
}
} else if (cmdType === 'InsertElementCommand' || cmdType === 'RemoveElementCommand') {
if (cmd.parent === svgCanvas.getSvgContent()) {
draw.identifyLayers()
}
if (cmdType === 'InsertElementCommand') {
if (isApply) {
svgCanvas.restoreRefElements(cmd.elem)
}
} else if (!isApply) {
svgCanvas.restoreRefElements(cmd.elem)
}
if (cmd.elem?.tagName === 'use') {
svgCanvas.setUseData(cmd.elem)
}
} else if (cmdType === 'ChangeElementCommand') {
// if we are changing layer names, re-identify all layers
if (cmd.elem.tagName === 'title' &&
cmd.elem.parentNode.parentNode === svgCanvas.getSvgContent()
) {
draw.identifyLayers()
}
const values = isApply ? cmd.newValues : cmd.oldValues
// If stdDeviation was changed, update the blur.
if (values.stdDeviation) {
svgCanvas.setBlurOffsets(cmd.elem.parentNode, values.stdDeviation)
}
if (cmd.elem.tagName === 'text') {
const [dx, dy] = [cmd.newValues.x - cmd.oldValues.x,
cmd.newValues.y - cmd.oldValues.y]
const tspans = cmd.elem.children
for (let i = 0; i < tspans.length; i++) {
let x = Number(tspans[i].getAttribute('x'))
let y = Number(tspans[i].getAttribute('y'))
const unapply = (eventType === EventTypes.AFTER_UNAPPLY)
x = unapply ? x - dx : x + dx
y = unapply ? y - dy : y + dy
tspans[i].setAttribute('x', x)
tspans[i].setAttribute('y', y)
}
}
}
}
}
})
}
/**
* Hack for Firefox bugs where text element features aren't updated or get
* messed up. See issue 136 and issue 137.
* This function clones the element and re-selects it.
* @function module:svgcanvas~ffClone
* @todo Test for this bug on load and add it to "support" object instead of
* browser sniffing
* @param {Element} elem - The (text) DOM element to clone
* @returns {Element} Cloned element
*/
export const ffClone = (elem) => {
if (!isGecko()) { return elem }
const clone = elem.cloneNode(true)
elem.before(clone)
elem.remove()
svgCanvas.selectorManager.releaseSelector(elem)
svgCanvas.setSelectedElements(0, clone)
svgCanvas.selectorManager.requestSelector(clone).showGrips(true)
return clone
}
/**
* This function makes the changes to the elements. It does not add the change
* to the history stack.
* @param {string} attr - Attribute name
* @param {string|Float} newValue - String or number with the new attribute value
* @param {Element[]} elems - The DOM elements to apply the change to
* @returns {void}
*/
export const changeSelectedAttributeNoUndoMethod = (attr, newValue, elems) => {
if (attr === 'id') {
// if the user is changing the id, then de-select the element first
// change the ID, then re-select it with the new ID
// as this change can impact other extensions, a 'renamedElement' event is thrown
const elem = elems[0]
const oldId = elem.id
if (oldId !== newValue) {
svgCanvas.clearSelection()
elem.id = newValue
svgCanvas.addToSelection([elem], true)
svgCanvas.call('elementRenamed', { elem, oldId, newId: newValue })
}
return
}
const selectedElements = svgCanvas.getSelectedElements()
const zoom = svgCanvas.getZoom()
if (svgCanvas.getCurrentMode() === 'pathedit') {
// Editing node
svgCanvas.pathActions.moveNode(attr, newValue)
}
elems = elems ?? selectedElements
let i = elems.length
const noXYElems = ['g', 'polyline', 'path']
while (i--) {
let elem = elems[i]
if (!elem) { continue }
// Set x,y vals on elements that don't have them
if ((attr === 'x' || attr === 'y') && noXYElems.includes(elem.tagName)) {
const bbox = getStrokedBBoxDefaultVisible([elem])
const diffX = attr === 'x' ? parseFloat(newValue) - bbox.x : 0
const diffY = attr === 'y' ? parseFloat(newValue) - bbox.y : 0
svgCanvas.moveSelectedElements(diffX * zoom, diffY * zoom, true)
continue
}
let oldval = attr === '#text' ? elem.textContent : elem.getAttribute(attr)
if (!oldval) { oldval = '' }
if (oldval !== String(newValue)) {
if (attr === '#text') {
// const oldW = utilsGetBBox(elem).width;
elem.textContent = newValue
// FF bug occurs on on rotated elements
if ((/rotate/).test(elem.getAttribute('transform'))) {
elem = ffClone(elem)
}
// Hoped to solve the issue of moving text with text-anchor="start",
// but this doesn't actually fix it. Hopefully on the right track, though. -Fyrd
} else if (attr === '#href') {
setHref(elem, newValue)
} else if (newValue) {
elem.setAttribute(attr, isNaN(parseFloat(newValue)) ? newValue : parseFloat(newValue))
} else if (typeof newValue === 'number') {
elem.setAttribute(attr, newValue)
} else {
elem.removeAttribute(attr)
}
// Go into "select" mode for text changes
// NOTE: Important that this happens AFTER elem.setAttribute() or else attributes like
// font-size can get reset to their old value, ultimately by svgEditor.updateContextPanel(),
// after calling textActions.toSelectMode() below
if (svgCanvas.getCurrentMode() === 'textedit' && attr !== '#text' && elem.textContent.length) {
svgCanvas.textActions.toSelectMode(elem)
}
// Use the Firefox ffClone hack for text elements with gradients or
// where other text attributes are changed.
if (isGecko() &&
elem.nodeName === 'text' &&
(/rotate/).test(elem.getAttribute('transform')) &&
(String(newValue).startsWith('url') || (['font-size', 'font-family', 'x', 'y'].includes(attr) && elem.textContent))) {
elem = ffClone(elem)
}
// Timeout needed for Opera & Firefox
// codedread: it is now possible for this to be called with elements
// that are not in the selectedElements array, we need to only request a
// selector if the element is in that array
if (selectedElements.includes(elem)) {
setTimeout(function () {
// Due to element replacement, this element may no longer
// be part of the DOM
if (!elem.parentNode) { return }
svgCanvas.selectorManager.requestSelector(elem).resize()
}, 0)
}
// 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' && BBOX_AFFECTING_ATTRS.has(attr)) {
const tlist = getTransformList(elem)
let n = tlist.numberOfItems
while (n--) {
const xform = tlist.getItem(n)
if (xform.type === 4) {
// 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)
// 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, centerMatrix
)
const newrot = svgCanvas.getSvgRoot().createSVGTransform()
newrot.setRotate(angle, center.x, center.y)
tlist.insertItemBefore(newrot, n)
break
}
}
}
} // if oldValue != newValue
} // for each elem
}
/**
* Change the given/selected element and add the original value to the history stack.
* If you want to change all `selectedElements`, ignore the `elems` argument.
* If you want to change only a subset of `selectedElements`, then send the
* subset to this function in the `elems` argument.
* @function module:svgcanvas.SvgCanvas#changeSelectedAttribute
* @param {string} attr - String with the attribute name
* @param {string|Float} val - String or number with the new attribute value
* @param {Element[]} elems - The DOM elements to apply the change to
* @returns {void}
*/
export const changeSelectedAttributeMethod = (attr, val, elems) => {
const selectedElements = svgCanvas.getSelectedElements()
elems = elems || selectedElements
svgCanvas.undoMgr.beginUndoableChange(attr, elems)
changeSelectedAttributeNoUndoMethod(attr, val, elems)
const batchCmd = svgCanvas.undoMgr.finishUndoableChange()
if (!batchCmd.isEmpty()) {
// svgCanvas.addCommandToHistory(batchCmd);
svgCanvas.undoMgr.addCommandToHistory(batchCmd)
}
}