From d09985edee3e4474c4851b917521ad2d5bd85ab6 Mon Sep 17 00:00:00 2001 From: Jeff <158072326+jeffrey701@users.noreply.github.com> Date: Thu, 30 Apr 2026 16:17:34 +0200 Subject: [PATCH] :bug: Preserve Inkscape labels when pasting SVGs (#9252) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Steps to reproduce: paste an SVG authored in Inkscape (or any editor that follows the inkscape:label convention) into a penpot file. The group/element names visible in the source editor are dropped — penpot shows generic auto-ids like 'g1234' or 'path5678' instead. Root cause: parse-svg-element in common/src/app/common/files/shapes_ builder.cljc derived the shape name from (or (:id attrs) (tag->name tag)). Inkscape stores user-given element labels in the inkscape:label and sodipodi:label namespaced attributes while id holds an auto- generated technical id, so the operator's chosen name was always overridden by the technical id when present. tubax/xml->clj (the SVG parser the import pipeline already uses for upload, paste, and library import) keeps namespaced attributes as :prefix:name keywords — the same shape this file already reads :xlink:href from on line 134, and that app.common.svg uses for the xlink: namespace at lines 300-307. Fix: extract the name-resolution logic into a public resolve-element- name helper that prefers :inkscape:label, then :sodipodi:label, then :id, then (tag->name tag). Existing SVGs that don't carry either label namespace fall through the same chain as before, so the behaviour for non-Inkscape-authored SVGs is unchanged. This restores the behaviour dfelinto's penpot-icon-generator-plugin relies on (linked from the issue body) — that plugin reads element names from the imported SVG to map Blender icons to penpot components. Tests: 6 deftest blocks in common/test/common_tests/files/shapes_ builder_test.cljc covering the priority order (inkscape > sodipodi > id > tag), each fallback in isolation, and the empty-attrs case. Registered in common-tests.runner. Closes #7869 Co-authored-by: Andrey Antukh --- .../src/app/common/files/shapes_builder.cljc | 18 ++++++- .../files/shapes_builder_test.cljc | 52 +++++++++++++++++++ common/test/common_tests/runner.cljc | 1 + 3 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 common/test/common_tests/files/shapes_builder_test.cljc diff --git a/common/src/app/common/files/shapes_builder.cljc b/common/src/app/common/files/shapes_builder.cljc index 668f50fcaf..a70d6eef9b 100644 --- a/common/src/app/common/files/shapes_builder.cljc +++ b/common/src/app/common/files/shapes_builder.cljc @@ -677,6 +677,22 @@ (remove is-style-fragment?) ;; Filter style fragments and hex colors (filter #(contains? defs %))))) ;; Only existing defs +(defn resolve-element-name + "Pick the most user-meaningful name for an SVG element. + + Inkscape (and editors following the same convention) write the + operator-given label to ``inkscape:label``/``sodipodi:label`` while + ``id`` holds an auto-generated technical id like ``path1234``. + Preferring the namespaced label keeps the layer/group/element names + the operator sees in their source editor across a paste/import + (#7869); the existing ``id`` and ``(tag->name tag)`` fallbacks keep + legacy SVGs that don't carry a label working unchanged." + [tag attrs] + (or (:inkscape:label attrs) + (:sodipodi:label attrs) + (:id attrs) + (tag->name tag))) + (defn parse-svg-element [frame-id svg-data {:keys [tag attrs hidden] :as element} unames] @@ -684,7 +700,7 @@ ;; think we should handle this case early and avoid some code ;; execution - (let [name (or (:id attrs) (tag->name tag)) + (let [name (resolve-element-name tag attrs) att-refs (csvg/find-attr-references attrs) defs (get svg-data :defs) valid-refs (filter-valid-def-references att-refs defs) diff --git a/common/test/common_tests/files/shapes_builder_test.cljc b/common/test/common_tests/files/shapes_builder_test.cljc new file mode 100644 index 0000000000..05956918b2 --- /dev/null +++ b/common/test/common_tests/files/shapes_builder_test.cljc @@ -0,0 +1,52 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS INC + +(ns common-tests.files.shapes-builder-test + (:require + [app.common.files.shapes-builder :as sb] + [clojure.test :as t])) + +;; Regression for https://github.com/penpot/penpot/issues/7869. +;; ``parse-svg-element`` used to derive the shape name from +;; ``(or (:id attrs) (tag->name tag))`` which dropped Inkscape-authored +;; labels. ``tubax/xml->clj`` (the SVG parser the rest of the import +;; pipeline already feeds these maps to) keeps namespaced attributes as +;; ``:prefix:name`` keywords — same shape the codebase already reads +;; ``:xlink:href`` from in this file (line 134) and in +;; ``app.common.svg``. + +(t/deftest resolve-element-name-prefers-inkscape-label + (t/is (= "Layer 1" + (sb/resolve-element-name :g {:inkscape:label "Layer 1" + :id "g1234"})))) + +(t/deftest resolve-element-name-prefers-sodipodi-label-when-no-inkscape-label + (t/is (= "phone-icon" + (sb/resolve-element-name :path {:sodipodi:label "phone-icon" + :id "path5678"})))) + +(t/deftest resolve-element-name-falls-back-to-id-when-no-label-namespace + (t/is (= "manual-id" + (sb/resolve-element-name :rect {:id "manual-id"})))) + +(t/deftest resolve-element-name-falls-back-to-tag-name-when-no-id-and-no-label + ;; The tag->name mapping returns generic names for known SVG element + ;; tags. Asserting on the call result here (rather than a hardcoded + ;; string) keeps the test stable if the tag->name mapping is updated. + (t/is (some? (sb/resolve-element-name :rect {}))) + (t/is (string? (sb/resolve-element-name :rect {})))) + +(t/deftest resolve-element-name-inkscape-label-wins-over-sodipodi-and-id + ;; Both label conventions and an id present together; the priority is + ;; inkscape > sodipodi > id > tag, matching the order operators expect + ;; (Inkscape's own UI shows ``inkscape:label`` as the canonical name). + (t/is (= "user-name" + (sb/resolve-element-name :g {:inkscape:label "user-name" + :sodipodi:label "stale-label" + :id "g1"})))) + +(t/deftest resolve-element-name-empty-attrs-uses-tag-fallback + (t/is (some? (sb/resolve-element-name :path {})))) diff --git a/common/test/common_tests/runner.cljc b/common/test/common_tests/runner.cljc index 06f7926c47..29540525db 100644 --- a/common/test/common_tests/runner.cljc +++ b/common/test/common_tests/runner.cljc @@ -15,6 +15,7 @@ [common-tests.files-builder-test] [common-tests.files-changes-test] [common-tests.files-migrations-test] + [common-tests.files.shapes-builder-test] [common-tests.geom-align-test] [common-tests.geom-bounds-map-test] [common-tests.geom-flex-layout-test]