Repository navigation
test(Dialog): the reduced-motion entrance contract reads a fade that ended before its listener ran - #8924
Merged
Conversation
…ended before its listener ran On a loaded runner the animationstart listener can run after the 200 ms touch-screen cross-fade has finished, when getAnimations() no longer returns it, so the contract saw no entrance. getAnimations() stays the primary source; when it is empty, the entrance is read from the @Keyframes rule the event names and the computed animation longhands. Assertions are unchanged. Test only. Original contract by @vjeux in #8327.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Recommended for approval: the fallback records a reduced-motion fade that ended before the animationstart listener ran, and every DR1-DR3 assertion is unchanged. When the listener runs late, a correct Dialog now passes, while a 300 ms or travelling entrance still fails through the new path. No changes are needed.
[Automated review]
This branch was successfully deployed
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.
Maintainer impact
The release-time accessibility contracts can go red on a correct Dialog. On a loaded CI runner,
Dialog.reduced-motion.a11y.browser.spec.tssometimes reports that the dialog ran no entrance on a touch screen with reduced motion (animations.lengthis 0), even though the 200 ms cross-fade did run. A red release check on a healthy commit holds the release and sends maintainers chasing a regression that isn't there.Intended invariant
The contract is unchanged (DR1, DR2, DR3 in
docs/design/reduced-motion.md). With reduced motion on a touch screen, the dialog's entrance is an opacity-only cross-fade of 200 ms or less. With a mouse, it appears at once with no animation. Without reduced motion, the standard entrance still rises and scales in. Every assertion stays as it was.Change
The spec records each entrance from
animationstartby readingtarget.getAnimations()inside the listener. On a busy machine that listener can run after a 200 ms fade has already finished, and a finished animation (fill modebackwards) is no longer returned bygetAnimations(). So nothing was recorded, and the "a cross-fade happened" assertion failed.getAnimations()stays the primary source. When it is empty at event time, the spec now builds the record from the event itself: it finds the@keyframesrule named byevent.animationNameto get the animated properties, and reads the duration and delay from the dialog's computed animation longhands. The fine-pointer case still expects noanimationstartat all, so the fallback cannot hide an animation that shouldn't run.Test only: no product code and no Changeset.
Evidence
All runs were in Chromium against the Storybook built for the failing commit, with Playwright 1.61.1.
Expected: > 0, Received: 0) on all three stories. Default fails 30/40, fullscreen 33/40, alert dialog 33/40. A probe showedanimationstartfiring with the fade's 0.2s keyframes whilegetAnimations()was already empty.Scope
Testing
pnpm exec playwright test packages/core/src/Dialog/__tests__/Dialog.reduced-motion.a11y.browser.spec.ts --project chromium --repeat-each=10at normal load, and with--repeat-each=40 --fully-parallel -g 'coarse pointer'under CPU saturation, before and after the change.prettier --checkandeslinton the file are clean.Credit: @vjeux wrote this contract in #8327. This change only makes its measurement robust to slow runners.
Posted by Cindy's agent on her behalf.