Fix canvas click hitch: gate SelectionChanged on actual selection change

Clicking an already-selected element to start a drag fired
SelectionChanged unconditionally, triggering TemplateDesignerForm's
un-gated full RefreshPropertiesPanel() every click (~40-60ms, measured).
Now only raises the event when the selection identity actually changes.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

backlog/epics/02_template_designer_gui_foundation.md

@@ -53,6 +53,8 @@ As a **print operator**, I want to place static and dynamic text placeholders on
 
 Tests: removed 8 obsolete font-scale-specific tests, added 15 new ones (3 `RemoveSelected*` tests; the rest updated/added resize-handle and `HasBox`/`HasHeightClip` semantic tests across `CanvasElementEditorTests`, `TextElementPropertiesEditorTests`, `TemplateLayoutXmlSerializerTests`, and the CLI's `TemplateXmlParserTests`/`DebenuPdfRendererWrapTests`). Full suite 476/476 -> 488/488. Live-verified via a reflection-driven built-form harness: a real mouse-driven drag on the real resize handle grew `Width` to 578.9pt while `Height` stayed null and `FontSize` stayed exactly 12; the real Delete key removed the selected element and cleared selection; a fresh narrow-boxed (120pt) element's canvas screenshot shows genuine multi-line wrap plus the dashed-orange wrap indicator (see the epic 4 indicator story) correctly appearing. One disclosed evidence-depth caveat: the toolbar "Delete Selected" button's own `PerformClick()` did not reliably fire in this synthetic reflection harness (a known WinForms quirk for a button never shown in a real top-level window with a running message loop) — the underlying `RemoveSelectedElement()` method was proven correct via a direct call returning `true` and removing the control, and the button's `Click` wiring is structurally identical to every other already-shipped, real-world-proven toolbar button in this form, but the button's own click-to-delete path specifically was not re-observed through a real mouse click.
 
+**Post-Sprint-10-Batch-1 user-reported regression fix (2026-10-27, handled outside formal ceremony, mid-Sprint-10):** User reported that clicking on and moving a dynamic placeholder was "not smooth any more" — a new regression, distinct from the Sprint 8 drag/rotate-tick choppiness fixed above (that fix's `IsInteracting` gating was confirmed still fully intact by inspection before looking anywhere else). Root cause found by instrumenting `TemplateCanvasControl.OnMouseDown` directly (per-line `Stopwatch` timing) rather than guessing: `SelectionChanged` was raised **unconditionally** at the end of every left mouse-down — including a click on an element that was *already* selected, the ordinary way to begin dragging it — and `TemplateDesignerForm`'s `SelectionChanged` handler runs the full `RefreshPropertiesPanel()` (combo-box rebind repopulation, etc.) with no `IsInteracting`-style gating at all, unlike the `ElementsChanged` path. This cost was always there, but only became perceptible (~40-60ms measured per click, confirmed via a reflection-driven harness against the real built assembly) once this session's added property-panel rows (box width/height inputs, the delete button) made each refresh heavier — explaining why the user noticed it now and not before. Fixed by capturing the pre-click selection identity (selected element reference, selected Address Control reference, selected address line index) at the top of `OnMouseDown` and only raising `SelectionChanged` when that identity actually differs afterward, via a new `RaiseSelectionChangedIfDifferent` helper called at both of `OnMouseDown`'s `SelectionChanged` sites. A genuine selection change (nothing selected -> element, or switching elements) still pays the refresh cost exactly as before; only the redundant "click an already-selected item to move it" case is now free. Live-verified with the same instrumented reflection harness: before the fix, a second click on an already-selected dynamic placeholder cost ~50-56ms end-to-end in `OnMouseDown`; after the fix, the identical click cost 0.295ms, with the genuinely-selection-changing first click unaffected (still pays the warranted ~50ms once). `OnMouseMove` itself was confirmed cheap throughout (never the bottleneck) both before and after. Full test suite re-run: Desktop.Core 364/364, CLI 124/124, both unaffected (the fix is isolated to the WinForms-only `TemplateCanvasControl`, which has no automated test project — verified live per this project's established pattern for that layer). Not logged as technical debt (resolved same-day); documented here per this team's established pattern for same-day ad-hoc user feedback.
+
 **Estimate:** 5 points
 **Dependencies:** None
 

backlog/sprints/sprint-10.md

@@ -34,3 +34,4 @@ Sequenced by dependency and risk: the small, independent indicator story first (
 | Day | Date | Completed | Planned | Blocked/At risk |
 |---|---|---|---|---|
 | 1 | 2026-10-27 | **Batch 1** ("Add a live wrap/clip indicator for text elements", 3 points) done, all ACs met. New framework-free `WrapClipDetector` (Desktop.Core) compares an element's natural (unwrapped) GDI+-measured width against its box width, and its GDI+-wrapped height against an explicit clip ceiling when one is set (never flags clipping otherwise, since there is no ceiling to exceed). Wired into `TemplateCanvasControl.DrawElement` and `TemplatePreviewControl.DrawItem`'s existing box-mode branches, drawing a dashed-orange outline (the unmapped-column warning's visual language) inside the same rotation transform every other per-element visual already uses, so it orbits for free when rotated. **Mid-batch, the human product owner raised two live-build issues, addressed together before continuing to Batch 2** (full detail in `epics/02_template_designer_gui_foundation.md`'s and `epics/04_live_preview_and_record_navigation.md`'s post-Sprint-9 correction notes): (1) no way to delete a placed element or Address Control — added `CanvasElementEditor.RemoveSelected()`/`TemplateCanvasControl.RemoveSelectedElement()`, wired to the Delete/Backspace key and a new toolbar button; (2) the resize handle should control only box width, font size set elsewhere — removed the Sprint 8 uniform-font-scale handle behavior entirely, `ResizeDragTo` now only ever sets `Width`, and `TextElementLayout.HasBox`/`TemplateElement.HasBox` were decoupled from `Height` (a new independent `HasHeightClip` flag) as a direct consequence, with `DebenuPdfRenderer.AddPage` gaining a "Width alone" native-wrap-no-clip render path (`DrawWrappedText` unrotated, `GetWrappedText`+`DrawRotatedMultiLineText` rotated). Tests: net +12 (8 obsolete font-scale tests removed, 20 new/updated across `CanvasElementEditorTests`, `TextElementPropertiesEditorTests`, `TemplateLayoutXmlSerializerTests`, `TemplateXmlParserTests`, `DebenuPdfRendererWrapTests`, plus the new `WrapClipDetectorTests`). Full suite 476/476 -> 488/488. Live-verified: a real, licensed end-to-end CLI render of a width-only (no height) template showed correct unrotated and rotated auto-height wrap with no clipping; a reflection-driven built-form harness confirmed a real mouse-driven resize drag sets only `Width` (Height/FontSize unchanged), the real Delete key removes the selected element, and a real canvas screenshot shows both genuine wrap and the new indicator on a narrow boxed element. One disclosed evidence-depth caveat: the toolbar Delete button's own `PerformClick()` didn't reliably fire in the synthetic harness (a known WinForms quirk absent a real shown window) — the underlying method was proven correct via a direct call, and the button's wiring matches every other proven-working toolbar button. | Batch 2, "Select multiple elements at once on the canvas" (5 points, epic 6). | None. |
+| 2 | 2026-10-27 | **Ad-hoc regression fix, before starting Batch 2:** user reported clicking on and moving a dynamic placeholder had become "not smooth any more." Full detail in `epics/02_template_designer_gui_foundation.md`'s new post-Sprint-10-Batch-1 note. Root cause, found by direct `Stopwatch` instrumentation of `TemplateCanvasControl.OnMouseDown` rather than guessing: `SelectionChanged` fired unconditionally on every left-click, including a click on an already-selected element (the normal way to start a drag), and `TemplateDesignerForm`'s handler runs the full un-gated `RefreshPropertiesPanel()` every time that fires — a cost that only became perceptible (~40-60ms/click, measured) once this sprint's added property-panel rows made the refresh heavier. Fixed by only raising `SelectionChanged` when the pre-click and post-click selection identity actually differ. Live-verified with the same instrumented harness: an already-selected element's second click dropped from ~50ms to 0.295ms; a genuine selection change still pays the warranted refresh cost unchanged. Full suite re-run: Desktop.Core 364/364, CLI 124/124 (fix is isolated to the WinForms-only `TemplateCanvasControl`, which has no automated test project). | Batch 2, "Select multiple elements at once on the canvas" (5 points, epic 6). | None. |

code/src/EnvelopeRenderer.Desktop/Views/TemplateCanvasControl.cs

@@ -889,6 +889,20 @@ public sealed class TemplateCanvasControl : Control
             return;
         }
 
+        // Post-Sprint-10 user-reported regression fix: SelectionChanged used to fire unconditionally
+        // on every left-click, including a click on an item that was ALREADY selected (the normal
+        // way to begin dragging it). TemplateDesignerForm's SelectionChanged handler runs the full,
+        // non-trivial RefreshPropertiesPanel() (combo-box rebind repopulation etc.) with no
+        // IsInteracting-style gating the way the drag/rotate/resize ElementsChanged path has, so
+        // every single click paid that cost — a real, measured ~40-60ms hitch per click once this
+        // session's added property-panel rows (box width/height, delete button) made the refresh
+        // heavy enough to notice. Capturing the pre-click selection identity and only raising the
+        // event when it actually changed makes "click an already-selected element to move it" free
+        // again while still notifying on every real selection change.
+        var previousSelectedElement = _editor.Selected;
+        var previousSelectedAddressControl = _selectedAddressControl;
+        var previousSelectedAddressLineIndex = _selectedAddressLineIndex;
+
         var transform = CurrentTransform();
         var (x, y) = transform.ToPoints(e.X, e.Y);
 
@@ -919,7 +933,7 @@ public sealed class TemplateCanvasControl : Control
             _addressControlDragOffset = (x - addressHit.Control.X, y - addressHit.Control.Y);
             Capture = true;
             Invalidate();
-            SelectionChanged?.Invoke(this, EventArgs.Empty);
+            RaiseSelectionChangedIfDifferent(previousSelectedElement, previousSelectedAddressControl, previousSelectedAddressLineIndex);
             return;
         }
 
@@ -956,7 +970,21 @@ public sealed class TemplateCanvasControl : Control
         }
 
         Invalidate();
-        SelectionChanged?.Invoke(this, EventArgs.Empty);
+        RaiseSelectionChangedIfDifferent(previousSelectedElement, previousSelectedAddressControl, previousSelectedAddressLineIndex);
+    }
+
+    /// <summary>Raises <see cref="SelectionChanged"/> only when the selection identity captured
+    /// before a click actually differs from the current one — see <see cref="OnMouseDown"/>'s
+    /// remarks for why an unconditional raise on every click was a real, measured hitch.</summary>
+    private void RaiseSelectionChangedIfDifferent(
+        TextElementLayout? previousElement, AddressControlLayout? previousAddressControl, int previousAddressLineIndex)
+    {
+        if (!ReferenceEquals(previousElement, _editor.Selected)
+            || !ReferenceEquals(previousAddressControl, _selectedAddressControl)
+            || previousAddressLineIndex != _selectedAddressLineIndex)
+        {
+            SelectionChanged?.Invoke(this, EventArgs.Empty);
+        }
     }
 
     /// <summary>Sprint 8: a rotated control must remain correctly click-selectable at its actual

state.md

@@ -18,7 +18,9 @@
 
 **Sprint 10 Batch 1 complete (2026-10-27), plus two ad-hoc corrections mid-batch:** "Add a live wrap/clip indicator for text elements" (3/13 points) shipped — a new framework-free `WrapClipDetector` (Desktop.Core) drives a dashed-orange indicator on the canvas and preview panel, drawn inside the existing per-element rotation transform. Mid-batch, the human product owner raised two live-build issues, addressed together before continuing: (1) **no way to delete a placed element or Address Control** — added `CanvasElementEditor.RemoveSelected()`/`TemplateCanvasControl.RemoveSelectedElement()`, wired to Delete/Backspace and a new toolbar button; (2) **the resize handle should control only box width, font size set elsewhere** — removed the Sprint 8 uniform-font-scale handle behavior entirely; `TextElementLayout.HasBox`/`TemplateElement.HasBox` now mean "Width is set" alone, with `Height` independent and optional (new `HasHeightClip` flag), and `DebenuPdfRenderer.AddPage` gained a "Width alone, auto-height, no clip" render path (`DrawWrappedText`/`GetWrappedText`+`DrawRotatedMultiLineText`). Full suite 476/476 -> 488/488. Live-verified: a real licensed end-to-end CLI render (width-only, unrotated and rotated) showed correct auto-height wrap with no clipping; a reflection-driven built-form harness confirmed the real resize handle sets only Width, the real Delete key removes the selection, and a real canvas screenshot shows the wrap indicator correctly appearing. One disclosed caveat: the toolbar Delete button's `PerformClick()` didn't reliably fire in the synthetic harness (a WinForms quirk absent a real shown window) — proven correct via a direct method call instead. Full detail: `backlog/sprints/sprint-10.md`, `backlog/epics/02_template_designer_gui_foundation.md`, `backlog/epics/04_live_preview_and_record_navigation.md`.
 
-**Next action:** Continue Sprint 10 as `dev-team`: Batch 2 "Select multiple elements at once on the canvas" (5 pts, epic 6, no dependency but sequenced next as the epic-6 pair's foundation), then Batch 3 "Align and distribute multiple elements" (5 pts, depends on Batch 2), per `backlog/sprints/sprint-10.md`. Key design note for Batch 2, not to be rediscovered mid-sprint: add multi-select as an *additive* layer alongside the existing single-selection state (`CanvasElementEditor.Selected` for standalone elements, `TemplateCanvasControl._selectedAddressControl` for Address Controls) rather than replacing it, so every existing single-select code path (properties panel, rotate/resize handles, line drill-in) keeps working unchanged when only one item is selected. This is a natural mid-sprint checkpoint (a large, first-of-its-kind feature remains, plus the human product owner just gave live-build feedback twice this session) — worth confirming with the user before continuing into the multi-select implementation. Still open, non-blocking: "Add an adjustable width and height with text wrapping to Address Control lines" (epic 4, 8 pts, deferred to Sprint 11); "Undo and redo layout changes" (epic 6, 13 pts); "Warn on text overflow before render" (epic 4, provisional 5 pts, still blocked); both epic 7 impediments (asset path strategy, UNC timeout/retry, open since 2026-09-04); "Complete the first text-only operator workflow" (epic 1, 5 pts) remains flagged as stale; the Proposed process-improvement log entry above awaiting a human decision.
+**Post-Sprint-10-Batch-1 user-reported regression fix (2026-10-27, handled outside formal ceremony, mid-Sprint-10):** User reported that clicking on and moving a dynamic placeholder had become "not smooth any more." Root cause found by direct `Stopwatch` instrumentation of `TemplateCanvasControl.OnMouseDown` (not guessed): `SelectionChanged` fired unconditionally on every left-click, including a click on an already-selected element (the normal way to start a drag), and `TemplateDesignerForm`'s handler runs the full un-gated `RefreshPropertiesPanel()` every time — a cost (~40-60ms/click, measured) that only became perceptible once this sprint's added property-panel rows made the refresh heavier. Fixed by only raising `SelectionChanged` when the pre-click and post-click selection identity actually differ (`RaiseSelectionChangedIfDifferent`). Live-verified with the same instrumented harness: an already-selected element's second click dropped from ~50ms to 0.295ms; a genuine selection change still pays the warranted refresh unchanged. Full suite re-run: Desktop.Core 364/364, CLI 124/124 (fix isolated to the WinForms-only `TemplateCanvasControl`). Full detail: `backlog/sprints/sprint-10.md`, `backlog/epics/02_template_designer_gui_foundation.md`.
+
+**Next action:** Continue Sprint 10 as `dev-team`: Batch 2 "Select multiple elements at once on the canvas" (5 pts, epic 6, no dependency but sequenced next as the epic-6 pair's foundation), then Batch 3 "Align and distribute multiple elements" (5 pts, depends on Batch 2), per `backlog/sprints/sprint-10.md`. Key design note for Batch 2, not to be rediscovered mid-sprint: add multi-select as an *additive* layer alongside the existing single-selection state (`CanvasElementEditor.Selected` for standalone elements, `TemplateCanvasControl._selectedAddressControl` for Address Controls) rather than replacing it, so every existing single-select code path (properties panel, rotate/resize handles, line drill-in) keeps working unchanged when only one item is selected. This is a natural mid-sprint checkpoint (a large, first-of-its-kind feature remains, plus the human product owner just gave live-build feedback three times this session) — worth confirming with the user before continuing into the multi-select implementation. Still open, non-blocking: "Add an adjustable width and height with text wrapping to Address Control lines" (epic 4, 8 pts, deferred to Sprint 11); "Undo and redo layout changes" (epic 6, 13 pts); "Warn on text overflow before render" (epic 4, provisional 5 pts, still blocked); both epic 7 impediments (asset path strategy, UNC timeout/retry, open since 2026-09-04); "Complete the first text-only operator workflow" (epic 1, 5 pts) remains flagged as stale; the Proposed process-improvement log entry above awaiting a human decision.
 
 ## Phase reference
 
@@ -92,3 +94,4 @@ After phase 5, loop back to phase 1 for the next sprint.
 | 2026-10-27 | 1 - Backlog refinement | `product-owner` ran Sprint 10 backlog refinement after the user confirmed the post-Sprint-9-retrospective pause point with "continue." Per the Sprint 9 retrospective's own action item, re-confirmed "Support rotated wrapped text and a live wrap/clip indicator for text elements" (previously 8 pts) via real code inspection and a new licensed end-to-end test — found rotation composition was already delivered as a byproduct of Sprint 9's implementation (`DebenuPdfRenderer.AddPage`'s box branch already calls `DrawRotatedTextBox`; canvas/preview box-mode draws already run inside the existing rotation transform), proven with a real rendered screenshot of a 20°-rotated, wrapped, boxed element. Renamed to "Add a live wrap/clip indicator for text elements," re-sized down to 3 points. Also found and documented that automated pixel-row-count clip verification is unreliable under rotation, simplifying a new regression test accordingly. Full detail: `backlog/backlog.md`'s Sprint 10 backlog refinement outcome, `backlog/epics/04_live_preview_and_record_navigation.md`. |
 | 2026-10-27 | 2 - Sprint planning | `scrum-master` facilitated with `product-owner`/`dev-team` input. Capacity signal now has nine data points (20, 19, 18, 18, 15, 18, 18, 13, 13). Committed 13 points: the 3-point indicator story plus epic 6's multi-select/align pair (10 pts), finally pulled after being deferred twice (Sprint 8, Sprint 9) for fresher requests that no longer apply. Reasoned explicitly that pairing a small, additive story with one large/novel story (multi-select's new selection model) does not trip the established "two large/novel stories" caution, since only one side carries that risk profile. Recorded in `backlog/sprints/sprint-10.md`; full reasoning in `backlog/backlog.md`'s Sprint 10 planning outcome note. |
 | 2026-10-27 | 3 - Sprint execution | `dev-team` completed Sprint 10 Batch 1, "Add a live wrap/clip indicator for text elements" (3/13 points). New framework-free `WrapClipDetector` (Desktop.Core) drives a dashed-orange indicator on the canvas and preview panel. Mid-batch, the human product owner raised two live-build issues, addressed together: (1) no way to delete a placed element or Address Control — added `CanvasElementEditor.RemoveSelected()`/`TemplateCanvasControl.RemoveSelectedElement()`, wired to Delete/Backspace and a new toolbar button; (2) the resize handle should control only box width, font size set elsewhere — removed the Sprint 8 uniform-font-scale handle behavior entirely; `HasBox` now means "Width is set" alone, `Height` independent and optional (new `HasHeightClip`), and `DebenuPdfRenderer.AddPage` gained a "Width alone, auto-height, no clip" render path. Full suite 476/476 -> 488/488. Live-verified: a real licensed end-to-end CLI render (width-only, unrotated and rotated) confirmed auto-height wrap with no clipping; a reflection-driven built-form harness confirmed the real resize handle sets only Width, the real Delete key removes the selection, and a real canvas screenshot shows the wrap indicator. One disclosed caveat: the toolbar Delete button's `PerformClick()` didn't reliably fire in the synthetic harness — proven correct via a direct method call instead. Full detail: `backlog/sprints/sprint-10.md`, `backlog/epics/02_template_designer_gui_foundation.md`, `backlog/epics/04_live_preview_and_record_navigation.md`. |
+| 2026-10-27 | 3 - Sprint execution (ad hoc, outside formal ceremony) | User reported that clicking on and moving a dynamic placeholder had become "not smooth any more." Root cause found by directly instrumenting `TemplateCanvasControl.OnMouseDown` with `Stopwatch` timers rather than guessing: `SelectionChanged` fired unconditionally on every left-click — including a click on an already-selected element, the normal way to begin dragging it — and `TemplateDesignerForm`'s handler runs the full un-gated `RefreshPropertiesPanel()` every time, a cost (~40-60ms/click, measured) that only became perceptible once this sprint's added property-panel rows made the refresh heavier. Fixed by capturing the pre-click selection identity and only raising `SelectionChanged` when it actually changed (`RaiseSelectionChangedIfDifferent`). Live-verified with the same instrumented reflection harness: an already-selected element's second click dropped from ~50ms to 0.295ms end-to-end in `OnMouseDown`, while a genuine selection change still correctly pays the warranted refresh cost unchanged. Full suite re-run: Desktop.Core 364/364, CLI 124/124 (fix isolated to the WinForms-only `TemplateCanvasControl`, which has no automated test project). Documented in `backlog/epics/02_template_designer_gui_foundation.md` and `backlog/sprints/sprint-10.md`. |