fix: design-system audit — motion, interaction states, focus & accessibility#856
Open
ravisuhag wants to merge 61 commits into
Open
fix: design-system audit — motion, interaction states, focus & accessibility#856ravisuhag wants to merge 61 commits into
ravisuhag wants to merge 61 commits into
Conversation
Compose the shared easing, duration, pressed-scale and focus-ring tokens so components stop hand-rolling their own timings and curves.
Step body letter-spacing and h1-h4 line-heights so they change monotonically across sizes and follow Inter's own tracking curve.
Give numeric columns tabular figures so digits align, alongside the shared row-selection and sticky-header styling that lives in the same two stylesheets.
A slightly faster spinner raises perceived load speed. Recomputed all 8 pole animation-delays to keep the same 1/8-cycle stagger ratio at the new duration, so the sweep still rotates evenly.
Centering used translateX(-50%), which would collide with any transform-based entrance. Move centering to inset+margin-inline:auto (safe since the root is width:fit-content) so transform is free for motion. Add a fade-in for everyone, plus an 8px rise from the anchored edge under prefers-reduced-motion: no-preference. Exit stays instant (no mount controller exists to hold the node past removal).
The error handler swapped src to the fallback with no re-entry guard -- if the fallback URL itself 404s, onError fires again and re-assigns the same failing src, looping requests indefinitely. Track a one-shot ref so the fallback is only ever assigned once; a second failure falls through to showing the alt/broken rendering instead. Both Image and Avatar's photo popped in abruptly with no load transition. Image now tracks load state via a useLayoutEffect that checks node.complete on mount/src-change -- already-decoded (cached or SSR-painted) images stay visible with no fade; genuinely loading images fade in over 150ms on load. Avatar's image only mounts once Base UI's probe reports loaded, so its fade is a one-shot keyframe animation, skipped entirely for cache-fast loads (<100ms between loading and loaded) so a quick load isn't artificially delayed. Reduced motion shows both instantly at full opacity, never invisible.
A display:block heading already fills its container in normal flow; the explicit width:100% only mattered in flex/grid contexts, where it made a heading claim the full row/track and push siblings around. Ellipsis truncation is the only variant that needs an explicit width, and it already declares its own -- removing the base rule loses nothing there. BREAKING (mild): consumers relying on the base width:100% inside flex/ grid parents (e.g. a centered heading, or a heading used as a full-row flex child) will see it shrink to content width. One-line fix on their side: set width:100% via className where the old behavior was intentional.
The action button was the bar's only interactive element but had no hover, press, or focus feedback. Add opacity-step hover/press states and a currentColor focus ring that stays visible against the bar's coloured backgrounds.
…hiding it Both variants hid the shared Tabs.Indicator and painted the active state directly on the trigger, so switching tabs snapped instantly instead of gliding like the default segmented variant. The indicator already tracks the active tab via Base UI's --active-tab-* variables for free -- hiding it threw that away. Restyle the indicator per variant instead: standalone becomes the active chip fill (background + border) gliding between tabs, with the active trigger going transparent so the pill shows through (the same trick the segmented variant already uses); plain becomes a 1px underline sliding along the bottom edge, positioned via the trigger's own --active-tab-height so it lines up with the reserved border-bottom exactly. Both inherit the indicator's existing --rs-duration-moderate/--rs-ease-in-out transition, gated behind reduced motion, with no new media query needed.
Breadcrumb links changed colour instantly on hover while the rest of the library eases. Add a short colour transition, gated behind prefers-reduced-motion, so the hover matches every other interactive element.
Popover -- a larger floating surface than the tooltip -- cast a lighter shadow (--rs-shadow-soft) than the tiny tooltip (--rs-shadow-lifted), so elevation read backwards when both were on screen. Match menu, a peer surface of similar size, which already uses --rs-shadow-lifted.
overflow: scroll forced a permanently visible scrollbar gutter on platforms with classic scrollbars, even when content didn't overflow. Use auto. Also document via JSDoc that SidePanel intentionally has no enter/exit animation -- it's persistent layout chrome like Sidebar, not an overlay.
The scrollbar grew its whole track on hover, reflowing the page each time, and the thumb never showed it was grabbed. Keep the track a constant, invisible width so layout never shifts; draw the thumb thinner at rest and grow it to full width on hover; and deepen its colour one step on hover and again while dragging.
The positioner transitioned top/left/right/bottom and the popup transitioned width/height on every open -- but Base UI only writes inset positioning and drives --popup-width/--popup-height when a PreviewCard.Viewport child triggers its adaptiveOrigin mode. In the common case (no Viewport), Base UI positions purely via transform, so the four inset transitions were dead weight forcing unnecessary transition-list overhead on every reposition. Split into two rulesets: the default path transitions transform + opacity only (compositor-only), while `:has(.viewport)` keeps the inset/size transitions where they're the actual morph mechanism.
A sticky header drew its bottom hairline even at rest, before anything had scrolled beneath it. Drive the hairline from a scroll-linked animation so it fades in over the first bit of scroll, gated behind a real scroll container so the plain table keeps its static separator.
Resting day cells used radius-2 while hover/selected/range-cap states all used radius-5, so sweeping the pointer across the grid made each cell's shape pop between rounded-square and pill. The radius-2 resting state was never actually visible (the cell's background matches the calendar root's), so unify on radius-5 -- zero visible states change, one invisible one gets fixed. Hover/selection highlights stay instant, per the audit's explicit rejection of transitions on direct-manipulation surfaces. Loading hard-swapped the whole table for a differently-sized Skeleton in one frame, flashing on fast loads and jumping the popover height. Keep the table mounted and overlay the skeleton, cross-fading via opacity/visibility (~150ms, instant under reduced motion) so the grid never disappears and the height never moves.
Dimming a link to 80% opacity on hover is a weak signal and makes the text harder to read at the exact moment the user is targeting it. The underline geometry now always exists (text-decoration-color: transparent at rest) so hover is a pure color fade to currentColor -- zero layout shift, works for every Text color variant automatically. Press gets a brief variant-agnostic dim (:active, opacity 0.7) instead of a hardcoded color, since a fixed active color would clobber non-accent variants. Timing moves from a hardcoded 0.2s to --rs-duration-fast.
Proportional digits meant amounts jittered as values changed and misaligned in table columns. Add a CSS module with font-variant-numeric: tabular-nums as the default; className/style still merge through as the opt-out escape hatch.
The floating copy button had no transition of its own duration (it inherited the interactive tier's opacity fade), so its reveal moved at hover-feedback speed instead of a dedicated reveal timing. Re-compose the IconButton base transition and override just the opacity entry with --rs-duration-fast/--rs-ease-out. The :focus-visible reveal rule (added by the earlier focus-indicators pass) is merged into the same selector as the hover reveal for clarity.
Row selection was wired up in data-table and data-view-beta (both set
data-state="selected") but no CSS anywhere styled it -- selecting a
row changed nothing on screen. Add an accent-subtle background for
selected rows (wins over hover/pressed) and a one-step-darker pressed
background for clickable rows, instant (no transition, matching the
house rule for direct-manipulation highlights like menu items).
Virtualized cells painted an opaque background that covered the row's
color entirely -- add `.virtualRow .virtualCell { background: inherit }`
so selection/hover/press show through in virtualized mode, same as the
non-virtualized cells already do.
data-view (no selection support) gets the same pressed-row treatment.
The bare Table primitive had no row-state hooks at all. Table.Row now
accepts an `interactive` prop (cursor + hover + pressed) and responds
to a consumer-set data-state="selected", with cells inheriting the row
background through the opaque .cell rule.
…bled cursor Hovering an off toggle looked identical to the on state -- the control lied about whether it was selected. Give hover a lighter tint and pressed (Base UI's toggled-on state) a deeper, inset-shadowed look in both standalone and group mode; hovering an already-on toggle now deepens it one more step instead of reverting to the hover tint. The content background and inset shadow had no transition at all (only color did), so state changes snapped. Add a fast-tier transition for both. pointer-events: none on disabled toggles suppressed the declared cursor: not-allowed, so disabled toggles gave zero affordance. Restore pointer-events so the cursor shows, with explicit hover neutralizers so a disabled-but-off toggle doesn't pick up hover styling (clicks stay blocked natively via the disabled <button>).
The select positioner painted an opaque square that flashed behind the popup as it scaled, and multi-select options nested a real focusable checkbox inside each row, adding a stray tab stop. Drop the positioner background and use a presentational check indicator; the trigger shows a static up/down caret to signal it opens a menu.
A chip with onClick rendered as a <span onClick>: no keyboard activation, not in the tab order, no focus ring, and cursor: default even though it was clickable. Render a <button type="button"> when onClick is passed and the chip isn't dismissible (a button containing the dismiss button would be invalid nested-interactive HTML, so dismissible chips keep the span). Interactive chips get cursor: pointer, inherit the app font, and a focus-visible accent ring.
The Field error popped in with no transition and replaced the description in one jarring jump, and the DatePicker set an error that was never shown. Cross-fade the error in over the description so the swap is smooth, and render the DatePicker error as reserved-space helper text below the input.
…es in The 2D area thumb was positioned via left/top on every pointermove, forcing layout on each frame of a drag. Drive it from two CSS custom properties (--thumb-x/--thumb-y, unitless 0..1 fractions set from JS) composed with container query units and the existing -50% self-centering transform -- compositor-only, no transition on transform so the drag stays 1:1 with the pointer. Also add touch-action: none / user-select: none to .selectionRoot (the sliders already had this; the 2D area didn't, so dragging on touch also scrolled the page), and a fast opacity transition on the thumb so its first-render reveal fades instead of popping.
.nav-text declared an opacity transition, but the collapsed state set display: none, which can't transition and short-circuited it -- labels popped out of existence while the frame animated its width. Same for any consumer content marked data-collapse-hidden. Replace display:none with opacity + visibility (visibility keeps collapsed content out of the accessibility tree and unfocusable, same intent as display:none, but participates in the transition). Also fix the resize handle's cursor: it only click-toggles collapse, but showed ew-resize, promising a drag that doesn't exist. Use pointer on both left- and right-positioned sidebars (per the audit's explicit decision not to implement drag-to-resize).
Uncontrolled dismiss returned null on the next render, so the callout vanished in one frame and content below it snapped up by its full height. There was no enter transition either. Wrap the callout in a transition shell driven by a data-state attribute (open/closing/closed). Dismiss now plays a 200ms fade + grid-row height collapse before unmounting, so content below glides up instead of snapping. Mount fades in over the same duration. Reduced motion is instant both ways. The controlled API (onDismiss provided) is unchanged -- the consumer still owns removal.
Tooltip animated with one-shot @Keyframes: not interruptible (restarts from zero instead of retargeting), no exit at all (popup unmounted instantly), a dead unused fade keyframe, and a dead transform-origin (every keyframe was a pure translate, origin-independent). Replace with [data-starting-style]/[data-ending-style] transitions at --rs-duration-fast/--rs-ease-out, keeping the exact per-side and per-align 2px directional offsets the keyframes used. Enter and exit now mirror each other, rapid hover toggling retargets smoothly instead of restarting, data-instant still skips the animation via transition-duration: 0, and reduced motion gets an opacity-only fade (previously nothing, since every rule lived inside the no-preference media query).
Four independent gaps: - Toast starting-style set a transform but no opacity: 0, so under reduced motion (where the transform never transitions) toasts popped in at full opacity instead of fading. - Navbar's hide-on-scroll transform had no reduced-motion fallback at all, so the bar teleported in/out; add an additive `reduce` block that fades it instead. - Spinner poles had no static opacity, so reduced motion (where the spin animation never applies) rendered a static asterisk instead of a graded ring that still reads as loading. - Dialog and command's backdrop blur had no prefers-reduced-transparency guard; add one with a heavier solid scrim (alert-dialog shares dialog's stylesheet, so it's covered too).
…uced motion The popup kept its 450ms transform transition active while swiping, so it eased toward the finger instead of tracking it directly (the backdrop already zeroed its own transition). Add transition: none on [data-swiping]. The five swipe-strength exit calcs hardcoded 400ms while enter runs 450ms via --rs-duration-drawer -- non-swipe exits ran measurably faster than opens for no reason. Use the token so enter/exit match and retuning the drawer duration doesn't require finding five literals. The panel had no transition outside the motion media query, so under prefers-reduced-motion it popped open/closed instantly. Add a base opacity fade (with matching [data-starting-style]/[data-ending-style] opacity: 0) so reduced motion gets a fade instead of a teleport, and move the per-side slide transforms into the no-preference block alongside the transition they animate.
The code-block language selector renders a Select, whose trigger now uses CaretSortIcon; add it to the test's @radix-ui/react-icons mock.
…README - Gate a handful of ungated transforms/animations behind prefers-reduced-motion so reduced-motion users get a plain fade like the rest of the library: dialog nested-scale, empty-state entrance, preview-card morph, sidebar chevron, and the tour cards. - Remove dead and redundant rules across components: unused classes (menu/meter/progress/tooltip), no-op hover/focus/disabled overrides, duplicated declarations, and a stale commented-out block. - Consolidate the duplicated light-theme shadow tokens into one grouped selector so they cannot drift. - Rewrite the package README.
Two sides of the same house rule: direct-manipulation highlights are instant, single controls ease. .nav-item eased its hover/press background through --rs-transition-interactive. The ease came from main (tokenized in 1915309) and made the sidebar the only pointer-following list whose highlight trailed the cursor -- menu items, select options, command items, and table rows are all instant. Drop the transition. The More trigger composes .nav-item, so it now matches the popup items it opens, which were already instant. The accordion trigger is the opposite case: a single full-width control whose hover/press background flipped instantly while the same component eases its chevron and panel. Give it the shared interactive token inside the file's existing reduced-motion block, matching the button pattern (plan 12, accordion section). Plan 12's other targets were dropped: calendar day cells are a sweep surface (instant, per the rule above), and the sidebar More item and group header only needed to match .nav-item -- which is now instant.
Select.Item hand-rolled its multi-select indicator: a styled span plus
a local copy of the checkmark svg, conditionally mounted -- so the fill
snapped between white and accent and the checkmark popped in and out
with no fade. Combobox, the sibling popup surface, renders the real
Checkbox inside its items and gets the transitions, keep-mounted
fade-out, and reduced-motion handling for free.
Mirror combobox exactly: a Checkbox with checked={state.selected} and
no onCheckedChange (fully controlled and inert; the click bubbles to
the item, which owns selection), delete the local CheckMarkIcon (its
path was byte-identical to Checkbox's), and drop the three
.checkIndicator rules. Checkbox's default large size is the same
--rs-space-5 box with the same tokens, so rows do not shift.
This deliberately reverses 684eade, which had replaced the nested
Checkbox with the presentational span to avoid a stray tab stop. In
practice the checkbox's tabIndex={0} span is not reachable -- the popup
manages focus on the options and Tab closes it -- and combobox has
shipped the nested Checkbox all along. Consistency between the two
surfaces wins; tightening the nested-checkbox semantics in both at
once stays open as a follow-up (plan 15).
Tests: multi-select options contain exactly one checkbox whose
aria-checked follows selection, and a click fires onValueChange
exactly once.
The floating copy button's icon box was 12x16 (--rs-space-4 wide, --rs-space-5 tall). Radix icons use a square viewBox, so the glyph rendered ~1.33x taller than wide on every floating copy button. Use --rs-space-5 for both sides. Keeping the 16px height means the pill's height and layout do not change, and 16px is the natural glyph size of the size-2 IconButton that CopyButton renders, so the floating variant stops fighting the component's own sizing (plan 16).
dcaaa30 gave the DatePicker a reserved-space error line below its input (invisible until an error shows) so appearing errors never shift the layout under the field. Standalone that is right, but FilterChip embeds the DatePicker inline in a 24px chip row: the invisible line inflated the date-field wrapper to 38px, and flex centering pushed the input ~7px above the chip's centerline -- "Select date" poked out over the chip border next to a correctly centered label and operator. Hide the error line inside the chip's date wrapper, same as the chip already strips Input helper text there. Date errors inside chips keep their existing signal: the danger border on the input wrapper.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/raystack/components/headline/headline.module.css (1)
31-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming physical class names to logical ones.
Since the alignments are now using logical properties (
startandend) for better RTL support, you might want to rename the utility classes from.headline-align-leftand.headline-align-rightto.headline-align-startand.headline-align-end. This avoids confusion where a-leftclass visually aligns text to the right in an RTL context.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raystack/components/headline/headline.module.css` around lines 31 - 41, Rename the alignment utility classes from .headline-align-left and .headline-align-right to .headline-align-start and .headline-align-end, while preserving their existing text-align values. Update all references to these classes so the public utility names consistently describe the logical alignment behavior.packages/raystack/styles/typography.css (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse string notation for the
@importURL.The static analysis tool Stylelint flags the use of
url("...")for this import. Prefer using the direct string notation to resolve theimport-notationlinter error.♻️ Proposed fix
-@import url("https://fonts.googleapis.com/css2?family=JetBrains+Mono:ital,wght@0,400;0,500;1,400;1,500&display=swap"); +@import "https://fonts.googleapis.com/css2?family=JetBrains+Mono:ital,wght@0,400;0,500;1,400;1,500&display=swap";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raystack/styles/typography.css` at line 4, Update the font `@import` declaration in typography.css to use direct quoted-string notation for the Google Fonts URL instead of the url() wrapper, preserving the existing URL and import behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/raystack/components/announcement-bar/announcement-bar.module.css`:
- Around line 62-64: Update the .action-btn:active pressed-state styling so its
scale transform is disabled when prefers-reduced-motion is reduce, while
preserving the existing transform for users with no motion preference. Apply the
same reduced-motion handling to the related styles referenced at lines 71–78.
In `@packages/raystack/components/data-view/components/list.tsx`:
- Around line 357-363: Update handleRowKeyDown to ignore bubbled keydown events
unless event.target is the row element itself, before handling Enter or Space.
Preserve the existing preventDefault and handleRowActivate behavior for direct
row keyboard activation.
---
Nitpick comments:
In `@packages/raystack/components/headline/headline.module.css`:
- Around line 31-41: Rename the alignment utility classes from
.headline-align-left and .headline-align-right to .headline-align-start and
.headline-align-end, while preserving their existing text-align values. Update
all references to these classes so the public utility names consistently
describe the logical alignment behavior.
In `@packages/raystack/styles/typography.css`:
- Line 4: Update the font `@import` declaration in typography.css to use direct
quoted-string notation for the Google Fonts URL instead of the url() wrapper,
preserving the existing URL and import behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 986853d4-b855-4252-befa-8e6315664446
📒 Files selected for processing (48)
packages/raystack/README.mdpackages/raystack/components/accordion/accordion.module.csspackages/raystack/components/announcement-bar/announcement-bar.module.csspackages/raystack/components/avatar/avatar.module.csspackages/raystack/components/button/button.module.csspackages/raystack/components/calendar/calendar.module.csspackages/raystack/components/checkbox/checkbox.module.csspackages/raystack/components/code-block/__tests__/code-block.test.tsxpackages/raystack/components/code-block/code-block.module.csspackages/raystack/components/color-picker/__tests__/color-picker.test.tsxpackages/raystack/components/color-picker/color-picker-area.tsxpackages/raystack/components/color-picker/color-picker.module.csspackages/raystack/components/combobox/combobox.module.csspackages/raystack/components/command/command.module.csspackages/raystack/components/data-view/__tests__/data-view.test.tsxpackages/raystack/components/data-view/components/list.tsxpackages/raystack/components/data-view/data-view.module.csspackages/raystack/components/dialog/dialog.module.csspackages/raystack/components/drawer/drawer-content.tsxpackages/raystack/components/drawer/drawer.module.csspackages/raystack/components/empty-state/empty-state.module.csspackages/raystack/components/field/field.module.csspackages/raystack/components/filter-chip/filter-chip.module.csspackages/raystack/components/headline/headline.module.csspackages/raystack/components/icon-button/icon-button.module.csspackages/raystack/components/image/image.module.csspackages/raystack/components/input/input.module.csspackages/raystack/components/menu/menu.module.csspackages/raystack/components/meter/meter.module.csspackages/raystack/components/popover/popover.module.csspackages/raystack/components/preview-card/preview-card.module.csspackages/raystack/components/progress/progress.module.csspackages/raystack/components/radio/radio.module.csspackages/raystack/components/search/search.module.csspackages/raystack/components/select/__tests__/select.test.tsxpackages/raystack/components/select/select.module.csspackages/raystack/components/sidebar/__tests__/sidebar.test.tsxpackages/raystack/components/sidebar/sidebar.module.csspackages/raystack/components/tabs/tabs.module.csspackages/raystack/components/text-area/text-area.module.csspackages/raystack/components/theme-provider/__tests__/theme.test.tsxpackages/raystack/components/theme-provider/switcher.tsxpackages/raystack/components/tooltip/tooltip.module.csspackages/raystack/components/tour-beta/tour.module.csspackages/raystack/components/tour/tour.module.csspackages/raystack/styles/effects.csspackages/raystack/styles/primitives/accent.csspackages/raystack/styles/typography.css
💤 Files with no reviewable changes (9)
- packages/raystack/components/tour-beta/tour.module.css
- packages/raystack/components/checkbox/checkbox.module.css
- packages/raystack/components/radio/radio.module.css
- packages/raystack/components/field/field.module.css
- packages/raystack/components/progress/progress.module.css
- packages/raystack/components/tabs/tabs.module.css
- packages/raystack/components/meter/meter.module.css
- packages/raystack/components/select/select.module.css
- packages/raystack/components/command/command.module.css
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/raystack/components/accordion/accordion.module.css
- packages/raystack/components/code-block/code-block.module.css
- packages/raystack/components/combobox/combobox.module.css
- packages/raystack/components/text-area/text-area.module.css
- packages/raystack/components/avatar/avatar.module.css
- packages/raystack/components/dialog/dialog.module.css
- packages/raystack/components/preview-card/preview-card.module.css
- packages/raystack/styles/effects.css
- packages/raystack/components/popover/popover.module.css
- packages/raystack/components/data-view/data-view.module.css
- packages/raystack/components/calendar/calendar.module.css
- packages/raystack/components/tour/tour.module.css
- packages/raystack/components/color-picker/color-picker.module.css
- packages/raystack/components/drawer/drawer.module.css
- packages/raystack/components/sidebar/sidebar.module.css
- packages/raystack/components/tooltip/tooltip.module.css
Comment on lines
+62
to
+64
| .action-btn:active { | ||
| transform: scale(var(--rs-scale-pressed)); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Suppress the pressed transform for reduced-motion users.
The scale transform is outside the prefers-reduced-motion: no-preference block, so users requesting reduced motion still receive the press animation. Gate the transform itself or override it under prefers-reduced-motion: reduce.
Proposed fix
-.action-btn:active {
- transform: scale(var(--rs-scale-pressed));
-}
-
`@media` (prefers-reduced-motion: no-preference) {
.action-btn {
transition: var(--rs-transition-interactive);
}
.action-btn:active {
+ transform: scale(var(--rs-scale-pressed));
transition: var(--rs-transition-pressed);
}
}Also applies to: 71-78
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/announcement-bar/announcement-bar.module.css`
around lines 62 - 64, Update the .action-btn:active pressed-state styling so its
scale transform is disabled when prefers-reduced-motion is reduce, while
preserving the existing transform for users with no motion preference. Apply the
same reduced-motion handling to the related styles referenced at lines 71–78.
Comment on lines
+357
to
+363
| const handleRowKeyDown = (event: KeyboardEvent<HTMLDivElement>) => { | ||
| // Enter and Space both activate, matching a native button. | ||
| if (event.key === 'Enter' || event.key === ' ') { | ||
| event.preventDefault(); // Space would otherwise scroll the page. | ||
| handleRowActivate(); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Ignore bubbled keydown events from interactive children.
If the row contains interactive elements (e.g., checkboxes for selection or buttons for row actions), keyboard events from those elements will bubble up to the row. Because native inputs and buttons do not stop propagation for onKeyDown, pressing Enter or Space on a child element will trigger the row's onRowClick and call preventDefault(), which breaks the child element's native keyboard activation.
You can safely ignore these bubbled events by ensuring the keydown target is the row itself.
🛠️ Proposed fix
const handleRowKeyDown = (event: KeyboardEvent<HTMLDivElement>) => {
+ // Ignore events bubbling up from interactive child elements (e.g. selection checkboxes)
+ if (event.target !== event.currentTarget) return;
+
// Enter and Space both activate, matching a native button.
if (event.key === 'Enter' || event.key === ' ') {
event.preventDefault(); // Space would otherwise scroll the page.
handleRowActivate();
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleRowKeyDown = (event: KeyboardEvent<HTMLDivElement>) => { | |
| // Enter and Space both activate, matching a native button. | |
| if (event.key === 'Enter' || event.key === ' ') { | |
| event.preventDefault(); // Space would otherwise scroll the page. | |
| handleRowActivate(); | |
| } | |
| }; | |
| const handleRowKeyDown = (event: KeyboardEvent<HTMLDivElement>) => { | |
| // Ignore events bubbling up from interactive child elements (e.g. selection checkboxes) | |
| if (event.target !== event.currentTarget) return; | |
| // Enter and Space both activate, matching a native button. | |
| if (event.key === 'Enter' || event.key === ' ') { | |
| event.preventDefault(); // Space would otherwise scroll the page. | |
| handleRowActivate(); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/data-view/components/list.tsx` around lines 357
- 363, Update handleRowKeyDown to ignore bubbled keydown events unless
event.target is the row element itself, before handling Enter or Space. Preserve
the existing preventDefault and handleRowActivate behavior for direct row
keyboard activation.
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
A design-system-wide audit of the component library, consolidating the animation work and the broader interaction/craft pass into a single branch.
What's covered
:focus-visiblering across the library.styles/effects.css; Inter body tracking and the h1–h4 type ramp retuned.Related issues
Closes #681 — consistent, tokenized
:focus-visiblering applied across the library.Closes #621 — Image error/loading handling: one-shot fallback guard plus load-state fade-in.
Closes #625 — Link's remaining items (focus-visible, reduced-motion) are now done.
Partially addresses:
TableRownow uses CVA and gains row-state hooks. Cell truncation accessibility is still open.announementBartypo are still open.Test plan
pnpm buildpasses.@base-uimodule-resolution failure affects a handful of unrelated test files onmainas well.