Skip to content

Fix/UI lifecycle a11y hardening - #11

Merged
hexplus merged 2 commits into
mainfrom
fix/ui-lifecycle-a11y-hardening
Sep 1, 2026
Merged

Fix/UI lifecycle a11y hardening#11
hexplus merged 2 commits into
mainfrom
fix/ui-lifecycle-a11y-hardening

Conversation

@hexplus

@hexplus hexplus commented Aug 30, 2026

Copy link
Copy Markdown
Owner

Summary

Fixes six classes of defect across lifecycle management, modal scroll locking, reactive children, AlertDialog accessibility, form participation, and the release script.

Every fix was reproduced against the unmodified base first: the new regression suites produce 68 failures when run against 70143b1, and all pass on this branch.

Base: 70143b1 -> Head: 0795f83 | 46 files changed (+4405 / -780) | 224 tests (was 102)

1. Reactive ownership and disposal

bindControlled() created an effect() and threw away its teardown, so a controlled component stayed subscribed to its parent's signal forever - reading the getter and writing into detached DOM long after dispose(). The same pattern appeared in ~35 queueMicrotask blocks across the component set, none of which guarded against the element being disposed before the microtask ran.

  • bindControlled() now returns its teardown and accepts an owner node. Every caller registers it on the component's in-tree root.
  • New src/lib/lifecycle.ts provides nodeOwner() / ownedEffect() / deferOwned(): teardowns are collected per DOM node, run exactly once, and cover timers, requestAnimationFrame, observers and document/window listeners. deferOwned() skips setup entirely if the node was already disposed, closing the microtask race.
  • Portaled content (DropdownMenuContent, sub-menus, portaled TooltipContent) anchors cleanup on the in-tree node - dispose() cannot reach a node that has been moved to <body> - and now disposes the portaled subtree before detaching it.
  • Six components used a raw signal(controlled ?? default) and could not accept a reactive getter at all. Migrated to bindControlled, widening open?: boolean to boolean | (() => boolean).
  • ResizeObserver construction is guarded in accordion.ts (it already was in scroll-area.ts), and ScrollArea's scrollHideDelay timer is now cancelled on disposal - it previously fired against detached DOM.

2. Coordinated body scroll lock

Dialog, AlertDialog, Sheet and Drawer each did document.body.style.overflow = "hidden" / "" independently. Mounting an initially closed modal erased the page's existing overflow; closing one modal unlocked the page while another was still open; disposal restored "" rather than the original value.

Replaced with a reference-counted createScrollLock() in src/lib/scroll-lock.ts:

  • The first acquisition snapshots the exact inline value and CSS priority; the last restores it, or removes the property when there was none.
  • Additional locks never re-snapshot, so a modal cannot capture another modal's hidden.
  • Each holder gets its own handle, idempotent per handle - rapid open/close/open and a disposal racing a close cannot drive the count negative or strand it.
  • An initially closed component performs no body-style mutation at all.
  • Safe to import without a DOM.

3. Reactive children preserved

toNodes() returns Node[], and a () => NodeChild getter has no Node representation - so it was silently dropped. DialogContent(() => "Reactive child") rendered nothing.

Added toChildren(): NodeChild[], which passes getters straight through to the SibuJS tag factory. The framework binds and disposes them itself, so there is no placeholder element, no manual effect(), and no innerHTML.

DialogContent(() => label());          // whole child reactive
DialogContent([icon, () => label()]);  // mixed, close button still lands last

All 25 call sites migrated, including the duplicate helper in command.ts, which was deleted. Ordering is preserved around internal nodes that components prepend or append (Drawer handle, Dialog close button, menu chevrons, Command group heading). Nested arrays flatten in order; null / undefined / booleans are ignored; nothing is stringified.

toNodes() keeps its exact Node[] signature for backward compatibility and is deprecated with a dev-mode warning - no breaking change.

4. AlertDialog accessible labelling

AlertDialogContent referenced aria-labelledby="alert-dialog-title", but no element ever carried that id, and two dialogs would have collided outright.

Per-instance ids are now generated with createId(), shared through the AlertDialog context, and adopted by that instance's own title and description. An explicitly supplied user id wins, and the content is re-pointed at it so the reference still resolves. A reference that resolves to nothing - because the title or description is intentionally absent - is removed rather than left dangling.

5. Checkbox, Switch and RadioGroup participate in forms

These exposed name / value / required but rendered type="button" controls, producing no entries in new FormData(form) and taking no part in constraint validation.

Each is now paired with a visually hidden native input (src/lib/form-control.ts) that mirrors its state - deliberately not display: none or type="hidden", both of which break constraint validation or make required a lie.

  • Submitted only while checked/selected, under its name; value defaults to "on" as a native checkbox does.
  • disabled excluded per spec; required enforced by real checkValidity().
  • form.reset() restores the default and the visible control follows.
  • The bridge is aria-hidden with tabindex="-1" - no duplicate tab stop, no duplicate form entry - and is removed on disposal.
  • Switch gains the missing value and required props.
  • RadioGroup implements the native radio keyboard model: a single roving tab stop, wrapping Up / Down / Left / Right, and Home / End, all skipping disabled items.

A name (or required) is what opts a control in; without either, no hidden input is created.

6. Release script fails closed

publish.mjs ignored lint failure, ran no tests or type-check, updated only package.json (leaving package-lock.json inconsistent), ignored git command results, created a commit and tag before discovering a publish problem, and printed success unconditionally. Its rollback was a literal no-op - it re-read the already-mutated file into a local object that was never written back.

Orchestration moved to scripts/release-core.mjs with every side effect injected, so ordering, aborts and rollback are testable without ever publishing or pushing.

  • All gates run before any version mutation: npm ci, npm run lint, npx tsc --noEmit, npm test -- --run, npm run build, npm pack --dry-run. A failure aborts with the repository untouched.
  • Version written via npm version --no-git-tag-version, keeping both files consistent and leaving tagging explicit.
  • The target version must not exist as a local tag, a remote tag, or an npm release. Nothing is ever overwritten or unpublished, and --force is never used.
  • Prerelease-aware bumping, and the new version must sort above the current one.
  • A failed publish removes the local commit and tag and restores the version files - no misleading release: vX commit survives. A failed push after a successful publish reports the exact recovery command and does not unpublish.

Also adds .github/workflows/ci.yml running install, lint, type-check, test and build on PRs and pushes to main across Node 22.3, 22 and 24 (SibuJS 4 requires >=22.3.0), plus a production-dependency audit. package.json now declares engines.node.

Documentation

  • Peer range corrected from the stale sibujs >= 1.3.0 to the actual >=3.2.0 <5.0.0, verified by running the full suite and type-check against 3.2.0, 3.4.1 and 4.0.1.
  • Remaining 1.3.0 wording removed.
  • Reactive children and form behaviour documented; supported Node version stated.
  • The unverified "Tree-shakeable" claim replaced with what the package actually configures (ESM + CJS, "sideEffects": false). Accessibility section states what is wired without claiming WCAG conformance.

Test plan

Command Result
npm ci pass
npm run lint pass
npx tsc --noEmit pass
npm test -- --run pass (224 passed)
npm run build pass
npm pack --dry-run pass
npm audit --omit=dev pass (0 vulnerabilities)
git diff --check pass
git status --short pass (clean)

Suite run 8 times including shuffled order and --no-file-parallelism (shared process) - 224/224 every time, no cleanup-related cross-test pollution.

New suites: lifecycle-disposal, scroll-lock, reactive-children, alert-dialog-a11y, forms, release. No test performs a real npm publication or remote push.

API compatibility

No breaking changes.

  • bindControlled() gained a 4th tuple element and an optional 3rd parameter - 3-element destructuring still works.
  • toNodes() signature unchanged; toChildren() newly exported.
  • Several open?: boolean props widened to also accept getters.
  • Switch gained value / required; package.json gained engines.node.

Known limitations

  • The bridge input is inserted as a sibling of the styled control. It is absolutely positioned and invisible, but a parent using :nth-child selectors or counting children would see one extra element.
  • Bridge inputs appear one microtask after construction (two for RadioGroupItem, which must locate its group first), so FormData read synchronously after construction is empty.
  • ARIA-reference pruning runs once after insertion; a title added later by a reactive child will not retroactively restore the dropped reference.
  • Pre-existing and not addressed here: package.json lists LICENSE in files, but no LICENSE file exists in the repo.
  • The CI workflow is structurally validated locally but has not yet run on GitHub.

hexplus added a commit that referenced this pull request Aug 30, 2026
@hexplus
fix(pr11): npm dist-tags, verified rollback, RadioGroupItem re-regist…
0671b8a 
…ration, membership-aware validity

Addresses the four remaining PR #11 review findings.

1. Prereleases never publish under npm's default `latest`
   The publish argv was always `npm publish --access public`, so a prerelease
   landed on the default dist-tag and would be served by `npm install
   sibujs-ui`. A git tag is not an npm dist-tag; the old test named "publishes
   a prerelease with an explicit tag" only asserted the git tag, and has been
   renamed to say so. npmDistTag() now derives the dist-tag from the first
   prerelease identifier (alpha/beta/rc/canary...), falling back to `next` when
   that identifier is numeric or literally "latest", since npm rejects a
   dist-tag that parses as a version. Stable versions publish `--tag latest`
   explicitly. The tag is logged before publishing and returned in the result.

2. Release rollback is genuine and verified
   `restoreFiles()` ran `git checkout -- package.json package-lock.json`, which
   restores the worktree *from the index* — after `git add` succeeded, the
   version bump stayed staged and the release was not actually undone. It now
   takes the validated revision and runs
   `git restore --source=<rev> --staged --worktree`, reverting index and
   worktree together.

   HEAD is also captured and validated (as a real commit id) *before* the
   gates and before any version mutation: without a verified rollback target
   the release refuses to start rather than proceeding un-undoable.

   rollback() returns {ok, failures} and every cleanup command result is
   checked. A failed cleanup raises stage "rollback" naming the original
   failure, the exact commands that failed, and the manual recovery steps —
   it never claims the repository was restored. A successful publish is still
   never rolled back automatically.

3. RadioGroupItem follows its current group
   An item bound its group context once inside deferOwned() and kept it for
   life, so an item mounted late got no ARIA state and no bridge, and an item
   moved from group a to b kept submitting under a. Items now expose an
   attach(ctx) handle returning an idempotent detach; the group owns a registry
   and reconciles membership, attaching joiners and detaching leavers. Bridges
   and effects are scoped (Owner.addScoped / scopedEffect / a cleanup handle
   from attachRadioBridge) so a binding can be replaced without disposing the
   item. Ownership is by nearest RadioGroup ancestor, so nested groups never
   claim each other's items.

4. Nameless required-group validity tracks membership
   Validity was computed by an effect depending only on the selection signal,
   while the MutationObserver updated only the roving tabindex — so removing or
   disabling the selected item left the group reporting valid. Reconciliation
   now drives item bindings, the roving tab stop and constraint validity from
   one membership model, and the validity bridge exposes refresh(). The valid
   selection is computed by comparing data-value across the group's own items
   rather than building an attribute selector out of consumer strings.

Tests: 325 total. New suites pr11-final-repro.test.ts (17) and
pr11-radio-registration.test.ts (15); 25 of them fail against ccca9ca. The
rollback finding also has an isolated integration test using a temporary git
repository that demonstrates `git checkout --` leaving the bump staged and
proves `git restore --source=<rev> --staged --worktree` clears index and
worktree. No test performs network access or publishes anything.
hexplus added a commit that referenced this pull request Aug 30, 2026
@hexplus
fix(pr11): uncertain-publish model, order-independent item binding, n…
71f4e8a 
…ested isolation, disabled bridge sync

Addresses the four remaining PR #11 core-review findings. Local changes only —
nothing was published, tagged or pushed, and the release script was not run.

1. A failed `npm publish` no longer proves nothing was published
   The exit code was treated as proof, so a registry that accepted the tarball
   before the connection died would be followed by deleting the git tag,
   resetting the release commit and restoring the old version — leaving a live
   release with no matching repository state, and no way back because
   unpublishing is not an option.

   The publish now runs through `runDetailed`, a structured adapter reporting
   {started, ok, status, signal, error}, and the result is classified as
   not_started / confirmed_published / confirmed_absent / uncertain:
     - never started -> nothing reached the registry, rollback is safe;
     - clean exit    -> continue;
     - ran and failed -> query `npm view <pkg>@<version>` once (never retry the
       publish) and let the registry decide.
   Confirmed present is treated as a success and keeps the commit and tag.
   Confirmed absent permits the verified rollback. Anything inconclusive —
   network, auth, DNS, timeout, empty or malformed output — raises a distinct
   `publish-uncertain` error that rolls nothing back, states the status is
   UNKNOWN, and gives separate recovery steps for "version exists" and "version
   does not exist". No path calls `npm unpublish`, and no message claims
   "Nothing was published" on the strength of an exit code alone.

2. Item binding no longer depends on MutationObserver ordering
   Observers fire in creation order, not DOM-operation order. With the
   destination group created first, it installed the new binding and the source
   group's observer then ran a stale detach that stripped the ARIA state,
   indicator and bridge the new binding had just installed. Each item now holds
   one authoritative binding identified by a token: attaching retires the
   previous binding immediately, and any later detach whose token is no longer
   current is a no-op. Both construction orders now produce identical results.

3. Nested RadioGroups are fully isolated
   `applyRovingTabIndex()` still used `el.querySelectorAll(ITEM_SELECTOR)`,
   claiming nested items and leaving the inner group with zero tab stops; the
   keydown handler also acted on bubbled events from nested groups, so
   ArrowRight inside the inner group invoked the outer `onValueChange`. Both now
   use nearest-group ownership (`item.closest(GROUP_SELECTOR) === el`).
   Propagation is left untouched, so consumer handlers still see the event.

4. Named bridges follow the effective disabled state
   Reconciliation skipped already-registered items, and the bridge captured
   `disabled` once at creation, so disabling a selected item left its native
   radio enabled and still submitting. Bindings now expose {detach, refresh};
   reconciliation refreshes retained bindings, and the bridge takes a disabled
   *getter*. A disabled bridge is also unchecked, because a radio group's
   `required` constraint is satisfied by any checked member — a
   selected-but-disabled member would otherwise keep an empty required group
   looking valid while contributing nothing to FormData. Writes are guarded to
   the changed value so the bridge, which lives inside the observed subtree,
   cannot feed its own attribute changes back into reconciliation.

Tests: 389 total (was 325). New suites pr11-core-repro.test.ts (12) and
pr11-core-hardening.test.ts (52); 39 of them fail against 0671b8a. Every
release test uses injected adapters that only record argv — no test contacts
npm or any remote.
@hexplus
fix(ui): harden lifecycle, a11y, forms and ARIA ownership; drop the a…
c72de8d 
…utomated npm release subsystem in favour of manual publishing
@hexplus
hexplus force-pushed the fix/ui-lifecycle-a11y-hardening branch from e8fca70 to c72de8d Compare August 31, 2026 23:56
@hexplus
hexplus merged commit 70702dd into main Sep 1, 2026
7 checks passed
@hexplus
hexplus deleted the fix/ui-lifecycle-a11y-hardening branch September 1, 2026 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@hexplus