🐛 Fix nested variant in a component doesn't keep inherited overrides

CHANGES.md

@@ -15,6 +15,7 @@
 - Fix pan cursor not disabling viewport guides [Github #6985](https://github.com/penpot/penpot/issues/6985)
 - Fix viewport resize on locked shapes [Taiga #11974](https://tree.taiga.io/project/penpot/issue/11974)
 - Fix variants not syncronizing tokens on switch [Taiga #12290](https://tree.taiga.io/project/penpot/issue/12290)
+- Fix nested variant in a component doesn't keep inherited overrides [Taiga #12299](https://tree.taiga.io/project/penpot/issue/12299)
 
 ## 2.11.0 (Unreleased)
 

common/src/app/common/logic/variants.cljc

@@ -11,7 +11,8 @@
    [app.common.types.container :as ctn]
    [app.common.types.file :as ctf]
    [app.common.types.variant :as ctv]
-   [app.common.uuid :as uuid]))
+   [app.common.uuid :as uuid]
+   [clojure.set :as set]))
 
 (defn generate-add-new-variant
   [changes shape variant-id new-component-id new-shape-id prop-num]
@@ -67,7 +68,6 @@
       [[] {}]
       shapes))))
 
-
 (defn- keep-swapped-item
   "As part of the keep-touched process on a switch, given a child on the original
    copy that was swapped (orig-swapped-child), and its related shape on the new copy
@@ -88,7 +88,6 @@
         current-parent  (get objects (:parent-id related-shape-in-new))
         pos             (d/index-of (:shapes current-parent) (:id related-shape-in-new))]
 
-
     (-> (pcb/concat-changes before-changes changes)
 
         ;; Move the previous shape to the new parent
@@ -122,6 +121,44 @@
                     (subvec (vec ancestors) 1 (dec num-ancestors)))]
     (some ctk/get-swap-slot ancestors)))
 
+(defn- find-shape-ref-child-of
+  "Get the shape referenced by the shape-ref of the near main of the shape,
+   recursively repeated until find a shape-ref with parent-id as ancestor.
+   It will return the shape or nil if it doesn't found any"
+  [container libraries shape parent-id]
+  (let [ref-shape             (ctf/find-ref-shape nil container libraries shape
+                                                  :with-context? true)
+
+        ref-shape-container   (when ref-shape (:container (meta ref-shape)))
+        ref-shape-parents-set (when ref-shape
+                                (->> (cfh/get-parents (:objects ref-shape-container) (:id ref-shape))
+                                     (into #{} d/xf:map-id)))]
+
+    (if (or (nil? ref-shape) (contains? ref-shape-parents-set parent-id))
+      ref-shape
+      (find-shape-ref-child-of ref-shape-container libraries ref-shape parent-id))))
+
+(defn- get-ref-chain
+  "Returns a vector with the shape ref chain including itself"
+  [container libraries shape]
+  (loop [chain [shape]
+         current shape]
+    (if-let [ref (ctf/find-ref-shape nil container libraries current :with-context? true)]
+      (recur (conj chain ref) ref)
+      chain)))
+
+(defn- add-touched-from-ref-chain
+  "Adds to the :touched attr of a shape the content of
+   the :touched of all its chain of ref shapes"
+  [container libraries shape]
+  (let [chain (get-ref-chain container libraries shape)
+        more-touched (->> chain
+                          (map :touched)
+                          (remove nil?)
+                          (apply set/union)
+                          (remove ctk/swap-slot?)
+                          set)]
+    (update shape :touched #(set/union (or % #{}) more-touched))))
 
 (defn generate-keep-touched
   "This is used as part of the switch process, when you switch from
@@ -141,7 +178,10 @@
         ;; Ignore children of swapped items, because
         ;; they will be moved without change when
         ;; managing their swapped ancestor
-        orig-touched       (->> (filter (comp seq :touched) original-shapes)
+        orig-touched       (->> original-shapes
+                                ;; Add to each shape also the touched of its ref chain
+                                (map #(add-touched-from-ref-chain container libraries %))
+                                (filter (comp seq :touched))
                                 (remove
                                  #(child-of-swapped? %
                                                      page-objects
@@ -158,7 +198,7 @@
 
         ;; The original-shape is in a copy. For the relation rules, we need the referenced
         ;; shape on the main component
-        orig-ref-shape     (ctf/find-ref-shape nil container libraries original-shape {:with-context? true})
+        orig-ref-shape     (ctf/find-remote-shape container libraries original-shape {:with-context? true})
         orig-ref-objects   (:objects (:container (meta orig-ref-shape)))
 
         ;; Adds a :shape-path attribute to the children of the orig-ref-shape,
@@ -171,7 +211,6 @@
         ;; Creates a map to quickly find a child of the orig-ref-shape by its shape-path
         o-ref-shapes-p-map  (into {} (map (juxt :id :shape-path)) o-ref-shapes-wp)
 
-
         ;; Process each touched children of the original-shape
         [changes parents-of-swapped]
         (reduce
@@ -182,8 +221,7 @@
                  ;; orig-child-touched is in a copy. Get the referenced shape on the main component
                  ;; If there is a swap slot, we will get the referenced shape in another way
                  orig-ref-shape (when-not swap-slot
-                                  ;; TODO Maybe just get it from o-ref-shapes-wp
-                                  (ctf/find-ref-shape nil container libraries orig-child-touched))
+                                  (find-shape-ref-child-of container libraries orig-child-touched (:id orig-ref-shape)))
 
                  orig-ref-id    (if swap-slot
                                   ;; If there is a swap slot, find the referenced shape id
@@ -196,6 +234,7 @@
                  ;; Get its related shape in the children of new-shape: the one that
                  ;; has the same shape-path
                  related-shape-in-new  (get new-shapes-map shape-path)
+
                  parents-of-swapped (if related-shape-in-new
                                       (conj parent-of-swapped (:parent-id related-shape-in-new))
                                       parent-of-swapped)

common/src/app/common/types/component.cljc

@@ -286,7 +286,7 @@
           (fn [touched]
             (into #{} (remove #(str/starts-with? (name %) "swap-slot-") touched)))))
 
-(defn get-component-root
+(defn get-deleted-component-root
   [component]
   (if (some? (:main-instance-id component))
     (get-in component [:objects (:main-instance-id component)])

common/src/app/common/types/file.cljc

@@ -276,7 +276,7 @@
     (-> file-data
         (get-component-page component)
         (ctn/get-shape (:main-instance-id component)))
-    (ctk/get-component-root component)))
+    (ctk/get-deleted-component-root component)))
 
 (defn get-component-shape
   "Retrieve one shape in the component by id. If with-context? is true, add the
@@ -355,7 +355,7 @@
 
 (defn find-remote-shape
   "Recursively go back by the :shape-ref of the shape until find the correct shape of the original component"
-  [container libraries shape]
+  [container libraries shape & {:keys [with-context?] :or {with-context? false}}]
   (let [top-instance        (ctn/get-component-shape (:objects container) shape)
         component-file      (get-in libraries [(:component-file top-instance) :data])
         component           (ctkl/get-component component-file (:component-id top-instance) true)
@@ -375,8 +375,12 @@
     (if (nil? remote-shape)
       nil
       (if (nil? (:shape-ref remote-shape))
-        remote-shape
-        (find-remote-shape component-container libraries remote-shape)))))
+        (cond-> remote-shape
+          (and remote-shape with-context?)
+          (with-meta {:file {:id (:id file-data)
+                             :data file-data}
+                      :container component-container}))
+        (find-remote-shape component-container libraries remote-shape :with-context? with-context?)))))
 
 (defn direct-copy?
   "Check if the shape is in a direct copy of the component (i.e. the shape-ref points to shapes inside
@@ -901,7 +905,7 @@
     (println))
 
   (when (seq (:objects component))
-    (let [root (ctk/get-component-root component)]
+    (let [root (ctk/get-deleted-component-root component)]
       (dump-shape (:id root)
                   1
                   (:objects component)

common/test/common_tests/logic/variants_switch_test.cljc

@@ -45,7 +45,6 @@
     ;; The rect has width 15 after the switch
     (t/is (= (:width rect02') 15))))
 
-
 (t/deftest test-switch-with-override
   (let [;; ==== Setup
         file      (-> (thf/sample-file :file1)
@@ -125,12 +124,10 @@
     ;; The rect has width 15 after the switch
     (t/is (= (:width rect02') 15))))
 
-
 (def font-size-path-paragraph [:content :children 0 :children 0 :font-size])
 (def font-size-path-0 [:content :children 0 :children 0 :children 0 :font-size])
 (def font-size-path-1 [:content :children 0 :children 0 :children 1 :font-size])
 
-
 (def text-path-0 [:content :children 0 :children 0 :children 0 :text])
 (def text-path-1 [:content :children 0 :children 0 :children 1 :text])
 (def text-lines-path [:content :children 0 :children 0 :children])
@@ -188,6 +185,8 @@
 
 
         ;; The copy clean has no overrides
+
+
         copy-clean       (ths/get-shape file :copy-clean)
         copy-clean-t     (ths/get-shape file :copy-clean-t)
 
@@ -209,6 +208,8 @@
 
 
         ;; ==== Action: Switch all the copies
+
+
         file' (-> file
                   (tho/swap-component copy-clean :c02 {:new-shape-label :copy-clean-2 :keep-touched? true})
                   (tho/swap-component copy-font-size :c02 {:new-shape-label :copy-font-size-2 :keep-touched? true})
@@ -234,6 +235,8 @@
     ;; Before the switch:
     ;;   * font size 14
     ;;   * text "hello world"
+
+
     (t/is (= (get-in copy-clean-t font-size-path-0) "14"))
     (t/is (= (get-in copy-clean-t text-path-0) "hello world"))
 
@@ -248,6 +251,8 @@
     ;; Before the switch:
     ;;   * font size 25
     ;;   * text "hello world"
+
+
     (t/is (= (get-in copy-font-size-t font-size-path-0) "25"))
     (t/is (= (get-in copy-font-size-t text-path-0) "hello world"))
 
@@ -306,6 +311,8 @@
 
 
         ;; The copy clean has no overrides
+
+
         copy-clean       (ths/get-shape file :copy-clean)
         copy-clean-t     (ths/get-shape file :copy-clean-t)
 
@@ -327,6 +334,8 @@
 
 
         ;; ==== Action: Switch all the copies
+
+
         file' (-> file
                   (tho/swap-component copy-clean :c02 {:new-shape-label :copy-clean-2 :keep-touched? true})
                   (tho/swap-component copy-font-size :c02 {:new-shape-label :copy-font-size-2 :keep-touched? true})
@@ -352,6 +361,8 @@
     ;; Before the switch:
     ;;   * font size 14
     ;;   * text "hello world"
+
+
     (t/is (= (get-in copy-clean-t font-size-path-0) "14"))
     (t/is (= (get-in copy-clean-t text-path-0) "hello world"))
 
@@ -366,6 +377,8 @@
     ;; Before the switch:
     ;;   * font size 25
     ;;   * text "hello world"
+
+
     (t/is (= (get-in copy-font-size-t font-size-path-0) "25"))
     (t/is (= (get-in copy-font-size-t text-path-0) "hello world"))
 
@@ -401,7 +414,6 @@
     (t/is (= (get-in copy-both-t' font-size-path-0) "50"))
     (t/is (= (get-in copy-both-t' text-path-0) "text overriden"))))
 
-
 (t/deftest test-switch-with-different-text-text-override
   (let [;; ==== Setup
         file (-> (thf/sample-file :file1)
@@ -423,6 +435,8 @@
 
 
         ;; The copy clean has no overrides
+
+
         copy-clean       (ths/get-shape file :copy-clean)
         copy-clean-t     (ths/get-shape file :copy-clean-t)
 
@@ -444,6 +458,8 @@
 
 
         ;; ==== Action: Switch all the copies
+
+
         file' (-> file
                   (tho/swap-component copy-clean :c02 {:new-shape-label :copy-clean-2 :keep-touched? true})
                   (tho/swap-component copy-font-size :c02 {:new-shape-label :copy-font-size-2 :keep-touched? true})
@@ -469,6 +485,8 @@
     ;; Before the switch:
     ;;   * font size 14
     ;;   * text "hello world"
+
+
     (t/is (= (get-in copy-clean-t font-size-path-0) "14"))
     (t/is (= (get-in copy-clean-t text-path-0) "hello world"))
 
@@ -483,6 +501,8 @@
     ;; Before the switch:
     ;;   * font size 25
     ;;   * text "hello world"
+
+
     (t/is (= (get-in copy-font-size-t font-size-path-0) "25"))
     (t/is (= (get-in copy-font-size-t text-path-0) "hello world"))
 
@@ -518,7 +538,6 @@
     (t/is (= (get-in copy-both-t' font-size-path-0) "25"))
     (t/is (= (get-in copy-both-t' text-path-0) "bye"))))
 
-
 (t/deftest test-switch-with-different-text-and-prop-text-override
   (let [;; ==== Setup
         file (-> (thf/sample-file :file1)
@@ -542,6 +561,8 @@
 
 
         ;; The copy clean has no overrides
+
+
         copy-clean       (ths/get-shape file :copy-clean)
         copy-clean-t     (ths/get-shape file :copy-clean-t)
 
@@ -563,6 +584,8 @@
 
 
         ;; ==== Action: Switch all the copies
+
+
         file' (-> file
                   (tho/swap-component copy-clean :c02 {:new-shape-label :copy-clean-2 :keep-touched? true})
                   (tho/swap-component copy-font-size :c02 {:new-shape-label :copy-font-size-2 :keep-touched? true})
@@ -588,6 +611,8 @@
     ;; Before the switch:
     ;;   * font size 14
     ;;   * text "hello world"
+
+
     (t/is (= (get-in copy-clean-t font-size-path-0) "14"))
     (t/is (= (get-in copy-clean-t text-path-0) "hello world"))
 
@@ -602,6 +627,8 @@
     ;; Before the switch:
     ;;   * font size 25
     ;;   * text "hello world"
+
+
     (t/is (= (get-in copy-font-size-t font-size-path-0) "25"))
     (t/is (= (get-in copy-font-size-t text-path-0) "hello world"))
 
@@ -637,7 +664,6 @@
     (t/is (= (get-in copy-both-t' font-size-path-0) "50"))
     (t/is (= (get-in copy-both-t' text-path-0) "bye"))))
 
-
 (t/deftest test-switch-with-identical-structure-text-override
   (let [;; ==== Setup
         file (-> (thf/sample-file :file1)
@@ -657,6 +683,8 @@
 
 
         ;; Duplicate a text line in copy-structure-clean
+
+
         file                   (change-structure file :copy-structure-clean-t)
         copy-structure-clean   (ths/get-shape file :copy-structure-clean)
         copy-structure-clean-t (ths/get-shape file :copy-structure-clean-t)
@@ -678,6 +706,8 @@
 
 
         ;; ==== Action: Switch all the copies
+
+
         file' (-> file
                   (tho/swap-component copy-structure-clean :c02 {:new-shape-label :copy-structure-clean-2 :keep-touched? true})
                   (tho/swap-component copy-structure-unif :c02 {:new-shape-label :copy-structure-unif-2 :keep-touched? true})
@@ -763,7 +793,6 @@
     (t/is (= (get-in copy-structure-mixed-t' font-size-path-1) "40"))
     (t/is (= (get-in copy-structure-mixed-t' text-path-1) "new line 2"))))
 
-
 (t/deftest test-switch-with-different-prop-structure-text-override
   (let [;; ==== Setup
         file (-> (thf/sample-file :file1)
@@ -784,6 +813,8 @@
 
 
         ;; Duplicate a text line in copy-structure-clean
+
+
         file                   (change-structure file :copy-structure-clean-t)
         copy-structure-clean   (ths/get-shape file :copy-structure-clean)
         copy-structure-clean-t (ths/get-shape file :copy-structure-clean-t)
@@ -805,6 +836,8 @@
 
 
         ;; ==== Action: Switch all the copies
+
+
         file' (-> file
                   (tho/swap-component copy-structure-clean :c02 {:new-shape-label :copy-structure-clean-2 :keep-touched? true})
                   (tho/swap-component copy-structure-unif :c02 {:new-shape-label :copy-structure-unif-2 :keep-touched? true})
@@ -906,6 +939,8 @@
 
 
         ;; Duplicate a text line in copy-structure-clean
+
+
         file                   (change-structure file :copy-structure-clean-t)
         copy-structure-clean   (ths/get-shape file :copy-structure-clean)
         copy-structure-clean-t (ths/get-shape file :copy-structure-clean-t)
@@ -927,6 +962,8 @@
 
 
         ;; ==== Action: Switch all the copies
+
+
         file' (-> file
                   (tho/swap-component copy-structure-clean :c02 {:new-shape-label :copy-structure-clean-2 :keep-touched? true})
                   (tho/swap-component copy-structure-unif :c02 {:new-shape-label :copy-structure-unif-2 :keep-touched? true})
@@ -971,6 +1008,8 @@
     ;; Second line:
     ;;   * font size 25
     ;;   * text "new line 2"
+
+
     (t/is (= (get-in copy-structure-unif-t font-size-path-0) "25"))
     (t/is (= (get-in copy-structure-unif-t text-path-0) "new line 1"))
     (t/is (= (get-in copy-structure-unif-t font-size-path-1) "25"))
@@ -992,6 +1031,8 @@
     ;; Before the switch, second line:
     ;;   * font size 40
     ;;   * text "new line 2"
+
+
     (t/is (= (get-in copy-structure-mixed-t font-size-path-0) "35"))
     (t/is (= (get-in copy-structure-mixed-t text-path-0) "new line 1"))
     (t/is (= (get-in copy-structure-mixed-t font-size-path-1) "40"))
@@ -1025,6 +1066,8 @@
 
 
         ;; Duplicate a text line in copy-structure-clean
+
+
         file                   (change-structure file :copy-structure-clean-t)
         copy-structure-clean   (ths/get-shape file :copy-structure-clean)
         copy-structure-clean-t (ths/get-shape file :copy-structure-clean-t)
@@ -1046,6 +1089,8 @@
 
 
         ;; ==== Action: Switch all the copies
+
+
         file' (-> file
                   (tho/swap-component copy-structure-clean :c02 {:new-shape-label :copy-structure-clean-2 :keep-touched? true})
                   (tho/swap-component copy-structure-unif :c02 {:new-shape-label :copy-structure-unif-2 :keep-touched? true})
@@ -1090,6 +1135,8 @@
     ;; Second line:
     ;;   * font size 25
     ;;   * text "new line 2"
+
+
     (t/is (= (get-in copy-structure-unif-t font-size-path-0) "25"))
     (t/is (= (get-in copy-structure-unif-t text-path-0) "new line 1"))
     (t/is (= (get-in copy-structure-unif-t font-size-path-1) "25"))
@@ -1111,6 +1158,8 @@
     ;; Before the switch, second line:
     ;;   * font size 40
     ;;   * text "new line 2"
+
+
     (t/is (= (get-in copy-structure-mixed-t font-size-path-0) "35"))
     (t/is (= (get-in copy-structure-mixed-t text-path-0) "new line 1"))
     (t/is (= (get-in copy-structure-mixed-t font-size-path-1) "40"))
@@ -1124,7 +1173,6 @@
     (t/is (= (get-in copy-structure-mixed-t' text-path-0) "bye"))
     (t/is (nil? (get-in copy-structure-mixed-t' font-size-path-1)))))
 
-
 (t/deftest test-switch-variant-for-other-with-same-nested-component
   (let [;; ==== Setup
         file      (-> (thf/sample-file :file1)
@@ -1144,6 +1192,8 @@
 
 
         ;; On :copy-cp01, change the width of the rect
+
+
         changes (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
                                             #{copy-cp01-rect-id}
                                             (fn [shape]
@@ -1166,8 +1216,6 @@
     ;; The width of copy-cp02-rect' is 25 (change is preserved)
     (t/is (= (:width copy-cp02-rect') 25))))
 
-
-
 (t/deftest test-switch-variant-that-has-swaped-copy
   (let [;; ==== Setup
         file      (-> (thf/sample-file :file1)
@@ -1193,7 +1241,6 @@
         ;; Switch :c01 for :c02
         file'     (tho/swap-component file copy01 :c02 {:new-shape-label :copy02 :keep-touched? true})
 
-
         copy02'    (ths/get-shape file' :copy02)
         copy-cp02' (ths/get-shape file' :copy-cp02)]
     (thf/dump-file file')
@@ -1207,7 +1254,6 @@
     ;;copy-02' had copy-cp02' as child
     (t/is (= (-> copy02' :shapes first) (:id copy-cp02')))))
 
-
 (t/deftest test-switch-variant-that-has-swaped-copy-with-changed-attr
   (let [;; ==== Setup
         file      (-> (thf/sample-file :file1)
@@ -1244,7 +1290,6 @@
         ;; Switch :c01 for :c02
         file'     (tho/swap-component file copy01 :c02 {:new-shape-label :copy02 :keep-touched? true})
 
-
         copy02'    (ths/get-shape file' :copy02)
         copy-cp02' (ths/get-shape file' :copy-cp02)
         copy-cp02-rect' (ths/get-shape-by-id file' (-> copy-cp02' :shapes first))]
@@ -1262,3 +1307,58 @@
     (t/is (= (-> copy02' :shapes first) (:id copy-cp02')))
     ;; The width of copy-cp02-rect' is 25 (change is preserved)
     (t/is (= (:width copy-cp02-rect') 25))))
+
+(t/deftest test-switch-variant-without-touched-but-touched-parent
+  (let [;; ==== Setup
+        file      (-> (thf/sample-file :file1)
+                      (thv/add-variant-with-child
+                       :v01 :c01 :m01 :c02 :m02 :r01 :r02
+                       {:child1-params {:width 5}
+                        :child2-params  {:width 5}})
+                      (tho/add-simple-component :external01 :external01-root :external01-child)
+
+                      (thc/instantiate-component :c01
+                                                 :c01-in-root
+                                                 :children-labels [:r01-in-c01-in-root]
+                                                 :parent-label :external01-root))
+
+        ;; Make a change on r01-in-c01-in-root so it is touched
+        page               (thf/current-page file)
+        r01-in-c01-in-root (ths/get-shape file :r01-in-c01-in-root)
+
+        changes            (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
+                                                       #{(:id r01-in-c01-in-root)}
+                                                       (fn [shape]
+                                                         (assoc shape :width 25))
+                                                       (:objects page)
+                                                       {})
+
+        file               (thf/apply-changes file changes)
+
+
+        ;; Instantiate the component :external01
+
+
+        file        (thc/instantiate-component file
+                                               :external01
+                                               :external-copy01
+                                               :children-labels [:external-copy01-rect :c01-in-copy])
+        page        (thf/current-page file)
+        c01-in-copy (ths/get-shape file :c01-in-copy)
+        rect01      (get-in page [:objects (-> c01-in-copy :shapes first)])
+
+
+        ;; ==== Action
+
+
+        file'        (tho/swap-component file c01-in-copy :c02 {:new-shape-label :c02-in-copy :keep-touched? true})
+
+        page'        (thf/current-page file')
+        c02-in-copy' (ths/get-shape file' :c02-in-copy)
+        rect02'      (get-in page' [:objects (-> c02-in-copy' :shapes first)])]
+
+    (thf/dump-file file :keys [:width :touched])
+    ;; The rect had width 25 before the switch
+    (t/is (= (:width rect01) 25))
+    ;; The rect still has width 25 after the switch
+    (t/is (= (:width rect02') 25))))

frontend/src/app/main/ui/workspace/sidebar/assets/common.cljs

@@ -391,7 +391,7 @@
         ;; and the variant-container in which it will be restored still exists
         (fn [shape]
           (let [component (find-component shape true)
-                main      (ctk/get-component-root component)
+                main      (ctk/get-deleted-component-root component)
                 objects   (dm/get-in libraries [(:component-file shape)
                                                 :data
                                                 :pages-index