github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-04-25 11:18:36 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Fix content attribute sync group resolution by shape type (#8724)

Browse Source 
* 🐛 Fix content attribute sync group resolution by shape type

The :content attribute was mapped to a single sync group (:content-group)
but it is used by both path and text shapes with different synchronization
needs. This caused incorrect component synchronization when editing content
on path shapes, as they should sync under :geometry-group instead of
:content-group.

Changes:
- Make sync-attrs allow type-dependent group mapping via maps
- Add resolve-sync-group and resolve-sync-groups helper functions
- Update all sync-attr lookups to use shape type for proper resolution
- Fix touched checks to handle multiple possible sync groups

Signed-off-by: Andrey Antukh <niwi@niwi.nz>

* ✨ Make PR feedback changes

* 🔥 Remove unused function

---------

Signed-off-by: Andrey Antukh <niwi@niwi.nz>
This commit is contained in:
Andrey Antukh 2026-03-30 13:11:01 +02:00 committed by GitHub
parent 264cd0aaac
commit 04892dd688
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 122 additions and 86 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
common/src/app/common/files/changes.cljc
View File

@ -1191,9 +1191,9 @@
; Check if the shape has changed any
; attribute that participates in components synchronization.
(and (= (:type operation) :set)
(get ctk/sync-attrs (:attr operation))))
any-sync? (some need-sync? operations)]
(when any-sync?
(contains? ctk/sync-attrs (:attr operation))))]
(when (some need-sync? operations)
(parents-frames id (:objects page))))))
(defmethod frames-changed :mov-objects

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

@ -48,11 +48,12 @@
(def log-shape-ids #{})
(def log-container-ids #{})
(def updatable-attrs (->> (seq (keys ctk/sync-attrs))
;; We don't update the flex-child attrs
(remove ctk/swap-keep-attrs)
;; We don't do automatic update of the `layout-grid-cells` property.
(remove #(= :layout-grid-cells %))))
(def updatable-attrs
(->> (keys ctk/sync-attrs)
;; We don't update the flex-child attrs
(remove ctk/swap-keep-attrs)
;; We don't do automatic update of the `layout-grid-cells` property.
(remove #(= :layout-grid-cells %))))
(defn enabled-shape?
[id container]
@ -1646,19 +1647,22 @@
(defn- generate-update-tokens
[changes container dest-shape origin-shape touched omit-touched? valid-attrs]
;; valid-attrs is a set of attrs to consider on the update. If it is nil, it will consider all the attrs
(let [attrs (->> (seq (keys ctk/sync-attrs))
;; We don't update the flex-child attrs
(remove #(= :layout-grid-cells %)))
(let [attrs
(->> (keys ctk/sync-attrs)
;; We don't update the flex-child attrs
(remove #(= :layout-grid-cells %)))
applied-tokens (reduce (fn [applied-tokens attr]
(let [attr-group (get ctk/sync-attrs attr)
token-attrs (cto/shape-attr->token-attrs attr)]
(if (and (or (not omit-touched?) (not (touched attr-group)))
(or (empty? valid-attrs) (contains? valid-attrs attr)))
(into applied-tokens token-attrs)
applied-tokens)))
#{}
attrs)]
applied-tokens
(reduce (fn [applied-tokens attr]
(let [sync-group (or (ctk/resolve-sync-group (:type origin-shape) attr)
(ctk/resolve-sync-group (:type dest-shape) attr))
token-attrs (cto/shape-attr->token-attrs attr)]
(if (and (or (not omit-touched?) (not (touched sync-group)))
(or (empty? valid-attrs) (contains? valid-attrs attr)))
(into applied-tokens token-attrs)
applied-tokens)))
#{}
attrs)]
(cond-> changes
(seq applied-tokens)
(update-tokens container dest-shape origin-shape applied-tokens))))
@ -1804,6 +1808,7 @@
uoperations '()]
(let [attr (first attrs)]
(if (nil? attr)
(cond-> changes
(seq roperations)
@ -1813,55 +1818,60 @@
:always
(generate-update-tokens container dest-shape origin-shape touched omit-touched? nil))
(let [attr-group (get ctk/sync-attrs attr)
(let [sync-group
(or (ctk/resolve-sync-group (:type origin-shape) attr)
(ctk/resolve-sync-group (:type dest-shape) attr))
;; position-data is a special case because can be affected by
;; :geometry-group and :content-group so, if the position-data
;; changes but the geometry is touched we need to reset the position-data
;; so it's calculated again
reset-pos-data? (and (cfh/text-shape? origin-shape)
(= attr :position-data)
(not= (:position-data origin-shape) (:position-data dest-shape))
(touched :geometry-group))
;; :geometry-group and :content-group so, if the
;; position-data changes but the geometry is touched
;; we need to reset the position-data so it's
;; calculated again
reset-pos-data?
(and (cfh/text-shape? origin-shape)
(= attr :position-data)
(not= (:position-data origin-shape) (:position-data dest-shape))
(touched :geometry-group))
;; On texts, when we want to omit the touched attrs, both text (the actual letters)
;; and attrs (bold, font, etc) are in the same attr :content.
;; If only one of them is touched, we want to adress this case and
;; only update the untouched one
text-content-change?
(and
omit-touched?
(cfh/text-shape? origin-shape)
(= :content attr)
(touched attr-group))
(and omit-touched?
(cfh/text-shape? origin-shape)
(= :content attr)
(touched sync-group))
skip-operations?
(or (= (get origin-shape attr) (get dest-shape attr))
(and (touched attr-group)
(and (touched sync-group)
omit-touched?
;; When it is a text-partial-change, we should generate operations
;; even when omit-touched? is true, but updating only the text or
;; the attributes, omiting the other part
(not text-content-change?)))
attr-val (when-not skip-operations?
(cond
;; If position data changes and the geometry group is touched
;; we need to put to nil so we can regenerate it
reset-pos-data?
nil
attr-val
(when-not skip-operations?
(cond
;; If position data changes and the geometry group is touched
;; we need to put to nil so we can regenerate it
reset-pos-data?
nil
text-content-change?
(text-change-value (:content dest-shape)
(:content origin-shape)
touched)
text-content-change?
(text-change-value (:content dest-shape)
(:content origin-shape)
touched)
:else
(get origin-shape attr)))
:else
(get origin-shape attr)))
;; If the final attr-value is the actual value, skip
skip-operations? (or skip-operations?
(= attr-val (get dest-shape attr)))
skip-operations?
(or skip-operations?
(= attr-val (get dest-shape attr)))
;; On a text-partial-change, we want to force a position-data reset
;; so it's calculated again
@ -2079,7 +2089,9 @@
roperations [{:type :set-touched :touched (:touched previous-shape)}]
uoperations (list {:type :set-touched :touched (:touched current-shape)})]
(if-let [attr (first attrs)]
(let [attr-group (get ctk/sync-attrs attr)
(let [sync-group
(ctk/resolve-sync-group (:type previous-shape) attr)
skip-operations?
(or
;; For auto text, avoid copying geometry-driven attrs on switch.
@ -2096,7 +2108,7 @@
(= (get previous-shape attr) (get origin-ref-shape attr))
;; If the attr is not touched, don't copy it
(not (touched attr-group))
(not (touched sync-group))
;; If both variants (origin and destiny) don't have the same value
;; for that attribute, don't copy it.
@ -2120,12 +2132,11 @@
;; If only one of them is touched, we want to adress this case and
;; only update the untouched one
text-change?
(and
(not skip-operations?)
(cfh/text-shape? current-shape)
(cfh/text-shape? previous-shape)
(= :content attr)
(touched attr-group))
(and (not skip-operations?)
(cfh/text-shape? current-shape)
(cfh/text-shape? previous-shape)
(= :content attr)
(touched sync-group))
path-change?
(and (= :path (:type current-shape))
@ -2218,9 +2229,12 @@
(->> attrs
(reduce
(fn [dest attr]
(let [attr-group (get ctk/sync-attrs attr)]
(let [sync-group
(or (ctk/resolve-sync-group (:type origin) attr)
(ctk/resolve-sync-group (:type dest) attr))]
(cond-> dest
(or (not (touched attr-group)) (not omit-touched?))
(or (not (touched sync-group))
(not omit-touched?))
(assoc attr (get origin attr)))))
dest))))

45
common/src/app/common/types/component.cljc
View File

@ -7,6 +7,7 @@
(ns app.common.types.component
(:require
[app.common.data :as d]
[app.common.exceptions :as ex]
[app.common.schema :as sm]
[app.common.time :as-alias ct]
[app.common.types.page :as ctp]
@ -39,15 +40,17 @@
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Attributes that may be synced in components, and the group they belong to.
;; When one attribute is modified in a shape inside a component, the corresponding
;; group is marked as :touched. Then, if the shape is synced with the remote shape
;; in the main component, none of the attributes of the same group is changed.
;; When one attribute is modified in a shape inside a component, the
;; corresponding group is marked as :touched. Then, if the shape is synced with
;; the remote shape in the main component, none of the attributes of the same
;; group is changed.
(def sync-attrs
{:name :name-group
:fills :fill-group
:hide-fill-on-export :fill-group
:content :content-group
:content {:path :geometry-group
:text :content-group}
:position-data :content-group
:hidden :visibility-group
:blocked :modifiable-group
@ -143,6 +146,18 @@
:layout-item-align-self
:interactions})
(defn resolve-sync-group
"Makes a by type resolution of the sync group. This is necessary
because we have several properties that has different group
depending on the shape type. Per example the attr `:content` is used
by path and text shapes and the sync groups are different for each
shape type."
[type attr]
(when-let [group (get sync-attrs attr)]
(if (map? group)
(get group type)
group)))
(defn component-attr?
"Check if some attribute is one that is involved in component syncrhonization.
Note that design tokens also are involved, although they go by an alternate
@ -150,7 +165,7 @@
Also when detaching a nested copy it also needs to trigger a synchronization,
even though :shape-ref is not a synced attribute per se"
[attr]
(or (get sync-attrs attr)
(or (contains? sync-attrs attr)
(= :shape-ref attr)
(= :applied-tokens attr)))
@ -356,15 +371,17 @@
(or (not (instance-head? shape))
(not (in-component-copy? parent))))))
(defn all-touched-groups
[]
(into #{} (vals sync-attrs)))
(def ^:private all-touched-groups
(reduce-kv (fn [acc _ v]
(if (map? v)
(into acc (vals v))
(conj acc v)))
#{}
sync-attrs))
(defn valid-touched-group?
[group]
(try
(or (contains? (all-touched-groups) group)
(and (swap-slot? group)
(some? (group->swap-slot group))))
(catch #?(:clj Throwable :cljs :default) _
false)))
(ex/ignoring
(or (contains? all-touched-groups group)
(and (swap-slot? group)
(some? (group->swap-slot group))))))

31
common/src/app/common/types/container.cljc
View File

@ -554,14 +554,14 @@
(let [old-applied-tokens (d/nilv (:applied-tokens shape) #{})
changed-token-attrs (filter #(not= (get old-applied-tokens %) (get new-applied-tokens %))
ctt/all-keys)
text-shape? (= (:type shape) :text)
shape-type (get shape :type)
text-shape? (= shape-type :text)
attrs-in-text-content? (some #(ctt/attrs-in-text-content %)
changed-token-attrs)
changed-groups (into #{}
(comp (map ctt/token-attr->shape-attr)
(map #(get ctk/sync-attrs %))
(filter some?))
(keep #(ctk/resolve-sync-group shape-type %)))
changed-token-attrs)
changed-groups (if (and text-shape?
@ -577,8 +577,9 @@
The returned shape will contain a metadata associated with it
indicating if shape is touched or not."
[shape attr val & {:keys [ignore-touched ignore-geometry]}]
(let [group (get ctk/sync-attrs attr)
shape-val (get shape attr)
(let [type (get shape :type)
group (ctk/resolve-sync-group type attr)
shape-val (get shape attr)
ignore?
(or ignore-touched
@ -612,16 +613,20 @@
(not equal?)
(not (and ignore-geometry is-geometry?)))
content-diff-type (when (and (= (:type shape) :text) (= attr :content))
(cttx/get-diff-type (:content shape) val))
content-diff-type
(when (and (= type :text) (= attr :content))
(cttx/get-diff-type (:content shape) val))
token-groups (if (= attr :applied-tokens)
(get-token-groups shape val)
#{})
token-groups
(if (= attr :applied-tokens)
(get-token-groups shape val)
#{})
groups
(cond-> token-groups
(and group (not equal?))
(set/union #{group} content-diff-type))]
groups (cond-> token-groups
(and group (not equal?))
(set/union #{group} content-diff-type))]
(cond-> shape
;; Depending on the origin of the attribute change, we need or not to
;; set the "touched" flag for the group the attribute belongs to.