[labs/virtualizer] ScrollerController is not shared between virtualizers on the same scrolling ancestor
- Dominant language
- TypeScript
- Stars
- 21.8k
- Forks
- 1.1k
- Avg merge
- 18h 25m
- Merged PRs (30d)
- 2
Description
## Summary
When multiple virtualizers share an ancestor scroller (commonly `window`), each one constructs its own `ScrollerController` for that node. The controllers end up in a tangled state that causes at least two bugs: scroll corrections from one virtualizer break pinning on the other, and detach ordering can wipe out another controller's patches.
This is a latent issue that exists independently of the zombie-virtualizer scroll-flake being fixed in a separate PR (see #TBD).
## Current behavior
`Virtualizer` constructs a `ScrollerController` per-instance: `new ScrollerController(this, scrollingElement)`.
`ScrollerController`'s constructor captures `node.scrollTo` as `_originalScrollTo` and then `_attach(client)` installs `node.scrollTo = this.scrollTo` on first-client.
So if VA is constructed first on `window`, then VB on `window`:
- VA: `_originalScrollTo = (real native)`; patches `window.scrollTo = VA.scrollTo`.
- VB: `_originalScrollTo = (VA.scrollTo)`; patches `window.scrollTo = VB.scrollTo`.
Calls route VB → VA → real native. `_clients` on each controller contains only itself.
## Concrete bugs
### 1. Scroll correction from one virtualizer looks like a user scroll to another
`correctingScrollError` is a per-controller flag. When VA fires `correctScrollError(...)`, only VA's flag goes true. The resulting scroll event is dispatched on `window` and heard by both controllers' listeners.
In `Virtualizer._handleScrollEvent`:
```ts
if (this._scrollerController!.correctingScrollError === false) {
// This is a user-initiated scroll, so we unpin the layout
this._layout?.unpin();
...
}
```
VB sees its own `correctingScrollError === false` and treats the correction as a user scroll — it unpins its layout and reflows. VA's internal correction silently breaks VB's pin.
### 2. Detach ordering wipes patches
In `ScrollerController.detach`:
```ts
if (this._clients.size === 0) {
this._node.scrollTo = this._originalScrollTo;
...
}
```
Each controller thinks it's the only patcher. When whichever one detaches first has `_clients.size === 0`, it overwrites `window.scrollTo` with its own captured `_originalScrollTo` — which may be the other virtualizer's patched `scrollTo` or may be the true native. Depending on construction/detach order, some patches survive and some don't. There is no scenario where this is correct.
## Root cause
The `_clients` ref-counting inside `ScrollerController` looks like it was designed to support a single shared controller per node, ref-counted across clients. But nothing shares the controller — `Virtualizer` constructs a fresh one per virtualizer.
## Proposed fix
Introduce a module-level registry keyed by scrolling node:
```ts
const controllers = new WeakMap();
export function getOrCreateScrollerController(client: unknown, node: Element | Window) {
let c = controllers.get(node);
if (!c) {
c = new ScrollerController(client, node);
controllers.set(node, c);
} else {
c.attach(client);
}
return c;
}
```
Then the existing `_clients` set does what it was designed for: first client creates/attaches, subsequent clients attach; last detach tears down the node patches.
With this:
- `_originalScrollTo` captures the true native, exactly once per node.
- `correctingScrollError` is naturally shared across all clients of the same node — a correction from any client is correctly ignored by all.
- Detach restores the true native, exactly once per node.
## Scope
- Change the construction path in `Virtualizer.ts` to go through the registry.
- `ScrollerController.detach` already correctly restores on last detach; the only change there is making `_attach` public (or adding `attach`) so subsequent clients can join.
- Regression tests:
- Two virtualizers on `window`, one pinned, other triggers a scroll correction — confirm the pinned one stays pinned.
- Two virtualizers on `window`, disconnect in various orders — confirm no scroll-method leaks (`window.scrollTo` should return to true native after all detach).
Contributor guide
Research direction
Start in Virtualizer.ts and ScrollerController, tracing controller construction, _attach, and detach for virtualizers sharing window. Add regression tests for two virtualizers where one scroll correction must not unpin the other, and for disconnecting them in different orders. Done means the pinned layout remains pinned and window.scrollTo returns to the true native method after all detach.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- frontend
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100