-
Notifications
You must be signed in to change notification settings - Fork 283
fix(hitgrid): stale hit grid after scroll/translate #480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Nice that you upgraded to 15, guess I have to refactor for ci to pass :) |
|
Oh yeah, that just happened earlier today. Finally. |
Add immediate hit grid sync for scroll and translate changes Before this change, when a scrollbox scrolled, the hit grid stayed stale until the next render completed. Hover states wouldn't update even though elements visually moved. Users would hover over items that had already scrolled away. The fix adds on-demand hit grid synchronization. When translateX or translateY changes, the renderer now: 1. Marks the hit grid dirty 2. Rechecks hover state using the latest pointer position 3. Rebuilds the hit grid immediately (before the next render) if a hit test occurs while dirty The hit grid rebuild mirrors the render traversal but uses screen coordinates for scissor rects. Buffered renderables need this because they render at (0,0) in their local buffer, but hit testing happens in screen space. Captured renderable (during drag) is excluded from the hit grid so drop targets receive events.
0fdc3fe to
44f0af2
Compare
|
Cool, this is very close to what I had in mind. The |
Good call. I removed the recursive layout dirty check + update from syncHitGridIfNeeded. Hitgrid rebuilds now just mirror the render traversal with screen‑space scissors and use the last computed layout + current translate. Layout sync stays in the normal render pass, so hit tests remain consistent with what's on screen while avoiding Yoga calls on hover/scroll. Left is branch, right is recording.mp4 |
|
@simonklee I simplified the implementation to eliminate the need for an additional tree walk to gather the hitgrid, dirty tracking and force recheck for hover state. As far as I can see the behaviour for the hover state is still correct this way. WDYT? |
Thanks for simplifying, but it also changes semantics. With the new code, hit‑grid updates only after a render and hover only updates on pointer move (translateX/Y no longer rechecks). So if the pointer stays still after a scroll, hover remains stale until the user moves the mouse. I'll record a quick video. |
semantics.mp4See how the hovered doesnt update in the first example while scrolling and not moving mouse. |
|
I see. Let me try reproducing that in a test. It should be possible to conditionally recheck after a frame was rendered to solve that. |
|
A fun test: if you try this in "Chrome" you'll notice that if you scroll the "hover" doesn't detect things under the cursor until after you stop scrolling and ~500ms have passed. I just randomly thought I'd try. Chrome scrolls on the compositor thread and throttles pointer/hover work, so :hover doesn't update until scrolling settles. I think:
If we do something similar to Chrome then probably best to implement that as an improvement in a new branch later. With your latest changes the |
|
Yeah, let me merge this and do the hover state update in another PR. It should be possible to reproduce this in a test. I would not want the renderer to know anything about scroll state, as that could be nested and whatnot, making it complex again. It should be enough to debounce the hover event when a change was detected after a frame. Do you want to take that on? EDIT: simple debounce for hover event would also work for anything that is animated and moving etc. so not scroll specific. |
Agree.
Yeh, that sounds fun. I'll have a look tonight (CET). |
Add immediate hit grid sync for scroll and translate changes
Before this change, when a scrollbox scrolled, the hit grid stayed stale until the next render completed. Hover states wouldn't update even though elements visually moved. Users would hover over items that had already scrolled away.
The fix adds on-demand hit grid sync. When translateX/Y changes, the renderer
marks the grid dirty and rechecks hover state. The renderer now:
The hit grid rebuild mirrors the render traversal but uses screen coordinates for scissor rects. Buffered renderables need this because they render at (0,0) in their local buffer, but hit testing happens in screen space.
Captured renderable (during drag) is excluded from the hit grid so drop targets receive events correctly.
I added the demo from #467 as an example as well as another debug example i used when implementing it. Both should probably be deleted before merging. You can copy these examples to 'main' to see the difference between how things react in main vs the branch.
The PR is a slightly different approach than what you discussed in #470 (comment) comment, and I don't mind if you prefer your approach, but maybe you'll find some ideas here.