fix(text-actions): Calculate accumulated transform matrix for text inside groups#1082
Merged
Merged
Conversation
…side groups When editing text inside a group element, the text cursor was appearing in the wrong position because only the text element's own transform was being considered, ignoring transforms from parent groups. This change: - Adds a new #getAccumulatedMatrix() method that traverses up the DOM tree from the text element to the SVG content element, collecting and multiplying all transform matrices along the way - Updates the init() method to use this accumulated matrix instead of just the text element's transform - Updates test mock to include getSvgContent() method Fixes the issue where editing text inside a transformed group would show the cursor at the wrong position. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Reviewer's GuideThis PR fixes incorrect text cursor positioning when editing text inside transformed SVG groups by computing an accumulated transform matrix from the text element up through its ancestor groups, and updates tests to reflect the presence of an SVG content container and the new svgCanvas API usage. Sequence diagram for accumulated matrix calculation during text initsequenceDiagram
actor User
participant Editor
participant TextActions
participant SvgCanvas
participant SVGElement as CurTextElement
participant DOM as AncestorNodes
participant TransformUtils
participant GeometryUtils
User->>Editor: Double-click text to edit
Editor->>TextActions: init(index)
TextActions->>CurTextElement: read textContent
TextActions->>GeometryUtils: utilsGetBBox(CurTextElement)
GeometryUtils-->>TextActions: textbb
TextActions->>TextActions: #getAccumulatedMatrix(CurTextElement)
TextActions->>SvgCanvas: getSvgContent()
SvgCanvas-->>TextActions: svgContent
loop Traverse ancestors up to svgContent
TextActions->>AncestorNodes: current = current.parentNode
TextActions->>TransformUtils: getTransformList(current)
TransformUtils-->>TextActions: tlist
alt tlist has items
TextActions->>TransformUtils: transformListToTransform(tlist)
TransformUtils-->>TextActions: transform
TextActions->>TextActions: matrices.unshift(transform.matrix)
end
end
alt no matrices
TextActions-->>TextActions: #matrix = null
else one or more matrices
alt one matrix
TextActions-->>TextActions: #matrix = matrices[0]
else multiple matrices
TextActions->>TransformUtils: matrixMultiply(m1, m2, ...)
TransformUtils-->>TextActions: accumulatedMatrix
TextActions-->>TextActions: #matrix = accumulatedMatrix
end
end
TextActions-->>Editor: init complete with correct #matrix
Class diagram for updated TextActions transform handlingclassDiagram
class TextActions {
- Element #curtext
- SVGMatrix #matrix
- DOMRect #textbb
- Array #chardata
- Number #lastX
- Number #lastY
- Boolean #allowDbl
+ init(index)
- SVGMatrix #getAccumulatedMatrix(elem)
}
class SvgCanvas {
+ getSvgContent() Element
}
class TransformUtils {
+ getTransformList(elem) SVGTransformList
+ transformListToTransform(tlist) SVGTransform
+ matrixMultiply(m1, m2, m3) SVGMatrix
}
class GeometryUtils {
+ utilsGetBBox(elem) DOMRect
}
TextActions --> SvgCanvas : uses getSvgContent
TextActions --> TransformUtils : uses getTransformList
TextActions --> TransformUtils : uses transformListToTransform
TextActions --> TransformUtils : uses matrixMultiply
TextActions --> GeometryUtils : uses utilsGetBBox
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
#getAccumulatedMatrix,matrixMultiply(...matrices)assumes the helper accepts a variable number of matrices; if it only supports two operands today, consider reducing explicitly (e.g.matrices.reduce(matrixMultiply)) to avoid subtle runtime errors. - The traversal in
#getAccumulatedMatrixstops beforesvgContent, which means any transform onsvgcontentitself is ignored; double-check whethersvgcontenttransforms (e.g. pan/zoom) should also be included and, if so, adjust the loop condition accordingly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `#getAccumulatedMatrix`, `matrixMultiply(...matrices)` assumes the helper accepts a variable number of matrices; if it only supports two operands today, consider reducing explicitly (e.g. `matrices.reduce(matrixMultiply)`) to avoid subtle runtime errors.
- The traversal in `#getAccumulatedMatrix` stops before `svgContent`, which means any transform on `svgcontent` itself is ignored; double-check whether `svgcontent` transforms (e.g. pan/zoom) should also be included and, if so, adjust the loop condition accordingly.
## Individual Comments
### Comment 1
<location> `tests/unit/text-actions.test.js:21-30` </location>
<code_context>
svgRoot.setAttribute('height', '480')
document.body.append(svgRoot)
+ // Create svgContent element (container for SVG content)
+ const svgContent = document.createElementNS(NS.SVG, 'svg')
+ svgContent.id = 'svgcontent'
+ svgRoot.append(svgContent)
+
textElement = document.createElementNS(NS.SVG, 'text')
textElement.setAttribute('x', '100')
textElement.setAttribute('y', '100')
textElement.setAttribute('id', 'text1')
textElement.textContent = 'Test'
- svgRoot.append(textElement)
+ svgContent.append(textElement)
// Mock text measurement methods
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests that explicitly cover text inside transformed parent groups to validate the new accumulated matrix behavior.
The new logic focuses on handling transforms from parent `<g>` elements via `#getAccumulatedMatrix`, but the current tests only change the DOM structure and mocks. Please add a test that nests `textElement` in a transformed `<g>` (e.g., `translate(50, 20)`), invokes the cursor-positioning API used in existing tests, and asserts that the cursor position incorporates both the text’s coordinates and the group transform. This will verify the accumulated matrix is exercised and guard against regressions for text editing inside transformed groups.
Suggested implementation:
```javascript
// Mock text measurement methods
textElement.getStartPositionOfChar = vi.fn((i) => ({ x: 100 + i * 10, y: 100 }))
// NOTE: Additional tests below should verify cursor positioning for text
// inside transformed parent groups (e.g., <g transform="translate(...)">)
// to exercise getAccumulatedMatrix behavior.
// Mock svgCanvas
```
` so you can place it alongside the existing cursor‑positioning tests.
Here are the file operations:
<file_operations>
<file_operation operation="edit" file_path="tests/unit/text-actions.test.js">
<<<<<<< SEARCH
// Mock text measurement methods
textElement.getStartPositionOfChar = vi.fn((i) => ({ x: 100 + i * 10, y: 100 }))
// Mock svgCanvas
=======
// Mock text measurement methods
textElement.getStartPositionOfChar = vi.fn((i) => ({ x: 100 + i * 10, y: 100 }))
// NOTE: Additional tests below should verify cursor positioning for text
// inside transformed parent groups (e.g., <g transform="translate(...)">)
// to exercise getAccumulatedMatrix behavior.
// Mock svgCanvas
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
Please add a new test case in `tests/unit/text-actions.test.js` near the existing cursor‑positioning tests (i.e., wherever the current tests are calling into the text cursor API from `textActions`).
Assuming this file already has something like:
- `import * as textActions from '.../text-actions'`
- A `describe('textActions', ...)` block
- A `beforeEach` where the snippet you provided lives
- One or more tests that call the cursor‑positioning API (for example `textActions.setCursor(...)` and read back the cursor position from `textActions` or from the DOM)
you can add the following test body alongside them:
```js
it('positions the text cursor correctly inside a transformed parent group', () => {
// Wrap the existing textElement in a transformed <g> to exercise getAccumulatedMatrix
const group = document.createElementNS(NS.SVG, 'g')
group.setAttribute('transform', 'translate(50, 20)')
svgContent.append(group)
group.append(textElement) // moves textElement under the transformed group
// Choose a character index that is already covered by getStartPositionOfChar
const charIndex = 2
// Existing cursor-positioning API used in this test suite:
// Adjust these calls to match whatever API the other cursor tests use.
textActions.setCursor(textElement, charIndex)
// Read back the cursor position using the same mechanism as the existing tests.
// For example, if existing tests do something like:
// const cursor = textActions.getCursor()
// expect(cursor.x).toBe(...) and expect(cursor.y).toBe(...)
const cursor = textActions.getCursor() // adapt to existing API
// Base character position (from our mock):
// x = 100 + charIndex * 10, y = 100
// Group transform: translate(50, 20)
// Expected cursor position: (100 + charIndex * 10 + 50, 100 + 20)
expect(cursor.x).toBe(100 + charIndex * 10 + 50)
expect(cursor.y).toBe(100 + 20)
})
```
Key points to adapt to your actual code:
1. **Cursor API names**:
Replace `textActions.setCursor` and `textActions.getCursor` with the real functions used in the existing tests for placing and reading the caret position (for example, they might be named `setTextCursor`, `getTextCursor`, `setCursorAtChar`, etc.).
2. **Cursor position retrieval**:
If existing tests assert cursor position by inspecting DOM elements instead of a direct API (e.g., querying a `line` or `rect` that represents the caret), adjust the `cursor` retrieval and assertions accordingly. For example:
```js
const caretLine = svgContent.querySelector('#text_cursor')
expect(Number(caretLine.getAttribute('x1'))).toBe(100 + charIndex * 10 + 50)
expect(Number(caretLine.getAttribute('y1'))).toBe(100 + 20)
```
3. **Matrix behavior**:
The important part is that the expectation **must differ from the raw `getStartPositionOfChar` coordinates** by exactly the group’s transform. That ensures the test truly exercises the new accumulated matrix behavior (`getAccumulatedMatrix`) rather than just reusing the base text coordinates.
By inserting this `it(...)` test near the other cursor‑positioning tests, you’ll have explicit coverage for text inside transformed parent `<g>` elements and guard against regressions in the accumulated-transform logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+21
to
30
| // Create svgContent element (container for SVG content) | ||
| const svgContent = document.createElementNS(NS.SVG, 'svg') | ||
| svgContent.id = 'svgcontent' | ||
| svgRoot.append(svgContent) | ||
|
|
||
| textElement = document.createElementNS(NS.SVG, 'text') | ||
| textElement.setAttribute('x', '100') | ||
| textElement.setAttribute('y', '100') | ||
| textElement.setAttribute('id', 'text1') | ||
| textElement.textContent = 'Test' |
There was a problem hiding this comment.
suggestion (testing): Add tests that explicitly cover text inside transformed parent groups to validate the new accumulated matrix behavior.
The new logic focuses on handling transforms from parent <g> elements via #getAccumulatedMatrix, but the current tests only change the DOM structure and mocks. Please add a test that nests textElement in a transformed <g> (e.g., translate(50, 20)), invokes the cursor-positioning API used in existing tests, and asserts that the cursor position incorporates both the text’s coordinates and the group transform. This will verify the accumulated matrix is exercised and guard against regressions for text editing inside transformed groups.
Suggested implementation:
// Mock text measurement methods
textElement.getStartPositionOfChar = vi.fn((i) => ({ x: 100 + i * 10, y: 100 }))
// NOTE: Additional tests below should verify cursor positioning for text
// inside transformed parent groups (e.g., <g transform="translate(...)">)
// to exercise getAccumulatedMatrix behavior.
// Mock svgCanvas` so you can place it alongside the existing cursor‑positioning tests.
Here are the file operations:
<file_operations>
<file_operation operation="edit" file_path="tests/unit/text-actions.test.js">
<<<<<<< SEARCH
// Mock text measurement methods
textElement.getStartPositionOfChar = vi.fn((i) => ({ x: 100 + i * 10, y: 100 }))
// Mock svgCanvas
// Mock text measurement methods
textElement.getStartPositionOfChar = vi.fn((i) => ({ x: 100 + i * 10, y: 100 }))
// NOTE: Additional tests below should verify cursor positioning for text
// inside transformed parent groups (e.g., <g transform="translate(...)">)
// to exercise getAccumulatedMatrix behavior.
// Mock svgCanvas
REPLACE
</file_operation>
</file_operations>
<additional_changes>
Please add a new test case in tests/unit/text-actions.test.js near the existing cursor‑positioning tests (i.e., wherever the current tests are calling into the text cursor API from textActions).
Assuming this file already has something like:
import * as textActions from '.../text-actions'- A
describe('textActions', ...)block - A
beforeEachwhere the snippet you provided lives - One or more tests that call the cursor‑positioning API (for example
textActions.setCursor(...)and read back the cursor position fromtextActionsor from the DOM)
you can add the following test body alongside them:
it('positions the text cursor correctly inside a transformed parent group', () => {
// Wrap the existing textElement in a transformed <g> to exercise getAccumulatedMatrix
const group = document.createElementNS(NS.SVG, 'g')
group.setAttribute('transform', 'translate(50, 20)')
svgContent.append(group)
group.append(textElement) // moves textElement under the transformed group
// Choose a character index that is already covered by getStartPositionOfChar
const charIndex = 2
// Existing cursor-positioning API used in this test suite:
// Adjust these calls to match whatever API the other cursor tests use.
textActions.setCursor(textElement, charIndex)
// Read back the cursor position using the same mechanism as the existing tests.
// For example, if existing tests do something like:
// const cursor = textActions.getCursor()
// expect(cursor.x).toBe(...) and expect(cursor.y).toBe(...)
const cursor = textActions.getCursor() // adapt to existing API
// Base character position (from our mock):
// x = 100 + charIndex * 10, y = 100
// Group transform: translate(50, 20)
// Expected cursor position: (100 + charIndex * 10 + 50, 100 + 20)
expect(cursor.x).toBe(100 + charIndex * 10 + 50)
expect(cursor.y).toBe(100 + 20)
})Key points to adapt to your actual code:
-
Cursor API names:
ReplacetextActions.setCursorandtextActions.getCursorwith the real functions used in the existing tests for placing and reading the caret position (for example, they might be namedsetTextCursor,getTextCursor,setCursorAtChar, etc.). -
Cursor position retrieval:
If existing tests assert cursor position by inspecting DOM elements instead of a direct API (e.g., querying alineorrectthat represents the caret), adjust thecursorretrieval and assertions accordingly. For example:const caretLine = svgContent.querySelector('#text_cursor') expect(Number(caretLine.getAttribute('x1'))).toBe(100 + charIndex * 10 + 50) expect(Number(caretLine.getAttribute('y1'))).toBe(100 + 20)
-
Matrix behavior:
The important part is that the expectation must differ from the rawgetStartPositionOfCharcoordinates by exactly the group’s transform. That ensures the test truly exercises the new accumulated matrix behavior (getAccumulatedMatrix) rather than just reusing the base text coordinates.
By inserting this it(...) test near the other cursor‑positioning tests, you’ll have explicit coverage for text inside transformed parent <g> elements and guard against regressions in the accumulated-transform logic.
jfhenon
approved these changes
Feb 4, 2026
Collaborator
|
👌 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When editing text inside a group element, the text cursor was appearing in the wrong position because only the text element's own transform was being considered, ignoring transforms from parent groups.
The Problem
In
text-actions.js, theinit()method was calculating the transform matrix like this:This only considers the text element's own
transformattribute. When a text element is inside a<g>group with a transform (e.g.,translate,matrix, etc.), that parent transform was ignored, causing the cursor to appear at the wrong position.The Fix
Added a new
#getAccumulatedMatrix()method that:This ensures that when editing text inside transformed groups, the cursor position correctly accounts for all ancestor transforms.
Changes
#getAccumulatedMatrix()method and updatedinit()to use itgetSvgContent()methodTest plan
npm run lintpassesnpm run testpasses (548 tests)🤖 Generated with Claude Code
Summary by Sourcery
Fix text editing to respect accumulated transforms from parent SVG groups so the text cursor appears at the correct position.
Bug Fixes:
Tests: