ReviewOS

stacks/stx

Skeleton animates through prefers-reduced-motion, and an unknown variant renders the literal string "undefined" as a class

#1995
Closed glennmichael123 opened this 9 days ago ·
9 days ago

packages/components — verified on 0.2.321.

Two things in src/ui/skeleton/Skeleton.stx, found while replacing our hand-rolled skeletons with it.

1. It animates regardless of prefers-reduced-motion

const animationClasses = animate ? 'animate-pulse' : ''

animate-pulse compiles to an unconditional rule — I checked the generated sheet in one of our apps:

.animate-pulse { animation: pulse 2s cubic-bezier(0.4, 0, 0.6, 1) infinite; }

and grepped the whole stylesheet: no prefers-reduced-motion guard anywhere. So a viewer who has asked their OS to reduce motion gets a pulsing placeholder anyway. For skeletons specifically this matters more than for most animation, because they cover the whole page during load and there can be a dozen of them pulsing at once.

The animate prop is not a substitute: it is a global on/off at the call site, not a response to the user's preference. Turning it off for everyone to protect the few is the wrong trade, and the caller has no way to ask "animate unless the user objects".

This is the one reason we cannot adopt Skeleton as a drop-in. Every hand-rolled skeleton in our apps already guards it:

.skeleton { animation: pulse 1.5s ease-in-out infinite; }
@media (prefers-reduced-motion: reduce) { .skeleton { animation: none; } }

so swapping to the component would be an accessibility regression for us, not an improvement. WCAG 2.3.3, and it is the group of users for whom motion is a physical symptom rather than a preference.

Suggested fix — emit the motion-safe variant instead, which is one character of change and needs no new prop:

const animationClasses = animate ? 'motion-safe:animate-pulse' : ''

I confirmed crosswind generates both variants correctly:

@media (prefers-reduced-motion: no-preference) { .motion-safe\:animate-pulse { animation: pulse 2s …; } }
@media (prefers-reduced-motion: reduce)        { .motion-reduce\:animate-none { animation: none; } }

That keeps today's behaviour for everyone who has expressed no preference, and stops it for everyone who has.

Our workaround in the meantime is <Skeleton :animate="false" className="motion-safe:animate-pulse" />, which works but means every call site has to remember it.

2. An unknown variant puts undefined in the class attribute

const variantClasses = { text:…, title:…, avatar:…, thumbnail:…, button:…, card:…, rect:… }
const baseClasses = circle ? 'rounded-full' : variantClasses[variant]
export const skeletonClasses = `bg-surface-sunken ${baseClasses} ${animationClasses} ${className}`.trim()

variant defaults to 'text', but anything outside that set makes variantClasses[variant] undefined, and the template literal stringifies it:

<div class="bg-surface-sunken undefined animate-pulse">

Harmless in effect — undefined matches no rule — but the placeholder silently loses its height and radius, so <Skeleton variant="line"> renders a zero-height element and looks like nothing happened.

Same shape as the Switch bug in #1974, and the same one-line fix:

const baseClasses = circle ? 'rounded-full' : (variantClasses[variant] || variantClasses.text)

It is less severe than #1974 because this one is interpolated into the template rather than into <script client>, so it degrades rather than throwing a SyntaxError. I scanned the rest of the library for the template-side version of this pattern: Button (iconClasses), TextInput and PasswordInput (iconClasses) and Progress (circularSizeClass) have it too, all with the same cosmetic consequence.

Happy to send a PR for both if the approach is agreed.

Sign in to comment on this issue.