From d65dde27b90f5dfab463c51faf1bd88a511194ac Mon Sep 17 00:00:00 2001 From: Vaggelis Yfantis Date: Tue, 25 Feb 2025 18:02:40 +0200 Subject: [PATCH 1/6] fix(clerk-js): Correctly handle scroll lock for multiple modals --- .../clerk-js/src/ui/hooks/useScrollLock.ts | 32 +++++++++++-------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/packages/clerk-js/src/ui/hooks/useScrollLock.ts b/packages/clerk-js/src/ui/hooks/useScrollLock.ts index e41cbba3a1e..26a4be8fcf1 100644 --- a/packages/clerk-js/src/ui/hooks/useScrollLock.ts +++ b/packages/clerk-js/src/ui/hooks/useScrollLock.ts @@ -1,26 +1,32 @@ +let preventScrollCount = 0; +let oldPaddingRightPx: string; +let oldOverflow: string; /** * Disables scroll for an element. * Adds extra padding to prevent layout shifting * caused by hiding the scrollbar. */ export const useScrollLock = (el: T) => { - let oldPaddingRightPx: string; - let oldOverflow: string; - const disableScroll = () => { - oldPaddingRightPx = getComputedStyle(el).paddingRight; - oldOverflow = getComputedStyle(el).overflow; - const oldWidth = el.clientWidth; - el.style.overflow = 'hidden'; - const currentWidth = el.clientWidth; - const oldPaddingRight = Number.parseInt(oldPaddingRightPx.replace('px', '')); - el.style.paddingRight = `${currentWidth - oldWidth + oldPaddingRight}px`; + preventScrollCount++; + if (preventScrollCount === 1) { + oldPaddingRightPx = getComputedStyle(el).paddingRight; + oldOverflow = getComputedStyle(el).overflow; + const oldWidth = el.clientWidth; + el.style.overflow = 'hidden'; + const currentWidth = el.clientWidth; + const oldPaddingRight = Number.parseInt(oldPaddingRightPx.replace('px', '')); + el.style.paddingRight = `${currentWidth - oldWidth + oldPaddingRight}px`; + } }; const enableScroll = () => { - el.style.overflow = oldOverflow; - if (oldPaddingRightPx) { - el.style.paddingRight = oldPaddingRightPx; + preventScrollCount--; + if (preventScrollCount === 0) { + el.style.overflow = oldOverflow; + if (oldPaddingRightPx) { + el.style.paddingRight = oldPaddingRightPx; + } } }; From eeb3477a4b4d7e840e7da05526a11368eb8337fd Mon Sep 17 00:00:00 2001 From: Vaggelis Yfantis Date: Tue, 25 Feb 2025 18:54:21 +0200 Subject: [PATCH 2/6] chore(repo): Add changeset --- .changeset/eight-goats-suffer.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/eight-goats-suffer.md diff --git a/.changeset/eight-goats-suffer.md b/.changeset/eight-goats-suffer.md new file mode 100644 index 00000000000..c282fa59e9e --- /dev/null +++ b/.changeset/eight-goats-suffer.md @@ -0,0 +1,5 @@ +--- +"@clerk/clerk-js": patch +--- + +Fixe issue where scroll lock was not restored correctly when multiple modals were opening From 322db0f96ad2a1ca993810f72cf58a3590364ef9 Mon Sep 17 00:00:00 2001 From: Vaggelis Yfantis Date: Tue, 25 Feb 2025 19:23:33 +0200 Subject: [PATCH 3/6] Update .changeset/eight-goats-suffer.md Co-authored-by: Stefanos Anagnostou --- .changeset/eight-goats-suffer.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/eight-goats-suffer.md b/.changeset/eight-goats-suffer.md index c282fa59e9e..0da56a9ba50 100644 --- a/.changeset/eight-goats-suffer.md +++ b/.changeset/eight-goats-suffer.md @@ -2,4 +2,4 @@ "@clerk/clerk-js": patch --- -Fixe issue where scroll lock was not restored correctly when multiple modals were opening +Fix issue where scroll lock was not restored correctly when multiple modals were opening From 1c0612df3f73e592742b3906960bb34e7de36495 Mon Sep 17 00:00:00 2001 From: Vaggelis Yfantis Date: Tue, 25 Feb 2025 20:50:26 +0200 Subject: [PATCH 4/6] fix(clerk-js): Early return --- .../clerk-js/src/ui/hooks/useScrollLock.ts | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/packages/clerk-js/src/ui/hooks/useScrollLock.ts b/packages/clerk-js/src/ui/hooks/useScrollLock.ts index 26a4be8fcf1..b8bc68a316a 100644 --- a/packages/clerk-js/src/ui/hooks/useScrollLock.ts +++ b/packages/clerk-js/src/ui/hooks/useScrollLock.ts @@ -9,24 +9,22 @@ let oldOverflow: string; export const useScrollLock = (el: T) => { const disableScroll = () => { preventScrollCount++; - if (preventScrollCount === 1) { - oldPaddingRightPx = getComputedStyle(el).paddingRight; - oldOverflow = getComputedStyle(el).overflow; - const oldWidth = el.clientWidth; - el.style.overflow = 'hidden'; - const currentWidth = el.clientWidth; - const oldPaddingRight = Number.parseInt(oldPaddingRightPx.replace('px', '')); - el.style.paddingRight = `${currentWidth - oldWidth + oldPaddingRight}px`; - } + if (preventScrollCount !== 1) return; + oldPaddingRightPx = getComputedStyle(el).paddingRight; + oldOverflow = getComputedStyle(el).overflow; + const oldWidth = el.clientWidth; + el.style.overflow = 'hidden'; + const currentWidth = el.clientWidth; + const oldPaddingRight = Number.parseInt(oldPaddingRightPx.replace('px', '')); + el.style.paddingRight = `${currentWidth - oldWidth + oldPaddingRight}px`; }; const enableScroll = () => { preventScrollCount--; - if (preventScrollCount === 0) { - el.style.overflow = oldOverflow; - if (oldPaddingRightPx) { - el.style.paddingRight = oldPaddingRightPx; - } + if (preventScrollCount !== 0) return; + el.style.overflow = oldOverflow; + if (oldPaddingRightPx) { + el.style.paddingRight = oldPaddingRightPx; } }; From 8c1128010726dae29a848c153b5a1460d26e3e6b Mon Sep 17 00:00:00 2001 From: Vaggelis Yfantis Date: Wed, 26 Feb 2025 11:01:17 +0200 Subject: [PATCH 5/6] refactor(clerk-js): Use floating-ui's build in FloatingOverlay for scroll lock --- packages/clerk-js/src/ui/elements/Modal.tsx | 89 +++++++++---------- packages/clerk-js/src/ui/hooks/index.ts | 1 - .../clerk-js/src/ui/hooks/useScrollLock.ts | 32 ------- 3 files changed, 43 insertions(+), 79 deletions(-) delete mode 100644 packages/clerk-js/src/ui/hooks/useScrollLock.ts diff --git a/packages/clerk-js/src/ui/elements/Modal.tsx b/packages/clerk-js/src/ui/elements/Modal.tsx index ef72f1bb833..f0524ab6740 100644 --- a/packages/clerk-js/src/ui/elements/Modal.tsx +++ b/packages/clerk-js/src/ui/elements/Modal.tsx @@ -1,8 +1,9 @@ -import { createContextAndHook, useSafeLayoutEffect } from '@clerk/shared/react'; +import { createContextAndHook } from '@clerk/shared/react'; +import { FloatingOverlay } from '@floating-ui/react'; import React, { useRef } from 'react'; import { descriptors, Flex } from '../customizables'; -import { usePopover, useScrollLock } from '../hooks'; +import { usePopover } from '../hooks'; import type { ThemableCssProp } from '../styledSystem'; import { animations, mqu } from '../styledSystem'; import { withFloatingTree } from './contexts'; @@ -22,7 +23,6 @@ type ModalProps = React.PropsWithChildren<{ export const Modal = withFloatingTree((props: ModalProps) => { const { handleClose, handleOpen, contentSx, containerSx, canCloseModal, id, style } = props; - const { disableScroll, enableScroll } = useScrollLock(document.body); const overlayRef = useRef(null); const { floating, isOpen, context, nodeId, toggle } = usePopover({ defaultOpen: true, @@ -39,11 +39,6 @@ export const Modal = withFloatingTree((props: ModalProps) => { } }, [isOpen]); - useSafeLayoutEffect(() => { - disableScroll(); - return () => enableScroll(); - }); - const modalCtx = React.useMemo(() => ({ value: canCloseModal === false ? {} : { toggle } }), [toggle, canCloseModal]); return ( @@ -52,51 +47,53 @@ export const Modal = withFloatingTree((props: ModalProps) => { context={context} isOpen={isOpen} > - - ({ - animation: `${animations.fadeIn} 150ms ${t.transitionTiming.$common}`, - zIndex: t.zIndices.$modal, - backgroundColor: t.colors.$modalBackdrop, - alignItems: 'flex-start', - justifyContent: 'center', - overflow: 'auto', - width: '100vw', - height: ['100vh', '-webkit-fill-available'], - position: 'fixed', - left: 0, - top: 0, - }), - containerSx, - ]} - > + + ({ - position: 'relative', - outline: 0, - animation: `${animations.modalSlideAndFade} 180ms ${t.transitionTiming.$easeOut}`, - margin: `${t.space.$16} 0`, - [mqu.sm]: { - margin: `${t.space.$10} 0`, - }, + animation: `${animations.fadeIn} 150ms ${t.transitionTiming.$common}`, + zIndex: t.zIndices.$modal, + backgroundColor: t.colors.$modalBackdrop, + alignItems: 'flex-start', + justifyContent: 'center', + overflow: 'auto', + width: '100vw', + height: ['100vh', '-webkit-fill-available'], + position: 'fixed', + left: 0, + top: 0, }), - contentSx, + containerSx, ]} > - {props.children} + ({ + position: 'relative', + outline: 0, + animation: `${animations.modalSlideAndFade} 180ms ${t.transitionTiming.$easeOut}`, + margin: `${t.space.$16} 0`, + [mqu.sm]: { + margin: `${t.space.$10} 0`, + }, + }), + contentSx, + ]} + > + {props.children} + - - + + ); }); diff --git a/packages/clerk-js/src/ui/hooks/index.ts b/packages/clerk-js/src/ui/hooks/index.ts index 263683924b4..58996c9c9fa 100644 --- a/packages/clerk-js/src/ui/hooks/index.ts +++ b/packages/clerk-js/src/ui/hooks/index.ts @@ -15,7 +15,6 @@ export * from './useResizeObserver'; export * from './useSafeState'; export * from './useSearchInput'; export * from './useDebounce'; -export * from './useScrollLock'; export * from './useClerkModalStateParams'; export * from './useNavigateToFlowStart'; export * from './useEnterpriseSSOLink'; diff --git a/packages/clerk-js/src/ui/hooks/useScrollLock.ts b/packages/clerk-js/src/ui/hooks/useScrollLock.ts deleted file mode 100644 index b8bc68a316a..00000000000 --- a/packages/clerk-js/src/ui/hooks/useScrollLock.ts +++ /dev/null @@ -1,32 +0,0 @@ -let preventScrollCount = 0; -let oldPaddingRightPx: string; -let oldOverflow: string; -/** - * Disables scroll for an element. - * Adds extra padding to prevent layout shifting - * caused by hiding the scrollbar. - */ -export const useScrollLock = (el: T) => { - const disableScroll = () => { - preventScrollCount++; - if (preventScrollCount !== 1) return; - oldPaddingRightPx = getComputedStyle(el).paddingRight; - oldOverflow = getComputedStyle(el).overflow; - const oldWidth = el.clientWidth; - el.style.overflow = 'hidden'; - const currentWidth = el.clientWidth; - const oldPaddingRight = Number.parseInt(oldPaddingRightPx.replace('px', '')); - el.style.paddingRight = `${currentWidth - oldWidth + oldPaddingRight}px`; - }; - - const enableScroll = () => { - preventScrollCount--; - if (preventScrollCount !== 0) return; - el.style.overflow = oldOverflow; - if (oldPaddingRightPx) { - el.style.paddingRight = oldPaddingRightPx; - } - }; - - return { disableScroll, enableScroll }; -}; From c5185c6f3d1bd354ca3b61c64aa6fdf5934c4500 Mon Sep 17 00:00:00 2001 From: Vaggelis Yfantis Date: Wed, 26 Feb 2025 11:43:24 +0200 Subject: [PATCH 6/6] fix(clerk-js): Increase bundlewatch limit for ./dist/vendors*.js --- packages/clerk-js/bundlewatch.config.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/clerk-js/bundlewatch.config.json b/packages/clerk-js/bundlewatch.config.json index fe0ea8fb266..83b2baa71b9 100644 --- a/packages/clerk-js/bundlewatch.config.json +++ b/packages/clerk-js/bundlewatch.config.json @@ -4,7 +4,7 @@ { "path": "./dist/clerk.browser.js", "maxSize": "75kB" }, { "path": "./dist/clerk.headless.js", "maxSize": "48.2KB" }, { "path": "./dist/ui-common*.js", "maxSize": "89KB" }, - { "path": "./dist/vendors*.js", "maxSize": "25KB" }, + { "path": "./dist/vendors*.js", "maxSize": "25.1KB" }, { "path": "./dist/coinbase*.js", "maxSize": "35.5KB" }, { "path": "./dist/createorganization*.js", "maxSize": "5KB" }, { "path": "./dist/impersonationfab*.js", "maxSize": "5KB" },