🐛 Fix variants corner cases with selrect and points (#8882)

Co-authored-by: Andrey Antukh <niwi@niwi.nz>
This commit is contained in:
Pablo Alba 2026-04-10 11:23:03 +02:00 committed by GitHub
parent 9785a13e67
commit ef6eeb5693
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 523 additions and 8 deletions

View File

@ -1,5 +1,13 @@
# CHANGELOG
## 2.14.3 (Unreleased)
### :sparkles: New features & Enhancements
### :bug: Bugs fixed
- Fix component "broken" after switch variant [Taiga #12984](https://tree.taiga.io/project/penpot/issue/12984)
## 2.14.2
### :sparkles: New features & Enhancements

View File

@ -2006,14 +2006,17 @@
(defn- switch-fixed-layout-geom-change-value
[prev-shape ; The shape before the switch
current-shape ; The shape after the switch (a clean copy)
origin-shape ; The original shape
attr]
;; When there is a layout with fixed h or v sizing, we need
;; to keep the width/height (and recalculate selrect and points)
(let [prev-width (-> prev-shape :selrect :width)
current-width (-> current-shape :selrect :width)
origin-width (-> origin-shape :selrect :width)
prev-height (-> prev-shape :selrect :height)
current-height (-> current-shape :selrect :height)
origin-height (-> origin-shape :selrect :height)
x (-> current-shape :selrect :x)
y (-> current-shape :selrect :y)
@ -2024,10 +2027,16 @@
final-width (if (= :fix h-sizing)
current-width
prev-width)
(if (= origin-width current-width)
prev-width ;; same-size: preserve override
current-width)) ;; different-size: use new component's
final-height (if (= :fix v-sizing)
current-height
prev-height)
(if (= origin-height current-height)
prev-height ;; same-size: preserve override
current-height)) ;; different-size: use new component's
selrect (assoc (:selrect current-shape)
:width final-width
:height final-height
@ -2056,6 +2065,25 @@
(or (:transform current-shape) (gmt/matrix)))))))
(defn- equal-geometry?
"Returns true when the value of `attr` in `shape` is considered equal
to the corresponding value in `origin-shape`, ignoring positional
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."
[shape origin-shape attr]
(or (and (= attr :selrect)
(= (-> shape :selrect :width) (-> origin-shape :selrect :width))
(= (-> 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)))))))
(defn update-attrs-on-switch
"Copy attributes that have changed in the shape previous to the switch
@ -2092,8 +2120,9 @@
;; If the values are already equal, don't copy them
(= (get previous-shape attr) (get current-shape attr))
;; If the value is the same as the origin, don't copy it
(= (get previous-shape attr) (get origin-ref-shape attr))
;; If :selrect/:points values are already equal ignoring displacement,
;; don't copy them
(equal-geometry? previous-shape origin-ref-shape attr)
;; If the attr is not touched, don't copy it
(not (touched attr-group))
@ -2143,8 +2172,21 @@
skip-operations? (or skip-operations?
;; If we are going to reset the position data, skip the selrect attr
(and reset-pos-data? (= attr :selrect)))
(and reset-pos-data? (= attr :selrect))
;; Avoid copying composite geometry attrs (:selrect/:points) when the
;; variant dimensions differ but neither sizing is :fix. Without this,
;; :width/:height are correctly skipped by the check above
;; but :selrect/:points would still carry the old override dimensions,
;; leaving the shape in an inconsistent state. When :fix sizing is
;; present, switch-fixed-layout-geom-change-value handles the composite
;; attrs and must NOT be bypassed. Path shapes are also handled
;; separately via switch-path-change-value.
(and (contains? #{:selrect :points} attr)
(not path-change?)
(not (or (= :fix (:layout-item-h-sizing previous-shape))
(= :fix (:layout-item-v-sizing previous-shape))))
(or (not= (get origin-ref-shape :width) (get current-shape :width))
(not= (get origin-ref-shape :height) (get current-shape :height)))))
attr-val
(when-not skip-operations?
(cond
@ -2168,7 +2210,7 @@
(and (or (= :fix (:layout-item-h-sizing previous-shape))
(= :fix (:layout-item-v-sizing previous-shape)))
(contains? #{:points :selrect :width :height} attr))
(switch-fixed-layout-geom-change-value previous-shape current-shape attr)
(switch-fixed-layout-geom-change-value previous-shape current-shape origin-ref-shape attr)
:else
(get previous-shape attr)))

View File

@ -2257,4 +2257,469 @@
;; or if it needs recalculation, the test validates the behavior
(t/is (or (nil? old-position-data)
(nil? new-position-data)
(not= old-position-data new-position-data)))))
(not= old-position-data new-position-data)))))
;; ============================================================
;; SELRECT CONSISTENCY TESTS
;; These tests verify that after a variant switch, the composite
;; geometry attributes (:selrect, :points) stay consistent with
;; the scalar attributes (:width, :height) that are kept.
;; ============================================================
(t/deftest test-switch-selrect-consistent-no-sizing-different-widths
;; When no :fix sizing and variants have different widths,
;; :width is correctly skipped (stays at new component width),
;; but :selrect was being copied from the old shape, leaving
;; selrect.width inconsistent with :width. This test verifies the fix.
(let [;; ==== Setup
file (-> (thf/sample-file :file1)
(thv/add-variant-with-child
:v01 :c01 :m01 :c02 :m02 :r01 :r02
{:child1-params {:width 100 :height 50}
:child2-params {:width 200 :height 50}})
(thc/instantiate-component :c01
:copy01
:children-labels [:copy-r01]))
page (thf/current-page file)
copy01 (ths/get-shape file :copy01)
rect01 (get-in page [:objects (-> copy01 :shapes first)])
;; Override width AND selrect consistently (simulating a real resize)
changes (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
#{(:id rect01)}
(fn [shape]
(let [new-width 150
sr (:selrect shape)
new-sr (-> sr
(assoc :width new-width)
(assoc :x2 (+ (:x1 sr) new-width)))]
(-> shape
(assoc :width new-width)
(assoc :selrect new-sr))))
(:objects page)
{})
file (thf/apply-changes file changes)
page (thf/current-page file)
rect01 (get-in page [:objects (:id rect01)])
;; ==== Action
file' (tho/swap-component file copy01 :c02 {:new-shape-label :copy02 :keep-touched? true})
page' (thf/current-page file')
copy02' (ths/get-shape file' :copy02)
rect02' (get-in page' [:objects (-> copy02' :shapes first)])]
;; The rect had the width override before the switch
(t/is (= (:width rect01) 150))
(t/is (= (get-in rect01 [:selrect :width]) 150))
;; Since the variants have different widths (100 vs 200), the override is not preserved
(t/is (= (:width rect02') 200))
;; The selrect must be consistent with :width
(t/is (= (get-in rect02' [:selrect :width]) 200))))
(t/deftest test-switch-selrect-consistent-no-sizing-different-heights
;; Same as above but for height.
(let [;; ==== Setup
file (-> (thf/sample-file :file1)
(thv/add-variant-with-child
:v01 :c01 :m01 :c02 :m02 :r01 :r02
{:child1-params {:width 50 :height 100}
:child2-params {:width 50 :height 200}})
(thc/instantiate-component :c01
:copy01
:children-labels [:copy-r01]))
page (thf/current-page file)
copy01 (ths/get-shape file :copy01)
rect01 (get-in page [:objects (-> copy01 :shapes first)])
;; Override height AND selrect consistently
changes (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
#{(:id rect01)}
(fn [shape]
(let [new-height 150
sr (:selrect shape)
new-sr (-> sr
(assoc :height new-height)
(assoc :y2 (+ (:y1 sr) new-height)))]
(-> shape
(assoc :height new-height)
(assoc :selrect new-sr))))
(:objects page)
{})
file (thf/apply-changes file changes)
page (thf/current-page file)
rect01 (get-in page [:objects (:id rect01)])
;; ==== Action
file' (tho/swap-component file copy01 :c02 {:new-shape-label :copy02 :keep-touched? true})
page' (thf/current-page file')
copy02' (ths/get-shape file' :copy02)
rect02' (get-in page' [:objects (-> copy02' :shapes first)])]
;; The rect had the height override before the switch
(t/is (= (:height rect01) 150))
(t/is (= (get-in rect01 [:selrect :height]) 150))
;; Since the variants have different heights (100 vs 200), the override is not preserved
(t/is (= (:height rect02') 200))
;; The selrect must be consistent with :height
(t/is (= (get-in rect02' [:selrect :height]) 200))))
(t/deftest test-switch-with-v-sizing-fix-selrect-consistent-different-widths
;; mixed-sizing scenario: v-sizing=:fix but variants differ in WIDTH.
;; switch-fixed-layout-geom-change-value is triggered (because v-sizing=:fix).
;; Without the fix, the function returned prev-width for the non-:fix dimension,
;; leaving selrect.width inconsistent with :width.
(let [;; ==== Setup
file (-> (thf/sample-file :file1)
(thv/add-variant-with-child
:v01 :c01 :m01 :c02 :m02 :r01 :r02
{:child1-params {:width 100 :height 50 :layout-item-v-sizing :fix}
:child2-params {:width 200 :height 50 :layout-item-v-sizing :fix}})
(thc/instantiate-component :c01
:copy01
:children-labels [:copy-r01]))
page (thf/current-page file)
copy01 (ths/get-shape file :copy01)
rect01 (get-in page [:objects (-> copy01 :shapes first)])
;; Override width AND selrect consistently
changes (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
#{(:id rect01)}
(fn [shape]
(let [new-width 150
sr (:selrect shape)
new-sr (-> sr
(assoc :width new-width)
(assoc :x2 (+ (:x1 sr) new-width)))]
(-> shape
(assoc :width new-width)
(assoc :selrect new-sr))))
(:objects page)
{})
file (thf/apply-changes file changes)
page (thf/current-page file)
rect01 (get-in page [:objects (:id rect01)])
;; ==== Action
file' (tho/swap-component file copy01 :c02 {:new-shape-label :copy02 :keep-touched? true})
page' (thf/current-page file')
copy02' (ths/get-shape file' :copy02)
rect02' (get-in page' [:objects (-> copy02' :shapes first)])]
;; The rect had the width override before the switch
(t/is (= (:width rect01) 150))
(t/is (= (get-in rect01 [:selrect :width]) 150))
;; Since the variants have different widths (100 vs 200), the override is not preserved
;; (v-sizing=:fix does not affect the horizontal dimension)
(t/is (= (:width rect02') 200))
;; The selrect must be consistent with :width
(t/is (= (get-in rect02' [:selrect :width]) 200))
;; v-sizing is preserved
(t/is (= (:layout-item-v-sizing rect02') :fix))))
(t/deftest test-switch-with-h-sizing-fix-selrect-consistent-different-heights
;; mixed-sizing scenario: h-sizing=:fix but variants differ in HEIGHT.
;; switch-fixed-layout-geom-change-value is triggered (because h-sizing=:fix).
;; Without the fix, the function returned prev-height for the non-:fix dimension,
;; leaving selrect.height inconsistent with :height.
(let [;; ==== Setup
file (-> (thf/sample-file :file1)
(thv/add-variant-with-child
:v01 :c01 :m01 :c02 :m02 :r01 :r02
{:child1-params {:width 50 :height 100 :layout-item-h-sizing :fix}
:child2-params {:width 50 :height 200 :layout-item-h-sizing :fix}})
(thc/instantiate-component :c01
:copy01
:children-labels [:copy-r01]))
page (thf/current-page file)
copy01 (ths/get-shape file :copy01)
rect01 (get-in page [:objects (-> copy01 :shapes first)])
;; Override height AND selrect consistently
changes (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
#{(:id rect01)}
(fn [shape]
(let [new-height 150
sr (:selrect shape)
new-sr (-> sr
(assoc :height new-height)
(assoc :y2 (+ (:y1 sr) new-height)))]
(-> shape
(assoc :height new-height)
(assoc :selrect new-sr))))
(:objects page)
{})
file (thf/apply-changes file changes)
page (thf/current-page file)
rect01 (get-in page [:objects (:id rect01)])
;; ==== Action
file' (tho/swap-component file copy01 :c02 {:new-shape-label :copy02 :keep-touched? true})
page' (thf/current-page file')
copy02' (ths/get-shape file' :copy02)
rect02' (get-in page' [:objects (-> copy02' :shapes first)])]
;; The rect had the height override before the switch
(t/is (= (:height rect01) 150))
(t/is (= (get-in rect01 [:selrect :height]) 150))
;; Since the variants have different heights (100 vs 200), the override is not preserved
;; (h-sizing=:fix does not affect the vertical dimension)
(t/is (= (:height rect02') 200))
;; The selrect must be consistent with :height
(t/is (= (get-in rect02' [:selrect :height]) 200))
;; h-sizing is preserved
(t/is (= (:layout-item-h-sizing rect02') :fix))))
;; ============================================================
;; FIXED-SIZING: "SAME-SIZE → PRESERVE OVERRIDE" PATH TESTS
;; These tests exercise the branch inside switch-fixed-layout-geom-change-value
;; where variants share the same value in the non-:fix dimension:
;; (if (= origin-dim current-dim) prev-dim current-dim)
;; When origin-dim == current-dim the user's override for that dimension
;; must be preserved after the switch.
;; ============================================================
(t/deftest test-switch-with-h-sizing-fix-same-height-override-preserved
;; h-sizing=:fix, variants have SAME height (non-:fix dim, same-size).
;; switch-fixed-layout-geom-change-value must return prev-height for the
;; non-:fix dimension because origin-height == current-height.
(let [;; ==== Setup
file (-> (thf/sample-file :file1)
(thv/add-variant-with-child
:v01 :c01 :m01 :c02 :m02 :r01 :r02
{:child1-params {:width 100 :height 50 :layout-item-h-sizing :fix}
:child2-params {:width 200 :height 50 :layout-item-h-sizing :fix}})
(thc/instantiate-component :c01
:copy01
:children-labels [:copy-r01]))
page (thf/current-page file)
copy01 (ths/get-shape file :copy01)
rect01 (get-in page [:objects (-> copy01 :shapes first)])
;; Override height (the non-:fix dimension) and selrect consistently
changes (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
#{(:id rect01)}
(fn [shape]
(let [new-height 75
sr (:selrect shape)
new-sr (-> sr
(assoc :height new-height)
(assoc :y2 (+ (:y1 sr) new-height)))]
(-> shape
(assoc :height new-height)
(assoc :selrect new-sr))))
(:objects page)
{})
file (thf/apply-changes file changes)
page (thf/current-page file)
rect01 (get-in page [:objects (:id rect01)])
;; ==== Action
file' (tho/swap-component file copy01 :c02 {:new-shape-label :copy02 :keep-touched? true})
page' (thf/current-page file')
copy02' (ths/get-shape file' :copy02)
rect02' (get-in page' [:objects (-> copy02' :shapes first)])]
;; The rect had the height override 75 before the switch
(t/is (= (:height rect01) 75))
;; h-sizing=:fix means width always takes the new component's value
(t/is (= (:width rect02') 200))
;; Height (non-:fix dim) is preserved because both variants have same height (50)
(t/is (= (:height rect02') 75))
;; selrect must be consistent with the preserved height
(t/is (= (get-in rect02' [:selrect :height]) 75))
(t/is (= (get-in rect02' [:selrect :width]) 200))
;; h-sizing is preserved
(t/is (= (:layout-item-h-sizing rect02') :fix))))
(t/deftest test-switch-with-v-sizing-fix-same-width-override-preserved
;; v-sizing=:fix, variants have SAME width (non-:fix dim, same-size).
;; switch-fixed-layout-geom-change-value must return prev-width for the
;; non-:fix dimension because origin-width == current-width.
(let [;; ==== Setup
file (-> (thf/sample-file :file1)
(thv/add-variant-with-child
:v01 :c01 :m01 :c02 :m02 :r01 :r02
{:child1-params {:width 100 :height 50 :layout-item-v-sizing :fix}
:child2-params {:width 100 :height 100 :layout-item-v-sizing :fix}})
(thc/instantiate-component :c01
:copy01
:children-labels [:copy-r01]))
page (thf/current-page file)
copy01 (ths/get-shape file :copy01)
rect01 (get-in page [:objects (-> copy01 :shapes first)])
;; Override width (the non-:fix dimension) and selrect consistently
changes (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
#{(:id rect01)}
(fn [shape]
(let [new-width 150
sr (:selrect shape)
new-sr (-> sr
(assoc :width new-width)
(assoc :x2 (+ (:x1 sr) new-width)))]
(-> shape
(assoc :width new-width)
(assoc :selrect new-sr))))
(:objects page)
{})
file (thf/apply-changes file changes)
page (thf/current-page file)
rect01 (get-in page [:objects (:id rect01)])
;; ==== Action
file' (tho/swap-component file copy01 :c02 {:new-shape-label :copy02 :keep-touched? true})
page' (thf/current-page file')
copy02' (ths/get-shape file' :copy02)
rect02' (get-in page' [:objects (-> copy02' :shapes first)])]
;; The rect had the width override 150 before the switch
(t/is (= (:width rect01) 150))
;; Width (non-:fix dim) is preserved because both variants have same width (100)
(t/is (= (:width rect02') 150))
;; selrect must be consistent with the preserved width
(t/is (= (get-in rect02' [:selrect :width]) 150))
;; v-sizing=:fix means height always takes the new component's value
(t/is (= (:height rect02') 100))
(t/is (= (get-in rect02' [:selrect :height]) 100))
;; v-sizing is preserved
(t/is (= (:layout-item-v-sizing rect02') :fix))))
(t/deftest test-switch-with-both-sizing-fix-overrides-discarded
;; When both h-sizing=:fix and v-sizing=:fix, switch-fixed-layout-geom-change-value
;; always uses current-width and current-height (the new component's values).
;; Both width and height overrides are discarded because :fix always
;; defers to the new component's dimension regardless of same-size or not.
(let [;; ==== Setup
file (-> (thf/sample-file :file1)
(thv/add-variant-with-child
:v01 :c01 :m01 :c02 :m02 :r01 :r02
{:child1-params {:width 100 :height 50
:layout-item-h-sizing :fix
:layout-item-v-sizing :fix}
:child2-params {:width 200 :height 100
:layout-item-h-sizing :fix
:layout-item-v-sizing :fix}})
(thc/instantiate-component :c01
:copy01
:children-labels [:copy-r01]))
page (thf/current-page file)
copy01 (ths/get-shape file :copy01)
rect01 (get-in page [:objects (-> copy01 :shapes first)])
;; Override both width and height (and selrect) consistently
changes (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
#{(:id rect01)}
(fn [shape]
(let [new-width 150
new-height 75
sr (:selrect shape)
new-sr (-> sr
(assoc :width new-width)
(assoc :height new-height)
(assoc :x2 (+ (:x1 sr) new-width))
(assoc :y2 (+ (:y1 sr) new-height)))]
(-> shape
(assoc :width new-width)
(assoc :height new-height)
(assoc :selrect new-sr))))
(:objects page)
{})
file (thf/apply-changes file changes)
page (thf/current-page file)
rect01 (get-in page [:objects (:id rect01)])
;; ==== Action
file' (tho/swap-component file copy01 :c02 {:new-shape-label :copy02 :keep-touched? true})
page' (thf/current-page file')
copy02' (ths/get-shape file' :copy02)
rect02' (get-in page' [:objects (-> copy02' :shapes first)])]
;; The rect had both overrides before the switch
(t/is (= (:width rect01) 150))
(t/is (= (:height rect01) 75))
;; With both sizing :fix, both dimensions take the new component's values
(t/is (= (:width rect02') 200))
(t/is (= (:height rect02') 100))
;; selrect must be consistent
(t/is (= (get-in rect02' [:selrect :width]) 200))
(t/is (= (get-in rect02' [:selrect :height]) 100))
(t/is (= (:layout-item-h-sizing rect02') :fix))
(t/is (= (:layout-item-v-sizing rect02') :fix))))
(t/deftest test-switch-same-size-variants-geometry-override-preserved
;; When both variants have IDENTICAL dimensions (width=100, height=50),
;; the guard that skips :selrect/:points must NOT fire
;; (its condition `(or (not= origin.width current.width) ...)` is false).
;; A geometry override should therefore be carried through correctly.
(let [;; ==== Setup
file (-> (thf/sample-file :file1)
(thv/add-variant-with-child
:v01 :c01 :m01 :c02 :m02 :r01 :r02
{:child1-params {:width 100 :height 50}
:child2-params {:width 100 :height 50}}) ; same size!
(thc/instantiate-component :c01
:copy01
:children-labels [:copy-r01]))
page (thf/current-page file)
copy01 (ths/get-shape file :copy01)
rect01 (get-in page [:objects (-> copy01 :shapes first)])
;; Override width AND selrect consistently (simulating a real resize)
changes (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
#{(:id rect01)}
(fn [shape]
(let [new-width 150
sr (:selrect shape)
new-sr (-> sr
(assoc :width new-width)
(assoc :x2 (+ (:x1 sr) new-width)))]
(-> shape
(assoc :width new-width)
(assoc :selrect new-sr))))
(:objects page)
{})
file (thf/apply-changes file changes)
page (thf/current-page file)
rect01 (get-in page [:objects (:id rect01)])
;; ==== Action
file' (tho/swap-component file copy01 :c02 {:new-shape-label :copy02 :keep-touched? true})
page' (thf/current-page file')
copy02' (ths/get-shape file' :copy02)
rect02' (get-in page' [:objects (-> copy02' :shapes first)])]
;; The rect had the width override 150 before the switch
(t/is (= (:width rect01) 150))
(t/is (= (get-in rect01 [:selrect :width]) 150))
;; 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))))