Skip to content

@radix-ui/react-presence doesn't handle the animationend event correctly when keyframe has escapable character #2763

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
frarillotta opened this issue Mar 6, 2024 · 3 comments · May be fixed by #3466
Open

@radix-ui/react-presence doesn't handle the animationend event correctly when keyframe has escapable character #2763

frarillotta opened this issue Mar 6, 2024 · 3 comments · May be fixed by #3466
Labels
Package: react/presence Type: Bug Confirmed bug

Comments

@frarillotta
Copy link

Bug report

Current Behavior

When using an exit animation on @radix-ui/react-dialog for the overlay, we noticed that if the keyframe has a special character (in our case \+ generated from webpack hashing classes using base64) in its name, Presence does not correctly unmount the element, but is rather left hanging for the animation to finish. This is due to the Presence component using getComputedStyles which seems to escape those character and fails to correctly resolve isCurrentAnimation due to the mismatch between the two strings (check screenshot for the example we have seen), being left in unmountSuspended state.

image

Expected behavior

When dialog is closed, all Presence elements should be unmounted correctly.

Reproducible example

Very crude example with dialog. To reproduce, open the modal, click "Save changes" and you will see the exit animation playing correctly, but the overlay element (.DialogOverlay) not unmounting. The correct behaviour can be replicated by changing the keyframe overlayFadeOut\+R8 (and the consuming animation in .DialogOverlay[data-animate="true"]) to not have the \+ (i.e. overlayFadeOutR8)

Suggested solution

A potential solution could be not to use event.animationName in the animationend handler but rather to use getAnimationName(getComputedStyles(event.target)), or simply to escape the event.animationName similarly to what getComputedStyles returns.

Additional context

Your environment

Same as sandbox

Software Name(s) Version
Radix Package(s) react-presence 1.0.1
React n/a 17.0.2
Browser Chrome 121.0.6167.184
Assistive tech
Node n/a 16.17.0
npm/yarn pnpm 8.15.4
Operating System macOS 13.6.1
@benoitgrelard
Copy link
Contributor

benoitgrelard commented Mar 12, 2024 •
edited
Loading

Do you know what sort of escaping it's currently doing? ie. is there a built-in function we could call to get the same result?

@frarillotta
Copy link
Author

Do you know what sort of escaping it's currently doing? ie. is there a built-in function we could call to get the same result?

Not entirely sure, from what I could get from logging what the function sees seems to come from stylesRef which is set here, which in turn uses getComputedStyles. We could also use getComputedStyles on the event (event also has a target property)

haoqunjiang added a commit to haoqunjiang/primitives that referenced this issue Apr 13, 2025
@haoqunjiang
fix(Presence): handle escape characters in animation names correctly
5afaed2 
Fixes radix-ui#2763.

Using [CSS.escape](https://developer.mozilla.org/en-US/docs/Web/API/CSS/escape_static_method).

As I checked, this API is supported in all browsers even in the
[2022 browserslist targets](radix-ui#1019 (comment))
of this project.

Further explanation of the bug:

`getComputedStyle()` returns a `CSSStyleDeclaration``, which is
serialized according to the spec.
Meanwhile, the values from `AnimationEvent` are not required to be
serialized (which makes sense, as it's intended for JavaScript usage,
where escaping for CSS syntax is unnecessary). Therefore the
`animationName` is returned as-is in the event handler, causing the
mismatch.
haoqunjiang added a commit to haoqunjiang/primitives that referenced this issue Apr 13, 2025
@haoqunjiang
fix(Presence): handle escape characters in animation names correctly
49e599e 
Fixes radix-ui#2763.

Using [CSS.escape](https://developer.mozilla.org/en-US/docs/Web/API/CSS/escape_static_method).

As I checked, this API is supported in all browsers even in the
[2022 browserslist targets](radix-ui#1019 (comment))
of this project.

Further explanation of the bug:

`getComputedStyle()` returns a `CSSStyleDeclaration`, which is
serialized according to the spec.
Meanwhile, the values from `AnimationEvent` are not required to be
serialized (which makes sense, as it's intended for JavaScript usage,
where escaping for CSS syntax is unnecessary). Therefore the
`animationName` is returned as-is in the event handler, causing the
mismatch.
@haoqunjiang haoqunjiang linked a pull request Apr 13, 2025 that will close this issue
Open
@haoqunjiang
Copy link

I opened a PR to fix this issue using CSS.escape: #3466

Here's the fixed example with patches applied: https://stackblitz.com/edit/vitejs-vite-dm8pji1o (I couldn't figure out how to apply patches in CodeSandbox, so I recreated a StackBlitz example)

haoqunjiang added a commit to haoqunjiang/primitives that referenced this issue Apr 13, 2025
@haoqunjiang
fix(Presence): handle escape characters in animation names correctly
8b6ce90 
Fixes radix-ui#2763.

Using [CSS.escape](https://developer.mozilla.org/en-US/docs/Web/API/CSS/escape_static).

As I checked, this API is supported in all browsers even in the
[2022 browserslist targets](radix-ui#1019 (comment))
of this project.

Further explanation of the bug:

`getComputedStyle()` returns a `CSSStyleDeclaration`, which is
serialized according to the spec.
Meanwhile, the values from `AnimationEvent` are not required to be
serialized (which makes sense, as it's intended for JavaScript usage,
where escaping for CSS syntax is unnecessary). Therefore the
`animationName` is returned as-is in the event handler, causing the
mismatch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Package: react/presence Type: Bug Confirmed bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(Presence): handle escape characters in animation names correctly haoqunjiang/primitives
3 participants
@benoitgrelard @haoqunjiang @frarillotta