-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
fix: fixes a bug with runAfterTransition getting stuck when elements are removed #8004
base: main
Are you sure you want to change the base?
fix: fixes a bug with runAfterTransition getting stuck when elements are removed #8004
Conversation
…are suddenly removed without calling transitionend/transitioncancel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, two small comments, otherwise looks good
6685edb
to
680cbee
Compare
*/ | ||
function cleanupDetachedElements() { | ||
for (const [element] of transitionsByElement) { | ||
if (element instanceof HTMLElement && !element.isConnected) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last edgecase to avoid, if we're running code across iframes then we need to check in this way
if (element instanceof HTMLElement && !element.isConnected) { | |
let windowObject = getOwnerWindow(element); | |
if (element instanceof windowObject.HTMLElement && !element.isConnected) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
transitionsByElement
stores EventTarget, which doesn't seem to be an Element/HTMLElement unless you check against it. With the getOwnerWindow
, it expects an Element, but the element
is actually an EventTarget, so I'm getting a type mismatch error.
I could do if element instanceof Element
to narrow the type, but I imagine it'll be preferred to do windowObject.element
instead.
I have a few possible solutions though; let me know which one works best or if you have a different idea:
- Could do a type assertion to
Element
or addts-expect-error
here to ignore the type mismatch.
let windowObject = getOwnerWindow(element as Element);
or
// @ts-expect-error
let windowObject = getOwnerWindow(element);
- Could just check for
isConnected
within the object instead and avoid the checks against HTMLElement/Element altogether; this passes the typecheck. Personally leaning towards this option.
if ('isConnected' in element && !element.isConnected) {
transitionsByElement.delete(element);
}
- Saw that there is a
isNode
function. Maybe could build out aisElement
function. I'm not sure how robust this would be.
export function isElement(node: unknown): node is Element {
return isNode(node) && node instanceof getOwnerWindow(node as Element).Element;
}
- Could also try moving the checks earlier into the
onTransitionStart
so that we're storing Elements (or HTMLElements) explicitly, but I think you'll get the same issue where the node is an EventTarget and we'll need to somehow assert to anElement
to usegetOwnerWindow
safely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with just checking the isConnected
seems like it would handle all the cases needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
With the transition tracking mechanism, it is possible for elements to be stuck in there as
transitionend
is never called (such as suddenly removed from the DOM during a transition event). As a result, this can break the FocusScope component pretty hard, impacting Dialog and other components.This PR adds a check for detached elements and removes it from the transitionsByElement, prior to the callback/queue step.
Closes #7973
✅ Pull Request Checklist:
📝 Test Instructions:
I added unit tests to cover this path, but not sure how to replicate this elsewhere. You can take an existing implementation using a conditionally rendered FocusScope, add a CSS transition to a child element, and have that child element change background colors. This basically triggers the bug. You can see the before and after.
Before
Screen.Recording.2025-03-26.at.10.25.16.AM.mov
After
Screen.Recording.2025-03-26.at.10.25.48.AM.mov
🧢 Your Project: