ReviewOS

stacks/stx

Two instances of a component share one event element id, so only the first one's handlers bind

#2033
Closed glennmichael123 opened this 9 days ago · {{ comments.length }} {{ comments.length === 1 ? 'comment' : 'comments' }}
9 days ago

What breaks

Verified through the real pipeline (renderApp + the real runtime, packages/stx/test/router/spa-harness):

one instancetwo instances
<Listbox> — click an optionselectsdoes nothing
<Combobox> — click an optionselectsdoes nothing
two <Listbox> on one page — click the first trigger—neither opens (isOpen = [false, false])
<Combobox> chevronnever opens the listnever opens the list
<Dropdown> — click an itemselectsselects

So <Listbox> and <Combobox> cannot be used with more than one option, and two listboxes on a page have two dead triggers. There is no console error; the bindings are simply inert.

Cause

Scope processing for a component's <script client> is gated on the script naming one of SIGNAL_API_RE's identifiers (signal-processing.ts:1408 — state, derived, onMount, defineProps, defineEmits, …). A component script that only declares plain functions matches nothing, so no data-stx-scope is registered for the instance:

Dropdown with 1/2/4 items  ->  itemScopes = 1 / 2 / 4   works
Listbox with 1/2 options   ->  optionScopes = 0 / 0      works only at 1

With exactly one instance the @click still resolves — which is why this was never noticed. With two, nothing binds.

The only difference between the two components is one line. DropdownItem.stx opens its client script with

const emit = defineEmits()

and ListboxOption.stx does not. Adding that one line to ListboxOption takes optionScopes from 0 to N and makes selection work at every count — which is the diagnosis, not a proposed fix: a component should not need to call defineEmits() it has no use for in order for its own @click to bind.

The gate is wrong in the same way twice already

processScriptSetup carries a comment about the page-level version of exactly this (stacksjs/stacks#2394):

a script that factors its signals into a module names no signal API itself — const { signal } = makeProbe() matches nothing above — so the page was treated as static: no setup function, no data-stx on <body>, every binding inert with nothing logged

Same shape, one level down. "Does this script name a signal API?" is a proxy for "does this script need a scope?", and it is wrong whenever the answer comes from the template rather than the script — a script that declares onClick and a template that calls it in @click="onClick($event)" needs a scope with no signal API anywhere in sight.

Suggested direction

Decide the gate from the pairing rather than from the script alone: a component needs a per-instance scope when its template references an identifier its client script declares. That is cheap to compute (collect function/const declarations, test them against the directive attributes) and it is the condition that actually matters.

Components with the at-risk shape

A client-script handler referenced from the template, with neither defineEmits() nor a signal API in the script:

  • ui/listbox/ListboxOption.stx — onClick
  • ui/listbox/ListboxButton.stx — onClick
  • ui/combobox/ComboboxOption.stx — onClick
  • ui/combobox/ComboboxInput.stx — onInput
  • ui/combobox/ComboboxButton.stx — onClick
  • ui/radio-group/RadioGroupOption.stx — onClick, onKey

RadioGroup is not affected despite being on that list: it delegates clicks at the group root, so selection works (verified — two radios, second one clicks, selected = "pro"). The other two widgets rely on each part's own scope.

Found while adding keyboard navigation to these widgets for stacksjs/stx#2032; <Listbox>'s Enter-to-select could not be made to work until this was understood.

9 days ago

Correcting the "suggested direction" above — I tried two fixes and neither is right, but the dead ends narrow it usefully.

The framework already has a designed path for this case. component-instance-stamp.test.ts has two fixtures whose comments draw exactly the distinction:

// No signals, nothing in the template reads it: a plain IIFE.
'Plain.stx':   `<script client>window.__plainRuns = …</script><div class="plain">plain</div>`
// No signals, but the template binds a declaration: wrapped in stx.mount.
'Mounted.stx': `<script client>function label() { return 'x' }</script><div :text="label()"></div>`

So "no signals but the template reads a declaration" is meant to get an stx.mount() wrapper. <ListboxOption> is that case and was getting the Plain treatment.

Why it is classified wrong. The Phase 2 decision in client-script.ts (~1504) reads declarations out of the script and references out of options.templateContent. But skipEventDirectives is hasSignalScripts || clientScripts.length === 0 (utils.ts:1760), so for these components it is false — the @click is extracted into eventBindings and removed from the template before the mount decision reads it. A :text binding stays in the template and is seen; an @click cannot be. That single asymmetry is the whole difference between Mounted.stx working and <ListboxOption> not.

Attempt 1 — widen the scope gate (utils.ts:1701, hasSignalScripts to also be true when the template references a script declaration). Fixes every symptom: Listbox selects at any option count, two Listboxes get independent triggers, the Combobox chevron opens, RadioGroup keeps working, Dropdown unchanged. packages/components 1419 pass / 1 fail (unrelated), packages/stx 11373 pass / 4 fail — and those four are precisely the tests encoding the data-stx-instance / stx.mount design above. So it works by routing the case away from the mechanism built for it, rather than by fixing that mechanism.

Attempt 2 — feed eventBindings into the Phase 2 reference set so an extracted @click still counts. This emits the stx.mount() wrapper (mount calls go 0 → 2 and 0 → 3 for one and two options) and does not fix the behaviour at all: selection still dead at two options, chevron still dead. So the wrapper is necessary but not sufficient — stx.mount as currently implemented does not give these handlers a per-instance binding.

So the defect is in how stx.mount binds a no-signal component instance's handlers, not only in the classification. Attempt 1's result is evidence the scope path does handle multiple instances correctly, which may mean the honest fix is to decide that a template-bound component simply needs a scope and to retire the separate instance path for it — but that is an architectural call about #1958's design, not a patch.

Both attempts are reverted; packages/stx/src is untouched. Detection sweep for the affected shape and the verified symptom table are in the issue body.

9 days ago

Root cause found — supersedes both guesses above

It is not the scope gate and not stx.mount. It is a duplicate id.

events.ts:255 declares the element-id counter local to the parse:

let idCounter = 0
...
const elementId = `__stx_evt_${idCounter++}`   // events.ts:268

Each component instance's template is parsed separately, so every instance starts at 0 and gets id="__stx_evt_0". The generated binding then resolves the element by that id (events.ts:440, and again at 456 and 484):

var $el = document.getElementById('__stx_evt_0');

getElementById returns the first match, so every instance's handler binds to the first instance's element. One instance works; instances 2..N have no handler. That is the whole bug.

Minimal repro, no components library involved:

components/Tap.stx:
  <script client>
  function onTap(e) { void e }
  </script>
  <button data-tap @click="onTap($event)"><slot /></button>

page:  <Tap>a</Tap><Tap>b</Tap><Tap>c</Tap>
one instance     all ids: ["__stx_evt_0"]                              DUPLICATED: none
three instances  all ids: ["__stx_evt_0","__stx_evt_0","__stx_evt_0"]  DUPLICATED: [["__stx_evt_0", 3]]

Why <Dropdown> escapes it

skipEventDirectives is hasSignalScripts || clientScripts.length === 0 (utils.ts:1760). DropdownItem opens its client script with const emit = defineEmits(), so hasSignalScripts is true, so its @click is never extracted into this id-based path — it stays an attribute and the client runtime binds it per element through the scope. ListboxOption and ComboboxOption name no signal API, so their events go through events.ts and collide. The defineEmits difference I spotted first was a real signal; it just pointed one layer away from the cause.

This is also a correctness bug on its own

Duplicate ids are invalid HTML and break everything id-based, regardless of event handling: getElementById, <label for>, and every id-based ARIA relationship — aria-labelledby, aria-describedby, aria-controls, aria-activedescendant.

It bit the Combobox work directly (ec97f05c9f): aria-activedescendant is only meaningful against a unique id, and the options arrived sharing one, so pointing at it resolved to an arbitrary option. That component now keeps an existing id only when it is genuinely unique in the document and assigns its own otherwise — a local guard around this bug, not a fix for it.

On fixing it

The id needs to be unique per instance, and it cannot be made unique with a module-level counter or any randomness: utils.ts documents at length why component ids must be identical across renders or the render becomes uncacheable (~5MB of native churn per render was the measured cost). The component uid already satisfies all three constraints there — identical across renders, unique within a document, distinct across pages — so threading it in as the id prefix looks like the shape of the fix, rather than seeding a counter.

Not attempting it in this pass: it touches the event path for every component, and events.ts also reads elementId back out of existing tags (609–615, 703–711), which wants reading properly first.

9 days ago

Fixed in 32040328b9.

The element id is namespaced with the component's uid, so two instances no longer share one. Verified against every symptom in the table above:

beforeafter
<Listbox> two options, click onenothingselects
<Combobox> two options, click onenothingselects
<Combobox> chevronnever openedopens
two <Listbox>, click the first trigger[false, false][true, false]
<Dropdown> any countselectsselects (unchanged)
<RadioGroup>workedworks (unchanged)

packages/components 1433 pass, packages/stx 11382 pass, both 0 fail. Format, lint and typecheck clean.

The uid, not a counter. utils.ts documents why a component id must be identical across renders — a render producing different bytes each time is uncacheable, measured at ~5MB of native churn per render — so a module-level counter or any randomness was out. The uid already satisfies that plus uniqueness within a document and distinctness across pages, which is exactly the three constraints this needed. A page's context has no $uid, so a page's own ids stay bare and the change reaches only the case that was broken.

The scope classification is deliberately untouched. My first attempt widened the scope gate; it fixed the symptoms but routed this case away from the data-stx-instance / stx.mount path that #1958 built for it, and failed the four tests encoding that design. Those tests pass unchanged here, which is a good sign the fix is in the right layer.

event-id-uniqueness.test.ts pins the contract, including the part that actually bit: that each getElementById lookup in the generated binding resolves to exactly one element. Removing the namespace fails 4 of its 5 tests.

Also worth noting for anyone who hits something similar: <Listbox>, <Combobox> and <RadioGroup> still start with selected() returning the string "null", from the null idiom — the interpolation quotes it. Harmless for comparison (nothing equals it) but it is emitted by emit('change', selected()) on open. Calendar and DateRangePicker already carry comments explaining they avoided that idiom for this reason. Not part of this fix; filing separately if it is not already known.

Sign in to comment on this issue.