Improve SVG transform handling and numeric toolbar input behavior - #1099
Merged
Conversation
Reviewer's GuideRefactors SVG transform normalization across recalculate, event handling, selection, and grouping flows so transforms are baked into geometry when safe, retained as normalized rotate+matrix when necessary, and properly undoable, while hardening drag/move events and the SESpinInput numeric component for more predictable user interactions and e2e tooling for cross‑platform coverage. Sequence diagram for drag/move mouse interaction and transform normalizationsequenceDiagram
actor User
participant Browser
participant event_js as event.js
participant svgCanvas
participant recalc_js as recalculate.js
participant history_js as history.js
User->>Browser: drag selection
Browser->>event_js: mouseMoveEvent(evt)
event_js->>event_js: beginDragTransform(svgRoot, selectedElements)
event_js->>event_js: updateTransformList(svgRoot, element, dx, dy)
User->>Browser: mouse up
Browser->>event_js: mouseUpEvent(evt)
event_js->>event_js: compute shouldCommitTransform
alt shouldCommitTransform
loop for each elem in selectedElements
event_js->>svgCanvas: getStartTransform()
event_js->>recalc_js: withStartTransform(svgCanvas, oldTransform, recalculateDimensions)
recalc_js->>recalc_js: recalculateDimensions(elem)
alt cmd returned
event_js->>history_js: addSubCommand(cmd) on BatchCommand
else transform attribute changed
event_js->>history_js: new ChangeElementCommand(elem, { transform: oldTransform })
end
end
event_js->>history_js: addCommandToHistory(BatchCommand)
event_js->>svgCanvas: call('changed', changedElements)
end
event_js->>svgCanvas: clear dragStartTransforms
Sequence diagram for toolbar rotation via setRotationAnglesequenceDiagram
actor User
participant Toolbar
participant selection_js as selection.js
participant svgCanvas
participant recalc_js as recalculate.js
participant history_js as history.js
User->>Toolbar: change rotation input
Toolbar->>selection_js: setRotationAngle(val, preventUndo=false)
selection_js->>selection_js: normalizeRotationAngle(val)
selection_js->>svgCanvas: getSelectedElements()
selection_js->>selection_js: getRotationTransformSummary(tlist)
alt angle unchanged or no bbox
selection_js-->>Toolbar: return
else
selection_js->>selection_js: rewrite tlist to rotate+matrix
selection_js->>selection_js: oldValues = getTransformGeometryValues(elem)
selection_js->>recalc_js: recalculateDimensions(elem)
selection_js->>selection_js: hasTransformGeometryChange(elem, oldValues)
alt geometry changed
selection_js->>history_js: addCommandToHistory(new ChangeElementCommand(elem, oldValues, 'transform'))
end
selection_js->>svgCanvas: call('changed', selectedElements)
end
Sequence diagram for SESpinInput manual entry and commit/revert behaviorsequenceDiagram
actor User
participant SESpinInput
participant ElixBox as elix-number-spin-box
participant Application
User->>SESpinInput: element added to DOM
SESpinInput->>SESpinInput: connectedCallback()
SESpinInput->>SESpinInput: queueMicrotask(#connectInputEvents)
SESpinInput->>ElixBox: #connectInputEvents() attaches listeners
User->>ElixBox: focus input
ElixBox-->>SESpinInput: focus event
SESpinInput->>SESpinInput: committedValue = String(value), input.select()
User->>ElixBox: type value
ElixBox-->>SESpinInput: input event
SESpinInput->>SESpinInput: manualEdit = true
alt User presses Enter
ElixBox-->>SESpinInput: keydown Enter
SESpinInput->>SESpinInput: #commitValue(input.value)
alt #isValidValue(value)
SESpinInput->>SESpinInput: value setter
SESpinInput->>SESpinInput: committedValue = String(value), manualEdit = false
SESpinInput->>SESpinInput: #dispatchChange()
SESpinInput-->>Application: CustomEvent change
else invalid
SESpinInput->>SESpinInput: #revertValue()
end
else User presses Escape
ElixBox-->>SESpinInput: keydown Escape
SESpinInput->>SESpinInput: #revertValue()
else User blurs input
ElixBox-->>SESpinInput: blur event
alt manualEdit
SESpinInput->>SESpinInput: #commitValue(input.value)
end
end
User->>ElixBox: click spin button
ElixBox-->>SESpinInput: mousedown upButton/downButton
SESpinInput->>SESpinInput: manualEdit = false
ElixBox-->>SESpinInput: change event
SESpinInput->>SESpinInput: value = e.target.value, committedValue = String(value)
SESpinInput->>SESpinInput: #dispatchChange()
SESpinInput-->>Application: CustomEvent change
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
jfhenon
approved these changes
Jul 16, 2026
veselin-kutsarov
marked this pull request as ready for review
July 20, 2026 11:45
veselin-kutsarov
changed the title
![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 left some high level feedback:
- There’s duplicated rotation-center/angle logic across modules (e.g.
getRotationCenterFromTransforminrecalculate.jsandgetTransformRotationCenter/normalizeRotationValueinselected-elem.js); consider centralizing this math in a shared helper to keep behavior consistent and easier to maintain. - The
NEAR_ZEROtolerance constant is redefined in multiple files (recalculate.js,selection.js,selected-elem.js); extracting a single shared numeric-tolerance utility would reduce drift and make it easier to tune precision in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There’s duplicated rotation-center/angle logic across modules (e.g. `getRotationCenterFromTransform` in `recalculate.js` and `getTransformRotationCenter`/`normalizeRotationValue` in `selected-elem.js`); consider centralizing this math in a shared helper to keep behavior consistent and easier to maintain.
- The `NEAR_ZERO` tolerance constant is redefined in multiple files (`recalculate.js`, `selection.js`, `selected-elem.js`); extracting a single shared numeric-tolerance utility would reduce drift and make it easier to tune precision in one place.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
veselin-kutsarov
marked this pull request as draft
July 20, 2026 14:15
veselin-kutsarov
marked this pull request as ready for review
July 20, 2026 14:15
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
setRotationAngle, you callsvgCanvas.recalculateDimensions(elem)but ignore the returned command and only add aChangeElementCommandfor tracked attributes; consider capturing the batch command fromrecalculateDimensionsand adding/merging it into history so all geometry/transform changes from rotation normalization are undoable together.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `setRotationAngle`, you call `svgCanvas.recalculateDimensions(elem)` but ignore the returned command and only add a `ChangeElementCommand` for tracked attributes; consider capturing the batch command from `recalculateDimensions` and adding/merging it into history so all geometry/transform changes from rotation normalization are undoable together.
## Individual Comments
### Comment 1
<location path="src/editor/components/seSpinInput.js" line_range="115-120" />
<code_context>
+ this.value = this.committedValue
+ }
+
+ #connectInputEvents () {
+ if (!this.isConnected || this.inputEventsConnected) return
+
+ const input = this.#getTextInput()
+ if (!input) {
+ setTimeout(() => this.#connectInputEvents(), 0)
+ return
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid unbounded recursive timeouts when wiring up the internal input element.
`#connectInputEvents` keeps rescheduling itself with `setTimeout` until `#getTextInput()` succeeds. If the internal template changes or the input never appears, this becomes an infinite retry loop for the component’s lifetime.
Add a guard to bound these retries (e.g., max retry count) or switch to a more reliable readiness signal (e.g., `MutationObserver` on the shadow root or a lifecycle event from the Elix control) so the component doesn’t spin indefinitely if its internals change.
Suggested implementation:
```javascript
#connectInputEvents () {
if (!this.isConnected || this.inputEventsConnected) return
const input = this.#getTextInput()
if (!input) {
// Bound retries to avoid unbounded recursive timeouts if the internal
// input element never appears.
this._inputEventsRetryCount = (this._inputEventsRetryCount ?? 0) + 1
const maxRetries = 20
if (this._inputEventsRetryCount > maxRetries) {
return
}
setTimeout(() => this.#connectInputEvents(), 0)
return
}
// Reset retry counter once the input has been successfully resolved.
this._inputEventsRetryCount = 0
```
To fully integrate this change with the rest of the component, you should also:
1. Initialize `this._inputEventsRetryCount` in the constructor or `connectedCallback` (e.g., set it to `0`).
2. Optionally reset `this._inputEventsRetryCount` in `disconnectedCallback` if the component supports being disconnected and reconnected.
These ensure the retry counter starts from a known state and doesn't leak across lifecycles.
</issue_to_address>
### Comment 2
<location path="packages/svgcanvas/core/math.js" line_range="97-106" />
<code_context>
+ * @param {SVGTransform} transform - The rotation transform
+ * @returns {XYObject} The rotation center
+ */
+export const getRotationCenterFromTransform = (transform) => {
+ if (Number.isFinite(transform.cx) && Number.isFinite(transform.cy)) {
+ return { x: transform.cx, y: transform.cy }
+ }
+
+ const angle = transform.angle * Math.PI / 180
+ const cos = Math.cos(angle)
+ const sin = Math.sin(angle)
+ const det = (1 - cos) * (1 - cos) + sin * sin
+
+ if (Math.abs(det) < NEAR_ZERO) {
+ return { x: 0, y: 0 }
+ }
+
+ const { e, f } = transform.matrix
+ return {
+ x: ((1 - cos) * e - sin * f) / det,
+ y: (sin * e + (1 - cos) * f) / det
+ }
+}
</code_context>
<issue_to_address>
**suggestion:** Clarify or constrain `getRotationCenterFromTransform` assumptions about the underlying matrix.
This logic only yields a correct center if the matrix encodes a pure rotation + translation. If scale or shear are present, the computed center will be wrong.
Since this helper is exported and fairly general, consider either enforcing this invariant (e.g. assert/guard that the matrix is approximately orthonormal) or making the constraint explicit via documentation/renaming so it’s only used with transforms known to come from `setRotate`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
veselin-kutsarov
marked this pull request as draft
July 20, 2026 15:03
veselin-kutsarov
marked this pull request as ready for review
July 20, 2026 15:03
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
selection.setRotationAngle, you callsvgCanvas.recalculateDimensions(elem)but ignore its returnedBatchCommandand instead add a separateChangeElementCommandto history; this likely leaves geometry changes from recalc untracked/undoable, so consider wiring the recalc command into history (or skipping recalc entirely and relying solely on a single undo command) to keep rotation updates fully undoable and consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `selection.setRotationAngle`, you call `svgCanvas.recalculateDimensions(elem)` but ignore its returned `BatchCommand` and instead add a separate `ChangeElementCommand` to history; this likely leaves geometry changes from recalc untracked/undoable, so consider wiring the recalc command into history (or skipping recalc entirely and relying solely on a single undo command) to keep rotation updates fully undoable and consistent.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
veselin-kutsarov
marked this pull request as draft
July 20, 2026 16:21
veselin-kutsarov
marked this pull request as ready for review
July 20, 2026 16:21
Collaborator
|
Thanks for the detailed PR — the transform-normalization work and the spin-input rewrite are both solid, and the added unit/e2e coverage is appreciated. A few things worth clarifying/addressing before merge: 1. Drag threshold: 4px → 1px — please confirm intentIn Was lowering the threshold to 1px intentional, or should it have stayed at 4px (or somewhere in between) while only the "don't insert a drag transform until the threshold is reached" logic changed? At 1px, trackpad jitter or a slightly unsteady click is enough to register as a drag — exactly the accidental-move problem the original 4px threshold was meant to prevent. If this was just to avoid an off-by-one in a test, it'd be better to keep the threshold at 4 and fix the test instead. 2. Make the shape/path baking branch consistent about no-op detectionThe group branch and the text/tspan/use branch of The generic shape/path branch doesn't do this: Suggestion: capture a "did anything actually change" snapshot (transform signature plus the geometry attrs baking can touch — 3. Consolidate the
|
- restore the 4px selection drag threshold - avoid no-op history commands during transform baking - centralize temporary start-transform handling
veselin-kutsarov
marked this pull request as draft
July 20, 2026 20:22
veselin-kutsarov
marked this pull request as ready for review
July 20, 2026 20:25
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The transform normalization helpers (
normalizeRetainedTransformList,bakeTransformsIntoAttributes,transformListSignature,elementTransformStateSignature) are quite intertwined; consider extracting them into a dedicated module or clearly documenting their intended invariants to make future reasoning about transform and undo behavior easier. - The SESpinInput implementation and tests rely on Elix internals like
shadowRootstructure and element ids (input,upButton,downButton); if Elix changes, this will be fragile—consider encapsulating these lookups behind a minimal adapter or asserting their presence once with a clear failure mode.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The transform normalization helpers (`normalizeRetainedTransformList`, `bakeTransformsIntoAttributes`, `transformListSignature`, `elementTransformStateSignature`) are quite intertwined; consider extracting them into a dedicated module or clearly documenting their intended invariants to make future reasoning about transform and undo behavior easier.
- The SESpinInput implementation and tests rely on Elix internals like `shadowRoot` structure and element ids (`input`, `upButton`, `downButton`); if Elix changes, this will be fragile—consider encapsulating these lookups behind a minimal adapter or asserting their presence once with a clear failure mode.
## Individual Comments
### Comment 1
<location path="src/editor/components/seSpinInput.js" line_range="84-86" />
<code_context>
+ const number = Number(normalized)
+ if (!Number.isFinite(number)) return false
+
+ const min = this.getAttribute('min')
+ const max = this.getAttribute('max')
+ return (min === null || number >= Number(min)) &&
+ (max === null || number <= Number(max))
+ }
</code_context>
<issue_to_address>
**suggestion:** Min/max parsing is brittle; non-numeric attributes can incorrectly invalidate user input.
In `#isValidValue`, `min`/`max` are converted with `Number()` but never checked for `NaN`. If a consumer sets a non-numeric `min`/`max` (e.g. `""`, `"abc"`, localized formats), `Number(min)`/`Number(max)` become `NaN`, all comparisons with `number` are false, and valid input is rejected. Please guard against this by only enforcing bounds when the parsed values are finite numbers, e.g.:
```js
const minAttr = this.getAttribute('min')
const min = minAttr != null ? Number(minAttr) : null
const hasMin = Number.isFinite(min)
// same for max
return (!hasMin || number >= min) && (!hasMax || number <= max)
```
Suggested implementation:
```javascript
#isValidValue (value) {
const normalized = String(value).trim()
if (normalized === '') return false
const number = Number(normalized)
if (!Number.isFinite(number)) return false
const minAttr = this.getAttribute('min')
const maxAttr = this.getAttribute('max')
const min = minAttr != null ? Number(minAttr) : null
const max = maxAttr != null ? Number(maxAttr) : null
const hasMin = Number.isFinite(min)
const hasMax = Number.isFinite(max)
return (!hasMin || number >= min) &&
(!hasMax || number <= max)
```
The snippet you shared appears to contain a duplicate `#isValidValue` definition (one without min/max handling, then one with it). In the actual file, ensure there is only a single `#isValidValue` method: remove or update any earlier duplicate so that this consolidated version is the only one present.
</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
+84
to
+86
| const min = this.getAttribute('min') | ||
| const max = this.getAttribute('max') | ||
| return (min === null || number >= Number(min)) && |
There was a problem hiding this comment.
suggestion: Min/max parsing is brittle; non-numeric attributes can incorrectly invalidate user input.
In #isValidValue, min/max are converted with Number() but never checked for NaN. If a consumer sets a non-numeric min/max (e.g. "", "abc", localized formats), Number(min)/Number(max) become NaN, all comparisons with number are false, and valid input is rejected. Please guard against this by only enforcing bounds when the parsed values are finite numbers, e.g.:
const minAttr = this.getAttribute('min')
const min = minAttr != null ? Number(minAttr) : null
const hasMin = Number.isFinite(min)
// same for max
return (!hasMin || number >= min) && (!hasMax || number <= max)Suggested implementation:
#isValidValue (value) {
const normalized = String(value).trim()
if (normalized === '') return false
const number = Number(normalized)
if (!Number.isFinite(number)) return false
const minAttr = this.getAttribute('min')
const maxAttr = this.getAttribute('max')
const min = minAttr != null ? Number(minAttr) : null
const max = maxAttr != null ? Number(maxAttr) : null
const hasMin = Number.isFinite(min)
const hasMax = Number.isFinite(max)
return (!hasMin || number >= min) &&
(!hasMax || number <= max)The snippet you shared appears to contain a duplicate #isValidValue definition (one without min/max handling, then one with it). In the actual file, ensure there is only a single #isValidValue method: remove or update any earlier duplicate so that this consolidated version is the only one present.
veselin-kutsarov
marked this pull request as draft
July 20, 2026 20:33
veselin-kutsarov
marked this pull request as ready for review
July 20, 2026 20:33
veselin-kutsarov
changed the title
jfhenon
approved these changes
Jul 20, 2026
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.
PR description
Summary
This PR improves SVG transform handling across editing workflows and makes numeric toolbar inputs safer and more predictable.
Transform handling
<use>elements in a normalizedrotate(...) matrix(...)form when necessary.font-size.changedandselectedevents so toolbar values and multi-selection actions remain synchronized.Numeric spin inputs
Verification
selection events, transformed bounding boxes, undo history, drag thresholds,
and spin-input behavior.
Checklist
npm test; lint, unit tests, coverage collection, production build,and Playwright tests passed.
internal editing behavior and is documented through focused unit and
Playwright regression tests.
Summary by Sourcery
Improve SVG transform normalization across editing workflows and make numeric spin inputs commit changes more safely and predictably.
New Features:
Bug Fixes:
Enhancements:
Tests: