diff --git a/.serena/memories/common/data-model-change-checklist.md b/.serena/memories/common/data-model-change-checklist.md index f4540f9ea3..3ccc3297a7 100644 --- a/.serena/memories/common/data-model-change-checklist.md +++ b/.serena/memories/common/data-model-change-checklist.md @@ -6,7 +6,7 @@ - Do not treat nil as a distinct persisted state from absence. Import/export and cleanup paths may filter nil attrs away. - Avoid Clojure-special naming in exported object attrs, especially boolean names ending in `?`; exported/imported data must survive JSON/SVG/Transit and external tooling. - Any new shape attr that participates in component sync must be listed in `app.common.types.component/sync-attrs` with the correct touched group. Attrs absent from `sync-attrs` are ignored by component synchronization. -- Shape `:svg-attrs` is the one map whose keys are camelCase React prop names (`fillRule`, not `fill-rule`), because the SVG import path runs them through `app.common.svg/attrs->props`. The `.penpot` reader (`app.common.json/read-kebab-key`) rewrites every json key of every entry to kebab-case, nested maps included, so any code that decodes a binary file must run `:svg-attrs` back through `attrs->props` or the names are lost. `:svg-defs` and the `:content` tree of `svg-raw` shapes keep their source names and round-trip on their own. +- Shape `:svg-attrs` keys are kebab-case (`fill-rule`, not `fillRule`): birth (`app.common.svg/attrs->kebab-props`, applied everywhere data lands on a shape) normalizes any outside spelling through the `svg-props` whitelist, and render boundaries (`get-svg-props`, `render-xml*`, `svg-node*`) translate back to camelCase for React. `:svg-defs` node attrs follow the same rule recursively; svg-raw `:content` attrs are kebab-ized spelling-only, never filtered — the sidebar reads `:stroke-style`, which is outside the whitelist. Non-spec attrs Penpot keeps live in `penpot-extra-attrs` (seeded with `:stroke-style`) and travel in source spelling. Excluded, each with its reason: `:tag` values (React and tag sets need them verbatim; the schema decoder restores keywords from stored strings), outer `:svg-defs` keys (id strings at birth, keywords or strings after JSON decode — accept both), all values (the kebab reader never touches values). Repair sites: birth plus migration `0034-fix-shape-svg-attrs-and-defs` (replaces the withdrawn `0021`); the binfile cleaner carries no svg repair. ## Cross-module update checklist diff --git a/.serena/memories/common/file-change-validation-migration-subtleties.md b/.serena/memories/common/file-change-validation-migration-subtleties.md index d013415730..a319782804 100644 --- a/.serena/memories/common/file-change-validation-migration-subtleties.md +++ b/.serena/memories/common/file-change-validation-migration-subtleties.md @@ -29,4 +29,5 @@ - Migrations are an ordered set mixing legacy version-derived ids and newer named ids. Keep append order stable; `migrate` applies the set difference between available migrations and file migrations. - `migrate-file` synthesizes legacy migration ids from old numeric versions when `:migrations` is absent, migrates legacy features, and records feature flags created through `cfeat/*new*`. - When a file had no previous `:migrations`, `migrate-file` marks all migrations as migrated in metadata so callers persist the complete migration set, not only transformations that changed data. -- Closed schemas (`schema:stroke-attrs` and others) reject a key that was dropped from the declaration, so removing a declared attr needs a migration in the same change, for data already written with it. \ No newline at end of file +- Closed schemas (`schema:stroke-attrs` and others) reject a key that was dropped from the declaration, so removing a declared attr needs a migration in the same change, for data already written with it. +- Rebase collision on a migration number: upstream keeps the id. Renumber the branch migration to the next free one, keep it last in `available-migrations`, and rename everything that spells the number — its test file and namespace (`files_migrations_00NN_test.cljc`), the test names, and comments in production code or memories that name it. \ No newline at end of file diff --git a/.serena/memories/common/testing.md b/.serena/memories/common/testing.md index 4eda8e871f..b18fa3904b 100644 --- a/.serena/memories/common/testing.md +++ b/.serena/memories/common/testing.md @@ -38,6 +38,10 @@ Useful builders: `add-variant-with-copy` does not accept position params for children; use `gsh/absolute-move` after creation if positions matter. +## Generative sampling caution + +- `sg/sample` is unseeded per run. If a test filters samples by type (e.g. keep only `:svg-raw`) then `first` can yield nil on unlucky seeds; `assoc` onto nil builds a typeless map that fails `:multi` dispatch on `:type`. Pin `:seed` in `sg/sample` opts whenever a test depends on a filtered subtype being present (verified seed 42 deals svg-raw shapes on JVM and JS). + ## Driving production paths For shape mutations, prefer production-path helpers such as `cls/generate-update-shapes` plus `thf/apply-changes`. For component swaps with keep-touched behavior, use `tho/swap-component-in-shape` with `{:keep-touched? true}`. diff --git a/backend/src/app/binfile/cleaner.clj b/backend/src/app/binfile/cleaner.clj index a7fcd456db..c846a90149 100644 --- a/backend/src/app/binfile/cleaner.clj +++ b/backend/src/app/binfile/cleaner.clj @@ -9,7 +9,6 @@ for recently imported shapes." (:require [app.common.data :as d] - [app.common.svg :as csvg] [app.common.types.shape :as cts] [app.common.uuid :as uuid])) @@ -106,15 +105,6 @@ :reverse-column :column-reverse dir)))) -(defn- fix-svg-attrs - "The json reader of the binfile rewrites every key of every entry to - kebab-case, but `:svg-attrs` keys are react prop names and are stored - in camelCase. `attrs->props` is the transform the svg import path - already applies to them, so running it again restores the names; it - is idempotent, so shapes that come in correct are left untouched." - [shape] - (d/update-when shape :svg-attrs csvg/attrs->props)) - (defn clean-shape-post-decode "A shape procesor that expected to be executed after schema decoding process but before validation." @@ -122,8 +112,7 @@ (-> shape (fix-shape-shadow-color) (fix-root-shape) - (fix-legacy-flex-dir) - (fix-svg-attrs))) + (fix-legacy-flex-dir))) (defn- fix-container [container] diff --git a/backend/test/backend_tests/binfile_test.clj b/backend/test/backend_tests/binfile_test.clj index 1b75b3a6cd..df522f1314 100644 --- a/backend/test/backend_tests/binfile_test.clj +++ b/backend/test/backend_tests/binfile_test.clj @@ -30,7 +30,6 @@ [app.storage.tmp :as tmp] [backend-tests.helpers :as th] [backend-tests.storage-test :as stt] - [clojure.java.io :as jio] [clojure.test :as t] [cuerdas.core :as str] [datoteka.fs :as fs] @@ -316,12 +315,12 @@ (defn- import-svg-attrs-asset "Imports the `svg-attrs-camel-case.penpot` asset, a real penpot export - whose shapes carry `:svg-attrs` keys in camelCase (the format the - binary export writes), and returns the imported file." + whose shapes carry `:svg-attrs` keys in camelCase (the spelling the + binary export wrote at the time), and returns the imported file. The + v3 reader kebab-izes every key, so the shapes come back with + kebab-case keys." [profile] - (let [input (-> "backend_tests/test_files/svg-attrs-camel-case.penpot" - io/resource - jio/file) + (let [input (th/tempfile "backend_tests/test_files/svg-attrs-camel-case.penpot") result (-> th/*system* (assoc ::bfc/project-id (:default-project-id profile)) (assoc ::bfc/profile-id (:id profile)) @@ -330,13 +329,11 @@ (v3/import-files!))] (bfc/get-file th/*system* (first (:file-ids result))))) -(t/deftest import-binfile-v3-preserves-camel-case-svg-attrs +(t/deftest import-binfile-v3-normalizes-svg-attrs-to-kebab ;; The json reader used by the v3 import rewrites every key of every - ;; zip entry to kebab-case, and `:svg-attrs` is the one shape map - ;; whose keys are camelCase react prop names. A shape exported with - ;; `fillRule: "evenodd"` must not come back as `:fill-rule`, or the - ;; renderer falls back to the default fill rule and the shape is - ;; painted without its hole. + ;; zip entry to kebab-case, and birth stores `:svg-attrs` in kebab-case + ;; too, so a shape exported with `fillRule: "evenodd"` stays kebab and + ;; the renderer finds the fill rule it expects. (let [profile (th/create-profile* 1) file (import-svg-attrs-asset profile) shape (get-in file [:data :pages-index @@ -344,10 +341,11 @@ :objects (uuid/uuid "ce3641bd-48c8-804c-8008-b54d35cc6f80")])] - (t/is (= {:fillRule "evenodd"} + (t/is (some? shape) "the fixture shape was found") + (t/is (= {:fill-rule "evenodd"} (:svg-attrs shape))))) -(t/deftest import-binfile-v3-preserves-camel-case-svg-attrs-on-components +(t/deftest import-binfile-v3-normalizes-svg-attrs-to-kebab-on-components ;; Same guarantee for shapes stored inside a component: the v3 import ;; cleans those in a different code path than page shapes. (let [profile (th/create-profile* 1) @@ -358,14 +356,127 @@ (get (uuid/uuid shape-id)) :svg-attrs))] - (t/is (= {:fillRule "evenodd"} + (t/is (= {:fill-rule "evenodd"} (shape "fae4bc76-0cc2-8057-8008-b540912cdf78" "fae4bc76-0cc2-8057-8008-b540912774cb"))) - (t/is (= {:fillRule "nonzero"} + (t/is (= {:fill-rule "nonzero"} (shape "fae4bc76-0cc2-8057-8008-b540912c917b" "fae4bc76-0cc2-8057-8008-b540912774c8"))))) +(def ^:private svg-defs-page-id (uuid/custom 4 1)) +(def ^:private svg-defs-shape-id (uuid/custom 4 2)) + +(defn- prepare-svg-defs-file + "A file with an svg-raw shape carrying a filter def, the stored + kebab form birth produces (what an SVG import leaves behind)." + ([profile] (prepare-svg-defs-file profile 1)) + ([profile idx] + (let [page-id svg-defs-page-id + shape-id svg-defs-shape-id + + file (th/create-file* idx {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared false})] + (update-file! + :file-id (:id file) + :profile-id (:id profile) + :revn 0 + :vern 0 + :changes + [{:type :add-page + :name "page 1" + :id page-id}]) + + (update-file! + :file-id (:id file) + :profile-id (:id profile) + :revn 0 + :vern 0 + :changes + [{:type :add-obj + :page-id page-id + :id shape-id + :parent-id uuid/zero + :frame-id uuid/zero + :components-v2 true + :obj (cts/setup-shape + {:id shape-id + :name "svg-filter" + :frame-id uuid/zero + :parent-id uuid/zero + :type :svg-raw + :content {:tag :rect + :attrs {:x "10" :fill "red"} + :content []} + :svg-attrs {:fill-rule "evenodd"} + :svg-defs {"f1" {:tag :filter + :attrs {:id "f1"} + :content [{:tag :feGaussianBlur + :attrs {:std-deviation "2"} + :content []}]}}})}]) + + (dissoc file :data)))) + +(defn- export-import-file + "Exports `file` to a zip and imports it back, returning a map with + the export output and the imported file." + [profile file] + (let [output (tmp/tempfile :suffix ".zip")] + (v3/export-files! + (-> th/*system* + (assoc ::bfc/ids #{(:id file)}) + (assoc ::bfc/export-type :detach-libraries)) + (io/output-stream output)) + (let [result (-> th/*system* + (assoc ::bfc/project-id (:default-project-id profile)) + (assoc ::bfc/profile-id (:id profile)) + (assoc ::bfc/input output) + (v3/import-files!))] + {:output output + :imported (bfc/get-file th/*system* (first (:file-ids result)))}))) + +(defn- svg-defs-shape + [file] + (get-in file [:data :pages-index svg-defs-page-id :objects svg-defs-shape-id])) + +(t/deftest import-binfile-v3-normalizes-svg-defs-to-kebab + ;; Def node attrs converge to the stored kebab convention through an + ;; export/import cycle: tags come back as keywords (the schema decoder + ;; restores them from the stored strings), attr keys stay kebab, outer + ;; def keys come back as strings (the decoder prefers the string + ;; branch), and the string ids inside survive byte-identical. + (let [profile (th/create-profile* 1) + file (prepare-svg-defs-file profile) + {:keys [imported]} (export-import-file profile file) + shape (svg-defs-shape imported) + node (get (:svg-defs shape) "f1")] + (t/is (some? shape) "the svg shape was found") + (t/is (= {:fill-rule "evenodd"} (:svg-attrs shape))) + (t/is (some? node) "the filter def was found") + (t/is (= :filter (:tag node)) "tags decode back to keywords") + (t/is (= "f1" (get-in node [:attrs :id])) "def ids survive byte-identical") + (t/is (= "2" (get-in node [:content 0 :attrs :std-deviation]))))) + +(t/deftest export-import-svg-roundtrip-converges + ;; The determinism proof: export, import, export, import. The first + ;; import lands on the kebab end state and the second trip leaves it + ;; exactly there. + (let [profile (th/create-profile* 1) + file (prepare-svg-defs-file profile) + trip-1 (:imported (export-import-file profile file)) + shape-1 (svg-defs-shape trip-1) + trip-2 (:imported (export-import-file profile trip-1)) + shape-2 (svg-defs-shape trip-2)] + (t/is (some? shape-1) "the svg shape survived the first trip") + (t/is (some? shape-2) "the svg shape survived the second trip") + (t/is (= {:fill-rule "evenodd"} (:svg-attrs shape-1)) + "first trip lands on the kebab end state") + (t/is (= (:svg-attrs shape-1) (:svg-attrs shape-2)) + "svg-attrs are a fixed point") + (t/is (= (:svg-defs shape-1) (:svg-defs shape-2)) + "svg-defs are a fixed point"))) + (t/deftest export-binfile-v3 (let [profile (th/create-profile* 1) file (prepare-simple-file profile) diff --git a/common/src/app/common/files/builder.cljc b/common/src/app/common/files/builder.cljc index 2e02166a93..d9b86ce36d 100644 --- a/common/src/app/common/files/builder.cljc +++ b/common/src/app/common/files/builder.cljc @@ -414,7 +414,14 @@ (defn add-shape [state params] (let [obj (-> params - (d/update-when :svg-attrs csvg/attrs->props) + (d/update-when :svg-attrs csvg/attrs->kebab-props) + (d/update-when :svg-defs + (fn [defs] + (if (map? defs) + (d/mapm (fn [_ node] (csvg/normalize-def-node node)) defs) + defs))) + (cond-> (= :svg-raw (:type params)) + (d/update-when :content csvg/kebabize-content-node)) (types.shape/setup-shape) (assign-shape-name state))] (-> state diff --git a/common/src/app/common/files/migrations.cljc b/common/src/app/common/files/migrations.cljc index d0312f5a56..15f03e13c6 100644 --- a/common/src/app/common/files/migrations.cljc +++ b/common/src/app/common/files/migrations.cljc @@ -1813,21 +1813,6 @@ {})] (cfcp/sync-component-id-with-ref-shape data libraries))) -(defmethod migrate-data "0021-fix-shape-svg-attrs" - [data _] - (some-> cfeat/*new* (swap! conj "fdata/shape-data-type")) - (letfn [(update-object [object] - (-> object - (d/update-when :svg-attrs csvg/attrs->props) - (d/update-when :svg-viewbox grc/make-rect))) - - (update-container [container] - (d/update-when container :objects d/update-vals update-object))] - - (-> data - (update :pages-index d/update-vals update-container) - (d/update-when :components d/update-vals update-container)))) - ;; Re-run the 0019 and 0020 fixers after normalizing :component-root. ;; Migrations 0019 and 0020 missed shapes with an explicit :component-root ;; false because subcopy-head? expects nil. Normalize first, then re-run. @@ -2282,6 +2267,47 @@ (update :pages-index d/update-vals update-container) (d/update-when :components d/update-vals update-container)))) +(defmethod migrate-data "0034-fix-shape-svg-attrs-and-defs" + ;; Stored svg attribute keys are kebab-case (issue #11947): the v3 reader + ;; already outputs kebab and birth normalizes any outside spelling, so + ;; export/import converges to the same state. Old birth and the cleaner + ;; era stored camelCase React prop names instead, which the reader + ;; kebab-ized on import beyond repair. Convert once, with the same + ;; birth transforms: `:svg-attrs` and `:svg-defs` node attrs go through + ;; the whitelist+kebab transform (unknown attrs drop, same as birth), + ;; while svg-raw `:content` attrs are kebab-ized spelling-only (unknown + ;; keys kept: the sidebar reads keys outside the whitelist). Kebab + ;; input passes through unchanged, so re-running is safe. This replaces + ;; "0021-fix-shape-svg-attrs" (id withdrawn from `available-migrations` + ;; below): files that recorded it migrate again through this one. + ;; Non-lowercase XML ids stay lossy: the reader lowercases them before + ;; anything runs. + [data _] + (some-> cfeat/*new* (swap! conj "fdata/shape-data-type")) + (letfn [(migrate-defs [defs] + (if (map? defs) + (d/mapm (fn [_ node] (csvg/normalize-def-node node)) defs) + defs)) + + (migrate-shape [shape] + (-> shape + (d/update-when :svg-attrs + (fn [attrs] + (if (map? attrs) + (csvg/attrs->kebab-props attrs) + attrs))) + (d/update-when :svg-defs migrate-defs) + (d/update-when :svg-viewbox grc/make-rect) + (cond-> (= :svg-raw (:type shape)) + (d/update-when :content csvg/kebabize-content-node)))) + + (migrate-container [container] + (d/update-when container :objects d/update-vals migrate-shape))] + + (-> data + (update :pages-index d/update-vals migrate-container) + (d/update-when :components d/update-vals migrate-container)))) + (def available-migrations (into (d/ordered-set) ["legacy-2" @@ -2360,7 +2386,6 @@ "0018-remove-unneeded-objects-from-components" "0019-fix-missing-swap-slots" "0020-sync-component-id-with-near-main" - "0021-fix-shape-svg-attrs" "0022-normalize-component-root-and-resync" "0023-repair-token-themes-with-inexistent-sets" "0024b-fix-stroke-cap-placement" @@ -2372,4 +2397,5 @@ "0030-remove-stroke-per-side-attr" "0031-migrate-stroke-width-token-attr" "0032-fix-children-not-in-parent" - "0033-normalize-main-instance-names"])) + "0033-normalize-main-instance-names" + "0034-fix-shape-svg-attrs-and-defs"])) diff --git a/common/src/app/common/files/shapes_builder.cljc b/common/src/app/common/files/shapes_builder.cljc index 9577fb528d..ad2d90f610 100644 --- a/common/src/app/common/files/shapes_builder.cljc +++ b/common/src/app/common/files/shapes_builder.cljc @@ -122,7 +122,11 @@ - The referencing gradient's attributes override the base ones - If the referencing gradient has stops, they replace the base stops - Returns the defs map with all gradient href references resolved." + Returns the defs map with all gradient href references resolved. + + NOTE: reads raw parser keys (`:xlink:href` with colon, + `:gradientTransform` camelCase). Runs before birth normalization; + do not switch to kebab." [defs] (letfn [(resolve-gradient [gradient-id gradient-node defs visited] (if (contains? visited gradient-id) @@ -220,7 +224,9 @@ (csvg/extract-defs)) ;; Resolve gradient href references in all defs before processing shapes - def-nodes (resolve-gradient-href def-nodes) + ;; then normalize them to the stored kebab convention + def-nodes (d/mapm (fn [_ node] (csvg/normalize-def-node node)) + (resolve-gradient-href def-nodes)) ;; In penpot groups have the size of their children. To ;; respect the imported svg size and empty space let's create @@ -269,8 +275,9 @@ (defn create-raw-svg [name frame-id {:keys [x y width height offset-x offset-y defs] :as svg-data} {:keys [attrs] :as data}] - (let [props (csvg/attrs->props attrs) - vbox (grc/make-rect offset-x offset-y width height)] + (let [props (csvg/attrs->kebab-props attrs) + content (csvg/kebabize-content-node data) + vbox (grc/make-rect offset-x offset-y width height)] (cts/setup-shape {:type :svg-raw :name name @@ -279,7 +286,7 @@ :height height :x x :y y - :content data + :content content :svg-attrs props :svg-viewbox vbox :svg-defs defs}))) @@ -288,7 +295,7 @@ [id frame-id parent-id {:keys [name x y width height offset-x offset-y attrs defs] :as svg-data}] (let [props (-> (dissoc attrs :viewBox :view-box :xmlns) (d/without-keys csvg/inheritable-props) - (csvg/attrs->props))] + (csvg/attrs->kebab-props))] (cts/setup-shape {:id id :type :group @@ -325,7 +332,7 @@ (let [transform (csvg/parse-transform (:transform attrs)) attrs (-> attrs (d/without-keys csvg/inheritable-props) - (csvg/attrs->props)) + (csvg/attrs->kebab-props)) vbox (grc/make-rect offset-x offset-y width height)] (cts/setup-shape {:type :group @@ -365,7 +372,7 @@ points (grc/rect->points selrect) origin (gpt/negate (gpt/point svg-data)) attrs (-> (dissoc attrs :d :transform) - (csvg/attrs->props))] + (csvg/attrs->kebab-props))] (-> (cts/setup-shape {:type :path :name name @@ -434,7 +441,7 @@ (update :y - (:y origin))) props (-> (dissoc attrs :x :y :width :height :rx :ry :transform) - (csvg/attrs->props)) + (csvg/attrs->kebab-props)) radius-attrs (parse-radius-attrs attrs)] (cts/setup-shape @@ -480,7 +487,7 @@ (* 2 rx) (* 2 ry)) props (-> (dissoc attrs :cx :cy :r :rx :ry :transform) - (csvg/attrs->props))] + (csvg/attrs->kebab-props))] (cts/setup-shape (-> (calculate-rect-metadata rect transform) @@ -510,7 +517,7 @@ (update :x - (:x origin)) (update :y - (:y origin))) props (-> (dissoc attrs :x :y :width :height :href :xlink:href) - (csvg/attrs->props))] + (csvg/attrs->kebab-props))] (when (some? image-data) (cts/setup-shape @@ -545,17 +552,17 @@ ;; Only create an opacity if the color is set. Otherwise can create problems down the line (and (or (clr/color-string? color-attr) (clr/color-string? color-style)) - (dm/get-in shape [:svg-attrs :fillOpacity])) - (-> (update :svg-attrs dissoc :fillOpacity) - (update-in [:svg-attrs :style] dissoc :fillOpacity) - (assoc-in [:fills 0 :fill-opacity] (-> (dm/get-in shape [:svg-attrs :fillOpacity]) + (dm/get-in shape [:svg-attrs :fill-opacity])) + (-> (update :svg-attrs dissoc :fill-opacity) + (update-in [:svg-attrs :style] dissoc :fill-opacity) + (assoc-in [:fills 0 :fill-opacity] (-> (dm/get-in shape [:svg-attrs :fill-opacity]) (d/parse-double 1)))) (and (or (clr/color-string? color-attr) (clr/color-string? color-style)) - (dm/get-in shape [:svg-attrs :style :fillOpacity])) - (-> (update-in [:svg-attrs :style] dissoc :fillOpacity) - (update :svg-attrs dissoc :fillOpacity) - (assoc-in [:fills 0 :fill-opacity] (-> (dm/get-in shape [:svg-attrs :style :fillOpacity]) + (dm/get-in shape [:svg-attrs :style :fill-opacity])) + (-> (update-in [:svg-attrs :style] dissoc :fill-opacity) + (update :svg-attrs dissoc :fill-opacity) + (assoc-in [:fills 0 :fill-opacity] (-> (dm/get-in shape [:svg-attrs :style :fill-opacity]) (d/parse-double 1))))))) (defn- setup-stroke @@ -573,30 +580,30 @@ opacity (when (some? color) (d/parse-double - (or (:strokeOpacity attrs) - (:strokeOpacity style)) + (or (:stroke-opacity attrs) + (:stroke-opacity style)) 1)) width (when (some? color) (d/parse-double - (or (:strokeWidth attrs) - (:strokeWidth style)) + (or (:stroke-width attrs) + (:stroke-width style)) 1)) - linecap (or (get attrs :strokeLinecap) - (get style :strokeLinecap)) + linecap (or (get attrs :stroke-linecap) + (get style :stroke-linecap)) linecap (some-> linecap str/trim keyword) attrs (-> attrs (cond-> (some? color) - (dissoc :stroke :strokeWidth :strokeOpacity)) + (dissoc :stroke :stroke-width :stroke-opacity)) (update :style (fn [style] (-> style (cond-> (some? color) - (dissoc :stroke :strokeWidth :strokeOpacity))))) + (dissoc :stroke :stroke-width :stroke-opacity))))) (d/without-nils))] (cond-> (assoc shape :svg-attrs attrs) @@ -629,13 +636,13 @@ (assoc :opacity (-> (dm/get-in shape [:svg-attrs :style :opacity]) (d/parse-double 1)))) - (dm/get-in shape [:svg-attrs :mixBlendMode]) - (-> (update :svg-attrs dissoc :mixBlendMode) - (assoc :blend-mode (-> (dm/get-in shape [:svg-attrs :mixBlendMode]) assert-valid-blend-mode))) + (dm/get-in shape [:svg-attrs :mix-blend-mode]) + (-> (update :svg-attrs dissoc :mix-blend-mode) + (assoc :blend-mode (-> (dm/get-in shape [:svg-attrs :mix-blend-mode]) assert-valid-blend-mode))) - (dm/get-in shape [:svg-attrs :style :mixBlendMode]) - (-> (update-in [:svg-attrs :style] dissoc :mixBlendMode) - (assoc :blend-mode (-> (dm/get-in shape [:svg-attrs :style :mixBlendMode]) assert-valid-blend-mode))))) + (dm/get-in shape [:svg-attrs :style :mix-blend-mode]) + (-> (update-in [:svg-attrs :style] dissoc :mix-blend-mode) + (assoc :blend-mode (-> (dm/get-in shape [:svg-attrs :style :mix-blend-mode]) assert-valid-blend-mode))))) (defn setup-other [shape] (cond-> shape diff --git a/common/src/app/common/render_wasm/api/props.cljs b/common/src/app/common/render_wasm/api/props.cljs index c8f71571e5..5c2eb2606d 100644 --- a/common/src/app/common/render_wasm/api/props.cljs +++ b/common/src/app/common/render_wasm/api/props.cljs @@ -157,9 +157,9 @@ (defn set-shape-svg-attrs [attrs] (let [style (:style attrs) - fill-rule (-> (or (:fillRule style) (:fillRule attrs)) sr/translate-fill-rule) - stroke-linecap (-> (or (:strokeLinecap style) (:strokeLinecap attrs)) sr/translate-stroke-linecap) - stroke-linejoin (-> (or (:strokeLinejoin style) (:strokeLinejoin attrs)) sr/translate-stroke-linejoin) + fill-rule (-> (or (:fill-rule style) (:fill-rule attrs)) sr/translate-fill-rule) + stroke-linecap (-> (or (:stroke-linecap style) (:stroke-linecap attrs)) sr/translate-stroke-linecap) + stroke-linejoin (-> (or (:stroke-linejoin style) (:stroke-linejoin attrs)) sr/translate-stroke-linejoin) fill-none (= "none" (or (:fill style) (:fill attrs)))] (h/call wasm/internal-module "_set_shape_svg_attrs" fill-rule stroke-linecap stroke-linejoin fill-none))) diff --git a/common/src/app/common/render_wasm/svg_derived.cljs b/common/src/app/common/render_wasm/svg_derived.cljs index f93a07bdf3..7391807a4a 100644 --- a/common/src/app/common/render_wasm/svg_derived.cljs +++ b/common/src/app/common/render_wasm/svg_derived.cljs @@ -221,10 +221,19 @@ (let [result (update combined :content #(or % []))] result)))))))) +(defn- style-map + "Stop `:style` as a map, whatever its spelling: pre-migration files + carry it as a CSS string, stored (post-0034) files as a kebab map." + [style] + (cond + (string? style) (csvg/parse-style style) + (map? style) style + :else nil)) + (defn- parse-gradient-stop [stop-node] (let [attrs (normalize-attrs (:attrs stop-node)) - style (some-> (get attrs :style) csvg/parse-style) + style (some-> (get attrs :style) style-map) color-value (or (get attrs :stop-color) (get attrs :stopcolor) (get style :stop-color) @@ -356,9 +365,9 @@ attr-fill (parse-svg-fill shape (dm/get-in shape [:svg-attrs :fill])) {:keys [type value]} (or style-fill attr-fill)] (when (some? type) - (let [opacity (or (some-> (dm/get-in shape [:svg-attrs :style :fillOpacity]) + (let [opacity (or (some-> (dm/get-in shape [:svg-attrs :style :fill-opacity]) (d/parse-double 1)) - (some-> (dm/get-in shape [:svg-attrs :fillOpacity]) + (some-> (dm/get-in shape [:svg-attrs :fill-opacity]) (d/parse-double 1))) base-fill (case type :color {:fill-color value} @@ -402,8 +411,8 @@ (when gaussian-blur {:id (uuid/next) :type :layer-blur - ;; For layer blur the value matches stdDeviation directly - :value (-> (dm/get-in gaussian-blur [:attrs :stdDeviation]) + ;; For layer blur the value matches std-deviation directly + :value (-> (dm/get-in gaussian-blur [:attrs :std-deviation]) (d/parse-double 0)) :hidden false})) @@ -470,7 +479,7 @@ (let [blur-elem (find-filter-element filter-content :feGaussianBlur)] (drop-shadow (d/parse-double (filter-attr offset-elem :dx) 0) (d/parse-double (filter-attr offset-elem :dy) 0) - (d/parse-double (filter-attr blur-elem :stddeviation) 0) + (d/parse-double (filter-attr blur-elem :std-deviation :stddeviation) 0) (or (shadow-color filter-content) {:color clr/black :opacity 1}))))) @@ -479,7 +488,7 @@ [elem] (drop-shadow (d/parse-double (filter-attr elem :dx) 2) (d/parse-double (filter-attr elem :dy) 2) - (d/parse-double (filter-attr elem :stddeviation) 2) + (d/parse-double (filter-attr elem :std-deviation :stddeviation) 2) (flood->shadow-color elem))) (defn apply-svg-filters diff --git a/common/src/app/common/svg.cljc b/common/src/app/common/svg.cljc index e51cad731e..7bed2eaca1 100644 --- a/common/src/app/common/svg.cljc +++ b/common/src/app/common/svg.cljc @@ -179,6 +179,7 @@ :horiz-adv-x :horiz-origin-x :horiz-origin-y + :href :id :ideographic :in @@ -379,6 +380,13 @@ :writing-mode :mask-type}) +(def penpot-extra-attrs + "Non-spec attrs Penpot deliberately keeps because exporters emit them + and Penpot readers need them. Seeded with `:stroke-style`, read by the + svg-raw sidebar. Only add an entry with a reader that needs it plus a + test; this set is not a catch-all for unknown attrs." + #{:stroke-style}) + (def inheritable-props #{:style :clip-rule @@ -520,11 +528,45 @@ (let [xf (map prop-key)] (-> #{} (into xf svg-attrs) - (into xf svg-presentation-attrs)))) + (into xf svg-presentation-attrs) + (into xf penpot-extra-attrs)))) + +(def penpot-extra-prop-keys + "Camel render key -> source spelling for `penpot-extra-attrs`. + `attrs->props` keeps these keys in source spelling (instead of + camelizing them) so React passes them to the DOM silently, with no + unknown-prop warning, and export writes them back as emitted." + (into {} (map (fn [k] [(prop-key k) k])) penpot-extra-attrs)) + +(def svg-stored-attr-keys + "Kebab-case keys storable in `:svg-attrs` and `:svg-defs` node + `:attrs`: every `svg-props` whitelist key in stored spelling, sorted + for stable schema derivation. Birth (`attrs->kebab-props`) can only + produce these keys, so the shape schema derived from them cannot + drift from birth." + (sort-by name (map (comp keyword str/kebab name) svg-props))) + +(def stored-attr-display-names + "Stored kebab key -> spec/source spelling, for user-facing labels + (the svg-attrs menu shows these). Kebab storage matches the spec for + most keys (`:stroke-width` shows `stroke-width`), but spec-camel keys + do not (`:class-name` shows `class`, `:view-box` shows `viewBox`)." + (into {} + (map (fn [k] [(keyword (str/kebab (name (prop-key k)))) (name k)])) + (concat svg-attrs svg-presentation-attrs penpot-extra-attrs))) + +(defn stored-attr-display-name + "User-facing label for a stored kebab attr key. Unknown keys fall + back to their name." + [k] + (get stored-attr-display-names k (name k))) ;; Defaults for some tags per spec https://www.w3.org/TR/SVG11/single-page.html ;; they are basically the defaults that can be percents and we need to replace because ;; otherwise won't work as expected in the workspace +;; NOTE: camelCase keys on purpose. These defaults run on the raw +;; parser output before birth normalizes stored keys to kebab-case; +;; do not "fix" them to kebab. (def svg-tag-defaults (let [filter-default {:units :filterUnits :default "objectBoundingBox" @@ -605,29 +647,64 @@ ([attrs whitelist?] (reduce-kv (fn [res k v] - (let [k (prop-key k)] + (let [ck (prop-key k)] (cond - (nil? k) + (nil? ck) res (nil? v) res - (= k :style) + (= ck :style) (let [v (if (string? v) (parse-style v) v) v (not-empty (attrs->props v false))] (if v - (assoc res k v) + (assoc res ck v) res)) :else - (if (or (not whitelist?) (contains? svg-props k)) - (let [v (if (string? v) (str/trim v) v)] - (assoc res k v)) + (if (or (not whitelist?) (contains? svg-props ck)) + (let [v (if (string? v) (str/trim v) v) + ;; Extra attrs keep their source spelling so + ;; React passes them to the DOM silently. + out-k (get penpot-extra-prop-keys ck ck)] + (assoc res out-k v)) res)))) {} attrs))) +(defn attrs->kebab-props + "Like `attrs->props` (same whitelist, trim and `:style` parsing) but + with kebab-case keys, the stored convention for `:svg-attrs` and + `:svg-defs` node attrs. Kebab is a fixed point: running it twice + returns the same map." + [attrs] + (-> attrs + (attrs->props) + (d/kebab-keys))) + +(defn- normalize-node-tree + [attrs-fn node] + (if-not (map? node) + node + (-> node + (d/update-when :attrs attrs-fn) + (d/update-when :content #(mapv (partial normalize-node-tree attrs-fn) %))))) + +(defn normalize-def-node + "Normalizes a `:svg-defs` node to the stored kebab convention: `:attrs` + go through `attrs->kebab-props` (whitelist + kebab), recursively + through nested `:content`. Tags, ids and values are left untouched." + [node] + (normalize-node-tree attrs->kebab-props node)) + +(defn kebabize-content-node + "Kebab-izes the `:attrs` keys of an svg-raw `:content` tree, spelling + only: unknown keys are kept because the sidebar reads keys outside + the whitelist (e.g. `:stroke-style`)." + [node] + (normalize-node-tree d/kebab-keys node)) + (defn update-attr-ids "Replaces the ids inside a property" [attrs replace-fn] @@ -885,7 +962,7 @@ ;; Don't inherit a property that is already in the style attribute inherit-style (-> (:style group-attrs) (d/without-keys (keys attrs))) - inheritable-props (->> inheritable-props (remove #(contains? (:styles attrs) %))) + inheritable-props (->> inheritable-props (remove #(contains? (:style attrs) %))) group-attrs (-> group-attrs (assoc :style inherit-style)) attrs (-> (select-keys group-attrs inheritable-props) @@ -925,7 +1002,10 @@ (defn fix-default-values "Gives values to some SVG elements which defaults won't work when - imported into the platform" + imported into the platform. + + NOTE: reads raw parser keys (camelCase). Runs before birth + normalization; do not switch to kebab." [svg-data] (let [add-defaults (fn [{:keys [tag attrs] :as node}] @@ -950,7 +1030,10 @@ (mth/sqrt 2))) (defn fix-percents - "Changes percents to a value according to the size of the svg imported" + "Changes percents to a value according to the size of the svg imported. + + NOTE: reads raw parser keys (camelCase). Runs before birth + normalization; do not switch to kebab." [svg-data] ;; https://www.w3.org/TR/SVG11/single-page.html#coords-Units (let [viewbox {:x (:offset-x svg-data) @@ -1023,7 +1106,10 @@ (map-nodes fix-percent-values svg-data)))) -(defn collect-images [svg-data] +(defn collect-images + "NOTE: reads raw parser keys (`:xlink:href` with colon). Runs before + birth normalization; do not switch to kebab." + [svg-data] (let [redfn (fn [acc {:keys [tag attrs]}] (cond-> acc (= :image tag) diff --git a/common/src/app/common/types/shape.cljc b/common/src/app/common/types/shape.cljc index 80f2e608f6..f1f9071f65 100644 --- a/common/src/app/common/types/shape.cljc +++ b/common/src/app/common/types/shape.cljc @@ -17,6 +17,7 @@ [app.common.record :as cr] [app.common.schema :as sm] [app.common.schema.generators :as sg] + [app.common.svg :as csvg] [app.common.transit :as t] [app.common.types.color :as clr] [app.common.types.fills :refer [schema:fills fill->color]] @@ -186,8 +187,83 @@ [:width ::sm/safe-number] [:height ::sm/safe-number]]) +(def ^:private schema:svg-scalar-gen + "Small generation-only scalar for svg attr values. Validation stays + wide (`:string`, any `::sm/number`, `:boolean`), but the default + `::sm/number` generator emits unbounded doubles whose own precision + pass overflows float: bound everything explicitly here." + (sg/one-of + (sg/word-string) + (sg/small-int :min -1000 :max 1000) + (sg/small-double :min -1000 :max 1000) + (sg/elements [true false]))) + +(def ^:private schema:svg-scalar + "A scalar svg attr value: trimmed strings or numbers/booleans + straight from programmatic svg data." + [:or {:gen/gen schema:svg-scalar-gen} + :string ::sm/number :boolean]) + +(def schema:svg-attrs + "Closed `:svg-attrs`: every key birth can store, derived from + `csvg/svg-stored-attr-keys` (the whitelist in stored spelling), so + birth and validation cannot drift apart. Values are the scalars birth + stores (trimmed strings, or numbers/booleans straight from + programmatic svg data), except `:style`, which birth parses into a + map. `:tag` stays a keyword: switching it to string would touch ~22 + comparison sites and 7 tag sets for zero behavior gain." + (into [:map {:title "SvgAttrs" :closed true}] + (map (fn [k] + [k {:optional true} + (if (= k :style) + [:map-of :keyword schema:svg-scalar] + schema:svg-scalar)])) + csvg/svg-stored-attr-keys)) + +(def ^:private schema:svg-shallow-node-gen + "Bounded generator for the svg node schemas below: one shallow node. + The schemas are recursive, and unbounded recursive generation grows + monster trees that OOM around ~200 sampled shapes. Hand-written tests + cover deep nesting; generation only needs small valid nodes." + (sg/one-of + (sg/elements [{:tag :g :attrs {}} + {:tag :g :attrs {} :content []} + {:tag :g :attrs {} :content ["text"]} + {:tag :rect :attrs {:x "0"} :content []}]))) + +(def schema:svg-def-node + "A `:svg-defs` node: keyword tag (the JSON decoder restores keywords + from the stored strings), whitelisted attrs, recursive content that + may interleave text. Closed: birth and migration normalize def nodes + to exactly these keys." + [:schema {:registry + {::svg-def-node + [:map {:closed true + :gen/gen schema:svg-shallow-node-gen} + [:tag :keyword] + [:attrs schema:svg-attrs] + [:content {:optional true} + [:vector [:or :string [:ref ::svg-def-node]]]]]}} + [:ref ::svg-def-node]]) + +(def schema:svg-content-node + "An svg-raw `:content` tree node: keyword tag, spelling-only + kebab-ized attrs (open: the sidebar reads keys outside the whitelist, + such as `:stroke-style`), recursive content that may interleave text." + [:schema {:registry + {::svg-content-node + [:map {:gen/gen schema:svg-shallow-node-gen} + [:tag :keyword] + [:attrs {:optional true} :map] + [:content {:optional true} + [:vector [:or :string [:ref ::svg-content-node]]]]]}} + [:ref ::svg-content-node]]) + (def schema:shape-generic-attrs - [:map {:title "ShapeGenericAttrs"} + [:map {:title "ShapeGenericAttrs" + ;; Generated shapes must not carry svg provenance: the closed + ;; svg schemas above would fill them with random attrs. + :gen/fmap (fn [m] (dissoc m :svg-attrs :svg-defs))} [:page-id {:optional true} ::sm/uuid] [:component-id {:optional true} ::sm/uuid] [:component-file {:optional true} ::sm/uuid] @@ -264,15 +340,19 @@ ;; it, and `setup-shape` drops it when a caller passes nil. [:hide-in-viewer {:optional true} :boolean] - ;; The SVG provenance an import leaves on a shape. Typed `:map` rather than - ;; more precisely on purpose: legacy files hold `svg-transform` as a plain - ;; `{:a … :f}` map rather than a `::gmt/matrix` record, and `svg-viewbox` as - ;; either a `::grc/rect` record or a plain map, so a tighter schema here - ;; would reject files that are otherwise valid. The graph *column* types are - ;; tightened separately, where a wrong guess costs a column rather than a - ;; rejected file (`app.graph.schema.contract/type-overrides`). - [:svg-attrs {:optional true} :map] - [:svg-defs {:optional true} :map] + ;; The SVG provenance an import leaves on a shape. `:svg-attrs` is + ;; closed over the whitelist in stored spelling (see + ;; `schema:svg-attrs`): legacy files hold `svg-transform` as a plain + ;; `{:a … :f}` map rather than a `::gmt/matrix` record, and + ;; `svg-viewbox` as either a `::grc/rect` record or a plain map, so a + ;; tighter schema on those two would reject files that are otherwise + ;; valid. The graph *column* types are tightened separately, where a + ;; wrong guess costs a column rather than a rejected file + ;; (`app.graph.schema.contract/type-overrides`). Outer `:svg-defs` + ;; keys are id strings at birth and keywords after JSON decode, so + ;; both are accepted; node tags decode back to keywords. + [:svg-attrs {:optional true} schema:svg-attrs] + [:svg-defs {:optional true} [:map-of [:or :string :keyword] schema:svg-def-node]] [:svg-transform {:optional true} :map] [:svg-viewbox {:optional true} :map]]) @@ -347,7 +427,9 @@ ;; string itself: `hi` becomes one svg-raw for the element ;; and another for `"hi"`. `app.common.files.shapes-builder/parse-svg-element` ;; carries a FIXME about exactly that. Both forms are legal and stored. - [:content {:optional true} [:or :map :string]]]) + ;; Element nodes follow `schema:svg-content-node` (tags decode back to + ;; keywords, attrs stay spelling-only). + [:content {:optional true} [:or schema:svg-content-node :string]]]) (def schema:image-attrs [:map {:title "ImageAttrs"} diff --git a/common/test/common_tests/files/shapes_builder_test.cljc b/common/test/common_tests/files/shapes_builder_test.cljc index 614ff7cf77..0c9418ffde 100644 --- a/common/test/common_tests/files/shapes_builder_test.cljc +++ b/common/test/common_tests/files/shapes_builder_test.cljc @@ -7,6 +7,8 @@ (ns common-tests.files.shapes-builder-test (:require [app.common.files.shapes-builder :as sb] + [app.common.svg :as csvg] + [app.common.types.color :as clr] [clojure.test :as t])) ;; Regression for https://github.com/penpot/penpot/issues/7869. @@ -78,8 +80,8 @@ (doseq [shape path-shapes] (let [stroke (first (:strokes shape))] ;; svg-attrs must carry stroke-linecap and stroke-linejoin for the renderers - (t/is (= "round" (get-in shape [:svg-attrs :strokeLinecap]))) - (t/is (= "round" (get-in shape [:svg-attrs :strokeLinejoin]))) + (t/is (= "round" (get-in shape [:svg-attrs :stroke-linecap]))) + (t/is (= "round" (get-in shape [:svg-attrs :stroke-linejoin]))) ;; stroke-cap-start/end must be inside the stroke entry, not at shape level (t/is (= :round (:stroke-cap-start stroke)) @@ -91,3 +93,234 @@ ;; stroke-style is not set at import time (nil means default solid rendering) (t/is (nil? (:stroke-style stroke))))))) + +;; Kebab storage convention for https://github.com/penpot/penpot/issues/11947. +;; Stored attribute keys (`:svg-attrs`, `:svg-defs` node `:attrs`, +;; `:content` `:attrs`) are always kebab-case: birth normalizes any +;; outside spelling, so export/import converges to the same state. + +(t/deftest attrs->kebab-props-whitelists-trims-and-kebabizes + (let [props (csvg/attrs->kebab-props {:fillRule "evenodd" + :stroke-width " 2 " + :viewBox "0 0 10 10" + :stdDeviation "3" + :xlink:href "#g" + :href "#g" + :stroke-style "dotted" + :style "fill:red;stroke-width:2" + :data-foo "drop-me"})] + (t/is (= "evenodd" (:fill-rule props))) + (t/is (= "2" (:stroke-width props)) "values are trimmed") + (t/is (= "0 0 10 10" (:view-box props))) + (t/is (= "3" (:std-deviation props))) + (t/is (= "#g" (:xlink-href props)) "colon is lost through the camel roundtrip") + (t/is (= "#g" (:href props))) + (t/is (= "dotted" (:stroke-style props)) "penpot-extra-attrs survive") + (t/is (= {:fill "red" :stroke-width "2"} (:style props)) "style maps are kebab too") + (t/is (not (contains? props :data-foo)) "unknown attrs are dropped") + (t/is (not (contains? props :fillRule)) "no camel keys survive") + (t/is (= props (csvg/attrs->kebab-props props)) "kebab is a fixed point"))) + +(t/deftest attrs->props-keeps-extra-attrs-in-source-spelling + ;; Extra attrs skip camelization so React passes them to the DOM + ;; silently (lowercase + hyphenated: no unknown-prop warning). + (t/is (= {:stroke-style "dotted" :fillRule "evenodd"} + (csvg/attrs->props {:stroke-style "dotted" :fillRule "evenodd"}))) + (t/is (= {:stroke-style "dotted"} + (:style (csvg/attrs->props {:style "stroke-style:dotted"})))) + (t/is (= {:stroke-style "dotted"} + (:style (csvg/attrs->kebab-props {:style "stroke-style:dotted"}))) + "birth agrees: kebab stays kebab")) + +(t/deftest collect-images-resolves-href-and-xlink-href + ;; Both href spellings resolve: plain `:href` (SVG2 style, kept by the + ;; whitelist so gradients and images survive birth) and `:xlink:href`. + (let [svg-data {:tag :svg + :attrs {} + :content [{:tag :image + :attrs {:xlink:href "#a" :width "10" :height "20"} + :content []} + {:tag :image + :attrs {:href "#b" :width "30" :height "40"} + :content []}]}] + (t/is (= [{:href "#a" :width 10 :height 20} + {:href "#b" :width 30 :height 40}] + (csvg/collect-images svg-data)))) + (t/is (= {:href "#b"} (csvg/attrs->kebab-props {:href "#b"})) + "plain href survives the whitelist")) + +(t/deftest birth-stores-svg-attrs-as-kebab + (let [svg-data {:name "kebab" + :tag :svg + :attrs {:width "100" :height "100" :viewBox "0 0 100 100"} + :content [{:tag :rect + :attrs {:x "10" :y "10" :width "80" :height "80" + :fill "red" + :fillRule "evenodd" + :stroke-width "2" + :stroke-style "dotted" + :data-foo "drop-me"} + :content []}]} + [_ children] (sb/create-svg-shapes svg-data {:x 0 :y 0} {} nil nil #{} false) + ;; NOTE: `create-svg-shapes` prepends a hidden background rect, skip it + rect (first (filter #(and (= :rect (:type %)) (not (:hidden %))) children))] + (t/is (some? rect)) + (let [attrs (:svg-attrs rect)] + (t/is (= "evenodd" (:fill-rule attrs))) + (t/is (= "2" (:stroke-width attrs))) + (t/is (= "dotted" (:stroke-style attrs)) "penpot-extra-attrs survive birth") + (t/is (not (contains? attrs :data-foo)) "unknown attrs are dropped") + (t/is (not (contains? attrs :fillRule)) "no camel keys survive")))) + +(t/deftest birth-stores-def-node-attrs-as-kebab + (let [svg-data {:name "defs" + :tag :svg + :attrs {:width "100" :height "100" :viewBox "0 0 100 100"} + :content [{:tag :linearGradient + :attrs {:id "MyGrad" + :gradientUnits "userSpaceOnUse" + :gradientTransform "rotate(45)" + :xlink:href "#missing" + :data-foo "drop-me"} + :content [{:tag :stop + :attrs {:offset "0" + :stop-color "#ffffff" + :style "stop-opacity:0.5"} + :content []}]} + {:tag :filter + :attrs {:id "blur1" :filterUnits "userSpaceOnUse"} + :content [{:tag :feGaussianBlur + :attrs {:stdDeviation "5"} + :content []}]} + {:tag :rect + :attrs {:x "10" :y "10" :width "80" :height "80" + :fill "url(#MyGrad)"} + :content []}]} + [root _] (sb/create-svg-shapes svg-data {:x 0 :y 0} {} nil nil #{} false) + defs (:svg-defs root) + grad (get defs "MyGrad") + blur (get defs "blur1")] + (t/is (contains? defs "MyGrad") "outer def id strings are byte-identical") + (t/is (= :linearGradient (:tag grad)) "tags are untouched") + (t/is (= "MyGrad" (get-in grad [:attrs :id])) ":id survives") + (t/is (= "userSpaceOnUse" (get-in grad [:attrs :gradient-units]))) + (t/is (= "rotate(45)" (get-in grad [:attrs :gradient-transform]))) + (t/is (= "#missing" (get-in grad [:attrs :xlink-href]))) + (t/is (not (contains? (:attrs grad) :data-foo)) "unknown def attrs are dropped") + (t/is (not (contains? (:attrs grad) :gradientUnits)) "no camel keys survive") + (t/is (= "0.5" (get-in grad [:content 0 :attrs :stop-opacity])) "nested stops are normalized") + (t/is (= "#ffffff" (get-in grad [:content 0 :attrs :stop-color]))) + (t/is (= "userSpaceOnUse" (get-in blur [:attrs :filter-units]))) + (t/is (= "5" (get-in blur [:content 0 :attrs :std-deviation]))))) + +(t/deftest birth-kebabizes-content-attrs-without-filtering + (let [svg-data {:name "raw" + :tag :svg + :attrs {:width "100" :height "100" :viewBox "0 0 100 100"} + :content [{:tag :text + :attrs {:x "10" :y "20" + :stroke-style "dotted" + :strokeWidth "3" + :data-foo "keep-me"} + :content ["hello"]}]} + [_ children] (sb/create-svg-shapes svg-data {:x 0 :y 0} {} nil nil #{} false) + raw (first (filter #(= :svg-raw (:type %)) children))] + (t/is (some? raw)) + (let [content (:content raw)] + (t/is (= :text (:tag content)) "tags are untouched") + (t/is (= "dotted" (get-in content [:attrs :stroke-style])) "the sidebar reads this key") + (t/is (= "3" (get-in content [:attrs :stroke-width])) "camel is kebab-ized") + (t/is (= "keep-me" (get-in content [:attrs :data-foo])) "unknown content keys survive") + (t/is (= ["hello"] (:content content)) "values are untouched")))) + +(t/deftest birth-setup-fill-reads-kebab-attrs + (let [svg-data {:name "fill" + :tag :svg + :attrs {:width "100" :height "100" :viewBox "0 0 100 100"} + :content [{:tag :rect + :attrs {:x "10" :y "10" :width "80" :height "80" + :fill "#ff0000" + :fill-opacity "0.5"} + :content []} + {:tag :rect + :attrs {:x "10" :y "50" :width "80" :height "40" + :style "fill:#00ff00;fill-opacity:0.25"} + :content []}]} + [_ children] (sb/create-svg-shapes svg-data {:x 0 :y 0} {} nil nil #{} false) + ;; NOTE: `create-svg-shapes` prepends a hidden background rect, skip it + [r1 r2] (filter #(and (= :rect (:type %)) (not (:hidden %))) children) + f1 (first (:fills r1)) + f2 (first (:fills r2))] + (t/is (some? (:fill-color f1))) + (t/is (= 0.5 (:fill-opacity f1))) + (t/is (nil? (get-in r1 [:svg-attrs :fill])) "consumed keys are removed") + (t/is (nil? (get-in r1 [:svg-attrs :fill-opacity]))) + (t/is (some? (:fill-color f2)) "style-form fills work too") + (t/is (= 0.25 (:fill-opacity f2))) + (t/is (nil? (get-in r2 [:svg-attrs :style :fill]))) + (t/is (nil? (get-in r2 [:svg-attrs :style :fill-opacity]))))) + +(t/deftest birth-setup-stroke-reads-kebab-attrs + (let [svg-data {:name "stroke" + :tag :svg + :attrs {:width "100" :height "100" :viewBox "0 0 100 100"} + :content [{:tag :path + :attrs {:d "M0 0L10 10" + :fill "none" + :stroke "#ff0000" + :stroke-width "3" + :stroke-opacity "0.5" + :stroke-linecap "round"} + :content []}]} + [_ children] (sb/create-svg-shapes svg-data {:x 0 :y 0} {} nil nil #{} false) + path (first (filter #(= :path (:type %)) children)) + stroke (first (:strokes path))] + (t/is (some? path)) + (t/is (some? (:stroke-color stroke))) + (t/is (= 3.0 (:stroke-width stroke))) + (t/is (= 0.5 (:stroke-opacity stroke))) + (t/is (= :round (:stroke-cap-start stroke))) + (t/is (= :round (:stroke-cap-end stroke))) + (t/is (= "round" (get-in path [:svg-attrs :stroke-linecap])) "linecap stays for renderers") + (t/is (nil? (get-in path [:svg-attrs :stroke-width])) "consumed keys are removed") + (t/is (nil? (get-in path [:svg-attrs :stroke-opacity]))))) + +(t/deftest birth-setup-opacity-reads-kebab-blend-mode + ;; Regression for PR #12006 review: `setup-opacity` still looked up + ;; `:mixBlendMode` (camel) after birth started storing kebab keys, so + ;; `mix-blend-mode` survived in `:svg-attrs` and `:blend-mode` was + ;; never set. Birth stores the style map kebab-ized. + (let [svg-data {:name "blend" + :tag :svg + :attrs {:width "100" :height "100" :viewBox "0 0 100 100"} + :content [{:tag :rect + :attrs {:x "10" :y "10" :width "80" :height "80" + :style "mix-blend-mode: multiply"} + :content []}]} + [_ children] (sb/create-svg-shapes svg-data {:x 0 :y 0} {} nil nil #{} false) + rect (first (filter #(and (= :rect (:type %)) (not (:hidden %))) children))] + (t/is (some? rect)) + (t/is (= :multiply (:blend-mode rect))) + (t/is (nil? (get-in rect [:svg-attrs :style :mix-blend-mode])) "consumed keys are removed"))) + +(t/deftest birth-child-style-beats-inherited-group-attr + ;; Regression: `inherit-attributes` filtered on `(:styles attrs)` + ;; (plural, always nil) instead of `(:style attrs)`, so a group attr + ;; leaked next to the child's own style and `setup-stroke` preferred + ;; the group color. Per CSS the child's inline style wins. + (let [svg-data {:name "inherit" + :tag :svg + :attrs {:width "100" :height "100" :viewBox "0 0 100 100"} + :content [{:tag :g + :attrs {:stroke "#ff0000"} + :content [{:tag :rect + :attrs {:x "10" :y "10" :width "80" :height "80" + :fill "none" + :style "stroke:#0000ff"} + :content []}]}]} + [_ children] (sb/create-svg-shapes svg-data {:x 0 :y 0} {} nil nil #{} false) + rect (first (filter #(and (= :rect (:type %)) (not (:hidden %))) children)) + stroke (first (:strokes rect))] + (t/is (some? rect)) + (t/is (= (clr/parse "#0000ff") (:stroke-color stroke)) "child style beats group attr") + (t/is (nil? (get-in rect [:svg-attrs :stroke])) "group attr is not inherited over child style"))) diff --git a/common/test/common_tests/files_builder_test.cljc b/common/test/common_tests/files_builder_test.cljc index 23891a583b..db0242ea93 100644 --- a/common/test/common_tests/files_builder_test.cljc +++ b/common/test/common_tests/files_builder_test.cljc @@ -18,6 +18,39 @@ :stroke-color color :stroke-opacity 1}]) +(t/deftest add-shape-normalizes-svg-defs-and-content-to-kebab + ;; Defense in depth: generic producers that bypass `shapes-builder` + ;; still land on the stored kebab convention for defs and content. + (let [file-id (uuid/next) + page-id (uuid/next) + shape-id (uuid/next) + state (-> (fb/create-state) + (fb/add-file {:id file-id :name "Test file"}) + (fb/add-page {:id page-id :name "Page 1"}) + (fb/add-shape {:id shape-id + :type :svg-raw + :name "raw" + :x 0 + :y 0 + :width 10 + :height 10 + :content {:tag :g + :attrs {:strokeWidth "2" + :data-foo "keep-me"} + :content []} + :svg-attrs {:fillRule "evenodd"} + :svg-defs {"g1" {:tag :linearGradient + :attrs {:id "g1" + :gradientUnits "userSpaceOnUse"} + :content []}}})) + shape (fb/get-shape state shape-id) + node (get (:svg-defs shape) "g1")] + (t/is (= "evenodd" (get-in shape [:svg-attrs :fill-rule]))) + (t/is (= "userSpaceOnUse" (get-in node [:attrs :gradient-units]))) + (t/is (= "2" (get-in shape [:content :attrs :stroke-width]))) + (t/is (= "keep-me" (get-in shape [:content :attrs :data-foo])) + "unknown content keys survive"))) + (t/deftest add-bool-uses-difference-head-style (let [file-id (uuid/next) page-id (uuid/next) diff --git a/common/test/common_tests/files_migrations_0034_test.cljc b/common/test/common_tests/files_migrations_0034_test.cljc new file mode 100644 index 0000000000..b9f1972ffa --- /dev/null +++ b/common/test/common_tests/files_migrations_0034_test.cljc @@ -0,0 +1,231 @@ +;; 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 SUBSIDIARY SL + +(ns common-tests.files-migrations-0034-test + (:require + [app.common.files.migrations :as cfm] + [app.common.geom.rect :as grc] + [app.common.types.file :as ctf] + [app.common.types.shape :as cts] + [app.common.uuid :as uuid] + [clojure.test :as t])) + +;; 0034-fix-shape-svg-attrs-and-defs +;; Stored svg attribute keys are kebab-case (issue #11947). Old birth and +;; the cleaner era stored camelCase React prop names, which the v3 reader +;; kebab-ized on import beyond repair. This migration converts once, with +;; the same birth transforms: `:svg-attrs` and `:svg-defs` node attrs go +;; through the whitelist+kebab transform, svg-raw `:content` attrs are +;; kebab-ized spelling-only. It replaces "0021-fix-shape-svg-attrs" (id +;; withdrawn): files that recorded it migrate again through this one. + +(def ^:private migration-id + "0034-fix-shape-svg-attrs-and-defs") + +(defn- make-shape + [shape-id shape] + (assoc shape :id shape-id)) + +(defn- camel-shape + [shape-id] + (make-shape shape-id + {:type :rect + :svg-attrs {:fillRule "evenodd" + :stroke-width "2" + :stroke-style "dotted" + :data-foo "drop-me"}})) + +(defn- camel-defs + [] + {"g1" {:tag :linearGradient + :attrs {:id "g1" + :gradientUnits "userSpaceOnUse" + :xlink:href "#base" + :data-x "drop-me"} + :content [{:tag :stop + :attrs {:offset "0" + :stopColor "#ffffff"} + :content []}]}}) + +(t/deftest migration-0034-converts-camel-svg-attrs-to-kebab-in-pages + (let [shape-id (uuid/next) + page-id (uuid/next) + data {:pages-index + {page-id + {:objects + {shape-id (camel-shape shape-id)}}}} + data' (cfm/migrate-data data migration-id) + attrs (get-in data' [:pages-index page-id :objects shape-id :svg-attrs])] + (t/is (= "evenodd" (:fill-rule attrs))) + (t/is (= "2" (:stroke-width attrs))) + (t/is (= "dotted" (:stroke-style attrs)) "penpot-extra-attrs survive") + (t/is (not (contains? attrs :data-foo)) "unknown attrs are dropped") + (t/is (not (contains? attrs :fillRule)) "no camel keys survive"))) + +(t/deftest migration-0034-converts-camel-svg-attrs-to-kebab-in-components + (let [shape-id (uuid/next) + component-id (uuid/next) + data {:components + {component-id + {:objects + {shape-id (camel-shape shape-id)}}}} + data' (cfm/migrate-data data migration-id) + attrs (get-in data' [:components component-id :objects shape-id :svg-attrs])] + (t/is (= "evenodd" (:fill-rule attrs))) + (t/is (not (contains? attrs :fillRule)) "no camel keys survive"))) + +(t/deftest migration-0034-normalizes-def-node-attrs + (let [shape-id (uuid/next) + page-id (uuid/next) + data {:pages-index + {page-id + {:objects + {shape-id (make-shape shape-id + {:type :rect + :svg-defs (camel-defs)})}}}} + data' (cfm/migrate-data data migration-id) + grad (get-in data' [:pages-index page-id :objects shape-id :svg-defs "g1"])] + (t/is (= :linearGradient (:tag grad)) "tags are untouched") + (t/is (= "g1" (get-in grad [:attrs :id])) ":id survives") + (t/is (= "userSpaceOnUse" (get-in grad [:attrs :gradient-units]))) + (t/is (= "#base" (get-in grad [:attrs :xlink-href]))) + (t/is (not (contains? (:attrs grad) :data-x)) "unknown def attrs are dropped") + (t/is (= "#ffffff" (get-in grad [:content 0 :attrs :stop-color])) + "nested stops are normalized"))) + +(t/deftest migration-0034-kebabizes-content-spelling-only + (let [shape-id (uuid/next) + page-id (uuid/next) + raw-id (uuid/next) + data {:pages-index + {page-id + {:objects + {shape-id (make-shape shape-id + {:type :path + :content [{:marker "untouched"}]}) + raw-id (make-shape raw-id + {:type :svg-raw + :content {:tag :g + :attrs {:strokeWidth "2" + :data-foo "keep-me"} + :content [{:tag :path + :attrs {:strokeLinecap "round"} + :content []}]}})}}}} + data' (cfm/migrate-data data migration-id) + path (get-in data' [:pages-index page-id :objects shape-id]) + raw (get-in data' [:pages-index page-id :objects raw-id]) + content (:content raw)] + (t/is (= [{:marker "untouched"}] (:content path)) + "non-svg-raw content is untouched") + (t/is (= "2" (get-in content [:attrs :stroke-width]))) + (t/is (= "keep-me" (get-in content [:attrs :data-foo])) + "unknown content keys survive") + (t/is (= "round" (get-in content [:content 0 :attrs :stroke-linecap]))))) + +(t/deftest migration-0034-leaves-kebab-files-equal + (let [shape-id (uuid/next) + page-id (uuid/next) + data {:pages-index + {page-id + {:objects + {shape-id (make-shape shape-id + {:type :rect + :svg-attrs {:fill-rule "evenodd" + :stroke-width "2"} + :svg-defs {"g1" {:tag :linearGradient + :attrs {:id "g1" + :gradient-units "userSpaceOnUse"} + :content []}}})}}}} + data' (cfm/migrate-data data migration-id)] + (t/is (= data data') "kebab is a fixed point"))) + +(t/deftest migration-0034-is-idempotent + (let [shape-id (uuid/next) + page-id (uuid/next) + data {:pages-index + {page-id + {:objects + {shape-id (camel-shape shape-id)}}}} + data' (cfm/migrate-data data migration-id)] + (t/is (= data' (cfm/migrate-data data' migration-id))))) + +(t/deftest migration-0034-normalizes-flat-svg-viewbox-to-rect + ;; Carried over from the withdrawn 0021: legacy files hold + ;; `:svg-viewbox` as a plain map instead of a rect record. + (let [shape-id (uuid/next) + page-id (uuid/next) + data {:pages-index + {page-id + {:objects + {shape-id (make-shape shape-id + {:type :rect + :svg-viewbox {:x 1 :y 2 :width 3 :height 4}})}}}} + data' (cfm/migrate-data data migration-id) + viewbox (get-in data' [:pages-index page-id :objects shape-id :svg-viewbox])] + (t/is (grc/rect? viewbox) "flat viewbox becomes a rect record") + (t/is (= 1 (:x viewbox))) + (t/is (= 2 (:y viewbox))) + (t/is (= 3 (:width viewbox))) + (t/is (= 4 (:height viewbox))) + (t/is (= data' (cfm/migrate-data data' migration-id)) + "re-running is stable"))) + +(t/deftest migration-0034-passes-non-maps-through + (let [shape-id (uuid/next) + page-id (uuid/next) + data {:pages-index + {page-id + {:objects + {shape-id (make-shape shape-id + {:type :rect + :svg-attrs "junk" + :svg-defs "junk"})}}}} + data' (cfm/migrate-data data migration-id) + shape (get-in data' [:pages-index page-id :objects shape-id])] + (t/is (= "junk" (:svg-attrs shape)) "non-maps reach validation untouched") + (t/is (= "junk" (:svg-defs shape))))) + +(t/deftest migration-0034-junk-viewbox-becomes-default-rect + ;; Unlike `:svg-attrs`/`:svg-defs` (passed through to validation), + ;; `:svg-viewbox` goes through `make-rect`, which does not throw on + ;; junk: it falls back to the default rect. Inherited from 0021. + (let [shape-id (uuid/next) + page-id (uuid/next) + data {:pages-index + {page-id + {:objects + {shape-id (make-shape shape-id + {:type :rect + :svg-viewbox "junk"})}}}} + data' (cfm/migrate-data data migration-id) + viewbox (get-in data' [:pages-index page-id :objects shape-id :svg-viewbox])] + (t/is (grc/rect? viewbox) "junk viewbox becomes a rect, not an error") + (t/is (= 0 (:x viewbox))) + (t/is (= 0.01 (:width viewbox))))) + +(t/deftest migration-0034-runs-through-file-migration + ;; A file that recorded the withdrawn "0021-fix-shape-svg-attrs" (and + ;; everything else current) still needs migration: 0021 left + ;; `available-migrations`, which re-arms it for 0034. + (let [shape-id (uuid/next) + file (ctf/make-file {:name "Legacy camel svg attrs"}) + page-id (first (get-in file [:data :pages])) + shape (-> (cts/setup-shape {:id shape-id :type :rect}) + (assoc :svg-attrs {:fillRule "evenodd"})) + file (-> file + (assoc :migrations (-> cfm/available-migrations + (disj migration-id) + (conj "0021-fix-shape-svg-attrs"))) + (assoc-in [:data :pages-index page-id :objects shape-id] shape)) + file' (cfm/migrate-file file {})] + (t/is (cfm/need-migration? file) "new migration detected") + (t/is (not (cfm/need-migration? file')) "new migration recorded") + (t/is (contains? (:migrations file') migration-id) "migration id persisted") + (t/is (not (contains? cfm/available-migrations "0021-fix-shape-svg-attrs")) + "the withdrawn 0021 id stays out: its camel-restoring transform must never run again") + (t/is (= "evenodd" + (get-in file' [:data :pages-index page-id :objects shape-id :svg-attrs :fill-rule])) + "migration repaired file data before schema validation"))) diff --git a/common/test/common_tests/runner.cljc b/common/test/common_tests/runner.cljc index 88e10df432..80590a030b 100644 --- a/common/test/common_tests/runner.cljc +++ b/common/test/common_tests/runner.cljc @@ -21,6 +21,7 @@ [common-tests.files-changes-test] [common-tests.files-migrations-0025-test] [common-tests.files-migrations-0026-test] + [common-tests.files-migrations-0034-test] [common-tests.files-migrations-test] [common-tests.files.comp-processors-test] [common-tests.files.helpers-test] @@ -103,6 +104,7 @@ [common-tests.types.shape-decode-encode-test] [common-tests.types.shape-interactions-test] [common-tests.types.shape-layout-test] + [common-tests.types.shape-test] [common-tests.types.stroke-test] [common-tests.types.text-test] [common-tests.types.token-test] @@ -125,6 +127,7 @@ 'common-tests.files-changes-test 'common-tests.files-migrations-0025-test 'common-tests.files-migrations-0026-test + 'common-tests.files-migrations-0034-test 'common-tests.files-migrations-test 'common-tests.files.comp-processors-test 'common-tests.files.helpers-test @@ -207,6 +210,7 @@ 'common-tests.types.shape-decode-encode-test 'common-tests.types.shape-interactions-test 'common-tests.types.shape-layout-test + 'common-tests.types.shape-test 'common-tests.types.stroke-test 'common-tests.types.text-test 'common-tests.types.token-test diff --git a/common/test/common_tests/svg_test.cljc b/common/test/common_tests/svg_test.cljc index a8c8aa17e2..b2b6afa9ae 100644 --- a/common/test/common_tests/svg_test.cljc +++ b/common/test/common_tests/svg_test.cljc @@ -8,7 +8,8 @@ (:require [app.common.data :as d] [app.common.svg :as svg] - [clojure.test :as t])) + [clojure.test :as t] + [cuerdas.core :as str])) (t/deftest clean-attrs-1 (let [attrs {:class "foobar"} @@ -41,3 +42,25 @@ :attrs {:id "shadow"} :content [{:tag :feDropShadow}]})] (t/is (= [:feDropShadow] (mapv :tag (get-in defs ["shadow" :content])))))) + +(t/deftest stored-attr-display-name-uses-spec-spelling + ;; The svg-attrs menu shows these labels: kebab storage matches the + ;; spec for most keys, but spec-camel keys show their source spelling. + (t/is (= "class" (svg/stored-attr-display-name :class-name))) + (t/is (= "viewBox" (svg/stored-attr-display-name :view-box))) + (t/is (= "stroke-width" (svg/stored-attr-display-name :stroke-width))) + (t/is (= "stdDeviation" (svg/stored-attr-display-name :std-deviation))) + (t/is (= "fill-rule" (svg/stored-attr-display-name :fill-rule))) + (t/is (= "data-foo" (svg/stored-attr-display-name :data-foo)) + "unknown keys fall back to their name")) + +(t/deftest stored-attr-display-names-round-trip + ;; Every whitelist source key maps back to its own spelling: guards + ;; against two source keys sharing one stored key (last-wins would + ;; silently mislabel one of them). + (doseq [k (concat svg/svg-attrs + svg/svg-presentation-attrs + svg/penpot-extra-attrs)] + (let [stored ((comp keyword str/kebab name) (svg/prop-key k))] + (t/is (= (name k) (svg/stored-attr-display-name stored)) + (str "display round-trip for " k))))) diff --git a/common/test/common_tests/types/shape_test.cljc b/common/test/common_tests/types/shape_test.cljc new file mode 100644 index 0000000000..2d98a7d405 --- /dev/null +++ b/common/test/common_tests/types/shape_test.cljc @@ -0,0 +1,109 @@ +;; 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 SUBSIDIARY SL + +(ns common-tests.types.shape-test + (:require + [app.common.json :as json] + [app.common.schema :as sm] + [app.common.schema.generators :as sg] + [app.common.types.shape :as tsh :refer [schema:shape]] + [clojure.test :as t])) + +;; Shape schema hardening for https://github.com/penpot/penpot/issues/11947. +;; Stored svg attribute keys are kebab-case: `:svg-attrs` is closed over +;; the whitelist in stored spelling (derived from the same set birth +;; uses, so the two cannot drift apart), and `:svg-defs` nodes share one +;; node schema with svg-raw `:content`. + +(defn- sample-shape + "A generated shape: valid by construction, a base to assoc svg keys onto." + [] + (first (sg/sample (sg/generator schema:shape) {:size 20}))) + +(t/deftest svg-attrs-accepts-kebab-maps + (let [shape (assoc (sample-shape) + :svg-attrs {:fill-rule "evenodd" + :stroke-width "2" + :stroke-style "dotted" + :style {:fill-opacity "0.5"}})] + (t/is (tsh/check-shape-attrs shape) "kebab svg-attrs validate"))) + +(t/deftest svg-attrs-rejects-non-maps-and-unknown-keys + (let [shape (sample-shape) + throws? (fn [s] + (try (tsh/check-shape-attrs s) false + (catch #?(:clj Exception :cljs js/Error) _ true)))] + (t/is (throws? (assoc shape :svg-attrs "junk")) + "non-map svg-attrs fail validation") + (t/is (throws? (assoc shape :svg-attrs {:fillRule "evenodd"})) + "camel keys fail validation: storage is kebab-only") + (t/is (throws? (assoc shape :svg-attrs {:data-foo "x"})) + "unknown keys fail validation"))) + +(t/deftest svg-def-node-validates-tag-and-attrs + (let [shape (sample-shape) + node {:tag :linearGradient + :attrs {:id "g1" + :gradient-units "userSpaceOnUse"} + :content [{:tag :stop + :attrs {:offset "0" + :stop-color "#ffffff"} + :content []}]} + with-defs (fn [defs] (assoc shape :svg-defs defs)) + throws? (fn [s] + (try (tsh/check-shape-attrs s) false + (catch #?(:clj Exception :cljs js/Error) _ true)))] + (t/is (tsh/check-shape-attrs (with-defs {"g1" node})) + "string outer keys validate (birth spelling)") + (t/is (tsh/check-shape-attrs (with-defs {:g1 node})) + "keyword outer keys validate (post-decode spelling)") + (t/is (throws? (with-defs {"g1" (assoc node :tag "linearGradient")})) + "non-keyword tags fail validation") + (t/is (throws? (with-defs {"g1" (assoc node :attrs {:data-x "1"})})) + "unknown def attrs fail validation") + (t/is (throws? (with-defs {"g1" (assoc node :attrs "junk")})) + "non-map def attrs fail validation"))) + +(t/deftest svg-content-keeps-unknown-keys + ;; `:content` attrs are spelling-only: the sidebar reads keys outside + ;; the whitelist (e.g. `:stroke-style`), so the schema stays open. + ;; NOTE: the sample seed is pinned. Unpinned sampling flaked (~1% + ;; of runs): some seeds deal zero `:svg-raw` shapes in 50 samples, + ;; `first` yields nil, and the `assoc` below builds a typeless + ;; `{:content …}` map that fails the `:multi` dispatch on `:type`. + (let [shape (->> (sg/sample (sg/generator schema:shape) {:size 50 :seed 42}) + (filter #(= :svg-raw (:type %))) + (first) + (#(assoc % :content {:tag :g + :attrs {:stroke-style "dotted" + :stroke-width "2" + :data-foo "keep-me"} + :content []})))] + (t/is (some? shape) "an svg-raw shape was sampled") + (t/is (tsh/check-shape-attrs shape) + ":stroke-style in content attrs validates"))) + +(t/deftest generated-shapes-carry-no-svg-provenance + ;; The `:gen/fmap` mitigation: svg provenance is import-only data, + ;; random attrs would pollute every generative test. + (let [shapes (sg/sample (sg/generator schema:shape) {:size 20})] + (t/is (= 20 (count shapes))) + (doseq [shape shapes] + (t/is (not (contains? shape :svg-attrs)) "no generated svg-attrs") + (t/is (not (contains? shape :svg-defs)) "no generated svg-defs")))) + +(t/deftest svg-attrs-survive-json-encoding-untouched + ;; What binfile export relies on: kebab keys (and nested `:style`) + ;; round-trip byte-identical through the JSON layer. + (let [attrs {:fill-rule "evenodd" + :stroke-style "dotted" + :style {:fill-opacity "0.5"}} + shape (assoc (sample-shape) :svg-attrs attrs) + encode (sm/encoder schema:shape (sm/json-transformer)) + data (-> (encode shape) + (json/encode :key-fn json/write-camel-key) + (json/decode :key-fn json/read-kebab-key))] + (t/is (= attrs (:svg-attrs data))))) diff --git a/frontend/src/app/main/data/workspace/path/shapes_to_path.cljs b/frontend/src/app/main/data/workspace/path/shapes_to_path.cljs index 7ea9afe315..fb522f6839 100644 --- a/frontend/src/app/main/data/workspace/path/shapes_to_path.cljs +++ b/frontend/src/app/main/data/workspace/path/shapes_to_path.cljs @@ -132,7 +132,7 @@ :fills [(stroke->fill stroke)] :strokes []} (:even-odd? result) - (assoc :svg-attrs {:fillRule "evenodd"}))))))) + (assoc :svg-attrs {:fill-rule "evenodd"}))))))) (:strokes shape))) (defn convert-selected-strokes-to-path diff --git a/frontend/src/app/main/ui/shapes/attrs.cljs b/frontend/src/app/main/ui/shapes/attrs.cljs index 20defd27f0..b03d37bc99 100644 --- a/frontend/src/app/main/ui/shapes/attrs.cljs +++ b/frontend/src/app/main/ui/shapes/attrs.cljs @@ -168,6 +168,10 @@ (dm/str render-id "-" id) id))) (dissoc :id) + ;; Stored keys are kebab-case; React needs camelCase props. + ;; Stored data is whitelist-clean by construction, so this + ;; conversion drops nothing. + (csvg/attrs->props) (json/->js :key-fn name))))) (defn get-fill-style diff --git a/frontend/src/app/main/ui/shapes/svg_defs.cljs b/frontend/src/app/main/ui/shapes/svg_defs.cljs index 32d6a0b6c5..a824f56e6b 100644 --- a/frontend/src/app/main/ui/shapes/svg_defs.cljs +++ b/frontend/src/app/main/ui/shapes/svg_defs.cljs @@ -33,20 +33,20 @@ (let [{:keys [tag attrs content]} node transform-gradient? (and (contains? csvg/gradient-tags tag) - (= "userSpaceOnUse" (get attrs :gradientUnits "objectBoundingBox"))) + (= "userSpaceOnUse" (get attrs :gradient-units "objectBoundingBox"))) transform-pattern? (and (= :pattern tag) - (= "userSpaceOnUse" (get attrs :patternContentUnits "userSpaceOnUse")) - (= "userSpaceOnUse" (get attrs :patternUnits "userSpaceOnUse"))) + (= "userSpaceOnUse" (get attrs :pattern-content-units "userSpaceOnUse")) + (= "userSpaceOnUse" (get attrs :pattern-units "userSpaceOnUse"))) transform-clippath? (and (= :clipPath tag) - (= "userSpaceOnUse" (get attrs :clipPathUnits "userSpaceOnUse"))) + (= "userSpaceOnUse" (get attrs :clip-path-units "userSpaceOnUse"))) transform-filter? (and (contains? csvg/filter-tags tag) - (= "userSpaceOnUse" (get attrs :filterUnits "objectBoundingBox"))) + (= "userSpaceOnUse" (get attrs :filter-units "objectBoundingBox"))) transform-mask? (and (= :mask tag) - (= "userSpaceOnUse" (get attrs :maskUnits "objectBoundingBox"))) + (= "userSpaceOnUse" (get attrs :mask-units "objectBoundingBox"))) attrs (-> attrs diff --git a/frontend/src/app/main/ui/workspace/sidebar/options/menus/svg_attrs.cljs b/frontend/src/app/main/ui/workspace/sidebar/options/menus/svg_attrs.cljs index ad07f27f6d..6c8bc5df52 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/options/menus/svg_attrs.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/options/menus/svg_attrs.cljs @@ -7,7 +7,7 @@ (ns app.main.ui.workspace.sidebar.options.menus.svg-attrs (:require-macros [app.main.style :as stl]) (:require - [app.common.data :as d] + [app.common.svg :as csvg] [app.main.data.workspace.shapes :as dwsh] [app.main.store :as st] [app.main.ui.components.title-bar :refer [title-bar*]] @@ -40,7 +40,7 @@ (fn [] (on-delete attr))) - label (->> attr last d/name)] + label (->> attr last csvg/stored-attr-display-name)] [:* (if (string? value) [:div {:class (stl/css :attr-content)} @@ -54,7 +54,7 @@ deprecated-icon/remove-icon]]] [:div {:class (stl/css :attr-nested-content)} [:div {:class (stl/css :attr-title)} - (str (d/name (last attr)))] + (str (csvg/stored-attr-display-name (last attr)))] (for [[key value] value] [:div {:class (stl/css :attr-row) :key key} [:> attribute-value* {:key key diff --git a/frontend/test/frontend_tests/svg_fills_test.cljs b/frontend/test/frontend_tests/svg_fills_test.cljs index cecb4ad3b8..cbeb787e17 100644 --- a/frontend/test/frontend_tests/svg_fills_test.cljs +++ b/frontend/test/frontend_tests/svg_fills_test.cljs @@ -7,16 +7,18 @@ (ns frontend-tests.svg-fills-test (:require [app.common.render-wasm.svg-derived :as svg-derived] + [app.main.ui.shapes.attrs :as attrs] + [app.util.object :as obj] [cljs.test :refer [deftest is testing]])) (def sample-shape {:selrect {:x 100 :y 200 :width 200 :height 100} - :svg-attrs {:fillOpacity "0.5" + :svg-attrs {:fill-opacity "0.5" :style {:fill "url(#grad)"}} :svg-defs {"grad" {:tag :radialGradient :attrs {:id "grad" - :gradientUnits "userSpaceOnUse" + :gradient-units "userSpaceOnUse" :cx "150" :cy "250" :r "50"} @@ -42,17 +44,45 @@ (deftest skips-when-no-svg-fill (is (nil? (svg-derived/svg-fill->fills {:svg-attrs {:fill "none"}})))) +;; Stored (post-0034) spelling: migration `0034` turned stop `:style` +;; strings into kebab maps via `attrs->kebab-props`. The WASM fill +;; derivation must read both spellings, or gradients lose their stops. +(def stored-shape + {:selrect {:x 100 :y 200 :width 200 :height 100} + :svg-attrs {:fill-opacity "0.5" + :style {:fill "url(#grad)"}} + :svg-defs {"grad" + {:tag :radialGradient + :attrs {:id "grad" + :gradient-units "userSpaceOnUse" + :cx "150" + :cy "250" + :r "50"} + :content [{:tag :stop + :attrs {:offset "0" + :style {:stop-color "#ff0000" :stop-opacity "1"}}} + {:tag :stop + :attrs {:offset "1" + :style {:stop-color "#00ff00" :stop-opacity "0"}}}]}}}) + +(deftest builds-gradient-fill-from-stored-map-styles + (let [fills (svg-derived/svg-fill->fills stored-shape) + gradient (get-in (first fills) [:fill-color-gradient])] + (is (= 1 (count fills))) + (is (= "#ff0000" (get-in gradient [:stops 0 :color]))) + (is (= "#00ff00" (get-in gradient [:stops 1 :color]))))) + (def elliptical-shape {:selrect {:x 0 :y 0 :width 200 :height 100} :svg-attrs {:style {:fill "url(#grad-ellipse)"}} :svg-defs {"grad-ellipse" {:tag :radialGradient :attrs {:id "grad-ellipse" - :gradientUnits "userSpaceOnUse" + :gradient-units "userSpaceOnUse" :cx "50" :cy "50" :r "50" - :gradientTransform "matrix(2 0 0 1 0 0)"} + :gradient-transform "matrix(2 0 0 1 0 0)"} :content [{:tag :stop :attrs {:offset "0" :style "stop-color:#000000;stop-opacity:1"}} @@ -94,4 +124,20 @@ (is (= 1 (count fills))) (is (= "#fabada" (:fill-color (first fills)))))) +;; The classic renderer funnel: stored kebab keys reach React as +;; camelCase props, so a shape renders identical props before and after +;; the kebab migration. Extra attrs keep their source spelling so React +;; passes them to the DOM silently (no unknown-prop warning). +(deftest svg-attrs-render-as-camel-props + (let [props (attrs/get-svg-props {:svg-attrs {:fill-rule "evenodd" + :stroke-width "2" + :stroke-style "dotted" + :style {:fill-opacity "0.5"}}} + "r1")] + (is (= "evenodd" (obj/get props "fillRule"))) + (is (= "2" (obj/get props "strokeWidth"))) + (is (= "dotted" (obj/get props "stroke-style"))) + (is (nil? (obj/get props "strokeStyle")) "no camel duplicate for extras") + (is (= "0.5" (obj/get (obj/get props "style") "fillOpacity"))))) + diff --git a/frontend/test/frontend_tests/svg_filters_test.cljs b/frontend/test/frontend_tests/svg_filters_test.cljs index b1eb4be721..f20d3a219e 100644 --- a/frontend/test/frontend_tests/svg_filters_test.cljs +++ b/frontend/test/frontend_tests/svg_filters_test.cljs @@ -14,7 +14,7 @@ :svg-defs {"simple-filter" {:tag :filter :content [{:tag :feOffset :attrs {:dx "2" :dy "3"}} - {:tag :feGaussianBlur :attrs {:stdDeviation "4"}}]}}}) + {:tag :feGaussianBlur :attrs {:std-deviation "4"}}]}}}) (defn- filter-shape [content] @@ -46,7 +46,7 @@ (deftest derives-layer-blur-from-plain-blur-filter (let [shape (svg-derived/apply-svg-filters - (filter-shape [{:tag :feGaussianBlur :attrs {:stdDeviation "4"}}]))] + (filter-shape [{:tag :feGaussianBlur :attrs {:std-deviation "4"}}]))] (is (= :layer-blur (get-in shape [:blur :type]))) (is (= 4.0 (get-in shape [:blur :value]))) (is (nil? (:shadow shape))))) @@ -55,7 +55,7 @@ (is (= [{:color "#ff0000" :opacity 0.3}] (map :color (derived-shadow [{:tag :feOffset :attrs {:in "SourceAlpha" :dx "12" :dy "12"}} - {:tag :feGaussianBlur :attrs {:stdDeviation "4"}} + {:tag :feGaussianBlur :attrs {:std-deviation "4"}} {:tag :feColorMatrix :attrs {:type "matrix" :values "0 0 0 0 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0.3 0"}}]))))) @@ -70,7 +70,7 @@ :type "matrix" :values "0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 127 0"}} {:tag :feOffset :attrs {:dy "4"}} - {:tag :feGaussianBlur :attrs {:stdDeviation "2"}} + {:tag :feGaussianBlur :attrs {:std-deviation "2"}} {:tag :feColorMatrix :attrs {:type "matrix" :values "0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0.25 0"}}]))))) @@ -79,12 +79,12 @@ (is (= [{:color "#00ff00" :opacity 0.5}] (map :color (derived-shadow [{:tag :feOffset :attrs {:dx "2" :dy "2"}} - {:tag :feGaussianBlur :attrs {:stdDeviation "2"}} + {:tag :feGaussianBlur :attrs {:std-deviation "2"}} {:tag :feFlood :attrs {:flood-color "#00ff00" :flood-opacity "0.5"}}]))))) (deftest derives-shadow-from-fe-drop-shadow (let [content [{:tag :feDropShadow - :attrs {:dx "12" :dy "8" :stdDeviation "4" + :attrs {:dx "12" :dy "8" :std-deviation "4" :flood-color "#000" :flood-opacity "0.3"}}] shape (svg-derived/apply-svg-filters (filter-shape content))] (is (nil? (:blur shape)))