Skip to content

fix(scroll): scope StyleCache.reset to the locked element#641

Merged
blvdmitry merged 2 commits into
canaryfrom
claude/fix-stylecache-scoped-reset
Jun 20, 2026
Merged

fix(scroll): scope StyleCache.reset to the locked element#641
blvdmitry merged 2 commits into
canaryfrom
claude/fix-stylecache-scoped-reset

Conversation

@blvdmitry

@blvdmitry blvdmitry commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Problem

StyleCache.reset() restored and cleared the entire cache, regardless of which element a given lock owned:

reset = () => {
  for (const [el, styles] of this.cache.entries()) Object.assign(el.style, styles);
  this.cache.clear();
};

lockStandardScroll and lockSafariScroll each use a module-level StyleCache shared across all their calls. So when two different elements are locked at once — e.g. an Overlay locking one container (useScrollLock({ containerRef })) while a ContextMenu locks another (useScrollLock({ originRef })) — restoring one lock wipes the saved originals of the other still-active lock. The survivor can no longer restore its styles (scroll lock leaks or releases early).

Note this is the cross-container case. The per-container guard in lockScroll (if (container.style.overflow === "hidden") return) only blocks re-locking the same element, so two different elements both land in the shared cache.

Fix

Make StyleCache a per-element lock registry:

  • reset(el) is now required and scoped — it restores and removes only that element's entry, never touching other active locks.
  • set(el, styles) is a no-op when the element is already cached — locking an already-locked element keeps the originals captured the first time, instead of merging.
  • lockStandardScroll builds its styles into one object and calls set() once (overflow + optional scrollbar-gutter/paddingRight together), so all keys are captured atomically — required now that set() is idempotent per element.
  • lockSafariScroll already does a single set(); its unlock now calls reset(document.body).
reset = (el: HTMLElement) => {
  const styles = this.cache.get(el);
  if (!styles) return;
  Object.assign(el.style, styles);
  this.cache.delete(el);
};

API note

reset() now requires an element (the global no-arg form is removed). The only callers are the two lock modules; both pass the element they locked.

Verification

  • tsc --noEmit → 0 errors. oxlint → clean.
  • StyleCache.test.ts updated for the new semantics (scoped reset, idempotent set, no-op reset for uncached element).
  • Added an integration test in scroll/tests/lock.test.ts: unlocking one container does not affect another locked container — confirmed it fails against the old global-wipe reset() and passes with the scoped fix.
  • Full css + scroll suites green (13 tests).

How to test

  1. Lock scroll on two different containers (e.g. an Overlay container and a ContextMenu's scrollable region), then unlock one.
  2. Before: the still-locked container loses its saved styles and can't restore on its own unlock. After: each unlock restores only its own element.

Found during a full package bug review. Part of the scroll-lock cluster.


Generated by Claude Code

@github-actions

github-actions Bot commented Jun 20, 2026

Copy link
Copy Markdown

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Library / JS 48.48 KB (0%) 970 ms (0%) 286 ms (-6.52% 🔽) 1.3 s
Library / CSS 22.18 KB (0%) 444 ms (0%) 0 ms (+100% 🔺) 444 ms
Theming / JS 7.81 KB (0%) 157 ms (0%) 121 ms (+348.38% 🔺) 277 ms
Theming with a default theme definition / JS 8.65 KB (0%) 174 ms (0%) 210 ms (+304.76% 🔺) 383 ms

@blvdmitry blvdmitry force-pushed the claude/fix-stylecache-scoped-reset branch from deb3e42 to f0b1035 Compare June 20, 2026 09:23
StyleCache.reset() restored and cleared the entire cache, so unlocking one
scroll lock wiped the saved styles of another still-active lock sharing the
module-level cache (e.g. an Overlay locking one container while a ContextMenu
locks another). Make it a per-element lock registry: reset(el) is required and
restores only that element, and set() is a no-op when the element is already
cached. Collapse lockStandardScroll to a single set() call so its style keys
are captured together.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01We9NqptZjfbvXRL4hE1xri
@blvdmitry blvdmitry force-pushed the claude/fix-stylecache-scoped-reset branch from f0b1035 to b74bcef Compare June 20, 2026 16:29
@blvdmitry blvdmitry marked this pull request as ready for review June 20, 2026 16:38
@blvdmitry blvdmitry merged commit 64b1574 into canary Jun 20, 2026
3 checks passed
@blvdmitry blvdmitry deleted the claude/fix-stylecache-scoped-reset branch June 20, 2026 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants