ReviewOS
@if (!issue)

No such issue

stacks/stx has no issue numbered 1931.

@else

stacks/stx

An inline @if(...)…@endif inside a COMPONENT tag applies to only its first attribute; the rest leak out unconditionally

#1931
Closed glennmichael123 opened this 14 days ago · 1 comment
14 days ago

Found while converting components to <StxLink> for #1930.

A conditional attribute group written inside a tag works on a plain element and is silently broken on a component tag: only the first attribute stays inside the conditional, and every attribute after it is emitted unconditionally.

Repro

<script server>const active = false</script>
<StxLink to="/x" class="row" @if(active)data-active="true" aria-current="page"@endif>Go</StxLink>
<a  href="/x" class="row" data-stx-link … aria-current="page">Go</a>

data-active is correctly dropped. aria-current="page" is not — with active === false.

The same markup on a plain element is correct:

<a href="/x" class="row" @if(active)data-active="true" aria-current="page"@endif>Go</a>
<a href="/x" class="row" >Go</a>

Two groups is worse

<StxLink to="/x" @if(off)a="1"@endif @if(off)b="2"@endif>Go</StxLink>
<a  @if(off)b="2" href="/x" data-stx-link …>Go</a>

The second group survives into the output as a literal attribute named @if(off)b.

Cause

Directives run after component processing, so on a plain element the @if group is still intact in the raw markup when processConditionals reaches it. A component tag is consumed first, and parseAllAttributes splits the group: @if(active)data-active="true" parses as one @-prefixed (event) attribute whose name carries the condition, and aria-current="page" parses as an ordinary static attribute. The first is re-emitted onto the root element still spelled @if(...)…, so the later conditional pass sees it and handles it; the second was separated from the group and is just forwarded.

Why it matters

The failure is silent and the wrong direction. aria-current="page" on every row tells a screen reader that every page is the current one; tabindex="-1" leaking onto every item removes the whole nav from tab order. Both are exactly the attributes people put in these groups.

Suggested

Either handle the group atomically in the component tag's attribute parsing — evaluate the condition against the server context and splice the attributes in or out before props are resolved — or reject it loudly. Silently applying half of it is the worst of the three.

Workaround, and what the components in #1930 now use: :-bound conditional attributes (:aria-current="active ? 'page' : null" — null drops the attribute since aa43fa7) or an interpolation with an explicit else-value (aria-disabled="").

14 days ago

Fixed in c207a345b8.

The group is now resolved against the server context before the tag's attributes are parsed, using the same processConditionals a plain element goes through — so @else and @elseif behave identically on both and the two can't drift.

Two notes on the shape of the fix:

  • The well-formed check ahead of it is a fast path, not the safety property. It keeps the conditional processor off the attribute string of every component tag in a codebase when almost none carry a group. Correctness for @click="say('@if(x)')" comes from processConditionals itself needing a closing token — which is also exactly how that string behaves on a plain element, so the two agree for the right reason rather than by coincidence. I sabotaged the guard to check: no test changed, and the comment now says so.
  • It works inside @foreach, which I'd assumed it wouldn't. Loop variables are available when component processing runs per iteration, so /