fix(components): keep overlay trigger buttons inside their trigger (part of #3014)

[mfal]

Part of #3014 — the A PressResponder was rendered without a pressable child half. The uncontrolled to controlled half is split out into #3048. Neither PR closes the issue on its own; close it once both are merged.

The warning was hiding a real accessibility defect rather than just noise — in both rendering paths, as the visual suite then proved.

A PressResponder was rendered without a pressable child (8×) — ModalTrigger, and it cost real a11y

Not ContextMenuTrigger. Confirmed with captureOwnerStack(): owner was FlowComponent(ModalTrigger) → OverlayTrigger → RAC DialogTrigger.

SectionHeader's props context tunnels every Button into its actions slot, and each overlay trigger opts its own button out with a nested Button: { tunnel: null } — but there was no ModalTrigger entry. So the trigger button was lifted out of the trigger's React subtree, RAC's PressResponder found no usePress child, and aria-expanded, aria-controls and the trigger ref never reached the button. It only kept working at all because Flow's own props context supplies onPress.

Fixed structurally rather than by adding one more entry to the list: OverlayTrigger now pins its own button with tunnel: null — a trigger's button is the react-aria trigger and may never be tunneled away — and a new overlayTriggersTunneledTo(tunnel) helper gives a host one place to send the triggers themselves. SectionHeader, Label and ListItemView were each missing entries; MarkdownEditor already had three. All four now use the helper, so the whole class is closed by construction rather than one component at a time. The invariant is recorded in packages/components/AGENTS.md.

The button still lands in the same slot, now inside its trigger. It does render differently, though: behind the modal backdrop it is now dimmed like the rest of the page instead of punching through it. That is the baseline change in this PR.

The host sees a different component, so the same defect survived in Remote

OverlayTrigger's props context covers the local tree only. ModalTrigger, PopoverTrigger and LightBoxTrigger are not @flr-generate: a remote tree emits DialogTriggerView, and the host materialises a bare DialogTrigger with no OverlayTrigger around it. A context keyed on ModalTrigger never matched there, SectionHeader kept lifting the button out of the react-aria trigger, and aria-expanded never arrived — the identical defect, one tree over, and pre-existing (PopoverTrigger had a SectionHeader entry before this PR and the same asymmetry).

Measured on the Modal in Section Header scenario, trigger button:

	before	after
Local	aria-expanded absent	"false" → "true" on open
Remote	aria-expanded absent	"false" → "true" on open

Three pieces close it:

• DialogTrigger is now a flowComponent that pins Button: { tunnel: null } itself — the invariant now sits where the PressResponder actually is, not only where the author wrote the trigger.
• overlayTriggersTunneledTo tunnels DialogTrigger as a whole, so the host sends the trigger — button included — into the actions slot.
• OverlayTrigger pins DialogTrigger: { tunnel: null } in turn, so the local tree keeps tunneling the trigger the author wrote and never the one inside it.

@flr-provider: registering the name alone breaks remote rendering

Registering DialogTrigger in the props-type registry is not free. createFlowRemoteComponent builds every registered name through flowComponent, which defaults to type: "ui" — and a UI component wraps its children in ClearPropsContext. That cut the onPress the remote OverlayTrigger had just set, and remote modals stopped opening at all, standalone ones included. Nothing errored; the click was simply inert.

So the provision type has to travel with the generated component. @flr-provider on the component's JSDoc makes the generator emit type: "provider", and createFlowRemoteComponent passes it through. It only matters where a provider actually runs inside the remote tree — i.e. where the component rendering it is not itself @flr-generate — so DialogTrigger is the only component carrying the tag today.

Why update-screenshots reported "No Screenshot Updates Required"

resolveScreenshotPath keys a reference on the screenshot description, not the test name, so (Local) and (Remote) share one baseline file. While the two environments disagreed, --update let each write it in turn and the last one won: Local wrote the fixed image, Remote overwrote it with the old one, and git status came back clean. The label could never have fixed this scenario — the divergence had to go first. With both environments agreeing, the same label produced the expected two-file commit. Both traps are now rows in the root AGENTS.md.

Verified

• Reproduced first against the real stories with captureOwnerStack() to name the culprit: owner was FlowComponent(ModalTrigger) → OverlayTrigger → RAC DialogTrigger, not ContextMenuTrigger.
• Red-green per fix, each checked by reverting that one change: reverting OverlayTrigger's tunnel: null fails the aria-expanded and PressResponder tests; reverting only the SectionHeader entry fails the actions-slot test.
• The host-side gap was bisected against a pristine control run — propTypes registration alone reproduced the broken remote modal, which is what surfaced the ClearPropsContext cause.
• New cross-environment test (OverlayTriggerInTunnel.browser.test.tsx): red on Remote, green on Local before the host-side fix; green on both after. It asserts behaviour — aria-expanded present and flipping, button inside the header's actions container.
• Screenshot parity measured directly, capturing each environment against a deleted reference: Local 205a37f5… vs Remote 8a5a9774… before, identical 205a37f5… after.
• Full local visual suite (webkit, 356 tests): Modal in Section Header is the only scenario that changed. CI agrees — every other Linux shard is green and shard 3 fails only these two, both by the same 1218 px.
• pnpm lint 0 errors, pnpm test green, 256 component browser tests, 12 remote browser tests, 30 e2e green on webkit, git status clean after pnpm build.
• run-cross-version-tests on this PR: the host output for a remote overlay trigger inside a tunneling context moves, so both harnesses run rather than only the PR-gated iframe one.

Baselines updated for the one scenario: -darwin locally, -linux by the update-screenshots label — two files, nothing else.

Follow-up

Beyond the Tabs source in #3048, a further uncontrolled to controlled source exists in Modal — isOpen goes undefined → defined between the prop-driven and controller-driven paths, 9 occurrences in Modal.browser.test.tsx. Verified pre-existing (identical count with these changes reverted) and outside this issue's reproduction list, so left alone. Filed separately.

MenuTrigger and TooltipTrigger are the same shape as DialogTrigger — a react-aria trigger wrapper whose button a surrounding context could tunnel away. Neither is reachable through a tunneling context today (MenuTrigger only renders inside the @flr-generate ContextMenuTrigger, so the host runs a real OverlayTrigger around it; TooltipTrigger is not a registered props-context name at all), so both are left untouched rather than changed without a failing case.

🤖 Generated with Claude Code

[github-actions]

Coverage Report for ./packages/components/

Status	Category	Percentage	Covered / Total
🔵	Lines	78.69%	746 / 948
🔵	Statements	78.57%	763 / 971
🔵	Functions	80.09%	165 / 206
🔵	Branches	70.33%	377 / 536

File Coverage

File	Stmts	Branches	Functions	Lines	Uncovered Lines
Changed Files
packages/components/src/components/propTypes/index.ts	100%	100%	100%	100%

Generated in workflow #6496 for commit 4cfb750 by the Vitest Coverage Report Action

[github-actions]

❌ Visual Regression Tests Failed

At least one shard did not pass. If snapshots differ from the committed baselines, download the visual-diffs-* artifacts from this run to inspect the actual/diff images (a shard that failed before comparison, e.g. during install or browser setup, leaves none).

If the differences are intentional, update the baselines by adding the update-screenshots label to the PR.

Run details

[github-actions]

🚀 Preview Deployment

Preview environments are ready:

Type	URL
docs	pr-3025.docs.review.flow-components.de
storybook	pr-3025.storybook.review.flow-components.de

Images:

• docs: ghcr.io/mittwald/flow/docs:pr-3025
• storybook: ghcr.io/mittwald/flow/storybook:pr-3025

[mfal]

Correction: rendered output does change

The PR body claims "rendered output is unchanged". That is wrong, and run-visual-tests caught it — which is why the label was on this PR rather than the claim being taken at face value.

Modal in Section Header (Local) differs reproducibly — both browsers, so both themes, at 1215 px (firefox) / 1218 px (webkit). The diff image marks exactly one element: the trigger button. Position, size and label are identical; only its fill changed, from bright brand blue to a dimmed navy.

That is the trigger button now sitting behind the modal's backdrop like every other element on the page. Before, hoisted out of its trigger's subtree by SectionHeader's tunnel, it escaped the dimming and punched through the overlay — visually reading as interactive while the modal blocked interaction. So the new frame looks like the correct one, and it is a direct consequence of the fix rather than an accident.

It is still a visual change to a shipped component, so it wants a deliberate confirmation before the -linux baselines get rewritten. Not applying update-screenshots yet for that reason.

The second failure was a flake

CodeBlock truncated (Local) failed in the required visual (2) shard at 1388 px, and passed in all six shards of the label run across both browsers. Nothing in this PR touches CodeBlock. Treating it as a known racy scenario in this suite, not as a finding — worth noting because it is exactly the kind of small-ratio diff that a blanket update-screenshots would silently bake in as the new truth.

🤖 Addressed by Claude Code

[mfal]

Rerun settles both failures

Re-ran the two failing shards:

• visual (2) now passes — CodeBlock truncated (Local) was a flake, confirmed rather than assumed.
• visual (3) fails again at exactly 1218 px on Modal in Section Header (Local) — the same count as the first run, so the diff is reproducible, not noise.

So the suite is otherwise green and the only red is the one baseline this fix legitimately changes. That is the precondition for update-screenshots — measured now, not inferred — so the label is applied.

Expected commit contents: two files only — Modal-in-Section-Header-ContextMenu-opened-{webkit,firefox}-linux.png. The label runs --update over the whole suite and git add -A, so anything beyond those two means a flaky scenario got its current frame written in as truth and has to come back out. I'll check the file list and say what it contained.

Reviewer note: the changed PNG is the visual record of the fix — the trigger button now dimmed behind the modal backdrop instead of punching through it. Worth a look at the image rather than just the green check.

🤖 Addressed by Claude Code

[mfal]

Retracting my previous explanation

I wrote above that the new frame "looks like the correct one" — trigger button now dimmed behind the backdrop. That is not supported by the evidence, and applying update-screenshots proved it: the run executed Modal in Section Header in both browsers, wrote the references, and reported "No changes detected". The regenerated PNG was byte-identical to the committed one. No commit was made, and the label removed itself.

So what is actually measured:

Run	Sharding	Result
visual (3), twice	--shard=3/4	fails, 1218 px
run-visual-tests (4)	6 shards, both browsers	fails, 1215 px (firefox) / 1218 px (webkit)
update-screenshots	unsharded, both browsers	matches the committed baseline exactly

The fix therefore does not deterministically change this rendering. It renders identically to main in the unsharded run, and differently in sharded runs. That is a nondeterministic capture, not a new correct frame — and a baseline rewrite cannot fix it, which the update run just demonstrated by writing the old bytes back.

Why the scenario is exposed

await trigger.click();
await testScreenshot("Modal in Section Header - ContextMenu opened");

Nothing synchronizes the capture beyond the preamble. Per this package's own src/tests/lib/scenarioFocus.ts, prepareForScreenshot "waits for the document to stop MUTATING" — and it documents the signature precisely: "Losing the race costs a ~1% diff in whichever environment lost it. Both environments share one reference, so it reads as a random per-run failure rather than as a race — and it is not one a bigger sleep fixes." Our diff is ratio 0.01, in (Local) only.

The trigger button is the one element whose appearance depends on whether the overlay is over it yet, so moving it inside its trigger gave that pre-existing window a visible consequence. The race is the defect here, not the fix.

What I did not do

No baseline was rewritten and nothing was pushed — the branch is unchanged. Rewriting the reference would have entrenched whichever frame a run happened to catch, which is the exact failure mode of #2945/#2985.

Making the capture deterministic is the real fix, and it needs a CI round trip to verify (a macOS run only writes -darwin, and this is a timing race). Flagging rather than guessing at it.

🤖 Addressed by Claude Code

[mfal]

Diagnosis complete: the baseline is right, the scenario is racy under sharding

Reproduced locally on this branch (fresh worktree, load average ~5) and measured the settled state directly:

Attempt	Result
Scenario + explicit 2.5 s settle before the capture, --update	regenerated PNG byte-identical to the committed webkit-darwin baseline
Scenario as-is, --update, three consecutive runs	zero changed snapshots every time

So with the fix in place the settled render equals the committed baseline, and the race does not reproduce on a fast, idle machine at all. Combined with the two earlier signals — the unsharded CI update-screenshots run writing the old bytes back, and (Remote) passing while only (Local) fails even though Remote applies interactions a serializer round trip later — the picture is consistent:

The committed baseline is the settled state. The ~1% frames are early captures, and they only surface under CI's sharded timing. update-screenshots cannot fix this, and rewriting the reference would replace a correct baseline with whichever frame a run happened to catch.

Why this fix exposed it

The scenario captures immediately after trigger.click(), and prepareForScreenshot only waits for the document to stop mutating — the trigger button's appearance depends on the overlay's stacking, which is not a mutation. Before this fix the button was tunneled out of the trigger's subtree, so its appearance did not depend on the overlay at all and the scenario was accidentally stable. It is the only element in this scenario with that dependency, which is why no other Modal scenario is affected.

Proposed fix — not pushed

The capture needs to synchronize on the thing it depends on, in the spirit of this package's src/tests/lib/scenarioFocus.ts: wait until the trigger is the topmost element at its own position, i.e. the overlay stacking has settled.

await trigger.click();

// The capture encodes whether the overlay sits over the trigger; that is
// stacking, not a mutation, so the screenshot preamble cannot see it settle.
await expect
  .poll(() => {
    const el = page.getByTestId("trigger").element();
    const { x, y, width, height } = el.getBoundingClientRect();
    return el.contains(
      document.elementFromPoint(x + width / 2, y + height / 2),
    );
  }, { timeout: 2000 })
  .toBe(true);

await testScreenshot("Modal in Section Header - ContextMenu opened");

I have deliberately not pushed this. The mechanism is inferred from the diff image, and it cannot be validated anywhere but CI — the race does not reproduce locally, so a local green run would prove nothing. Pushing an unvalidated guess onto a PR that is already under review, as the third attempt at the same red check, seemed worse than handing over a finished diagnosis. Say the word and I'll push it and watch the shards.

Nothing was committed to the branch; the temporary worktree is removed.

🤖 Addressed by Claude Code

[mfal]

The fix I proposed above is wrong — do not use it

I tried the elementFromPoint poll before pushing it. It fails, and for a reason worth recording:

AssertionError: expected false to be true
Caused by: Error: Matcher did not succeed in time.

The trigger is never the topmost element at its own position while the modal is open — the backdrop captures hit-testing across the whole viewport. document.elementFromPoint reports hit-test order, which pointer-events governs; it is not a probe for paint order. So the condition can never become true, and the poll just replaces a ~1% diff with a guaranteed 2 s timeout. My mistake, caught before it reached the branch: the snippet in my previous comment should be ignored.

Notably the same run confirms the local render is stable and matches the reference — the scenario passed the screenshot assertion three times in a row without any wait, and the only local failure is my own bad poll.

Where that leaves it

Still established, unchanged:

• The committed baseline is the settled state (explicit 2.5 s settle regenerates it byte-identically; three runs without a wait change nothing).
• update-screenshots cannot fix this — the unsharded CI run writes the old bytes back, which is exactly the row this branch added to the root AGENTS.md table.
• The ~1% divergence appears only in CI's sharded runs, in (Local) only, at 1215/1218 px.

Not established: what differs in the sharded environment. I had a plausible mechanism, tested it, and it was wrong — so I have no mechanism now, and I am not going to push another guess onto a PR under review. Finding it needs someone who can iterate against CI, since it does not reproduce locally at all.

The branch is untouched by all of this; only the main merge from earlier is on it.

🤖 Addressed by Claude Code

[github-actions]

✅ Cross-Version Tests Passed

The current host still renders old published remote versions correctly (iframe) and in the same DOM shape (in-process).

Iframe harness (attribute-accurate): ✅ passed

In-process harness (structure-only): ✅ passed

Run details

[mfal]

Green now — and my "sharded vs unsharded" framing was wrong too

All checks pass: 20 pass, 0 fail. Modal in Section Header (Local) no longer diverges. Nothing was pushed to make that happen beyond the main merge.

I checked whether the shard composition had shifted, since that was my last standing hypothesis. It had not:

Run	Shard holding Modal.browser.test.tsx	Files in that shard	Result
33377008565 (before merge)	visual (3)	21	fail, 1218 px
33480722599 (after merge)	visual (3)	the same 21	pass

Same shard, same co-tenants, same list. So the difference is not sharding, and I withdraw that framing along with the elementFromPoint mechanism from the two comments above.

What the record actually supports

The baseline matched in six measurements — four local runs (three plain, one with an explicit 2.5 s settle), the unsharded CI update-screenshots run, and now a sharded CI run. It diverged in three earlier CI runs, all before the main merge.

Two explanations remain, and I cannot separate them from this evidence:

1. Flakiness in the scenario, which passed once in the same configuration that failed three times.
2. Something in the merged main changed the timing — perf(components): stop shipping design-token build metadata to the browser #3007 (design-token build metadata no longer shipped to the browser) is the plausible candidate, since it changes what the browser loads before first paint.

What is settled, in every direction it was tested: the divergence was never this fix's settled rendering. That claim of mine from the first comment was the wrong one, and rewriting the baseline — which I nearly did — would have replaced a correct reference with a transient frame.

Nothing to do here

No push needed; the PR is unblocked and ready for review. The latent fragility of this scenario is worth watching if it resurfaces — the root AGENTS.md row this branch adds already documents the part that costs the most time, namely that update-screenshots reports "No Screenshots to update" and cannot fix it.

🤖 Addressed by Claude Code