github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-05-24 09:23:40 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Fix problem of path position on variant change (#9801)

Browse Source
This commit is contained in:
Alonso Torres 2026-05-21 16:39:26 +02:00 committed by GitHub
parent 4a73a97a32
commit a7b17f54f1
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 208 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

36
common/src/app/common/logic/libraries.cljc
View File

@ -20,6 +20,7 @@
[app.common.logging :as log]
[app.common.logic.shapes :as cls]
[app.common.logic.variant-properties :as clvp]
[app.common.math :as mth]
[app.common.path-names :as cpn]
[app.common.types.component :as ctk]
[app.common.types.components-list :as ctkl]
@ -2086,19 +2087,40 @@
displacement (x/y).
For :selrect we compare width/height only;
for :points we normalise each vector so the first point is the
origin before comparing."
origin before comparing.
For :content on path shapes we compare the bounding-box width/height
of the path segments, again ignoring absolute position.
Comparisons use `mth/close?` (and `gpt/close?` for points) rather than
exact `=` because `previous-shape` here may carry sub-pixel drift from
interactive transform modifiers (e.g. an alt-drag duplicate of a
variant whose children are component instances). Without tolerance
this guard would miss equivalent geometries and let the `:else` branch
in `update-attrs-on-switch` carry stale `:selrect`/`:points` from the
pre-switch shape onto the freshly instantiated target."
[shape origin-shape attr]
(or (and (= attr :selrect)
(= (-> shape :selrect :width) (-> origin-shape :selrect :width))
(= (-> shape :selrect :height) (-> origin-shape :selrect :height)))
(mth/close? (-> shape :selrect :width) (-> origin-shape :selrect :width))
(mth/close? (-> shape :selrect :height) (-> origin-shape :selrect :height)))
(and (= attr :points)
(let [normalize-pts (fn [pts]
(when (seq pts)
(let [f (first pts)]
(mapv #(gpt/subtract % f) pts))))]
(= (normalize-pts (get shape :points))
(normalize-pts (get origin-shape :points)))))))
(mapv #(gpt/subtract % f) pts))))
a (normalize-pts (get shape :points))
b (normalize-pts (get origin-shape :points))]
(and (= (count a) (count b))
(every? identity (map gpt/close? a b)))))
(and (= attr :content)
(cfh/path-shape? shape)
(let [ca (:content shape)
cb (:content origin-shape)]
(and (some? ca) (some? cb)
(let [selrect-a (segment/content->selrect ca)
selrect-b (segment/content->selrect cb)]
(and (some? selrect-a) (some? selrect-b)
(mth/close? (:width selrect-a) (:width selrect-b))
(mth/close? (:height selrect-a) (:height selrect-b)))))))))
(defn update-attrs-on-switch
"Copy attributes that have changed in the shape previous to the switch

2
common/test/common_tests/data_test.cljc
View File

@ -888,7 +888,7 @@
(t/is (not (d/editable-collection? "hello")))
(t/is (not (d/editable-collection? 42))))
(t/deftest num-predicate
(t/deftest num-predicate-2
(t/is (d/num? 1))
(t/is (d/num? 0))
(t/is (d/num? -3.14))

177
common/test/common_tests/logic/variants_switch_test.cljc
View File

@ -7,6 +7,9 @@
(ns common-tests.logic.variants-switch-test
(:require
[app.common.files.changes-builder :as pcb]
[app.common.files.helpers :as cfh]
[app.common.geom.point :as gpt]
[app.common.geom.shapes :as gsh]
[app.common.logic.shapes :as cls]
[app.common.test-helpers.components :as thc]
[app.common.test-helpers.compositions :as tho]
@ -2860,4 +2863,176 @@
;; Both variants are identical in size (100x50), so the override IS preserved
(t/is (= (:width rect02') 150))
;; The guard must not have suppressed :selrect — it should be consistent
(t/is (= (get-in rect02' [:selrect :width]) 150))))
(t/is (= (get-in rect02' [:selrect :width]) 150))))
(t/deftest test-switch-when-source-master-child-has-touched-geometry
;; Regression: when the previous-shape's geometry has sub-pixel drift
;; relative to its source master (a state produced by interactive transform
;; modifiers, e.g. alt-drag duplicate of a variant whose children are
;; component copies), the equal-geometry? guard in update-attrs-on-switch
;; uses exact equality and fails. The :else branch then copies
;; previous-shape's :selrect verbatim onto the freshly-instantiated target,
;; leaving :y correct (the per-attr y skip catches that) but :selrect.y
;; stale. The shape ends up internally inconsistent (:y disagrees with
;; :selrect.y); the renderer reads :selrect, so the child appears at the
;; source variant's position inside a parent that has resized to the
;; target's dimensions — the visible "cut off" symptom.
(let [;; ==== Setup
;; A self-contained Input/Button-like component, plus a variant
;; container whose two variants each instance that component
;; at different y positions. This mirrors the production setup
;; where the dragged variant's children are themselves component
;; copies (and thus carry :touched on geometry within the master).
file (-> (thf/sample-file :file1)
(tho/add-simple-component :btn-comp :btn-root :btn-rect)
(thv/add-variant-with-copy
:v01 :c01 :m01 :c02 :m02 :child1 :child2 :btn-comp))
;; Position child1 at y=101 (in m01) and child2 at y=73 (in m02).
;; Use gsh/absolute-move so :selrect/:points stay consistent with
;; :y — a plain (assoc :y …) would leave them out of sync and
;; produce a different (artificial) failure mode.
page (thf/current-page file)
child1 (ths/get-shape file :child1)
child2 (ths/get-shape file :child2)
changes (-> (pcb/empty-changes nil (:id page))
(cls/generate-update-shapes
#{(:id child1)}
#(gsh/absolute-move % (gpt/point (:x %) 101))
(:objects page) {})
(cls/generate-update-shapes
#{(:id child2)}
#(gsh/absolute-move % (gpt/point (:x %) 73))
(:objects page) {}))
file (thf/apply-changes file changes)
file (thc/instantiate-component file :c01 :copy01)
;; The copy carries an Input/Button instance (Frame1). Introduce
;; sub-pixel drift in its :width and :selrect.width — the kind of
;; floating-point error produced by the alt-drag modifier path in
;; production. This drift is what defeats equal-geometry?'s
;; exact-equality comparison and lets the bug surface.
page (thf/current-page file)
copy01 (ths/get-shape file :copy01)
copy-btn-id (->> (cfh/get-children-ids-with-self (:objects page) (:id copy01))
(map #(get-in page [:objects %]))
(filter #(= "Frame1" (:name %)))
first :id)
drift 0.00001
changes (cls/generate-update-shapes
(pcb/empty-changes nil (:id page))
#{copy-btn-id}
(fn [shape]
(let [w (+ (:width shape) drift)
sr (:selrect shape)]
(-> shape
(assoc :width w)
(assoc :selrect (-> sr
(assoc :width w)
(assoc :x2 (+ (:x1 sr) w)))))))
(:objects page) {})
file (thf/apply-changes file changes)
m02 (ths/get-shape file :m02)
child2 (ths/get-shape file :child2)
target-rel-y (- (:y child2) (:y m02))
;; ==== Action
file' (tho/swap-component-in-shape file :copy01 :c02
{:new-shape-label :copy02
:keep-touched? true})
page' (thf/current-page file')
copy02 (ths/get-shape file' :copy02)
post-btn (->> (cfh/get-children-ids-with-self (:objects page') (:id copy02))
(map #(get-in page' [:objects %]))
(filter #(= "Frame1" (:name %)))
first)
post-btn-rel-y (- (:y post-btn) (:y copy02))
post-btn-selrect-rel-y (- (get-in post-btn [:selrect :y])
(get-in copy02 [:selrect :y]))]
;; The post-switch button must sit at the target master's relative y.
;; Its :y field already does (the per-attr :y skip handles that
;; correctly); the failure is on :selrect.
(t/is (= target-rel-y post-btn-rel-y)
(str "Child's :y should match target master layout (" target-rel-y ")"))
;; The bug: :selrect.y is overwritten with the previous shape's value,
;; not regenerated from the target master's layout. After fix, this
;; assertion should pass.
(t/is (= target-rel-y post-btn-selrect-rel-y)
(str ":selrect.y should match target master layout (expected "
target-rel-y " got " post-btn-selrect-rel-y ")"))
;; And :y must agree with :selrect.y — a shape whose :y disagrees with
;; its :selrect.y is internally inconsistent and renders incorrectly.
(t/is (= post-btn-rel-y post-btn-selrect-rel-y)
":y and :selrect.y must agree after switch")))
(t/deftest test-switch-does-not-override-path-content-when-only-repositioned
;; Regression: when a path shape inside a variant has :geometry-group touched
;; (e.g. because auto-layout repositioned it after the copy's parent was
;; resized), switching variants must NOT copy the old variant's path position
;; to the new variant. The path should stay at the new variant's default position.
;;
;; Root cause: equal-geometry? did not handle the :content attr for path shapes,
;; so switch-path-change-value was always invoked and placed the new path at the
;; pre-switch absolute position instead of the target master's default position.
(let [;; A small closed triangle path whose bounding box is 24x14 px,
;; anchored at absolute position (x0, y0).
triangle (fn [x0 y0]
[{:command :move-to :params {:x x0 :y y0}}
{:command :line-to :params {:x (+ x0 24) :y y0}}
{:command :line-to :params {:x (+ x0 12) :y (+ y0 14)}}
{:command :close-path}])
;; V1 has the path at y=10; V2 has the same-shape path at y=30.
file (-> (thf/sample-file :file1)
(thv/add-variant :v01 :c01 :m01 :c02 :m02
{:variant1-params {:width 100 :height 100}
:variant2-params {:width 100 :height 100}})
(ths/add-sample-shape :path1 :type :path
:parent-label :m01
:content (triangle 0 10))
(ths/add-sample-shape :path2 :type :path
:parent-label :m02
:content (triangle 0 30))
(thc/instantiate-component :c01 :copy01))
;; Simulate auto-layout repositioning the path inside the copy by
;; moving it to y=50. This touches :geometry-group on the copy's path.
page (thf/current-page file)
copy01 (ths/get-shape file :copy01)
copy-path (->> (cfh/get-children-with-self (:objects page) (:id copy01))
(filter #(= :path (:type %)))
first)
changes (cls/generate-update-shapes
(pcb/empty-changes nil (:id page))
#{(:id copy-path)}
#(gsh/absolute-move % (gpt/point (:x %) 50))
(:objects page) {})
file (thf/apply-changes file changes)
;; Switch copy01 from V1 (c01) to V2 (c02).
file' (tho/swap-component-in-shape file :copy01 :c02 {:keep-touched? true})
page' (thf/current-page file')
copy01' (ths/get-shape file' :copy01)
copy-path' (->> (cfh/get-children-with-self (:objects page') (:id copy01'))
(filter #(= :path (:type %)))
first)
;; Expected: V2's path sits at y=30 (its master default), not y=50
;; (the pre-switch repositioned position).
m02 (ths/get-shape file :m02)
path2 (ths/get-shape file :path2)
target-rel-y (- (-> path2 :selrect :y) (-> m02 :selrect :y))
actual-rel-y (- (-> copy-path' :selrect :y) (-> copy01' :selrect :y))]
(t/is (some? copy-path') "path should exist in switched copy")
(t/is (= target-rel-y actual-rel-y)
(str "path :selrect.y should match target master layout (expected "
target-rel-y " got " actual-rel-y ")"))))

2
common/test/common_tests/runner.cljc
View File

@ -52,6 +52,7 @@
[common-tests.logic.swap-and-reset-test]
[common-tests.logic.swap-as-override-test]
[common-tests.logic.token-test]
[common-tests.logic.variants-switch-test]
[common-tests.media-test]
[common-tests.path-names-test]
[common-tests.record-test]
@ -132,6 +133,7 @@
'common-tests.logic.swap-and-reset-test
'common-tests.logic.swap-as-override-test
'common-tests.logic.token-test
'common-tests.logic.variants-switch-test
'common-tests.media-test
'common-tests.path-names-test
'common-tests.record-test