ReviewOS

also looking at this

stacks/stx

fix(router): a click the page already handled is not a navigation

#1948
Merged glennmichael123 wants to merge fix/router-respect-prevented-click into main
2 files +177 -1
8 days ago

Fixes stacksjs/stacks#2393.

The bug is worse than reported

The issue says the router ignores preventDefault and that "the handler does run". It doesn't. A probe against the real router script:

>> page handler ran: false
>> router fetched  : ["/partner"]

A page's own click handler on an intercepted link never runs at all.

Root cause

The SPA click listener was registered on document in the capture phase:

document.addEventListener('click',function(e){
  ...
  log('[router] click intercepted:',href,'container:',!!getContainer(),'defaultPrevented:',e.defaultPrevented);
  e.preventDefault();
  e.stopPropagation();
  navigate(withCurrentLocale(href));
},true);   // <- capture

Two consequences, and they compound:

  1. Capture runs before the target. So e.defaultPrevented is necessarily false when the router reads it. Note the log line: the value was read only to print it, never to branch on. Adding a guard without leaving capture would have fixed nothing.
  2. stopPropagation() in capture stops the event ever reaching the element. That is why the handler never runs - it is not losing a race, it is never dispatched.

The ,true has been there since the listener was first drafted, in a chore: wip commit, with no comment justifying it. It does not look like a considered decision.

Blast radius

Both intercept paths: a marked [data-stx-link] anchor, and any same-origin anchor under interceptAllLinks - which is how static-site mode serves every page. Any anchor meant to open a panel, dialog or inline form navigated away instead, and the workaround was to replace it with a <button> in script, which costs the no-JS reader the link.

By the same mechanism this also swallowed anything else bound to an intercepted anchor, since stopPropagation does not care who registered the listener.

The fix

Bubble instead of capture, and return early on a cancelled click. The page's handlers run first, which is where a decision about the page's own markup belongs, and the router honours the same contract the browser does for a plain anchor.

Verification

7 new tests in packages/router/test/link-preventdefault.test.ts, using the existing very-happy-dom harness that evaluates the real getRouterScript() and dispatches real events.

Each half of the fix is proven load-bearing by reverting it in isolation:

variantresult
restore capture, keep the guard5 of 7 fail, including runs the page handler at all
keep bubble, drop the guard3 of 7 fail
both7 pass

Every case is paired with a control that must still be intercepted, so a probe reporting "not intercepted" for both is not mistaken for a pass. One case covers a sibling link, since the guard reads the event rather than the element.

  • Router suite: 239 pass / 0 fail (baseline 232, +7 new).
  • stx build + owned-routes: 130 pass / 0 fail.
  • typecheck clean; lint 0 errors.

Note for reviewers

One behaviour genuinely changes beyond the bug: a page handler that calls stopPropagation() without preventDefault() will now stop the router seeing the click, giving a full page load. That is the correct reading of stopPropagation and matches other SPA routers, but it is a real difference from capture-phase interception.

2 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.