From c65b24495b5a590a4b99707c4b0a4d6fabf461e2 Mon Sep 17 00:00:00 2001 From: Alonso Torres Date: Wed, 13 May 2026 15:54:05 +0200 Subject: [PATCH] :bug: Fix several issues with color picker (#9558) --- .gitignore | 3 + .../app/main/ui/workspace/colorpicker.cljs | 7 +- .../ui/workspace/viewport/pixel_overlay.cljs | 213 ++++++++++++------ 3 files changed, 157 insertions(+), 66 deletions(-) diff --git a/.gitignore b/.gitignore index dc4861f51f..b295d6aeac 100644 --- a/.gitignore +++ b/.gitignore @@ -86,3 +86,6 @@ /**/.yarn/* /.pnpm-store /.vscode +/.idea +/.claude +/.playwright-mcp diff --git a/frontend/src/app/main/ui/workspace/colorpicker.cljs b/frontend/src/app/main/ui/workspace/colorpicker.cljs index 6dfd4f1798..c1f8ea396a 100644 --- a/frontend/src/app/main/ui/workspace/colorpicker.cljs +++ b/frontend/src/app/main/ui/workspace/colorpicker.cljs @@ -376,7 +376,12 @@ ;; Initialize colorpicker state (mf/with-effect [] (st/emit! (dc/initialize-colorpicker on-change active-fill-tab)) - (partial st/emit! (dc/finalize-colorpicker))) + ;; Always deactivate picking mode on unmount so that :picking-color? never + ;; stays true if the modal closes for any reason other than the normal + ;; pointer-up path (e.g. ESC, navigation, programmatic hide). + (fn [] + (st/emit! (dc/stop-picker) + (dc/finalize-colorpicker)))) ;; Update colorpicker with external color changes (mf/with-effect [data] diff --git a/frontend/src/app/main/ui/workspace/viewport/pixel_overlay.cljs b/frontend/src/app/main/ui/workspace/viewport/pixel_overlay.cljs index 73c6428a5f..1cf70067a6 100644 --- a/frontend/src/app/main/ui/workspace/viewport/pixel_overlay.cljs +++ b/frontend/src/app/main/ui/workspace/viewport/pixel_overlay.cljs @@ -53,26 +53,22 @@ new-canvas)))))))) (defn process-pointer-move - [viewport-node canvas canvas-image-data zoom-view-context client-x client-y] + [viewport-node canvas canvas-image-data zoom-view-context last-picked-color client-x client-y] (when-let [image-data (mf/ref-val canvas-image-data)] (when-let [zoom-view-node (dom/get-element "picker-detail")] (when-not (mf/ref-val zoom-view-context) (mf/set-ref-val! zoom-view-context (.getContext zoom-view-node "2d"))) - (let [canvas-width 260 + (let [canvas-width 260 canvas-height 140 {brx :left bry :top} (dom/get-bounding-rect viewport-node) x (mth/floor (- client-x brx)) y (mth/floor (- client-y bry)) - zoom-context (mf/ref-val zoom-view-context) - offset (* (+ (* y (unchecked-get image-data "width")) x) 4) - rgba (unchecked-get image-data "data") + img-width (unchecked-get image-data "width") + img-height (unchecked-get image-data "height") - r (obj/get rgba (+ 0 offset)) - g (obj/get rgba (+ 1 offset)) - b (obj/get rgba (+ 2 offset)) - a (obj/get rgba (+ 3 offset)) + zoom-context (mf/ref-val zoom-view-context) sx (- x 32) sy (if (cfg/check-browser? :safari) y (- y 17)) @@ -82,13 +78,27 @@ dy 0 dw canvas-width dh canvas-height] + (when (obj/get zoom-context "imageSmoothingEnabled") (obj/set! zoom-context "imageSmoothingEnabled" false)) (.clearRect zoom-context 0 0 canvas-width canvas-height) (.drawImage zoom-context canvas sx sy sw sh dx dy dw dh) - (js/requestAnimationFrame - (fn [] - (st/emit! (dwc/pick-color [r g b a])))))))) + + ;; Only pick color when cursor is within canvas bounds to avoid garbage pixels + (when (and (>= x 0) (< x img-width) (>= y 0) (< y img-height)) + (let [offset (* (+ (* y img-width) x) 4) + rgba (unchecked-get image-data "data") + r (obj/get rgba (+ 0 offset)) + g (obj/get rgba (+ 1 offset)) + b (obj/get rgba (+ 2 offset)) + a (obj/get rgba (+ 3 offset)) + color [r g b a]] + ;; Store latest color synchronously so the click handler always reads + ;; the correct pixel even before the rAF fires (fixes race condition) + (mf/set-ref-val! last-picked-color color) + (js/requestAnimationFrame + (fn [] + (st/emit! (dwc/pick-color color)))))))))) (mf/defc pixel-overlay @@ -103,8 +113,14 @@ canvas-context (.getContext canvas "2d" #js {:willReadFrequently true}) canvas-image-data (mf/use-ref nil) zoom-view-context (mf/use-ref nil) - canvas-ready (mf/use-state false) - initial-mouse-pos (mf/use-state {:x 0 :y 0}) + ;; Holds the last successfully picked [r g b a] synchronously so that + ;; the pointer-down handler always has the current pixel, regardless of + ;; whether the rAF-deferred store update has fired yet. + last-picked-color (mf/use-ref nil) + ;; Use a ref (not state) so tracking the cursor doesn't cause re-renders. + ;; Updated by both on-mouse-enter and a document-level pointermove listener + ;; so that the position is always current when the canvas first becomes ready. + initial-mouse-pos (mf/use-ref {:x 0 :y 0}) update-str (rx/subject) handle-keydown @@ -121,8 +137,15 @@ (fn [event] (dom/prevent-default event) (dom/stop-propagation event) - (st/emit! (dwu/start-undo-transaction :mouse-down-picker) - (dwc/pick-color-select true (kbd/shift? event))))) + ;; Emit pick-color synchronously with the latest pixel colour before + ;; pick-color-select, so the colorpicker effect never sees a stale value. + (let [color (mf/ref-val last-picked-color)] + (if (some? color) + (st/emit! (dwu/start-undo-transaction :mouse-down-picker) + (dwc/pick-color color) + (dwc/pick-color-select true (kbd/shift? event))) + (st/emit! (dwu/start-undo-transaction :mouse-down-picker) + (dwc/pick-color-select true (kbd/shift? event))))))) handle-pointer-up-picker (mf/use-callback @@ -143,16 +166,20 @@ :result "image-bitmap"}] (->> (fonts/render-font-styles-cached fonts) (rx/map (fn [styles] - (assoc result - :styles styles))) + (assoc result :styles styles))) (rx/mapcat thr/render-node) (rx/subs! (fn [image-bitmap] (.drawImage canvas-context image-bitmap 0 0) (let [width (unchecked-get canvas "width") height (unchecked-get canvas "height") - image-data (.getImageData canvas-context 0 0 width height)] + image-data (.getImageData canvas-context 0 0 width height) + ;; Read current mouse position from ref so the zoom + ;; is populated immediately even without a mouse-move. + {mx :x my :y} (mf/ref-val initial-mouse-pos)] (mf/set-ref-val! canvas-image-data image-data) - (reset! canvas-ready true)))))))) + (process-pointer-move viewport-node canvas canvas-image-data + zoom-view-context last-picked-color + mx my)))))))) handle-svg-change (mf/use-callback @@ -163,19 +190,28 @@ (mf/use-callback (mf/deps viewport-node) (fn [event] - (let [x (.-clientX event) - y (.-clientY event)] - (reset! initial-mouse-pos {:x x - :y y})))) + (mf/set-ref-val! initial-mouse-pos + {:x (.-clientX event) + :y (.-clientY event)}))) + handle-pointer-move-picker (mf/use-callback (mf/deps viewport-node) (fn [event] - (process-pointer-move viewport-node canvas canvas-image-data zoom-view-context (.-clientX event) (.-clientY event))))] + (process-pointer-move viewport-node canvas canvas-image-data zoom-view-context + last-picked-color (.-clientX event) (.-clientY event))))] (when (obj/get canvas-context "imageSmoothingEnabled") (obj/set! canvas-context "imageSmoothingEnabled" false)) + ;; Move focus to the overlay div on mount so the eyedropper button loses + ;; :focus styling immediately. Without this, prevent-default on pointer-down + ;; keeps focus on the button and it looks "selected" even after picking. + (mf/use-effect + (fn [] + (when-let [node (dom/get-element "pixel-overlay")] + (.focus node)))) + (mf/use-effect (fn [] (let [listener (events/listen ug/document "keydown" handle-keydown)] @@ -202,12 +238,17 @@ ;; Disconnect on unmount #(.disconnect observer)))) + ;; Track the cursor position at document level so initial-mouse-pos is always + ;; current when the canvas first becomes ready — even when the picker is opened + ;; via the "i" shortcut and the cursor hasn't entered/moved over the overlay yet. (mf/use-effect - (mf/deps viewport-node @canvas-ready) (fn [] - (when canvas-ready - (let [{:keys [x y]} @initial-mouse-pos] - (process-pointer-move viewport-node canvas canvas-image-data zoom-view-context x y))))) + (let [listener (events/listen ug/document "pointermove" + (fn [e] + (mf/set-ref-val! initial-mouse-pos + {:x (.-clientX e) + :y (.-clientY e)})))] + #(events/unlistenByKey listener)))) [:div {:id "pixel-overlay" :tab-index 0 @@ -218,12 +259,13 @@ :on-mouse-enter handle-mouse-enter}])) -(defn process-pointer-move-wasm [viewport-node canvas canvas-image-data zoom-view-context client-x client-y] +(defn process-pointer-move-wasm + [viewport-node canvas canvas-image-data zoom-view-context last-picked-color client-x client-y] (when-let [image-data (mf/ref-val canvas-image-data)] (when-let [zoom-view-node (dom/get-element "picker-detail")] (when-not (mf/ref-val zoom-view-context) (mf/set-ref-val! zoom-view-context (.getContext zoom-view-node "2d"))) - (let [zoom-view-width 260 + (let [zoom-view-width 260 zoom-view-height 140 {brx :left bry :top} (dom/get-bounding-rect viewport-node) x (mth/floor (- client-x brx)) @@ -232,31 +274,39 @@ canvas-x (* x wasm.api/dpr) canvas-y (* y wasm.api/dpr) - zoom-context (mf/ref-val zoom-view-context) - ;; the image-data we have is an array of pixels, starting from the - ;; bottom-left corner; so we need to calculate the offset accordingly - inverted-y (- (.-height image-data) canvas-y) - offset (* (+ (* inverted-y (.-width image-data)) canvas-x) 4) - rgba (.-data image-data) + img-width (.-width image-data) + img-height (.-height image-data) - r (obj/get rgba (+ 0 offset)) - g (obj/get rgba (+ 1 offset)) - b (obj/get rgba (+ 2 offset)) - a (obj/get rgba (+ 3 offset)) + zoom-context (mf/ref-val zoom-view-context) sx (- canvas-x 32) sy (if (cfg/check-browser? :safari) canvas-y (- canvas-y 17)) sw 65 sh 35] + (when (obj/get zoom-context "imageSmoothingEnabled") (obj/set! zoom-context "imageSmoothingEnabled" false)) (.clearRect zoom-context 0 0 zoom-view-width zoom-view-height) (.drawImage zoom-context canvas sx sy sw sh 0 0 zoom-view-width zoom-view-height) - ;; FIXME: this is throttled to avoid getting stuck in an inifinite react - ;; update loop. We should fix the global state instead. - (js/requestAnimationFrame - (fn [] - (st/emit! (dwc/pick-color [r g b a])))))))) + + ;; Only pick color when cursor is within canvas bounds to avoid garbage pixels + (when (and (>= canvas-x 0) (< canvas-x img-width) (>= canvas-y 0) (< canvas-y img-height)) + (let [;; image-data pixels start from the bottom-left corner; invert y accordingly + inverted-y (- img-height canvas-y) + offset (* (+ (* inverted-y img-width) canvas-x) 4) + rgba (.-data image-data) + r (obj/get rgba (+ 0 offset)) + g (obj/get rgba (+ 1 offset)) + b (obj/get rgba (+ 2 offset)) + a (obj/get rgba (+ 3 offset)) + color [r g b a]] + ;; Store latest color synchronously so the click handler always reads + ;; the correct pixel even before the rAF fires (fixes race condition) + (mf/set-ref-val! last-picked-color color) + ;; rAF throttles state updates to avoid an infinite React re-render loop + (js/requestAnimationFrame + (fn [] + (st/emit! (dwc/pick-color color)))))))))) (mf/defc pixel-overlay-wasm* {::mf/wrap-props false} @@ -266,7 +316,14 @@ canvas-context (mf/use-ref nil) canvas-image-data (mf/use-ref nil) zoom-view-context (mf/use-ref nil) - initial-mouse-pos (mf/use-state {:x 0 :y 0}) + ;; Holds the last successfully picked [r g b a] synchronously so that + ;; the pointer-down handler always has the current pixel, regardless of + ;; whether the rAF-deferred store update has fired yet. + last-picked-color (mf/use-ref nil) + ;; Use a ref (not state) so tracking the cursor doesn't cause re-renders. + ;; Updated by both on-mouse-enter and a document-level pointermove listener + ;; so that the position is always current when the canvas first becomes ready. + initial-mouse-pos (mf/use-ref {:x 0 :y 0}) update-str (rx/subject) handle-keydown @@ -283,8 +340,15 @@ (fn [event] (dom/prevent-default event) (dom/stop-propagation event) - (st/emit! (dwu/start-undo-transaction :mouse-down-picker) - (dwc/pick-color-select true (kbd/shift? event))))) + ;; Emit pick-color synchronously with the latest pixel colour before + ;; pick-color-select, so the colorpicker effect never sees a stale value. + (let [color (mf/ref-val last-picked-color)] + (if (some? color) + (st/emit! (dwu/start-undo-transaction :mouse-down-picker) + (dwc/pick-color color) + (dwc/pick-color-select true (kbd/shift? event))) + (st/emit! (dwu/start-undo-transaction :mouse-down-picker) + (dwc/pick-color-select true (kbd/shift? event))))))) handle-pointer-up-picker (mf/use-callback @@ -300,12 +364,18 @@ (mf/deps canvas-context) (fn [] (when-let [canvas-context (mf/ref-val canvas-context)] - (let [width (.-width canvas) + (let [width (.-width canvas) height (.-height canvas) - buffer (js/Uint8ClampedArray. (* width height 4)) - _ (.readPixels canvas-context 0 0 width height (.-RGBA canvas-context) (.-UNSIGNED_BYTE canvas-context) buffer) - image-data (js/ImageData. buffer width height)] - (mf/set-ref-val! canvas-image-data image-data))))) + buffer (js/Uint8ClampedArray. (* width height 4)) + _ (.readPixels canvas-context 0 0 width height (.-RGBA canvas-context) (.-UNSIGNED_BYTE canvas-context) buffer) + image-data (js/ImageData. buffer width height) + ;; Read current mouse position from ref so the zoom + ;; is populated immediately even without a mouse-move. + {mx :x my :y} (mf/ref-val initial-mouse-pos)] + (mf/set-ref-val! canvas-image-data image-data) + (process-pointer-move-wasm viewport-node canvas canvas-image-data + zoom-view-context last-picked-color + mx my))))) handle-canvas-changed (mf/use-callback @@ -316,25 +386,33 @@ (mf/use-callback (mf/deps viewport-node) (fn [event] - (let [x (.-clientX event) - y (.-clientY event)] - (reset! initial-mouse-pos {:x x - :y y})))) + (mf/set-ref-val! initial-mouse-pos + {:x (.-clientX event) + :y (.-clientY event)}))) + handle-pointer-move-picker (mf/use-callback (mf/deps viewport-node) (fn [event] - (process-pointer-move-wasm viewport-node canvas canvas-image-data zoom-view-context (.-clientX event) (.-clientY event))))] + (process-pointer-move-wasm viewport-node canvas canvas-image-data zoom-view-context last-picked-color (.-clientX event) (.-clientY event))))] (mf/use-effect (mf/deps canvas) (fn [] - (let [context (.getContext canvas "webgl2" #js {:willReadFrequently true, :preserveDrawingBuffer true})] + (let [context (.getContext canvas "webgl2" #js {:willReadFrequently true :preserveDrawingBuffer true})] (mf/set-ref-val! canvas-context context)))) + ;; Move focus to the overlay div on mount so the eyedropper button loses + ;; :focus styling immediately. Without this, prevent-default on pointer-down + ;; keeps focus on the button and it looks "selected" even after picking. + (mf/use-effect + (fn [] + (when-let [node (dom/get-element "pixel-overlay")] + (.focus node)))) + (mf/use-effect (fn [] - (let [listener (events/listen ug/document "keydown" handle-keydown)] + (let [listener (events/listen ug/document "keydown" handle-keydown)] #(events/unlistenByKey listener)))) (mf/use-effect @@ -350,12 +428,17 @@ (fn [] (.removeEventListener ug/document "penpot:wasm:render" handle-canvas-changed))) + ;; Track the cursor position at document level so initial-mouse-pos is always + ;; current when the canvas first becomes ready — even when the picker is opened + ;; via the "i" shortcut and the cursor hasn't entered/moved over the overlay yet. (mf/use-effect - (mf/deps viewport-node canvas canvas-image-data zoom-view-context) (fn [] - (when (some? canvas) - (let [{:keys [x y]} @initial-mouse-pos] - (process-pointer-move-wasm viewport-node canvas canvas-image-data zoom-view-context x y))))) + (let [listener (events/listen ug/document "pointermove" + (fn [e] + (mf/set-ref-val! initial-mouse-pos + {:x (.-clientX e) + :y (.-clientY e)})))] + #(events/unlistenByKey listener)))) [:div {:id "pixel-overlay" :tab-index 0