Skip to content

fix: Resolve DOM residue issue on activate #525

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

Closed
cactuser-Lu wants to merge 5 commits into from
+13 −1
Closed

fix: Resolve DOM residue issue on activate #525

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0955502
fix: Resolve DOM residue issue on activate
Dec 2, 2025
e169a3c
test: set CSSMotion key
Dec 3, 2025
db0d5eb
test: revert closeTimerRef
Dec 3, 2025
221caf4
Revert "test: revert closeTimerRef"
Dec 3, 2025
ad8512c
Revert "fix: Resolve DOM residue issue on activate"
Dec 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions src/Dialog/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ const Dialog: React.FC<IDialogPropTypes> = (props) => {
const lastOutSideActiveElementRef = useRef<HTMLElement>(null);
const wrapperRef = useRef<HTMLDivElement>(null);
const contentRef = useRef<ContentRef>(null);
const closeTimerRef = useRef<ReturnType<typeof setTimeout>>(null);

const [animatedVisible, setAnimatedVisible] = React.useState(visible);

Expand Down Expand Up @@ -157,22 +158,36 @@ const Dialog: React.FC<IDialogPropTypes> = (props) => {

// ========================= Effect =========================
useEffect(() => {
clearTimeout(closeTimerRef.current);

if (visible) {
setAnimatedVisible(true);
saveLastOutSideActiveElementRef();
} else if (
animatedVisible &&
contentRef.current.enableMotion() &&
!contentRef.current.inMotion()
) {
doClose();
} else if (animatedVisible) {
const hasMotion = contentRef.current?.enableMotion?.();
const inMotion = contentRef.current?.inMotion?.();

if (hasMotion && !inMotion) {
doClose();
} else {
closeTimerRef.current = setTimeout(() => {
if (!visible && animatedVisible) {
doClose();
}
}, 500);
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The timeout duration 500 is a magic number. It's better to define it as a constant with a descriptive name (e.g., const DIALOG_CLOSE_FALLBACK_TIMEOUT = 500;) at the top of the component. This improves readability and makes the purpose of the timeout clearer and easier to modify in the future.

}
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The current logic introduces an unnecessary 500ms delay for closing the dialog when motion is disabled (hasMotion is false). If there's no motion, doClose() should be called immediately. The condition should be updated to close the dialog if motion is disabled OR if it's not currently in motion.

Suggested change
if (hasMotion && !inMotion) {
doClose();
} else {
closeTimerRef.current = setTimeout(() => {
if (!visible && animatedVisible) {
doClose();
}
}, 500);
}
if (!hasMotion || !inMotion) {
doClose();
} else {
closeTimerRef.current = setTimeout(() => {
if (!visible && animatedVisible) {
doClose();
}
}, 500);
}

}

return () => {
clearTimeout(closeTimerRef.current);
};
}, [visible]);

// Remove direct should also check the scroll bar update
useEffect(
() => () => {
clearTimeout(contentTimeoutRef.current);
clearTimeout(closeTimerRef.current);
},
[],
);
Expand Down