Puck issue 1456 overlay position#1461
Puck issue 1456 overlay position#1461shannonhochkins wants to merge 10 commits intopuckeditor:mainfrom
Conversation
|
@shannonhochkins is attempting to deploy a commit to the Puck Team on Vercel. A member of the Team first needs to authorize it. |
chrisvxd
left a comment
There was a problem hiding this comment.
Sorry for slow review. Can confirm it works, but have performance concerns with the animation loop. See comment.
| ["style", "class"].includes(mutation.attributeName || "") | ||
| ) | ||
| ) { | ||
| sync(); |
There was a problem hiding this comment.
We should probably put this inside a requestAnimationFrame
| left: `${rect.left + scroll.x}px`, | ||
| top: `${rect.top + scroll.y}px`, |
There was a problem hiding this comment.
Hm bit confused why the transforms have been removed here, and how they're getting applied
There was a problem hiding this comment.
I did explain this in the main description of the PR a little more in detail, but a bounding box value is provided with transforms applied, including its parent(s), so theres no need to apply the transforms here
| const tick = () => { | ||
| const el = ref.current; | ||
|
|
||
| if (el) { | ||
| const rect = el.getBoundingClientRect(); | ||
| const prev = lastRectRef.current; | ||
|
|
||
| const changed = | ||
| !prev || | ||
| Math.abs(rect.x - prev.x) > 0.5 || | ||
| Math.abs(rect.y - prev.y) > 0.5 || | ||
| Math.abs(rect.width - prev.width) > 0.5 || | ||
| Math.abs(rect.height - prev.height) > 0.5; | ||
|
|
||
| if (changed) { | ||
| lastRectRef.current = rect; | ||
| sync(); | ||
| } | ||
| } | ||
|
|
||
| frame = requestAnimationFrame(tick); | ||
| }; |
There was a problem hiding this comment.
I'm not sure this animation loop is viable. The demo app now idles my MBP (M1 Max) CPU utilisation at 10%, up from under 2%. Can you find another solution?
There was a problem hiding this comment.
We can throttle it, the main benefit of the RAF is animations realistically, but theres something many other operations thst weren't not listening to that can affect position so i added the raf, would a throttled raf suffice?
|
@chrisvxd I've made a few changes, are you able to check them again for me? Perf change: removed the always-on overlay sync loop and switched to event-driven + throttled measurement. Previously, each DraggableComponent started a requestAnimationFrame loop that ran continuously, even when the app was otherwise idle. That meant we were calling getBoundingClientRect() every frame and occasionally updating overlay state, which kept the renderer active and raised baseline CPU usage. In this update:
Net effect: we keep overlay positioning correct during real movement (scroll/resize/animations), but avoid the previous “always hot” 60fps loop that increased idle CPU. The throttled measurement loop is not started on hover, only when selected/dragging, to avoid many components spinning up trackers as the mouse moves through the editor. |
|
I haven't yet reviewed the latest changes, but wanted to know if you'd considered using https://github.com/Shopify/position-observer for this? |
Haven't seen that before, happy to wire it in if you'd prefer |
Closes #1456
Description
Fix overlay positioning to fully cover components even when parents/targets apply CSS transforms or fixed positioning. We now rely on the browser’s transformed bounding boxes, add fixed-target handling, and resync overlays when style/class or geometry changes. Removed the legacy inverse-scale correction because getBoundingClientRect already reports transformed sizes/positions, so dividing by accumulated transforms caused misalignment under parent transforms.
Changes made
How to test
scale3d(0.79, 0.79, 1) translate3d(20%, 0, 0);and you should see the overlay move to match the new position when hovering, even other sub components if added to a parent with transforms like scale3d(0.79, 0.79, 1) translate3d(20%, 0, 0); overlay should match the rendered element bounds.I have done EXTENSIVE testing on this and it's pretty solid! Can't find any more issues
output.mp4
^ Fixes a bug in production in the puck demo that's present
And here's the difference in my software:
output_small.mp4