fix(makeDraggable): floating windows snap right on release#1087
Conversation
…ping clampToViewport was unconditionally writing rect.left/top back to style.left/top, which converted the browser's integer-rounded getBoundingClientRect value back into style, introducing a subpixel rightward snap on every mouse release. Now only writes when the position was actually adjusted by a clamp boundary. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
accius
left a comment
There was a problem hiding this comment.
Right symptom, and the guard itself is good hygiene (no reason to rewrite an unchanged position), but I don't think the stated root cause holds: getBoundingClientRect returns subpixel floats, not integer-rounded values, so rounding error alone can't produce a visible snap. It would also mean the snap is at most a pixel, and your video shows ~10px.
Here's the mechanism I believe is actually at work. These panels are Leaflet controls living inside the corner containers, and stock leaflet.css applies margins there: .leaflet-left .leaflet-control { margin-left: 10px } (same for top/right/bottom). For a position:fixed element, style.left positions the margin box but rect.left reads the border box, so rect.left = style.left + margin-left. Every unconditional write of rect.left back into style.left therefore shifts the panel right by exactly the margin, once per release. That's your snap, and it predicts the magnitude (10px) and the direction (right/down for left/top corner controls).
If that's confirmed (check computed margin-left on a snapping panel in devtools), the cleaner fix is to zero the margin at the places makeDraggable re-fixes the element (both restore paths and the dblclick reset): el.style.margin = '0'. Then style.left === rect.left identically and three other latent offsets disappear at the same time: the jump at first drag movement (startLeft includes the margin), the clamped position landing margin-px inward, and the percent-restore rendering margin-px off after reload. Your conditional write is worth keeping on top of that regardless.
Happy to re-review when it's out of draft.
K0CJH
…rift Leaflet's CSS sets margins on corner controls (e.g. margin-left: 10px). For position:fixed, style.left positions the margin box but getBoundingClientRect().left reads the border box, so rect.left = style.left + marginLeft. Every write of rect.left back into style.left shifted the panel right by the margin width on each release. Zeroing el.style.margin at all three places where makeDraggable converts the element to position:fixed (restore-from-storage, initial fix, and dblclick reset) makes rect.left === style.left unconditionally, fixing the snap on drop, the first-drag jump, and the percent-restore offset. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
You're right on both counts — the subpixel explanation doesn't hold up and the margin mechanism fits the 10px magnitude perfectly. Added el.style.margin = '0' at all three position:fixed conversion points (restore-from-storage, initial fix, dblclick reset) in d48098a, so rect.left === style.left unconditionally from that point on. The conditional write in clampToViewport is kept on top of that as you suggested. 73! |
|
It appears to be fixed now, thanks! 🥳 |
accius
left a comment
There was a problem hiding this comment.
This is it. The margin-box theory held up and the fix addresses it at the source: zeroing the margin at all three places makeDraggable re-fixes the element (both restore paths and the dblclick reset) means style.left and rect.left now agree to the pixel, so the unconditional readback that caused the snap is harmless. Keeping the conditional write guard on top of that is good hygiene regardless.
Worth calling out for anyone reading later: zeroing the margin is safe specifically because these are position:fixed elements with explicit left/top after makeDraggable takes over. The leaflet corner-container margin was only ever cosmetic offset once we pin absolute coordinates, and it was being double-counted into the saved position. This also quietly fixes the first-drag jump and the percent-restore-after-reload offset that came from the same root cause.
CI green. This should finally close #1017 for real.
K0CJH
Summary
clampToViewportwas unconditionally writingrect.left/rect.topback tostyle.left/style.topon every mouse release, even when no clamping was neededgetBoundingClientRect()returns integer-rounded pixel values, so this overwrote the element's exact subpixel position with a slightly different integer — causing a visible rightward snap on dropstyle.left/style.topwhen the position was actually adjusted by a clamp boundaryTest plan
🤖 Generated with Claude Code