Give the app a focus state, a brand selection, and honest reduced motion - #52
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes Limit details: You’ve used all 3 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe stylesheet adds focus indicators, custom selection colors, scrollbar styling, and reduced-motion overrides. It also defines a dedicated focus color and disables smooth scrolling for reduced-motion users. ChangesAccessibility and interaction styling
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to This localized stylesheet change adds focus visibility, selection styling, reduced-motion handling, and scrollbar states without changing component behavior; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Replaced the screenshots — the first set was bad evidence, and the before/after read as identical. Thanks for the nudge. My capture harness froze CSS animations with The CSS in this PR did not change. Only the photograph did: max per-pixel delta between before and after went from 75 to 407, which is what |
… motion Four foundations the design system was missing. All of it lives in src/styles.css with no component changes, so it composes with everything currently in flight. FOCUS. Nine components clear the UA outline with `outline-none`, and nothing anywhere put a ring back — there is not one `focus-visible:` utility in the app. Measured in the running app, all eight text fields focused invisibly: the composer, sidebar search, the answer box on an approval card, both onboarding fields, and all three API key fields. A keyboard user filling in App Settings could not tell which key they were typing into. Buttons kept the browser default, which is legible but reads as an unstyled page inside a hand-tuned dark UI. The rules are deliberately unlayered. Tailwind ships utilities in @layer utilities and unlayered styles outrank every layer, so this beats `outline-none` with no !important and without editing the nine components — every one of which has an open PR against it. Verified: a mouse click still paints nothing, Tab paints a 2px accent ring. The generic ring sets no radius, because an outline already follows the element's own corners and 26 controls here are `rounded-full`. Text fields are the exception: most carry a radius already, but the composer input has none, so its ring came out a hard rectangle inside a pill. SELECTION. ::selection was never styled, so dragging over a bot's reply painted Chrome's default blue through a palette that was pixel-sampled off the real app. It now tints the accent. REDUCED MOTION. The mascot was the only thing honouring prefers-reduced-motion. Panels still flew in, cards still popped, and every hover transition still ran. Now the app stills: panel-in 0.24s and pop-in 0.2s both collapse to 0.01ms under reduce, and normal playback is untouched without it. Durations collapse rather than `animation: none`, because a spinner frozen mid-turn reads as a hung app. The scrollbar thumb also gets a hover state; it was inert. No palette values changed, no radii tokens, no type. Nothing here restyles what was already designed — it fills in what had no styling.
47fbc1c to
e9880b7
Compare
|
Rebased onto Rebase was clean and the diff is unchanged at +70/−0 in
|
milind-soni
left a comment
milind-soni
left a comment
There was a problem hiding this comment.
The focus, selection, and scrollbar work looks good. The broad reduced-motion rule needs one correction before merge: setting animation-duration: 0.01ms on infinite animations leaves them infinite and makes bounce/spin animations advance thousands of cycles per frame, producing jitter/flicker rather than reduced motion. Please cap iteration count for motion animations or give continuous progress indicators an explicit reduced-motion state instead of globally accelerating them.
|
Reviewed against current main, and the focus work was measured in the real runtime (Electron 43 / Chromium 150) rather than read. The blocker is that the premise doesn't hold: Chromium always matches That's fixable — scope the ring so it doesn't fire for pointer focus on text widgets, or accept the ring deliberately and keep the existing radii — but as written it restyles focus for every mouse user, which isn't what the PR says it does. The reduced-motion half is sound. Happy to re-review if you rework it; a quick way to check is to click each of those four widgets in the packaged app and compare computed |
milind-soni
left a comment
milind-soni
left a comment
There was a problem hiding this comment.
Re-reviewed on current main. Text controls now keep their existing radii and intentionally show an inset focus ring for both pointer and keyboard focus. Reduced-motion mode stops spatial animation and uses a slow opacity pulse for loading indicators instead of accelerating infinite animations. Full local tests, production build, packaged-server smoke, and Electron syntax checks pass.
|
Both corrections were right, and thanks for fixing them in place rather than bouncing the PR. Reduced motion. You were right that this was concrete, not theoretical — at the reviewed commit there were 16 infinite animations outside Focus. Also right, and my evidence should have caught it: my "mouse click paints nothing" check clicked a The |
2 participants
The app has no focus state
outline-noneappears in nine components. Nothing anywhere puts a ring back — there is not a singlefocus-visible:utility insrc/. I ran the app and focused every text field in it:Message …)Eight of eight. A keyboard user pasting keys into App Settings cannot tell which of the three fields they are typing into.
In both shots below, the lower field (
ak_…) is focused — inbefore, the caret is the only thing that says so, and it is indistinguishable from the unfocused field above it.before
after
Buttons do keep the browser default ring, so this is not "the app is unusable by keyboard" — it is that a hand-tuned dark UI falls back to Chrome's stock halo on buttons and to nothing at all on inputs.
The composer, focused, before and after:
How it lands without touching those nine files
Every one of the nine components has an open PR against it —
Composer.tsxalone is in seven. So none of them are edited here.The rules are unlayered. Tailwind ships its utilities inside
@layer utilities, and unlayered styles outrank every layer regardless of specificity, so a plain:focus-visiblerule instyles.cssbeatsoutline-nonewith no!importantand no component change. Verified in the running app:That is
:focus-visibledoing its job: pointer users never see a ring, keyboard users always do.The generic ring sets no
border-radiuson purpose — an outline already follows the element's own corners, and 26 controls insrc/arerounded-full, which a fixed radius would square off. Text fields are the one exception: most already carry a radius, but the composer input has none, so its ring came out as a hard rectangle sitting inside a pill.Three smaller gaps in the same file
::selectionwas never styled. Dragging across a bot's reply painted Chrome's default blue straight through a palette that was pixel-sampled off the real app. It now tints--color-accent.prefers-reduced-motiononly ever reached the mascot. Panels still flew in, cards still popped, every hover transition still ran. Measured, before and after:animate-panel-inanimate-pop-inDurations collapse rather than
animation: none, because a spinner frozen mid-turn reads as a hung app..maus-motionkeeps its stronger existing rule — it has higher specificity, so the mascot still stops dead rather than ticking.The scrollbar thumb was inert — no hover state. It has one now, plus a corner rule for the horizontal track.
Scope
One file, +70 lines, 0 deletions, no component changes.
No palette value changed, no radius token, no type. This does not restyle anything that was already designed — it fills in the states that had no styling at all.
I checked this against all 26 open PRs by exact path:
src/styles.cssis touched by none of them, so this should merge clean regardless of what lands first.pnpm typecheckcleanpnpm test52 passed / 33 skipped, unchanged frommainpnpm exec vite buildsucceeds; CSS bundle 40.43 kB → 41.11 kB, gzipped 8.61 kB → 8.77 kB (+0.15 kB)Verified on
Windows 11 / Chrome 141, driving the real app against a throwaway
~/.openmausbot. Not verified on macOS — I do not have a machine, and the focus ring is plain CSS so I would not expect divergence, but the composer ring in particular is worth a glance from someone on the packaged app.Summary by CodeRabbit
Accessibility
Style