Fix three bugs: font-size units, imported-SVG ungroup, duplicate jPicker dialogs - #1092
Merged
Conversation
…949) changeSelectedAttributeNoUndoMethod used isNaN(parseFloat(newValue)) to decide whether to store a value as a parsed number or as the original string. parseFloat() only parses the leading numeric portion of a string, so a unit-suffixed value like font-size="10pt" or stroke-width="2px" was silently truncated to a bare "10"/"2", stripping the unit - a regression from #935. Number(), unlike parseFloat(), returns NaN unless the entire string is numeric, so it correctly rejects "10pt" while still converting a plain numeric string like "10.5" to a real number (preserving the behavior #935 was originally fixing for issue #930). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Importing an SVG wraps its content in a <use> referencing a <symbol> in <defs> (per importSvgString()). Ungrouping that content required two separate actions: ungroupSelectedElement() only converted the <use>/gsvg wrapper into an equivalent <g> via convertToGroup() and returned, so a single Ungroup silently swapped one wrapper for another instead of actually unwrapping anything - the toolbar Ungroup button appeared to do nothing on the first click. convertToGroup() now returns the group it produces (or created/reused for the gsvg case), and ungroupSelectedElement() reassigns its local `g` to that result and falls through to the existing flatten logic instead of returning early, so the conversion and the flatten happen in one call. Separately, the right-click context menu's Ungroup item was hard-disabled for anything whose tagName wasn't literally 'g', so it was inert for the <use> element imported content actually is - even though the underlying function already knew how to handle it. Enable it for 'use' as well. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Double-clicking a gradient stop's color swatch invokes jPickerMethod() on the same persistent container div each time (it's shown/hidden between opens, never recreated). initialize()'s non-expandable branch unconditionally appended a fresh #jPicker-table into that container without removing any table already there, so each reopen stacked a duplicate table on top of the previous ones instead of replacing it. Remove any existing #jPicker-table from the container immediately before building the new one, mirroring the same cleanup already done for the toolbar fill/stroke swatch pickers (#1033) but generalized to this container rather than hardcoded to those two element ids. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reviewer's GuideRefines attribute value handling, imported-SVG ungroup behavior, and jPicker dialog lifecycle, with accompanying regression tests and UI wiring updates. Sequence diagram for updated imported-SVG ungroup behaviorsequenceDiagram
actor User
participant Editor
participant svgCanvas
participant convertToGroup
User->>Editor: clickUngroup()
Editor->>svgCanvas: ungroupSelectedElement()
svgCanvas->>svgCanvas: getSelectedElems()
alt gsvg_or_symbol_on_selected
svgCanvas->>convertToGroup: convertToGroup(g)
convertToGroup-->>svgCanvas: g
svgCanvas->>svgCanvas: flatten_group(g)
else use_without_dataStorage_symbol
svgCanvas->>svgCanvas: getHref(g)
svgCanvas->>svgCanvas: getElementById(href)
svgCanvas->>svgCanvas: dataStorage.put(symbol)
svgCanvas->>convertToGroup: convertToGroup(g)
convertToGroup-->>svgCanvas: g
svgCanvas->>svgCanvas: flatten_group(g)
end
Flow diagram for jPicker container reuse and cleanupflowchart TD
A[jPickerMethod called] --> B{isExpandable}
B -- yes --> C[use new floating container]
B -- no --> D[container = that]
D --> E{container has #jPicker-table}
E -- yes --> F[remove existing #jPicker-table]
E -- no --> G[skip removal]
F --> H[create newDiv with controlHtml]
G --> H
H --> I[append newDiv children into container]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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
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
font-size="10pt",stroke-width="2px") were silently truncated to a bare number, stripping the unit - a regression from Fixes issue with xy panel #935.<use>referencing a<symbol>, perimportSvgString()) took two separate Ungroup actions instead of one, and the right-click context menu's Ungroup entry was hard-disabled for<use>elements entirely.#jPicker-tableelements into the picker's container instead of replacing the previous one.Root causes & fixes
#949 -
changeSelectedAttributeNoUndoMethod(packages/svgcanvas/core/undo.js) usedisNaN(parseFloat(newValue))to decide whether to store a parsed number or the raw string.parseFloatonly parses the leading numeric portion of a string, so"10pt"parses to10without failing, silently dropping the unit. Switched toNumber(newValue), which returnsNaNunless the entire string is numeric - correctly rejecting"10pt"while still converting a plain numeric string like"10.5"to a real number (preserving the fix #935 made for #930).#953 -
ungroupSelectedElement(packages/svgcanvas/core/selected-elem.js) calledconvertToGroup()on a<use>/gsvg-tagged element and returned immediately, so a single Ungroup only swapped the wrapper for an equivalent<g>without ever flattening it.convertToGroup()now returns the group it produces, andungroupSelectedElementreassigns its localgto that result and falls through to the existing flatten logic instead of returning early. Separately,TopPanel.js's context-menu enable/disable logic only recognizedtagName === 'g'as ungroupable, hard-disabling the menu item (viapointer-events: none) for the<use>that imported content actually is - now also enabled for'use'.#957 -
jQuery.jPicker.js'sinitialize()unconditionally appends a fresh#jPicker-tableinto its (persistent, shown/hidden-not-recreated) container on every open, with no check for a pre-existing one in the non-expandable branch used by the gradient stop-color picker. Added the same kind of cleanup already used for the toolbar fill/stroke swatch pickers (#1033), generalized to this container.Test plan
npm run lintnpx vitest run tests/unit- 567 tests pass, including a new regression test file for Font-size Property Restriction: No Support for 'pt' Units #949 (tests/unit/change-selected-attribute-units.test.js) and an updated test for Imported svg can't be ungrouped #953 (tests/unit/selected-elem.test.js)npm test(unit + all 81 e2e) - all passnpm run build- production build succeeds#jPicker-tableinstead of stacking duplicates.🤖 Generated with Claude Code
Summary by Sourcery
Fix attribute handling, ungrouping of imported SVG content, and gradient color picker behavior, and add regression coverage for these cases.
Bug Fixes:
Tests: