173dd2607a
* 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>
296 lines
16 KiB
HTML
296 lines
16 KiB
HTML
<!DOCTYPE html>
|
|
<html lang="en">
|
|
<head>
|
|
<meta charset="UTF-8">
|
|
<title>SVG-Edit Bug: Rotation Recalculation Corrupts Compound Transforms</title>
|
|
<style>
|
|
* { margin: 0; padding: 0; box-sizing: border-box; }
|
|
body {
|
|
font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', system-ui, sans-serif;
|
|
background: #0d1117;
|
|
color: #c9d1d9;
|
|
padding: 32px 40px;
|
|
}
|
|
h1 {
|
|
font-size: 22px;
|
|
font-weight: 600;
|
|
margin-bottom: 6px;
|
|
color: #e6edf3;
|
|
}
|
|
.subtitle {
|
|
font-size: 14px;
|
|
color: #8b949e;
|
|
margin-bottom: 28px;
|
|
max-width: 800px;
|
|
line-height: 1.5;
|
|
}
|
|
.section-title {
|
|
font-size: 16px;
|
|
font-weight: 600;
|
|
margin: 24px 0 14px;
|
|
padding-bottom: 6px;
|
|
border-bottom: 1px solid #21262d;
|
|
}
|
|
.bug1-title { color: #f0883e; }
|
|
.bug2-title { color: #bc8cff; }
|
|
.panels {
|
|
display: flex;
|
|
gap: 20px;
|
|
margin-bottom: 8px;
|
|
}
|
|
.panel {
|
|
flex: 1;
|
|
background: #161b22;
|
|
border: 1px solid #30363d;
|
|
border-radius: 8px;
|
|
padding: 16px;
|
|
}
|
|
.panel h3 {
|
|
font-size: 13px;
|
|
font-weight: 600;
|
|
margin-bottom: 4px;
|
|
display: flex;
|
|
align-items: center;
|
|
gap: 6px;
|
|
}
|
|
.panel .desc {
|
|
font-size: 11px;
|
|
color: #8b949e;
|
|
margin-bottom: 10px;
|
|
line-height: 1.4;
|
|
}
|
|
.badge {
|
|
display: inline-block;
|
|
font-size: 10px;
|
|
font-weight: 600;
|
|
padding: 1px 6px;
|
|
border-radius: 10px;
|
|
text-transform: uppercase;
|
|
letter-spacing: 0.5px;
|
|
}
|
|
.badge-original { background: #1f6feb33; color: #58a6ff; border: 1px solid #1f6feb; }
|
|
.badge-bug { background: #f8514933; color: #ff7b72; border: 1px solid #f85149; }
|
|
.badge-fixed { background: #23883433; color: #3fb950; border: 1px solid #238834; }
|
|
svg {
|
|
display: block;
|
|
background: #0d1117;
|
|
border-radius: 6px;
|
|
border: 1px solid #21262d;
|
|
}
|
|
.caption {
|
|
font-size: 11px;
|
|
color: #8b949e;
|
|
margin-top: 6px;
|
|
line-height: 1.4;
|
|
}
|
|
.code-block {
|
|
background: #161b22;
|
|
border: 1px solid #30363d;
|
|
border-radius: 6px;
|
|
padding: 10px 14px;
|
|
font-family: 'SF Mono', 'Fira Code', Consolas, monospace;
|
|
font-size: 11px;
|
|
line-height: 1.6;
|
|
color: #c9d1d9;
|
|
margin: 10px 0;
|
|
white-space: pre;
|
|
overflow-x: auto;
|
|
}
|
|
.code-comment { color: #8b949e; }
|
|
.code-bad { color: #ff7b72; }
|
|
.code-good { color: #3fb950; }
|
|
.arrow-label {
|
|
font-size: 28px;
|
|
align-self: center;
|
|
color: #484f58;
|
|
flex-shrink: 0;
|
|
width: 30px;
|
|
text-align: center;
|
|
}
|
|
</style>
|
|
</head>
|
|
<body>
|
|
|
|
<h1>Bug: Rotation Recalculation Corrupts Compound Transforms</h1>
|
|
<p class="subtitle">
|
|
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).
|
|
</p>
|
|
|
|
<!-- ======================= BUG 1: history.js ======================= -->
|
|
<div class="section-title bug1-title">Bug 1: Non-geometric attribute change destroys compound transforms (history.js)</div>
|
|
|
|
<p class="desc" style="margin-bottom: 14px; font-size: 12px; line-height: 1.5;">
|
|
When undoing/redoing a <code>stroke-width</code> change, the old code replaces the
|
|
<b>entire</b> transform attribute with just <code>rotate(...)</code>, destroying any
|
|
<code>translate()</code> or <code>scale()</code> transforms. Characters positioned via
|
|
compound transforms collapse to the SVG origin.
|
|
</p>
|
|
|
|
<div class="panels">
|
|
<!-- Original -->
|
|
<div class="panel">
|
|
<h3><span class="badge badge-original">Original</span> Characters on curve</h3>
|
|
<p class="desc">Each character has <code>translate(x,y) rotate(angle)</code></p>
|
|
<svg width="540" height="200" viewBox="0 0 540 200">
|
|
<!-- Curve guide -->
|
|
<path d="M 60,155 Q 270,75 480,155" fill="none" stroke="#1f6feb33" stroke-width="1" stroke-dasharray="4 3"/>
|
|
<!-- Characters with compound transforms: translate + rotate -->
|
|
<text transform="translate(80, 155) rotate(-15)" font-size="64" font-family="Georgia, serif" fill="#58a6ff" stroke="#1f6feb" stroke-width="2" paint-order="stroke" text-anchor="middle" dominant-baseline="alphabetic">S</text>
|
|
<text transform="translate(170, 132) rotate(-8)" font-size="64" font-family="Georgia, serif" fill="#58a6ff" stroke="#1f6feb" stroke-width="2" paint-order="stroke" text-anchor="middle" dominant-baseline="alphabetic">T</text>
|
|
<text transform="translate(260, 118) rotate(0)" font-size="64" font-family="Georgia, serif" fill="#58a6ff" stroke="#1f6feb" stroke-width="2" paint-order="stroke" text-anchor="middle" dominant-baseline="alphabetic">O</text>
|
|
<text transform="translate(350, 132) rotate(8)" font-size="64" font-family="Georgia, serif" fill="#58a6ff" stroke="#1f6feb" stroke-width="2" paint-order="stroke" text-anchor="middle" dominant-baseline="alphabetic">R</text>
|
|
<text transform="translate(450, 155) rotate(15)" font-size="64" font-family="Georgia, serif" fill="#58a6ff" stroke="#1f6feb" stroke-width="2" paint-order="stroke" text-anchor="middle" dominant-baseline="alphabetic">Y</text>
|
|
</svg>
|
|
<p class="caption">stroke-width: 2</p>
|
|
</div>
|
|
|
|
<div class="arrow-label">→</div>
|
|
|
|
<!-- Bug: old code -->
|
|
<div class="panel" style="border-color: #f85149;">
|
|
<h3><span class="badge badge-bug">Bug</span> After undo of stroke-width change</h3>
|
|
<p class="desc">Old code replaces transform with just <code>rotate()</code> — translate destroyed</p>
|
|
<svg width="540" height="200" viewBox="0 0 540 200">
|
|
<path d="M 60,155 Q 270,75 480,155" fill="none" stroke="#1f6feb33" stroke-width="1" stroke-dasharray="4 3"/>
|
|
<!--
|
|
Old history.js code does:
|
|
const bbox = getBBox(elem) // e.g. {x:-15, y:-50, width:35, height:55}
|
|
const cx = bbox.x + bbox.width / 2 // ~2.5
|
|
const cy = bbox.y + bbox.height / 2 // ~-22.5
|
|
elem.setAttribute('transform', `rotate(${angle}, ${cx}, ${cy})`)
|
|
This REPLACES the entire transform, destroying the translate.
|
|
All characters collapse to near the origin.
|
|
-->
|
|
<text transform="rotate(-15, 0, -20)" font-size="64" font-family="Georgia, serif" fill="#ff7b72" stroke="#f85149" stroke-width="2" paint-order="stroke" text-anchor="middle" dominant-baseline="alphabetic" opacity="0.8">S</text>
|
|
<text transform="rotate(-8, 0, -20)" font-size="64" font-family="Georgia, serif" fill="#ff7b72" stroke="#f85149" stroke-width="2" paint-order="stroke" text-anchor="middle" dominant-baseline="alphabetic" opacity="0.8">T</text>
|
|
<text transform="rotate(0, 0, -20)" font-size="64" font-family="Georgia, serif" fill="#ff7b72" stroke="#f85149" stroke-width="2" paint-order="stroke" text-anchor="middle" dominant-baseline="alphabetic" opacity="0.8">O</text>
|
|
<text transform="rotate(8, 0, -20)" font-size="64" font-family="Georgia, serif" fill="#ff7b72" stroke="#f85149" stroke-width="2" paint-order="stroke" text-anchor="middle" dominant-baseline="alphabetic" opacity="0.8">R</text>
|
|
<text transform="rotate(15, 0, -20)" font-size="64" font-family="Georgia, serif" fill="#ff7b72" stroke="#f85149" stroke-width="2" paint-order="stroke" text-anchor="middle" dominant-baseline="alphabetic" opacity="0.8">Y</text>
|
|
<!-- Ghost showing where they should be -->
|
|
<text transform="translate(80, 155) rotate(-15)" font-size="64" font-family="Georgia, serif" fill="none" stroke="#484f58" stroke-width="1" stroke-dasharray="3 2" text-anchor="middle" dominant-baseline="alphabetic">S</text>
|
|
<text transform="translate(170, 132) rotate(-8)" font-size="64" font-family="Georgia, serif" fill="none" stroke="#484f58" stroke-width="1" stroke-dasharray="3 2" text-anchor="middle" dominant-baseline="alphabetic">T</text>
|
|
<text transform="translate(260, 118) rotate(0)" font-size="64" font-family="Georgia, serif" fill="none" stroke="#484f58" stroke-width="1" stroke-dasharray="3 2" text-anchor="middle" dominant-baseline="alphabetic">O</text>
|
|
<text transform="translate(350, 132) rotate(8)" font-size="64" font-family="Georgia, serif" fill="none" stroke="#484f58" stroke-width="1" stroke-dasharray="3 2" text-anchor="middle" dominant-baseline="alphabetic">R</text>
|
|
<text transform="translate(450, 155) rotate(15)" font-size="64" font-family="Georgia, serif" fill="none" stroke="#484f58" stroke-width="1" stroke-dasharray="3 2" text-anchor="middle" dominant-baseline="alphabetic">Y</text>
|
|
</svg>
|
|
<p class="caption" style="color: #ff7b72;">Characters collapse to origin — translate() destroyed</p>
|
|
</div>
|
|
|
|
<div class="arrow-label" style="visibility: hidden;">→</div>
|
|
|
|
<!-- Fixed -->
|
|
<div class="panel" style="border-color: #238834;">
|
|
<h3><span class="badge badge-fixed">Fixed</span> After undo of stroke-width change</h3>
|
|
<p class="desc">New code skips recalculation for non-geometric attrs — transform untouched</p>
|
|
<svg width="540" height="200" viewBox="0 0 540 200">
|
|
<path d="M 60,155 Q 270,75 480,155" fill="none" stroke="#1f6feb33" stroke-width="1" stroke-dasharray="4 3"/>
|
|
<text transform="translate(80, 155) rotate(-15)" font-size="64" font-family="Georgia, serif" fill="#3fb950" stroke="#238834" stroke-width="2" paint-order="stroke" text-anchor="middle" dominant-baseline="alphabetic">S</text>
|
|
<text transform="translate(170, 132) rotate(-8)" font-size="64" font-family="Georgia, serif" fill="#3fb950" stroke="#238834" stroke-width="2" paint-order="stroke" text-anchor="middle" dominant-baseline="alphabetic">T</text>
|
|
<text transform="translate(260, 118) rotate(0)" font-size="64" font-family="Georgia, serif" fill="#3fb950" stroke="#238834" stroke-width="2" paint-order="stroke" text-anchor="middle" dominant-baseline="alphabetic">O</text>
|
|
<text transform="translate(350, 132) rotate(8)" font-size="64" font-family="Georgia, serif" fill="#3fb950" stroke="#238834" stroke-width="2" paint-order="stroke" text-anchor="middle" dominant-baseline="alphabetic">R</text>
|
|
<text transform="translate(450, 155) rotate(15)" font-size="64" font-family="Georgia, serif" fill="#3fb950" stroke="#238834" stroke-width="2" paint-order="stroke" text-anchor="middle" dominant-baseline="alphabetic">Y</text>
|
|
</svg>
|
|
<p class="caption" style="color: #3fb950;">Characters unchanged — non-geometric attrs skip recalculation</p>
|
|
</div>
|
|
</div>
|
|
|
|
<div class="code-block"><span class="code-comment">// history.js — old code (apply/unapply):</span>
|
|
<span class="code-bad">const rotate = `rotate(${angle}, ${cx}, ${cy})`
|
|
this.elem.setAttribute('transform', rotate) // replaces ENTIRE transform!</span>
|
|
|
|
<span class="code-comment">// history.js — new code:</span>
|
|
<span class="code-good">if (!BBOX_AFFECTING_ATTRS.has(attr)) return // skip for stroke-width, fill, etc.
|
|
// When needed, update only the rotation entry via transform list API</span></div>
|
|
|
|
|
|
<!-- ======================= BUG 2: undo.js ======================= -->
|
|
<div class="section-title bug2-title">Bug 2: Pre-rotation translate leaks into rotation center (undo.js)</div>
|
|
|
|
<p class="desc" style="margin-bottom: 14px; font-size: 12px; line-height: 1.5;">
|
|
When a geometric attribute (e.g. <code>width</code>) changes on an element with
|
|
<code>translate(tx,ty) rotate(angle)</code>, the rotation center should be computed
|
|
from the bbox center alone. The old code transforms the center through <b>all</b> remaining
|
|
transforms (including the pre-rotation translate), shifting the rotation center by (tx, ty).
|
|
</p>
|
|
|
|
<div class="panels">
|
|
<!-- Original -->
|
|
<div class="panel">
|
|
<h3><span class="badge badge-original">Original</span> Rect with translate + rotate</h3>
|
|
<p class="desc"><code>translate(200,60) rotate(25, 50, 40)</code> on 100×80 rect</p>
|
|
<svg width="540" height="200" viewBox="0 0 540 200">
|
|
<!-- Grid reference -->
|
|
<line x1="200" y1="0" x2="200" y2="200" stroke="#21262d" stroke-width="0.5"/>
|
|
<line x1="0" y1="60" x2="540" y2="60" stroke="#21262d" stroke-width="0.5"/>
|
|
<!-- Crosshair at translate target -->
|
|
<circle cx="200" cy="60" r="3" fill="none" stroke="#484f58" stroke-width="1"/>
|
|
<text x="208" y="55" font-size="10" fill="#484f58" font-family="system-ui">translate(200, 60)</text>
|
|
<!-- The rect -->
|
|
<g transform="translate(200, 60)">
|
|
<rect transform="rotate(25, 50, 40)" x="0" y="0" width="100" height="80" fill="#1f6feb33" stroke="#58a6ff" stroke-width="2" rx="3"/>
|
|
<!-- Center marker -->
|
|
<circle transform="rotate(25, 50, 40)" cx="50" cy="40" r="4" fill="#58a6ff"/>
|
|
<text transform="rotate(25, 50, 40)" x="60" y="38" font-size="10" fill="#58a6ff" font-family="system-ui">center (50, 40)</text>
|
|
</g>
|
|
</svg>
|
|
</div>
|
|
|
|
<div class="arrow-label">→</div>
|
|
|
|
<!-- Bug: old code -->
|
|
<div class="panel" style="border-color: #f85149;">
|
|
<h3><span class="badge badge-bug">Bug</span> After width change: center shifted</h3>
|
|
<p class="desc">Old code: center through all transforms → <code>rotate(25, 260, 100)</code></p>
|
|
<svg width="540" height="200" viewBox="0 0 540 200">
|
|
<line x1="200" y1="0" x2="200" y2="200" stroke="#21262d" stroke-width="0.5"/>
|
|
<line x1="0" y1="60" x2="540" y2="60" stroke="#21262d" stroke-width="0.5"/>
|
|
<circle cx="200" cy="60" r="3" fill="none" stroke="#484f58" stroke-width="1"/>
|
|
<!-- Ghost of correct position -->
|
|
<g transform="translate(200, 60)">
|
|
<rect transform="rotate(25, 60, 40)" x="0" y="0" width="120" height="80" fill="none" stroke="#484f58" stroke-width="1" stroke-dasharray="3 2" rx="3"/>
|
|
</g>
|
|
<!-- Bug: translate(200,60) leaks into center → rotate(25, 60+200, 40+60) = rotate(25, 260, 100) -->
|
|
<g transform="translate(200, 60)">
|
|
<rect transform="rotate(25, 260, 100)" x="0" y="0" width="120" height="80" fill="#f8514933" stroke="#ff7b72" stroke-width="2" rx="3"/>
|
|
<!-- Wrong center marker -->
|
|
<circle cx="260" cy="100" r="4" fill="#ff7b72"/>
|
|
<text x="270" y="98" font-size="10" fill="#ff7b72" font-family="system-ui">wrong! (260, 100)</text>
|
|
</g>
|
|
</svg>
|
|
<p class="caption" style="color: #ff7b72;">translate(200,60) leaks into rotation center calculation</p>
|
|
</div>
|
|
|
|
<div class="arrow-label" style="visibility: hidden;">→</div>
|
|
|
|
<!-- Fixed -->
|
|
<div class="panel" style="border-color: #238834;">
|
|
<h3><span class="badge badge-fixed">Fixed</span> After width change: center correct</h3>
|
|
<p class="desc">New code: center through post-rotation transforms only → <code>rotate(25, 60, 40)</code></p>
|
|
<svg width="540" height="200" viewBox="0 0 540 200">
|
|
<line x1="200" y1="0" x2="200" y2="200" stroke="#21262d" stroke-width="0.5"/>
|
|
<line x1="0" y1="60" x2="540" y2="60" stroke="#21262d" stroke-width="0.5"/>
|
|
<circle cx="200" cy="60" r="3" fill="none" stroke="#484f58" stroke-width="1"/>
|
|
<text x="208" y="55" font-size="10" fill="#484f58" font-family="system-ui">translate(200, 60)</text>
|
|
<g transform="translate(200, 60)">
|
|
<rect transform="rotate(25, 60, 40)" x="0" y="0" width="120" height="80" fill="#23883433" stroke="#3fb950" stroke-width="2" rx="3"/>
|
|
<!-- Correct center marker -->
|
|
<circle transform="rotate(25, 60, 40)" cx="60" cy="40" r="4" fill="#3fb950"/>
|
|
<text transform="rotate(25, 60, 40)" x="70" y="38" font-size="10" fill="#3fb950" font-family="system-ui">correct (60, 40)</text>
|
|
</g>
|
|
</svg>
|
|
<p class="caption" style="color: #3fb950;">Center computed from bbox only, translate excluded</p>
|
|
</div>
|
|
</div>
|
|
|
|
<div class="code-block"><span class="code-comment">// undo.js — old code (center through ALL remaining transforms):</span>
|
|
<span class="code-bad">center = transformPoint(bcx, bcy, transformListToTransform(tlist).matrix)
|
|
// For [translate(200,60), rotate(25)]: includes translate → center = (bcx+200, bcy+60)</span>
|
|
|
|
<span class="code-comment">// undo.js — new code (center through only POST-rotation transforms):</span>
|
|
<span class="code-good">centerMatrix = transformListToTransform(tlist, n, tlist.numberOfItems - 1).matrix
|
|
// For [translate(200,60)]: post-rotation = identity → center = (bcx, bcy)</span></div>
|
|
|
|
</body>
|
|
</html>
|