Skip to content

fix(TrapFocus): trap focus when content is added to an empty container#638

Merged
blvdmitry merged 2 commits into
canaryfrom
claude/fix-trapfocus-observer-leak
Jun 22, 2026
Merged

fix(TrapFocus): trap focus when content is added to an empty container#638
blvdmitry merged 2 commits into
canaryfrom
claude/fix-trapfocus-observer-leak

Conversation

@blvdmitry

@blvdmitry blvdmitry commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Problem

TrapFocus.trap() created a MutationObserver, started observing, and then early-returned when the container had no focusable elements (and no initialFocusEl) — before setting this.trapped / adding itself to the chain:

this.#mutationObserver.observe(this.#root, { childList: true, subtree: true });

// Don't trap in case there is nothing to focus inside
if (!focusable.length && !initialFocusEl) return;

Two problems with this:

  1. Leaked observer. release() bails on !this.trapped, so it never disconnected the observer. Every trap() against an empty container leaked a MutationObserver.
  2. The observer was dead anyway. Its callback guards on this.#isLast(), which compares the chain tail's root to this.#root. Since the empty path returns before chain.add(this), the instance is never in the chain, so #isLast() can never be true and the callback always bails. So a container that was empty at trap time and later received focusable content (e.g. async-loaded dialog content) would never trap focus.

Fix

Turn the empty case into a deferred trap instead of a no-op:

  • When there's nothing focusable yet, set up an observer that waits for focusable content to appear and then performs the real trap — but only if no other trap() was triggered after this one (tracked via a monotonic #trapCounter, matching "no more trap focuses triggered after").
  • release() now disconnects the deferred observer even when the instance never trapped, so a container that's closed before content loads can't trap afterwards (and doesn't leak).
this.#trapId = ++TrapFocus.#trapCounter;

this.#removeListeners();
this.#mutationObserver?.disconnect();

// Nothing to focus yet — defer trapping. Watch for focusable content being
// added (e.g. async-loaded dialog content) and trap once it appears, as long
// as no other trap was triggered in the meantime.
if (!focusable.length && !initialFocusEl) {
    this.#mutationObserver = new MutationObserver(() => {
        if (!this.#root) return;
        // A newer trap was triggered after this one — abandon the deferred attempt.
        if (this.#trapId !== TrapFocus.#trapCounter) {
            this.#mutationObserver?.disconnect();
            return;
        }
        const focusableNow = getFocusableElements(this.#root, {
            additionalElement: includeTrigger ? trigger : undefined,
        });
        if (!focusableNow.length) return;
        // Focusable content appeared — perform the actual trap now.
        this.#mutationObserver?.disconnect();
        this.trap(root, options);
    });
    this.#mutationObserver.observe(this.#root, { childList: true, subtree: true });
    return;
}

Re-calling this.trap() reuses the normal trapping path. The trigger is preserved naturally because the empty path never moved focus, so getActiveElement() still returns the original opener.

Verification

  • tsc --noEmit → 0 errors. oxlint → clean.
  • TrapFocus.test.ts → 22/22 passing, including 3 new cases:
    • traps focus once focusable content is added to a previously empty container
    • does not trap added content if another trap was triggered after
    • releasing an empty container stops it from trapping later

How to test

  1. Trap an empty container, then add a focusable element to it. Before: focus was never trapped. After: focus moves into the container and the trap activates.
  2. Trap an empty container, then trigger another trap, then add content to the first. The first stays inactive.
  3. Trap an empty container, release() it, then add content. It stays inactive (and no observer leaks).

Found during a full package bug review.


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.59 KB (0%) 972 ms (0%) 1.4 s (+18.17% 🔺) 2.4 s
Library / CSS 22.18 KB (0%) 444 ms (0%) 0 ms (+100% 🔺) 444 ms
Theming / JS 7.81 KB (0%) 157 ms (0%) 176 ms (-27.53% 🔽) 333 ms
Theming with a default theme definition / JS 8.65 KB (0%) 174 ms (0%) 388 ms (+23.35% 🔺) 561 ms

@blvdmitry blvdmitry force-pushed the claude/fix-trapfocus-observer-leak branch 3 times, most recently from b6b3212 to 39bfc71 Compare June 20, 2026 12:57
@blvdmitry blvdmitry changed the title fix(TrapFocus): disconnect observer when there's nothing to trap fix(TrapFocus): don't create a leaked observer when there's nothing to trap Jun 20, 2026
@blvdmitry blvdmitry force-pushed the claude/fix-trapfocus-observer-leak branch from f016c99 to 56c93fd Compare June 20, 2026 13:13
@blvdmitry blvdmitry changed the title fix(TrapFocus): don't create a leaked observer when there's nothing to trap fix(TrapFocus): trap focus when content is added to an empty container Jun 20, 2026
Trapping a container with nothing focusable inside used to bail out early,
leaving a leaked MutationObserver that could never trap (it was never added
to the chain, so its callback's isLast() guard always failed).

Now an empty container defers instead: it observes for focusable content
being added (e.g. async-loaded dialog content) and performs the actual trap
once it appears, as long as no other trap was triggered in the meantime
(tracked via a monotonic counter). release() also disconnects the deferred
observer when the instance never trapped, so it can't trap after close.

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-trapfocus-observer-leak branch from 56c93fd to 2d602b4 Compare June 20, 2026 13:28
@blvdmitry blvdmitry marked this pull request as ready for review June 22, 2026 19:56
@blvdmitry blvdmitry merged commit 193948e into canary Jun 22, 2026
3 checks passed
@blvdmitry blvdmitry deleted the claude/fix-trapfocus-observer-leak branch June 22, 2026 20:20
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