-
Notifications
You must be signed in to change notification settings - Fork 76
Fix for #296: Insert the virtual component bound back to DOM also for the case shouldRecycle=false
#479
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
base: main
Are you sure you want to change the base?
Conversation
bcd90d0 to
0f591a2
Compare
Consolidates component removal, prepend, and append operations into a single layout callback to prevent concurrent DOM manipulation that caused "removeChild" and "Cannot read properties of null" errors. Fixes html-next#296
0f591a2 to
8ca0edc
Compare
shouldRecycle=true
…ent runloop. If not, creates one.
…the current runloop. If not, creates one." This reverts commit ce6f221.
|
I think this should be quite uncontroversial. Pinging the ones I know have touched upon this issue before: @ahamedalthaf @Atrue @runspired @mixonic, as this seems to have had some history😅 please chip in if you see something concerning, or if you think it is ready for merge, thanks! |
shouldRecycle=trueshouldRecycle=false
| for (let i = _componentPool.length - 1; i >= 0; i--) { | ||
| const component = _componentPool[i]; | ||
| const item = objectAt(items, component.index); | ||
| if (shouldRecycle === true && item) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the last PR that worked on fixing this I'd left this comment for future follow up:
I think we should follow up with a check for whether we are in the document-fragment or still in the DOM, if we are not in the fragment there's no reason to re-insert in this way.
I suspect that might still be true but I'd need to sit down and reason through the code. The worst that could happen merging this as is I believe is that we just move some DOM "in-place" which isn't too big a deal.
Fix for #296 race condition with
shouldRecycle=falseduring fast scroll.The issue was that we had 'forgotten' to apply the fix 'Insert the virtual component bound back before removal to make sure Glimmer not confused about the state of the DOM.' also to the case
shouldRecycle=true.I was able to reproduce with scrolling fast up and down with the following component:
Here the scroll window was 350 px height, it is more reproducible when it is small, and scrolling up and down very quickly:
Screen.Recording.2025-10-03.at.22.55.31.mov