feat(web): improve task controls and keyboard reordering#25
Merged
Conversation
johannesjo
force-pushed
the
task/beautify-04-project-shell
branch
from
July 21, 2026 12:52
cf37a85 to
f09aec4
Compare
Gating onMoveUp/onMoveDown on reorderPending() dropped .reorderAvailable from the ⋯ trigger mid-flight, which desktop CSS display:none's — pulling the element out from under the focus Popover had just returned to it, so a keyboard reorder ended with focus on <body>. Offer the actions by position only and let commitReorder's guard refuse the second commit. A refused commit now toasts instead of no-oping: a drag dropped during a pending reorder had its DOM already reverted, so the row snapped back with no explanation. The toast is role="status", so it covers the keyboard path too. Also hover/focus-reveal the ⋯ on desktop like the other row actions (it was permanently visible), and neutralize the new <h2>'s UA type so the collapsed "· N" count stops inheriting bold.
role="checkbox" + aria-checked already announce "checkbox, not checked", so "Mark X complete" duplicated the state and flipped the accessible name as the row toggled. Name it after the task instead, per the APG checkbox pattern. Also flatten captureFocusFromRow's restore closure: a four-level nested ternary and a duplicated row lookup become two named helpers (checkboxIn / sourceControlIn) and a rowById lookup. No behaviour change.
npm audit --audit-level=high has been failing on this branch, and it ran BEFORE npm run check — so typecheck, lint and format:check never executed. Any lint or formatting regression could land unnoticed. Bump the two high-severity dev-only transitives (js-yaml 4.2.0 -> 4.3.0 via eslint, shell-quote 1.8.4 -> 1.9.0 via concurrently 9.2.4); lockfile-only, no majors. Then move the audit after npm run check with if:always(), so a newly published advisory can no longer mask the code checks on an unrelated run. The remaining 4 moderate esbuild advisories (via drizzle-kit) sit below the high threshold and would need a drizzle-kit major.
The keyboard reorder path had no e2e coverage, and the regression it
guards is invisible to jsdom: the focus loss came from a CSS rule
(.moreButton:not(.reorderAvailable) { display: none }) unrendering the
focused ⋯ trigger, and unit tests apply no CSS. The PATCH is delayed via
page.route so the in-flight window survives a style recalc.
NOT YET RUN — the sandbox cannot reach docker-published host ports, so
no local database was available. Typecheck and format pass; CI's e2e job
is the first real execution.
ListCard's onDeleteItem became Promise<boolean> so a failed delete can keep focus on the row. ChecklistCard's twin handler already returned it; the hero list's copy in Project.tsx still returned void, which only surfaced once the branch rebased onto main and 'npm run check' actually ran. Also drops a duplicate .titleGroup comment: main's merged #27 added the same heading reset, so this branch no longer needs to touch that file.
The previous fix kept the reorder actions mounted so the desktop rule
'.moreButton:not(.reorderAvailable) { display: none }' could not unrender the
menu trigger under focus. That was necessary but not sufficient, and the
invariant it relied on was not actually holdable:
- The optimistic store write reorders the row synchronously, and <For>
re-inserts it. Re-parenting a node blurs its focused descendant, so focus
hit <body> on every keyboard move, long before the PATCH resolved.
- .reorderAvailable is derived from props.items, so a collaborator's SSE echo
could drop it mid-interaction and display:none the trigger anyway.
Drop the display:none rule instead of defending it: .moreButton already sits in
the opacity reveal group, whose comment states the rule this violated --
buttons stay in layout, hit-testable, and focusable. That removes canReorder(),
the .reorderAvailable class, and the whole failure mode. Focus is then handed
back to the trigger after the synchronous write. Keep the trigger visible while
its own menu is open, since the portal puts focus outside :focus-within.
The existing tests mocked moveItem to a no-op, so no row ever moved and focus
was never at risk; the new test reorders for real and fails without the fix.
Rework the e2e test onto a released gate rather than a 600ms timer, drive it
from the keyboard, and assert focus after the response lands.
Every reorder test mocked moveItem/setItemPosition as no-ops, so no row ever moved. That is how a focus bug that broke the feature's central promise on every keyboard move survived the whole suite: with a static list, focus was never at risk, and the assertions could only ever check that the API was called with the right arguments. Have renderCard drive the card from a signal and give the mocks a real reposition-and-resort, mutating items in place so identity survives the way produce() does in store.ts. <For> then re-inserts rows exactly as it does in the app, and any future reorder test exercises the real DOM movement by default rather than opting in. Three tests needed adjusting because they had been asserting against a list that never changed: - the refusal test picked Move up on a row that is first once the optimistic move lands; it now moves the row that still has somewhere to go - two announcement assertions raced the extra render and now wait for it - the focus test waits for its commit to settle, because reorderPending is module scope and an in-flight move leaks into the next test
johannesjo
force-pushed
the
task/beautify-05-task-interactions
branch
from
July 21, 2026 17:00
cbcf129 to
c4d3eea
Compare
Toast is role="status", so raising a toast AND writing the live region read
every reorder failure to a screen reader twice. Let the toast own failures and
the live region own success. That also collapses commitReorder's
boolean | null tri-state: refused and failed are now both plain false, since
the only caller treats them identically -- both have already been toasted.
Refresh /_styleguide, which still described the ⋯ menu as touch-only. It is on
pointer devices too now, because keyboard reorder has no inline equivalent.
The demo rows passed no move handlers, so the menu could not show Move up /
Move down at all; give them positional availability the way ListCard computes
it, folding four near-identical blocks into a For.
Use if: ${{ !cancelled() }} for the audit step -- it expresses the actual
intent (never let the checks above short-circuit the audit) without also
burning a step on cancelled runs.
The focus restore ran once, after the optimistic write. A failed move rolls the position back -- a second synchronous store write, so <For> re-inserts the row a second time and blurs the trigger again, with nothing to catch it. Every focus assertion sat on the success path, so the suite missed it. Restore on both edges, and take the trigger from the argument Menu already hands to onSelect instead of sniffing document.activeElement: the node is then known rather than inferred from Popover's focus-return ordering. The <body> guard stays, so a user who deliberately focused elsewhere during the request keeps it. Also from review: - the styleguide demo gave move actions to its two checked rows, which ListCard excludes from reordering, while claiming to mirror it - reorderIndex's comment justified the position-only rule with a scenario that cannot happen (Menu closes before onSelect runs); state the real reason - drop an assertion that could not fail, rename a describe that still called the menu mobile-only, and assert the e2e menu focus instead of setting it
From an accessibility/correctness/simplicity review round. Two of these were reproduced with probes before being fixed, and both are the same shape as the earlier bugs: a path handled on one branch and not its sibling. - justArrived was only dropped when the outro was still running, so a toggle whose PATCH fails after the outro finishes orphans the marker and the row replays its entrance animation on the next remount. Drop it unconditionally. - moveByKeyboard held a raw trigger node, so a concurrent remote edit that remounts the row mid-save left focus stranded on <body>. Re-query by item id, the same way ListItem#captureFocusFromRow already does. - Tab out of an open menu walked past the portal into nothing: focus on <body>, menu still open, its backdrop swallowing every click. Close on Tab and keep the items out of the tab sequence (APG). Menu had no tests; it has three now. - Toasts each mounted their own role="status" together with its text, which is the unreliably-announced pattern — and they are the only channel for reorder failures. Move the live region to the always-mounted host. Also: two comments still called the ⋯ trigger mobile-only, one described the fade-out as keyboard-specific when the scrim makes it happen for pointers too, the opacity-not-display rationale was stated in four places (the CSS is where re-adding display:none would be tempting, so it keeps it), and sourceControlIn's last fallback was unreachable — every focusable control in a row carries a data-testid.
npm audit --audit-level=high went red on a fresh advisory, with no code change on our side: fast-uri <3.1.2 host confusion via failed IDN canonicalization (GHSA-4c8g-83qw-93j6). Both bumps are patch-level and within the declared ranges, so this is what a fresh install would resolve to anyway; hono 4.12.31 also clears three moderate advisories and supersedes dependabot #6. Only the dev-only esbuild/drizzle-kit chain remains, at moderate, with no non-breaking fix — below the gate's threshold. This is exactly the case the audit step's comment describes, and the reordering earned itself: npm run check ran and reported green on the same CI run instead of being short-circuited by the audit.
johannesjo
force-pushed
the
task/beautify-05-task-interactions
branch
from
July 21, 2026 20:30
c0247f0 to
d329fec
Compare
johannesjo
marked this pull request as ready for review
July 21, 2026 20:38
Moving role="status" off Toast and onto the Project toast host silenced this one, which renders a Toast outside that host. Its own host was inside the <Show>, so putting the role there would recreate the problem the move was meant to fix — a live region inserted in the same tick as its content is unreliably announced. Hoist the host out instead, so the region is always mounted and only its content changes.
Prevent Enter's default action from reopening the reminder picker after focus returns to its trigger. Update the assignee regression to assert the identity-preserving row behavior directly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
npm run checkbefore the audit gate in CI, so typecheck/lint/format actually reportWhy
Pointer reordering had no equivalent keyboard path, and asynchronous task updates could race animations or leave focus in an unstable position.
Keeping focus through a reorder
Worth recording, because the first two attempts at this were wrong.
Keyboard reorder is only reachable through the ⋯ menu, and desktop CSS used to
display: nonethat trigger unless the row was reorderable. An unrenderedelement cannot hold focus, so the trigger vanished out from under the focus the
closing menu had just returned to it.
The first fix kept the move actions mounted while a save was in flight, so the
trigger could not lose its
.reorderAvailablemarker mid-interaction. That wasnecessary but not sufficient, and the invariant was not holdable anyway: the
marker derives from
props.items, so a collaborator's SSE echo could drop it.This PR deletes the
display: nonerule instead of defending it..moreButtonalready sits in the opacity-reveal group, whose comment states the rule that was
being violated — buttons stay in layout, hit-testable, and focusable. That
removes
canReorder(), the.reorderAvailableclass, and the failure mode.Focus still has to be handed back explicitly, because the optimistic store write
reorders the row synchronously and Solid's
<For>re-inserts it — re-parenting anode blurs its focused descendant. It is restored on both edges of the
commit: after the optimistic write, and again after the response, because a
failure rolls the position back and that second write re-parents the row again.
Failures are reported by the toast alone.
Toastis itselfrole="status", soalso writing the live region read every failure to a screen reader twice.
Impact
Tasks can be reordered from the keyboard, save failures are reported and rolled
back, and focus stays on the row being manipulated throughout — including when
the save fails.
Desktop rows that were previously not reorderable (done rows, resting recurring
rows, single-item lists) now show the ⋯ trigger, revealed on hover/focus like
the four buttons beside it. That is one extra tab stop on those rows, and the
cost of the trigger never being able to disappear under focus.
Validation
restore is removed
The reorder tests previously mocked the store's
moveItemas a no-op, so no rowever moved and focus was never at risk — which is how the original bug survived
the whole suite. The mocks now reposition for real, so future reorder tests
exercise the real DOM movement by default rather than opting in.
Follow-ups
Deferred findings from review are tracked separately: the module-scope
reorderPendinghaving no release path, memo identity churn rebuilding openmenus, the touch-only
display: nonerule carrying the same latent bug this PRremoves, and the duplicated delete-with-undo handler.
Base:
main(the rest of the beautify stack has landed).