🐛 Fix undo stacking when changing color from color-picker

CHANGES.md

@@ -8,6 +8,7 @@
 ### :bug: Bugs fixed
 
 - Fix problem with exporting before the document is saved [Taiga #2189](https://tree.taiga.io/project/penpot/issue/2189)
+- Fix undo stacking when changing color from color-picker [Taiga #2191](https://tree.taiga.io/project/penpot/issue/2191)
 
 ### :arrow_up: Deps updates
 ### :heart: Community contributions by (Thank you!)

frontend/src/app/main/ui/workspace/colorpicker.cljs

@@ -9,6 +9,7 @@
    [app.main.data.modal :as modal]
    [app.main.data.workspace.colors :as dc]
    [app.main.data.workspace.libraries :as dwl]
+   [app.main.data.workspace.undo :as dwu]
    [app.main.refs :as refs]
    [app.main.store :as st]
    [app.main.ui.icons :as i]
@@ -137,12 +138,13 @@
                             editing-stop (update-in [:stops editing-stop] merge changes)))
             (reset! dirty? true)))
 
-        handle-click-picker (fn []
-                              (if picking-color?
-                                (do (modal/disallow-click-outside!)
-                                    (st/emit! (dc/stop-picker)))
-                                (do (modal/allow-click-outside!)
-                                    (st/emit! (dc/start-picker)))))
+        handle-click-picker
+        (fn []
+          (if picking-color?
+            (do (modal/disallow-click-outside!)
+                (st/emit! (dc/stop-picker)))
+            (do (modal/allow-click-outside!)
+                (st/emit! (dc/start-picker)))))
 
         handle-change-stop
         (fn [offset]
@@ -307,13 +309,19 @@
         (case @active-tab
           :ramp [:& ramp-selector {:color current-color
                                    :disable-opacity disable-opacity
-                                   :on-change handle-change-color}]
+                                   :on-change handle-change-color
+                                   :on-start-drag #(st/emit! (dwu/start-undo-transaction))
+                                   :on-finish-drag #(st/emit! (dwu/commit-undo-transaction))}]
           :harmony [:& harmony-selector {:color current-color
                                          :disable-opacity disable-opacity
-                                         :on-change handle-change-color}]
+                                         :on-change handle-change-color
+                                         :on-start-drag #(st/emit! (dwu/start-undo-transaction))
+                                         :on-finish-drag #(st/emit! (dwu/commit-undo-transaction))}]
           :hsva [:& hsva-selector {:color current-color
                                    :disable-opacity disable-opacity
-                                   :on-change handle-change-color}]
+                                   :on-change handle-change-color
+                                   :on-start-drag #(st/emit! (dwu/start-undo-transaction))
+                                   :on-finish-drag #(st/emit! (dwu/commit-undo-transaction))}]
           nil))
 
       [:& color-inputs {:type (if (= @active-tab :hsva) :hsv :rgb)
@@ -363,7 +371,7 @@
         position (or position :left)
         style (calculate-position vport position x y)
 
-        handle-change (fn [new-data _shift-clicked?]
+        handle-change (fn [new-data]
                         (reset! dirty? (not= data new-data))
                         (reset! last-change new-data)
                         (when on-change

frontend/src/app/main/ui/workspace/colorpicker/harmony.cljs

@@ -56,7 +56,7 @@
         y (+ (/ canvas-side 2) (* comp-y (/ canvas-side 2)))]
     (gpt/point x y)))
 
-(mf/defc harmony-selector [{:keys [color disable-opacity on-change]}]
+(mf/defc harmony-selector [{:keys [color disable-opacity on-change on-start-drag on-finish-drag]}]
   (let [canvas-ref (mf/use-ref nil)
         {hue :h saturation :s value :v alpha :alpha} color
 
@@ -84,6 +84,24 @@
                                       :h new-hue
                                       :s new-saturation})))
 
+        handle-start-drag
+        (mf/use-callback
+         (mf/deps on-start-drag)
+         (fn [event]
+           (dom/capture-pointer event)
+           (reset! dragging? true)
+           (when on-start-drag
+             (on-start-drag))))
+
+        handle-stop-drag
+        (mf/use-callback
+         (mf/deps on-finish-drag)
+         (fn [event]
+           (dom/release-pointer event)
+           (reset! dragging? false)
+           (when on-finish-drag
+             (on-finish-drag))))
+
         on-change-value (fn [new-value]
                           (let [hex (uc/hsv->hex [hue saturation new-value])
                                 [r g b] (uc/hex->rgb hex)]
@@ -112,11 +130,8 @@
        {:ref canvas-ref
         :width canvas-side
         :height canvas-side
-        :on-mouse-down #(reset! dragging? true)
-        :on-mouse-up #(reset! dragging? false)
-        :on-pointer-down (partial dom/capture-pointer)
-        :on-lost-pointer-capture #(do (dom/release-pointer %)
-                                      (reset! dragging? false))
+        :on-pointer-down handle-start-drag
+        :on-lost-pointer-capture handle-stop-drag
         :on-click calculate-pos
         :on-mouse-move #(when @dragging? (calculate-pos %))}]
       [:div.handler {:style {:pointer-events "none"
@@ -133,11 +148,15 @@
                            :value value
                            :max-value 255
                            :vertical true
-                           :on-change on-change-value}]
+                           :on-change on-change-value
+                           :on-start-drag on-start-drag
+                           :on-finish-drag on-finish-drag}]
       (when (not disable-opacity)
         [:& slider-selector {:class "opacity"
                              :vertical? true
                              :value alpha
                              :max-value 1
                              :vertical true
-                             :on-change on-change-opacity}])]]))
+                             :on-change on-change-opacity
+                             :on-start-drag on-start-drag
+                             :on-finish-drag on-finish-drag}])]]))

frontend/src/app/main/ui/workspace/colorpicker/hsva.cljs

@@ -10,7 +10,7 @@
    [app.util.color :as uc]
    [rumext.alpha :as mf]))
 
-(mf/defc hsva-selector [{:keys [color disable-opacity on-change]}]
+(mf/defc hsva-selector [{:keys [color disable-opacity on-change on-start-drag on-finish-drag]}]
   (let [{hue :h saturation :s value :v alpha :alpha} color
         handle-change-slider (fn [key]
                                (fn [new-value]
@@ -25,18 +25,39 @@
     [:div.hsva-selector
      [:span.hsva-selector-label "H"]
      [:& slider-selector
-      {:class "hue" :max-value 360 :value hue :on-change (handle-change-slider :h)}]
+      {:class "hue"
+       :max-value 360
+       :value hue
+       :on-change (handle-change-slider :h)
+       :on-start-drag on-start-drag
+       :on-finish-drag on-finish-drag}]
 
      [:span.hsva-selector-label "S"]
      [:& slider-selector
-      {:class "saturation" :max-value 1 :value saturation :on-change (handle-change-slider :s)}]
+      {:class "saturation"
+       :max-value 1
+       :value saturation
+       :on-change (handle-change-slider :s)
+       :on-start-drag on-start-drag
+       :on-finish-drag on-finish-drag}]
 
      [:span.hsva-selector-label "V"]
      [:& slider-selector
-      {:class "value" :reverse? true :max-value 255 :value value :on-change (handle-change-slider :v)}]
+      {:class "value"
+       :reverse? true
+       :max-value 255
+       :value value
+       :on-change (handle-change-slider :v)
+       :on-start-drag on-start-drag
+       :on-finish-drag on-finish-drag}]
 
      (when (not disable-opacity)
        [:*
         [:span.hsva-selector-label "A"]
         [:& slider-selector
-         {:class "opacity" :max-value 1 :value alpha :on-change on-change-opacity}]])]))
+         {:class "opacity"
+          :max-value 1
+          :value alpha
+          :on-change on-change-opacity
+          :on-start-drag on-start-drag
+          :on-finish-drag on-finish-drag}]])]))

frontend/src/app/main/ui/workspace/colorpicker/ramp.cljs

@@ -13,7 +13,7 @@
    [app.util.dom :as dom]
    [rumext.alpha :as mf]))
 
-(mf/defc value-saturation-selector [{:keys [saturation value on-change]}]
+(mf/defc value-saturation-selector [{:keys [saturation value on-change on-start-drag on-finish-drag]}]
   (let [dragging? (mf/use-state false)
         calculate-pos
         (fn [ev]
@@ -21,13 +21,27 @@
                 {:keys [x y]} (-> ev dom/get-client-position)
                 px (math/clamp (/ (- x left) (- right left)) 0 1)
                 py (* 255 (- 1 (math/clamp (/ (- y top) (- bottom top)) 0 1)))]
-            (on-change px py)))]
+            (on-change px py)))
+
+        handle-start-drag
+        (mf/use-callback
+         (mf/deps on-start-drag)
+         (fn [event]
+           (dom/capture-pointer event)
+           (reset! dragging? true)
+           (on-start-drag)))
+
+        handle-stop-drag
+        (mf/use-callback
+         (mf/deps on-finish-drag)
+         (fn [event]
+           (dom/release-pointer event)
+           (reset! dragging? false)
+           (on-finish-drag)))
+        ]
     [:div.value-saturation-selector
-     {:on-mouse-down #(reset! dragging? true)
-      :on-mouse-up #(reset! dragging? false)
-      :on-pointer-down (partial dom/capture-pointer)
-      :on-lost-pointer-capture #(do (dom/release-pointer %)
-                                    (reset! dragging? false))
+     {:on-pointer-down handle-start-drag
+      :on-lost-pointer-capture handle-stop-drag
       :on-click calculate-pos
       :on-mouse-move #(when @dragging? (calculate-pos %))}
      [:div.handler {:style {:pointer-events "none"
@@ -35,7 +49,7 @@
                             :top (str (* 100 (- 1 (/ value 255))) "%")}}]]))
 
 
-(mf/defc ramp-selector [{:keys [color disable-opacity on-change]}]
+(mf/defc ramp-selector [{:keys [color disable-opacity on-change on-start-drag on-finish-drag]}]
   (let [{hex :hex
          hue :h saturation :s value :v alpha :alpha} color
 
@@ -64,7 +78,9 @@
       {:hue hue
        :saturation saturation
        :value value
-       :on-change on-change-value-saturation}]
+       :on-change on-change-value-saturation
+       :on-start-drag on-start-drag
+       :on-finish-drag on-finish-drag}]
 
      [:div.shade-selector
       [:& color-bullet {:color {:color hex
@@ -72,10 +88,14 @@
       [:& slider-selector {:class "hue"
                            :max-value 360
                            :value hue
-                           :on-change on-change-hue}]
+                           :on-change on-change-hue
+                           :on-start-drag on-start-drag
+                           :on-finish-drag on-finish-drag}]
 
       (when (not disable-opacity)
         [:& slider-selector {:class "opacity"
                              :max-value 1
                              :value alpha
-                             :on-change on-change-opacity}])]]))
+                             :on-change on-change-opacity
+                             :on-start-drag on-start-drag
+                             :on-finish-drag on-finish-drag}])]]))

frontend/src/app/main/ui/workspace/colorpicker/slider_selector.cljs

@@ -12,10 +12,29 @@
    [rumext.alpha :as mf]))
 
 (mf/defc slider-selector
-  [{:keys [value class min-value max-value vertical? reverse? on-change]}]
+  [{:keys [value class min-value max-value vertical? reverse? on-change on-start-drag on-finish-drag]}]
   (let [min-value (or min-value 0)
         max-value (or max-value 1)
         dragging? (mf/use-state false)
+
+        handle-start-drag
+        (mf/use-callback
+         (mf/deps on-start-drag)
+         (fn [event]
+           (dom/capture-pointer event)
+           (reset! dragging? true)
+           (when on-start-drag
+             (on-start-drag))))
+
+        handle-stop-drag
+        (mf/use-callback
+         (mf/deps on-finish-drag)
+         (fn [event]
+           (dom/release-pointer event)
+           (reset! dragging? false)
+           (when on-finish-drag
+             (on-finish-drag))))
+
         calculate-pos
         (fn [ev]
           (when on-change
@@ -32,11 +51,8 @@
 
     [:div.slider-selector
      {:class (str (if vertical? "vertical " "") class)
-      :on-mouse-down #(reset! dragging? true)
-      :on-mouse-up #(reset! dragging? false)
-      :on-pointer-down (partial dom/capture-pointer)
-      :on-lost-pointer-capture #(do (dom/release-pointer %)
-                                    (reset! dragging? false))
+      :on-pointer-down handle-start-drag
+      :on-lost-pointer-capture handle-stop-drag
       :on-click calculate-pos
       :on-mouse-move #(when @dragging? (calculate-pos %))}
 

frontend/src/app/main/ui/workspace/sidebar/options/menus/fill.cljs

@@ -8,7 +8,6 @@
   (:require
    [app.common.pages :as cp]
    [app.main.data.workspace.colors :as dc]
-   [app.main.data.workspace.undo :as dwu]
    [app.main.store :as st]
    [app.main.ui.icons :as i]
    [app.main.ui.workspace.sidebar.options.rows.color-row :refer [color-row]]
@@ -70,17 +69,7 @@
                             (assoc :id nil :file-id nil))]
               (st/emit! (dc/change-fill ids color)))))
 
-        on-open-picker
-        (mf/use-callback
-          (mf/deps ids)
-          (fn [_value _opacity _id _file-id]
-            (st/emit! (dwu/start-undo-transaction))))
-
-        on-close-picker
-        (mf/use-callback
-          (mf/deps ids)
-          (fn [_value _opacity _id _file-id]
-            (st/emit! (dwu/commit-undo-transaction))))]
+        ]
 
     (if show?
       [:div.element-set
@@ -91,9 +80,7 @@
        [:div.element-set-content
         [:& color-row {:color color
                        :on-change on-change
-                       :on-detach on-detach
-                       :on-open on-open-picker
-                       :on-close on-close-picker}]]]
+                       :on-detach on-detach}]]]
 
       [:div.element-set
        [:div.element-set-title

frontend/src/app/main/ui/workspace/sidebar/options/menus/stroke.cljs

@@ -11,7 +11,6 @@
    [app.common.pages.spec :as spec]
    [app.main.data.workspace.changes :as dch]
    [app.main.data.workspace.colors :as dc]
-   [app.main.data.workspace.undo :as dwu]
    [app.main.store :as st]
    [app.main.ui.components.dropdown :refer [dropdown]]
    [app.main.ui.icons :as i]
@@ -188,19 +187,7 @@
 
         on-del-stroke
         (fn [_]
-          (st/emit! (dch/update-shapes ids #(assoc % :stroke-style :none))))
-
-        on-open-picker
-        (mf/use-callback
-         (mf/deps ids)
-         (fn [_value _opacity _id _file-id]
-           (st/emit! (dwu/start-undo-transaction))))
-
-        on-close-picker
-        (mf/use-callback
-         (mf/deps ids)
-         (fn [_value _opacity _id _file-id]
-           (st/emit! (dwu/commit-undo-transaction))))]
+          (st/emit! (dch/update-shapes ids #(assoc % :stroke-style :none))))]
 
     (if show-options
       [:div.element-set
@@ -212,9 +199,7 @@
         ;; Stroke Color
         [:& color-row {:color current-stroke-color
                        :on-change handle-change-stroke-color
-                       :on-detach handle-detach
-                       :on-open on-open-picker
-                       :on-close on-close-picker}]
+                       :on-detach handle-detach}]
 
         ;; Stroke Width, Alignment & Style
         [:div.row-flex

frontend/src/app/util/dom.cljs

@@ -288,7 +288,8 @@
   (-> event get-target (.setPointerCapture (.-pointerId event))))
 
 (defn release-pointer [event]
-  (-> event get-target (.releasePointerCapture (.-pointerId event))))
+  (when (.-pointerId event)
+    (-> event get-target (.releasePointerCapture (.-pointerId event)))))
 
 (defn get-root []
   (query globals/document "#app"))