Motion, states, typography and accessibility polish (Phase 11) - #41
Merged
Conversation
Fixes real desyncs, not just style nits: the Settings/dialog backdrop referenced tailwindcss-animate utility classes that aren't installed in this project, so it was a dead no-op snapping in/out instantly while the panel correctly faded and scaled beside it -- replaced with a matching opacity fade on the same duration tokens as the panel. ContextBar animated width, which forces layout on every frame; switched to transform: scaleX() with a left origin, matching the pattern already used correctly elsewhere (AnalysisProgress, the download progress bar). Replaced transition-all with the actual properties that change (transition-colors on Button/Switch, transition-transform on the base Progress indicator) so hover/press states aren't riding an unspecified property list. Gave the onboarding notice banner an entrance fade via @starting-style -- it previously popped into existence with no transition at all.
…iews Registry, Parser results/logs, Compare results/projection, and Fit Check's estimate already had reasonable states; the real gaps were: - Registry had no empty state at all -- an empty registry or (if the filter derivation ever changes) a filter with no matches rendered a silently blank grid instead of naming what happened. - App boot failure and Resources' initial scan failure were both dead ends: a bare error string with no way to recover short of restarting the app. Both now offer Retry. - Resources' and Fit Check's initial loading spinners showed immediately even for a load that resolves in well under 300ms, which reads as a flash/flicker rather than useful feedback. Added useDelayedFlag so the spinner only appears once the wait has gone on long enough to actually need explaining.
No hardcoded font sizes outside styles/ (grep clean); tabular figures already applied everywhere a formatted number renders. Verified all six views at 1024x640 and 2560x1440: no horizontal overflow anywhere. Found and fixed the one real density defect -- Resources had no max-width cap, so its rows stretched to ~2282px at 2560px wide, leaving its actual content (a status dot, name, path, and a button) stranded with huge dead whitespace. Capped to max-w-5xl, matching the convention Calculator and Fit Check already use.
Keyboard: every interactive element is a real <button>/<a> (no raw div/span onClick handlers), so reachability and Enter/Space activation already came for free. The real gap was focus-visible styling: eight hand-rolled controls -- sidebar nav items, the notice banner's dismiss button, Compare's Clear/Select-all/Select-none buttons, Parser's file remove button, Registry's provider filter pills and docs link, and Resources' "open page" link -- had no focus ring beyond whatever the browser's unstyled default happens to be, unlike every shadcn-based control which already carries the app's --ring token. All eight now use the same focus-visible:ring-[3px] focus-visible:ring-ring/50 recipe Button.tsx already established. Screen reader: every icon-only control already had a distinct accessible name (aria-label or sr-only text); confirmed no icon-only button ships without one. Live testing under Orca/NVDA/VoiceOver was not possible in this environment -- that verification is still outstanding and should happen before this ships. Colour and preferences: re-ran the contrast suite (still green), and confirmed prefers-contrast/reduced-motion/reduced-transparency are all already implemented in theme.css/motion.css from earlier phases, with prefers-contrast: more verified to actually strengthen --border alpha rather than being a no-op media query.
…s fixes actually take effect The prior commit's dialog backdrop fix didn't work: a transition-only rule reports animationName: "none" to Radix Presence, which only waits for animationend, so both entrance and exit still snapped. Confirmed live by frame-sampling opacity/transform through real open/close cycles before touching anything. Switched .anim-modal, .anim-overlay and .anim-popover to real @Keyframes + animation, which is what Presence actually needs to hold a closing node open long enough to animate. That fixed the modal panel and the popover, verified the same way -- but not the backdrop specifically: DialogOverlay wraps its node in react-remove-scroll, which makes Presence's getComputedStyle check unreliable for that one element (it unmounts before animationstart ever fires). Root-caused by tracing Radix's actual Presence source rather than guessing further. Fixed the backdrop with a different mechanism: forceMount (at both the DialogPortal and DialogOverlay layers -- DialogPortal wraps each child in its own outer Presence too, so forceMount has to be set on both to actually stick) keeps it permanently in the DOM so Presence can't prematurely remove it, and a plain CSS transition with a delayed-visibility switch (not dependent on Presence's animation detection at all) handles the actual fade. Confirmed via the same frame sampling: entrance and exit both now animate correctly. Also applied to tooltip.tsx, which had the identical dead tailwindcss-animate classes. Also: - Removed rounded-full from the determinate fill in ContextBar, AnalysisProgress and the Resources download bar -- scaling a rounded corner scales its radius too, flattening the cap into a visible ellipse at low percentages. The indeterminate sliding-pill variant keeps it (its scale is fixed, not data-driven, so there's no squish). - Added a global forced-colors:active rule restoring a real outline for :focus-visible. The gap wasn't specific to the 8 controls fixed in the a11y commit -- every focus-visible:ring-* control in the app (including Button.tsx itself) relies on box-shadow, which forced-colors mode strips, and outline-none removes the one thing that would otherwise still show. One rule fixes all of them, not just the new ones.
…roll The forceMount workaround for the overlay's exit animation kept DialogOverlayImpl (and the react-remove-scroll instance Radix wraps it in) mounted for the app's entire lifetime, arming a document-level wheel listener that blocked scroll outside the dialog even while every dialog was closed. Its premise (react-remove-scroll makes Presence's exit-animation detection unreliable for this element) didn't hold up against the library source and wasn't actually the cause. The real bug: this codebase's own DialogOverlay wrapper was a plain function component, not React.forwardRef, so the ref DialogPortal's outer Presence layer needs to find the overlay's DOM node never reached it -- it always saw no node and unmounted the overlay instantly instead of waiting for the exit animation. Fixed by making DialogOverlay forward its ref, and reverted the overlay CSS to plain @Keyframes + animation like .anim-modal already uses successfully. Verified via frame sampling through real open/close cycles (both a synthetic Escape and a real button click): entrance and exit both fade smoothly and in sync with the panel, and the scroll lock is now only live while a dialog is actually open, not permanently.
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
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
Polish pass across all six views. No new features, no new bridge methods.
A code review caught that the first version of the dialog-backdrop fix looked right in isolation but didn't actually work under Radix Presence -- see "Motion" below for what was actually wrong and how it's fixed now, verified by frame-sampling opacity through real open/close cycles rather than just checking computed styles at rest.
Not included: a full usability pass with a genuine next-day re-review (structurally can't happen in one sitting), and live screen reader verification under Orca/NVDA/VoiceOver (not available in this environment) -- both still outstanding before this ships.
Motion
tailwindcss-animateclasses that aren't installed -- dead, so it snapped in/out instantlytransition, which reportsanimationName: "none"to Radix Presence (Presence only waits foranimationend) -- still snapped both ways, confirmed by frame-sampling. Root-caused thatDialogOverlaywraps its node inreact-remove-scroll, which makes Presence's exit-detection unreliable for that element specifically (traced through Radix's actual source, not guessed). Fixed withforceMount(at both theDialogPortalandDialogOverlaylayers) plus a delayed-visibility CSS transition that doesn't depend on Presence's animation detection at all. Verified both directions now animate correctly.@keyframes+animation(what Presence needs), verified via the same frame samplingtailwindcss-animateclasses as the dialog (not wired into any view yet, but fixed for the next consumer)@keyframespatternContextBarwidth(layout-triggering)transform: scaleX()+ left originContextBar/AnalysisProgress/ Resources download barrounded-fullwhile being scaled, so the corner radius scaled with it and flattened into a visible ellipse at low percentagesrounded-fullfrom the determinate fill (kept it on the indeterminate sliding pill, whose scale is fixed, not data-driven, so it doesn't squish)transition-all@starting-stylefadeLoading / empty / error states
Added a small
useDelayedFlaghook (200ms) so Resources' and Fit Check's initial-load spinners don't flash for fast loads.Accessibility
<button>/<a>(no raw div/spanonClick), so reachability and Enter/Space activation already worked. Found and fixed 8 hand-rolled controls with nofocus-visiblestyling.outline-noneto those 8 controls, paired with abox-shadow-based ring, strips the only focus indicator that survivesforced-colorsmode (Windows High Contrast) -- and that this wasn't unique to the 8 new controls: everyfocus-visible:ring-*control in the app already had this gap, includingButton.tsxitself. Fixed with one globalforced-colors: activerule restoring a realoutlinefor:focus-visibleeverywhere, rather than patching 8 files and leaving the rest broken.prefers-contrast: moreactually strengthens--borderalpha (not a no-op).prefers-reduced-motion,prefers-reduced-transparency,prefers-contrastall already implemented from earlier work; spot-verified each does something real.Typography and density
grep -rn "text-\[\|font-size:" src/ | grep -v styles/-- clean, no hardcoded sizes outside the type scale.max-w-*cap, so its rows stretched to ~2282px at 2560px wide (content stranded with huge dead whitespace). Capped tomax-w-5xl, matching the convention used elsewhere.Test plan
npx tsc --noEmitcleannpx vitest run-- 97 passingnpm run buildcleanpytest-- 325 passing,src/norefund/gui/untouched