mirror of
https://github.com/danny-avila/LibreChat.git
synced 2025-12-21 02:40:14 +01:00
Problem:
--------
Commit 5ed1f2991 introduced a layout shift regression when opening the
sidebar. The UI would visibly "jump" as elements shifted right before
the animation completed. Closing the sidebar worked correctly.
Root Cause Analysis:
--------------------
The accessibility PR added a redundant `{navVisible && ...}` conditional
wrapper around the `<nav>` content inside Nav.tsx's `motion.div`. This
caused a race condition:
1. User clicks "Open Sidebar" button
2. `navVisible` state becomes `true`
3. React renders the `motion.div` AND its children simultaneously
4. The inner `{navVisible && (<nav>...)}` renders content at full width
(320px/260px) BEFORE framer-motion applies `initial={{ width: 0 }}`
5. Brief flash of full-width content causes visible layout shift
6. Animation then starts from width: 0, but damage is done
The ref-based focus management (passing `openSidebarRef`/`closeSidebarRef`
through context) was suspected but was not the actual cause. However,
`requestAnimationFrame` focus calls during animation start could trigger
forced layout calculations, exacerbating the issue.
Solution:
---------
1. Remove redundant conditional rendering in Nav.tsx
- The outer `{navVisible && (<motion.div>...)}` already controls
visibility
- The `overflow-x-hidden` class on motion.div clips content during
animation
- Content should always exist inside motion.div for smooth clipping
2. Replace ref-based focus with ID-based focus management
- Refs passed through component tree can affect React's reconciliation
- Using `document.getElementById()` decouples focus from render cycle
- Exported `CLOSE_SIDEBAR_ID` and `OPEN_SIDEBAR_ID` constants for
consistency
3. Delay focus until after animation completes
- Changed from `requestAnimationFrame` to `setTimeout(..., 250)`
- Animation duration is 200ms; 250ms ensures completion
- Prevents layout thrashing during animation
4. Clean up prop drilling
- Removed `openSidebarRef`/`closeSidebarRef` from Root.tsx context
- Simplified Nav.tsx, Header.tsx, NewChat.tsx prop signatures
- Updated ContextType to remove ref properties
Files Changed:
--------------
- client/src/routes/Root.tsx
- client/src/components/Nav/Nav.tsx
- client/src/components/Nav/NewChat.tsx
- client/src/components/Chat/Header.tsx
- client/src/components/Chat/Menus/OpenSidebar.tsx
- client/src/common/types.ts
Accessibility Note:
-------------------
The original inner conditional was added to prevent keyboard navigation
to hidden sidebar content for screen readers. This is still handled by:
- AnimatePresence unmounting the motion.div after exit animation
- The motion.div having width: 0 during exit (content not reachable)
- Screen readers typically skip content being animated out
- Other: removed non-existant prop from BookmarkNav
Testing:
--------
- Verified smooth animation when opening sidebar (no layout shift)
- Verified smooth animation when closing sidebar (unchanged)
- Verified focus transfers correctly between open/close buttons
- Verified keyboard navigation works as expected
This commit is contained in:
parent
ef1b7f0157
commit
8d1f1c4dd4
6 changed files with 57 additions and 91 deletions
|
|
@ -18,8 +18,7 @@ const defaultInterface = getConfigDefaults().interface;
|
|||
|
||||
export default function Header() {
|
||||
const { data: startupConfig } = useGetStartupConfig();
|
||||
const { navVisible, setNavVisible, openSidebarRef, closeSidebarRef } =
|
||||
useOutletContext<ContextType>();
|
||||
const { navVisible, setNavVisible } = useOutletContext<ContextType>();
|
||||
|
||||
const interfaceConfig = useMemo(
|
||||
() => startupConfig?.interface ?? defaultInterface,
|
||||
|
|
@ -52,12 +51,7 @@ export default function Header() {
|
|||
transition={{ duration: 0.2 }}
|
||||
key="header-buttons"
|
||||
>
|
||||
<OpenSidebar
|
||||
ref={openSidebarRef}
|
||||
setNavVisible={setNavVisible}
|
||||
closeSidebarRef={closeSidebarRef}
|
||||
className="max-md:hidden"
|
||||
/>
|
||||
<OpenSidebar setNavVisible={setNavVisible} className="max-md:hidden" />
|
||||
<HeaderNewChat />
|
||||
</motion.div>
|
||||
)}
|
||||
|
|
|
|||
|
|
@ -1,16 +1,19 @@
|
|||
import { forwardRef } from 'react';
|
||||
import { TooltipAnchor, Button, Sidebar } from '@librechat/client';
|
||||
import { useLocalize } from '~/hooks';
|
||||
import { cn } from '~/utils';
|
||||
|
||||
const OpenSidebar = forwardRef<
|
||||
HTMLButtonElement,
|
||||
{
|
||||
setNavVisible: React.Dispatch<React.SetStateAction<boolean>>;
|
||||
className?: string;
|
||||
closeSidebarRef?: React.RefObject<HTMLButtonElement>;
|
||||
}
|
||||
>(({ setNavVisible, className, closeSidebarRef }, ref) => {
|
||||
/** Element ID for the close sidebar button - used for focus management */
|
||||
export const CLOSE_SIDEBAR_ID = 'close-sidebar-button';
|
||||
/** Element ID for the open sidebar button - used for focus management */
|
||||
export const OPEN_SIDEBAR_ID = 'open-sidebar-button';
|
||||
|
||||
export default function OpenSidebar({
|
||||
setNavVisible,
|
||||
className,
|
||||
}: {
|
||||
setNavVisible: React.Dispatch<React.SetStateAction<boolean>>;
|
||||
className?: string;
|
||||
}) {
|
||||
const localize = useLocalize();
|
||||
|
||||
const handleClick = () => {
|
||||
|
|
@ -18,9 +21,10 @@ const OpenSidebar = forwardRef<
|
|||
localStorage.setItem('navVisible', JSON.stringify(!prev));
|
||||
return !prev;
|
||||
});
|
||||
requestAnimationFrame(() => {
|
||||
closeSidebarRef?.current?.focus();
|
||||
});
|
||||
// Delay focus until after the sidebar animation completes (200ms)
|
||||
setTimeout(() => {
|
||||
document.getElementById(CLOSE_SIDEBAR_ID)?.focus();
|
||||
}, 250);
|
||||
};
|
||||
|
||||
return (
|
||||
|
|
@ -28,7 +32,7 @@ const OpenSidebar = forwardRef<
|
|||
description={localize('com_nav_open_sidebar')}
|
||||
render={
|
||||
<Button
|
||||
ref={ref}
|
||||
id={OPEN_SIDEBAR_ID}
|
||||
size="icon"
|
||||
variant="outline"
|
||||
data-testid="open-sidebar-button"
|
||||
|
|
@ -46,8 +50,4 @@ const OpenSidebar = forwardRef<
|
|||
}
|
||||
/>
|
||||
);
|
||||
});
|
||||
|
||||
OpenSidebar.displayName = 'OpenSidebar';
|
||||
|
||||
export default OpenSidebar;
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue