Skip to content

fix: prevent background scroll when modal is open #128

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

Open
qian403 wants to merge 1 commit into
base: main
Choose a base branch
from
+90 −4
Open

fix: prevent background scroll when modal is open #128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
73 changes: 69 additions & 4 deletions src/components/page/MemberInfo.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { X } from "@lucide/astro";
const { id = "member-info-modal" } = Astro.props;
---

<div id={`${id}-overlay`} class="member-info-overlay"></div>
<div id={id} class="member-info">
Comment on lines +6 to 7
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

The overlay element should have appropriate ARIA attributes for accessibility. Consider adding role="presentation" or aria-hidden="true" when hidden, and removing it when shown. This helps screen readers understand that the overlay is a visual element and not interactive content. Additionally, the overlay should prevent focus from moving to elements behind it when the modal is open.

Suggested change
<div id={`${id}-overlay`} class="member-info-overlay"></div>
<div id={id} class="member-info">
<div
id={`${id}-overlay`}
class="member-info-overlay"
role="presentation"
aria-hidden="true"
></div>
<div
id={id}
class="member-info"
role="dialog"
aria-modal="true"
aria-labelledby="member-name"
aria-describedby="member-description"
>

Copilot uses AI. Check for mistakes.
<div class="header">
<button class="close-btn" aria-label="關閉">
Expand Down Expand Up @@ -61,6 +62,17 @@ const { id = "member-info-modal" } = Astro.props;
}
};

// 解鎖背景滾動
const unlockBodyScroll = () => {
const scrollY = document.body.dataset.scrollY || "0";
document.body.style.overflow = "";
document.body.style.position = "";
document.body.style.width = "";
document.body.style.top = "";
window.scrollTo(0, parseInt(scrollY, 10));
delete document.body.dataset.scrollY;
};
Comment on lines +65 to +74
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

The scroll lock/unlock logic is duplicated between this file and src/pages/info/index.astro (lines 196-205). Consider extracting this into a shared utility function to reduce duplication and ensure consistency. This would make future maintenance easier and reduce the risk of the implementations diverging.

Copilot uses AI. Check for mistakes.
Comment on lines +66 to +74
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

The unlockBodyScroll function doesn't check if a scroll lock is currently active before attempting to unlock. If called when no modal is open (e.g., double-clicking close button or pressing ESC multiple times), it could attempt to restore a scroll position that doesn't exist or is from a previous session. Consider adding a guard check to verify that dataset.scrollY exists before attempting restoration, or using a flag to track lock state.

Copilot uses AI. Check for mistakes.

// Unified Click Event Listener
document.addEventListener("keydown", e => {
if (e.key === "Enter" || e.key === " ") {
Expand All @@ -82,6 +94,7 @@ const { id = "member-info-modal" } = Astro.props;
const { name, position, link, team, description, mode, color } = trigger.dataset;

const modal = document.getElementById("member-info-modal");
const overlay = document.getElementById("member-info-modal-overlay");
if (modal) {
// Update Text
const nameEl = modal.querySelector("#member-name");
Expand Down Expand Up @@ -147,16 +160,37 @@ const { id = "member-info-modal" } = Astro.props;
}

modal.classList.add("show");
overlay?.classList.add("show");
Comment on lines 161 to +163
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

When the modal opens, focus should be moved to the modal or its close button for better keyboard navigation and accessibility. Currently, focus remains on the trigger element. Consider adding modal.focus() or closeBtn.focus() after showing the modal to ensure keyboard users can immediately interact with or close the modal without having to tab through background elements.

Suggested change
modal.classList.add("show");
overlay?.classList.add("show");
// Move focus into the modal (to the close button) for accessibility
const closeBtn = modal.querySelector(".close-btn") as HTMLButtonElement | null;
modal.classList.add("show");
overlay?.classList.add("show");
// Ensure keyboard users can immediately interact with or close the modal
if (closeBtn) {
closeBtn.focus();
}

Copilot uses AI. Check for mistakes.
// 鎖定背景滾動
document.body.style.overflow = "hidden";
document.body.style.position = "fixed";
document.body.style.width = "100%";
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

Setting document.body.style.width = "100%" when locking scroll may cause layout issues on pages where the body has a different width configuration or uses max-width. On pages with scrollbars, this could cause a layout shift when the scrollbar disappears (as the body becomes fixed). Consider also setting the right padding to match the scrollbar width to prevent content shift: document.body.style.paddingRight = ${window.innerWidth - document.documentElement.clientWidth}px

Suggested change
document.body.style.width = "100%";
const scrollBarWidth = window.innerWidth - document.documentElement.clientWidth;
if (scrollBarWidth > 0) {
document.body.style.paddingRight = `${scrollBarWidth}px`;
}

Copilot uses AI. Check for mistakes.
document.body.style.top = `-${window.scrollY}px`;
document.body.dataset.scrollY = String(window.scrollY);
Comment on lines +165 to +169
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

When the modal opens and position: fixed is applied to the body, the scroll position is lost visually because fixed positioning removes the element from the normal flow. While the code correctly saves scrollY and restores it on close, if the user has scrolled significantly before opening the modal, the sudden jump back to that position could be disorienting. Consider adding scroll-behavior: auto temporarily during restoration to ensure an instant jump rather than a smooth scroll.

Copilot uses AI. Check for mistakes.
Comment on lines +164 to +169
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

Similar to the transport dialog, if multiple modals can be opened simultaneously, the scroll lock implementation needs to handle this case. The current implementation overwrites document.body.dataset.scrollY which could lead to incorrect scroll restoration when closing modals in a different order than they were opened.

Suggested change
// 鎖定背景滾動
document.body.style.overflow = "hidden";
document.body.style.position = "fixed";
document.body.style.width = "100%";
document.body.style.top = `-${window.scrollY}px`;
document.body.dataset.scrollY = String(window.scrollY);
// 鎖定背景滾動（僅在尚未鎖定時執行，以支援多個同時開啟的 modal）
if (!document.body.dataset.scrollY) {
const scrollY = window.scrollY;
document.body.style.overflow = "hidden";
document.body.style.position = "fixed";
document.body.style.width = "100%";
document.body.style.top = `-${scrollY}px`;
document.body.dataset.scrollY = String(scrollY);
}

Copilot uses AI. Check for mistakes.
}
return;
}

// Close modal logic
// Close modal logic - close button
const closeBtn = target.closest(".member-info .close-btn");
if (closeBtn) {
const modal = closeBtn.closest(".member-info") as HTMLElement | null;
const overlay = document.getElementById("member-info-modal-overlay");
if (modal) {
modal.classList.remove("show");
overlay?.classList.remove("show");
unlockBodyScroll();
}
Comment on lines +180 to +183
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

When the modal closes, focus should be restored to the element that opened it (the trigger). Currently, closing the modal doesn't restore focus, which can be disorienting for keyboard users as focus may jump to an unexpected location. Consider storing a reference to the trigger element when opening the modal and calling trigger.focus() when closing.

Copilot uses AI. Check for mistakes.
return;
}

// Close modal logic - overlay click
if (target.classList.contains("member-info-overlay")) {
const modal = document.getElementById("member-info-modal");
if (modal) {
modal.classList.remove("show");
target.classList.remove("show");
unlockBodyScroll();
}
return;
}
Expand All @@ -174,8 +208,11 @@ const { id = "member-info-modal" } = Astro.props;
document.addEventListener("keydown", e => {
if (e.key === "Escape") {
const modal = document.getElementById("member-info-modal");
const overlay = document.getElementById("member-info-modal-overlay");
if (modal && modal.classList.contains("show")) {
modal.classList.remove("show");
overlay?.classList.remove("show");
unlockBodyScroll();
}
}
});
Expand Down Expand Up @@ -256,6 +293,29 @@ const { id = "member-info-modal" } = Astro.props;
</script>

<style>
.member-info-overlay {
position: fixed;
top: 0;
left: 0;
right: 0;
bottom: 0;
background-color: rgba(0, 0, 0, 0.5);
z-index: 100;
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

The overlay z-index is set to 100, which conflicts with the Nav component (src/components/Nav.astro:151) that also uses z-index: 100. This could cause layering issues where the navigation might appear above the modal overlay. Consider increasing the overlay z-index to at least 102 (modal is 101) to ensure proper stacking order, or better yet, use a higher range like 1000+ for modal overlays to avoid conflicts with other fixed elements.

Copilot uses AI. Check for mistakes.
opacity: 0;
visibility: hidden;
transition:
opacity 0.3s ease,
visibility 0s 0.3s;
}

.member-info-overlay.show {
opacity: 1;
visibility: visible;
transition:
opacity 0.3s ease,
visibility 0s 0s;
}

.member-info {
color: black;
background-color: white;
Expand Down Expand Up @@ -427,10 +487,15 @@ const { id = "member-info-modal" } = Astro.props;
.member-info {
width: calc(100vw - 4rem);
height: calc(100vh - 4rem);
align-self: center;
justify-self: center;
position: fixed;
top: 2rem;
left: 2rem;
border-radius: 1rem;
Comment on lines +490 to +492
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

The removed CSS properties 'align-self: center' and 'justify-self: center' were likely used for centering the modal. With the new approach using 'top: 2rem' and 'left: 2rem', the modal is now positioned explicitly. Ensure this change doesn't break the layout on various mobile screen sizes, particularly on very small or very large mobile devices.

Copilot uses AI. Check for mistakes.
padding: 2.5rem;
transform: translateY(100%);
}

.member-info.show {
transform: translateY(0);
Comment on lines +494 to +498
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

In the mobile breakpoint, the modal animation direction has changed from translateX(-105%) (slide from left) to translateY(100%) (slide from bottom). This changes the user experience on mobile. If this is intentional to improve UX on mobile devices, this is good. However, if this was an unintentional change while adding the position: fixed scroll lock, it should be reviewed to maintain consistency with the original design intent.

Suggested change
transform: translateY(100%);
}
.member-info.show {
transform: translateY(0);
transform: translateX(-105%);
}
.member-info.show {
transform: translateX(0);

Copilot uses AI. Check for mistakes.
}

.header {
Expand Down
21 changes: 21 additions & 0 deletions src/pages/info/index.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,15 +184,36 @@ const { sections } = info;
if (typeof dialog.showModal === "function") dialog.showModal();
else dialog.setAttribute("open", "");

document.body.style.overflow = "hidden";
document.body.style.position = "fixed";
document.body.style.width = "100%";
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

Setting document.body.style.width = "100%" when locking scroll may cause layout issues on pages where the body has a different width configuration. On pages with scrollbars, this could cause a layout shift when the scrollbar disappears. Consider also setting padding-right to compensate for the scrollbar width to prevent content shift.

Copilot uses AI. Check for mistakes.
document.body.style.top = `-${window.scrollY}px`;
document.body.dataset.scrollY = String(window.scrollY);

activateTab("shuttle");
});

// 恢復背景滾動
const unlockBodyScroll = () => {
const scrollY = document.body.dataset.scrollY || "0";
document.body.style.overflow = "";
document.body.style.position = "";
document.body.style.width = "";
document.body.style.top = "";
window.scrollTo(0, parseInt(scrollY, 10));
delete document.body.dataset.scrollY;
Comment on lines +191 to +204
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

If a user opens the transport dialog while the member info modal is already open (or vice versa), the second modal's scroll lock will overwrite the scrollY dataset value from the first modal. When closing the modals, this will result in restoring to the wrong scroll position. Consider using a stack-based approach or namespaced dataset keys (e.g., dataset.scrollYTransport, dataset.scrollYMember) to handle multiple simultaneous modals correctly.

Suggested change
document.body.dataset.scrollY = String(window.scrollY);
activateTab("shuttle");
});
// 恢復背景滾動
const unlockBodyScroll = () => {
const scrollY = document.body.dataset.scrollY || "0";
document.body.style.overflow = "";
document.body.style.position = "";
document.body.style.width = "";
document.body.style.top = "";
window.scrollTo(0, parseInt(scrollY, 10));
delete document.body.dataset.scrollY;
document.body.dataset.scrollYTransport = String(window.scrollY);
activateTab("shuttle");
});
// 恢復背景滾動
const unlockBodyScroll = () => {
const scrollY = document.body.dataset.scrollYTransport || "0";
document.body.style.overflow = "";
document.body.style.position = "";
document.body.style.width = "";
document.body.style.top = "";
window.scrollTo(0, parseInt(scrollY, 10));
delete document.body.dataset.scrollYTransport;

Copilot uses AI. Check for mistakes.
Comment on lines +198 to +204
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

The unlockBodyScroll function uses delete document.body.dataset.scrollY to clean up, but it doesn't verify that all style properties are fully cleared. If the unlock function is called when styles weren't set (edge case), setting them to empty strings is harmless. However, consider also checking if dataset.scrollY exists before attempting deletion to make the function more defensive.

Suggested change
const scrollY = document.body.dataset.scrollY || "0";
document.body.style.overflow = "";
document.body.style.position = "";
document.body.style.width = "";
document.body.style.top = "";
window.scrollTo(0, parseInt(scrollY, 10));
delete document.body.dataset.scrollY;
const storedScrollY = document.body.dataset.scrollY;
document.body.style.overflow = "";
document.body.style.position = "";
document.body.style.width = "";
document.body.style.top = "";
const scrollY = typeof storedScrollY === "string" ? parseInt(storedScrollY, 10) || 0 : window.scrollY || 0;
window.scrollTo(0, scrollY);
if ("scrollY" in document.body.dataset) {
delete document.body.dataset.scrollY;
}

Copilot uses AI. Check for mistakes.
};
Comment on lines +196 to +205
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

The scroll lock logic is duplicated between this file and src/components/page/MemberInfo.astro (lines 65-74). Consider extracting this into a shared utility function to reduce code duplication and ensure consistency across all modals.

Copilot uses AI. Check for mistakes.

closeButton.addEventListener("click", () => dialog.close());

dialog.addEventListener("click", (e: MouseEvent) => {
if (e.target === dialog) dialog.close();
});

dialog.addEventListener("close", () => {
unlockBodyScroll();
});

tabs.forEach(btn => {
btn.addEventListener("click", () => {
const name = btn.getAttribute("data-transport-tab");
Expand Down