Fix mobile navigation tabs expanding/collapsing when scrolling in modals (#3731)

* Update scroll provider so that it only captures the scroll on div container and not the whole window

* Fix lint + release notes

* Rewrite useScroll to be more performant by being ref based instead of state to avoid re-renders when scrolling

* Check undefined

* Rename to useScrollListener

* Remove small 1px gap under mobile nav tabs when fully open

* Cleanup

* Fix lint

* Coderabbit feedback

packages/desktop-client/src/components/App.tsx

@@ -40,7 +40,6 @@ import { FinancesApp } from './FinancesApp';
 import { ManagementApp } from './manager/ManagementApp';
 import { Modals } from './Modals';
 import { ResponsiveProvider } from './responsive/ResponsiveProvider';
-import { ScrollProvider } from './ScrollProvider';
 import { SidebarProvider } from './sidebar/SidebarProvider';
 import { UpdateNotification } from './UpdateNotification';
 
@@ -180,36 +179,34 @@ export function App() {
             <SidebarProvider>
               <BudgetMonthCountProvider>
                 <DndProvider backend={HTML5Backend}>
-                  <ScrollProvider>
+                  <View
+                    data-theme={theme}
+                    style={{
+                      height: '100%',
+                      display: 'flex',
+                      flexDirection: 'column',
+                    }}
+                  >
                     <View
-                      data-theme={theme}
+                      key={
+                        hiddenScrollbars ? 'hidden-scrollbars' : 'scrollbars'
+                      }
                       style={{
-                        height: '100%',
-                        display: 'flex',
-                        flexDirection: 'column',
+                        flexGrow: 1,
+                        overflow: 'hidden',
+                        ...styles.lightScrollbar,
                       }}
                     >
-                      <View
-                        key={
-                          hiddenScrollbars ? 'hidden-scrollbars' : 'scrollbars'
-                        }
-                        style={{
-                          flexGrow: 1,
-                          overflow: 'hidden',
-                          ...styles.lightScrollbar,
-                        }}
-                      >
-                        <ErrorBoundary FallbackComponent={ErrorFallback}>
-                          {process.env.REACT_APP_REVIEW_ID &&
-                            !Platform.isPlaywright && <DevelopmentTopBar />}
-                          <AppInner />
-                        </ErrorBoundary>
-                        <ThemeStyle />
-                        <Modals />
-                        <UpdateNotification />
-                      </View>
+                      <ErrorBoundary FallbackComponent={ErrorFallback}>
+                        {process.env.REACT_APP_REVIEW_ID &&
+                          !Platform.isPlaywright && <DevelopmentTopBar />}
+                        <AppInner />
+                      </ErrorBoundary>
+                      <ThemeStyle />
+                      <Modals />
+                      <UpdateNotification />
                     </View>
-                  </ScrollProvider>
+                  </View>
                 </DndProvider>
               </BudgetMonthCountProvider>
             </SidebarProvider>

packages/desktop-client/src/components/FinancesApp.tsx

@@ -1,5 +1,5 @@
 // @ts-strict-ignore
-import React, { type ReactElement, useEffect } from 'react';
+import React, { type ReactElement, useEffect, useRef } from 'react';
 import { useTranslation } from 'react-i18next';
 import { useDispatch, useSelector } from 'react-redux';
 import {
@@ -34,6 +34,7 @@ import { Reports } from './reports';
 import { LoadingIndicator } from './reports/LoadingIndicator';
 import { NarrowAlternate, WideComponent } from './responsive';
 import { useResponsive } from './responsive/ResponsiveProvider';
+import { ScrollProvider } from './ScrollProvider';
 import { Settings } from './settings';
 import { FloatableSidebar } from './sidebar';
 import { Titlebar } from './Titlebar';
@@ -156,6 +157,8 @@ export function FinancesApp() {
     run();
   }, [lastUsedVersion, setLastUsedVersion]);
 
+  const scrollableRef = useRef<HTMLDivElement>(null);
+
   return (
     <View style={{ height: '100%' }}>
       <RouterBehaviors />
@@ -179,113 +182,119 @@ export function FinancesApp() {
             width: '100%',
           }}
         >
-          <View
-            style={{
-              flex: 1,
-              overflow: 'auto',
-              position: 'relative',
-            }}
+          <ScrollProvider
+            isDisabled={!isNarrowWidth}
+            scrollableRef={scrollableRef}
           >
-            <Titlebar
+            <View
+              ref={scrollableRef}
               style={{
-                WebkitAppRegion: 'drag',
-                position: 'absolute',
-                top: 0,
-                left: 0,
-                right: 0,
-                zIndex: 1000,
+                flex: 1,
+                overflow: 'auto',
+                position: 'relative',
               }}
-            />
-            <Notifications />
-            <BankSyncStatus />
+            >
+              <Titlebar
+                style={{
+                  WebkitAppRegion: 'drag',
+                  position: 'absolute',
+                  top: 0,
+                  left: 0,
+                  right: 0,
+                  zIndex: 1000,
+                }}
+              />
+              <Notifications />
+              <BankSyncStatus />
+
+              <Routes>
+                <Route
+                  path="/"
+                  element={
+                    accountsLoaded ? (
+                      accounts.length > 0 ? (
+                        <Navigate to="/budget" replace />
+                      ) : (
+                        // If there are no accounts, we want to redirect the user to
+                        // the All Accounts screen which will prompt them to add an account
+                        <Navigate to="/accounts" replace />
+                      )
+                    ) : (
+                      <LoadingIndicator />
+                    )
+                  }
+                />
+
+                <Route path="/reports/*" element={<Reports />} />
+
+                <Route
+                  path="/budget"
+                  element={<NarrowAlternate name="Budget" />}
+                />
+
+                <Route
+                  path="/schedules"
+                  element={
+                    <NarrowNotSupported>
+                      <WideComponent name="Schedules" />
+                    </NarrowNotSupported>
+                  }
+                />
+
+                <Route path="/payees" element={<ManagePayeesPage />} />
+                <Route path="/rules" element={<ManageRulesPage />} />
+                <Route path="/settings" element={<Settings />} />
+
+                <Route
+                  path="/gocardless/link"
+                  element={
+                    <NarrowNotSupported>
+                      <WideComponent name="GoCardlessLink" />
+                    </NarrowNotSupported>
+                  }
+                />
+
+                <Route
+                  path="/accounts"
+                  element={<NarrowAlternate name="Accounts" />}
+                />
+
+                <Route
+                  path="/accounts/:id"
+                  element={<NarrowAlternate name="Account" />}
+                />
+
+                <Route
+                  path="/transactions/:transactionId"
+                  element={
+                    <WideNotSupported>
+                      <TransactionEdit />
+                    </WideNotSupported>
+                  }
+                />
+
+                <Route
+                  path="/categories/:id"
+                  element={
+                    <WideNotSupported>
+                      <Category />
+                    </WideNotSupported>
+                  }
+                />
+
+                {/* redirect all other traffic to the budget page */}
+                <Route path="/*" element={<Navigate to="/budget" replace />} />
+              </Routes>
+            </View>
 
             <Routes>
-              <Route
-                path="/"
-                element={
-                  accountsLoaded ? (
-                    accounts.length > 0 ? (
-                      <Navigate to="/budget" replace />
-                    ) : (
-                      // If there are no accounts, we want to redirect the user to
-                      // the All Accounts screen which will prompt them to add an account
-                      <Navigate to="/accounts" replace />
-                    )
-                  ) : (
-                    <LoadingIndicator />
-                  )
-                }
-              />
-
-              <Route path="/reports/*" element={<Reports />} />
-
-              <Route
-                path="/budget"
-                element={<NarrowAlternate name="Budget" />}
-              />
-
-              <Route
-                path="/schedules"
-                element={
-                  <NarrowNotSupported>
-                    <WideComponent name="Schedules" />
-                  </NarrowNotSupported>
-                }
-              />
-
-              <Route path="/payees" element={<ManagePayeesPage />} />
-              <Route path="/rules" element={<ManageRulesPage />} />
-              <Route path="/settings" element={<Settings />} />
-
-              <Route
-                path="/gocardless/link"
-                element={
-                  <NarrowNotSupported>
-                    <WideComponent name="GoCardlessLink" />
-                  </NarrowNotSupported>
-                }
-              />
-
-              <Route
-                path="/accounts"
-                element={<NarrowAlternate name="Accounts" />}
-              />
-
-              <Route
-                path="/accounts/:id"
-                element={<NarrowAlternate name="Account" />}
-              />
-
-              <Route
-                path="/transactions/:transactionId"
-                element={
-                  <WideNotSupported>
-                    <TransactionEdit />
-                  </WideNotSupported>
-                }
-              />
-
-              <Route
-                path="/categories/:id"
-                element={
-                  <WideNotSupported>
-                    <Category />
-                  </WideNotSupported>
-                }
-              />
-
-              {/* redirect all other traffic to the budget page */}
-              <Route path="/*" element={<Navigate to="/budget" replace />} />
+              <Route path="/budget" element={<MobileNavTabs />} />
+              <Route path="/accounts" element={<MobileNavTabs />} />
+              <Route path="/settings" element={<MobileNavTabs />} />
+              <Route path="/reports" element={<MobileNavTabs />} />
+              <Route path="*" element={null} />
             </Routes>
-          </View>
-
-          <Routes>
-            <Route path="/budget" element={<MobileNavTabs />} />
-            <Route path="/accounts" element={<MobileNavTabs />} />
-            <Route path="/settings" element={<MobileNavTabs />} />
-            <Route path="/reports" element={<MobileNavTabs />} />
-            <Route path="*" element={null} />
-          </Routes>
+          </ScrollProvider>
         </View>
       </View>
     </View>

packages/desktop-client/src/components/ScrollProvider.tsx

@@ -1,61 +1,204 @@
-// @ts-strict-ignore
 import React, {
   type ReactNode,
+  type RefObject,
   createContext,
-  useState,
   useContext,
   useEffect,
   useCallback,
+  useRef,
 } from 'react';
 
 import debounce from 'debounce';
 
+type ScrollDirection = 'up' | 'down' | 'left' | 'right';
+
+type ScrollListenerArgs = {
+  scrollX: number;
+  scrollY: number;
+  isScrolling: (direction: ScrollDirection) => boolean;
+  hasScrolledToEnd: (direction: ScrollDirection, tolerance?: number) => boolean;
+};
+
+type ScrollListener = (args: ScrollListenerArgs) => void;
+type UnregisterScrollListener = () => void;
+type RegisterScrollListener = (
+  listener: ScrollListener,
+) => UnregisterScrollListener;
+
 type IScrollContext = {
-  scrollY: number | undefined;
-  hasScrolledToBottom: (tolerance?: number) => boolean;
+  registerScrollListener: RegisterScrollListener;
 };
 
 const ScrollContext = createContext<IScrollContext | undefined>(undefined);
 
-type ScrollProviderProps = {
+type ScrollProviderProps<T extends Element> = {
+  scrollableRef: RefObject<T>;
+  isDisabled?: boolean;
+  delayMs?: number;
   children?: ReactNode;
 };
 
-export function ScrollProvider({ children }: ScrollProviderProps) {
-  const [scrollY, setScrollY] = useState(undefined);
-  const [scrollHeight, setScrollHeight] = useState(undefined);
-  const [clientHeight, setClientHeight] = useState(undefined);
+export function ScrollProvider<T extends Element>({
+  scrollableRef,
+  isDisabled,
+  delayMs = 10,
+  children,
+}: ScrollProviderProps<T>) {
+  const previousScrollX = useRef<number | undefined>(undefined);
+  const scrollX = useRef<number | undefined>(undefined);
+  const previousScrollY = useRef<number | undefined>(undefined);
+  const scrollY = useRef<number | undefined>(undefined);
+  const scrollWidth = useRef<number | undefined>(undefined);
+  const scrollHeight = useRef<number | undefined>(undefined);
+  const clientWidth = useRef<number | undefined>(undefined);
+  const clientHeight = useRef<number | undefined>(undefined);
+  const listeners = useRef<ScrollListener[]>([]);
 
-  const hasScrolledToBottom = useCallback(
-    (tolerance = 1) => scrollHeight - scrollY <= clientHeight + tolerance,
-    [clientHeight, scrollHeight, scrollY],
+  const hasScrolledToEnd = useCallback(
+    (direction: ScrollDirection, tolerance = 1) => {
+      const isAtStart = (currentCoordinate?: number) =>
+        currentCoordinate !== undefined && currentCoordinate <= tolerance;
+
+      const isAtEnd = (
+        totalSize?: number,
+        currentCoordinate?: number,
+        viewportSize?: number,
+      ) =>
+        totalSize !== undefined &&
+        currentCoordinate !== undefined &&
+        viewportSize !== undefined &&
+        totalSize - currentCoordinate <= viewportSize + tolerance;
+
+      switch (direction) {
+        case 'up': {
+          return isAtStart(scrollY.current);
+        }
+        case 'down': {
+          return isAtEnd(
+            scrollHeight.current,
+            scrollY.current,
+            clientHeight.current,
+          );
+        }
+        case 'left': {
+          return isAtStart(scrollX.current);
+        }
+        case 'right': {
+          return isAtEnd(
+            scrollWidth.current,
+            scrollX.current,
+            clientWidth.current,
+          );
+        }
+        default:
+          return false;
+      }
+    },
+    [],
   );
 
-  useEffect(() => {
-    const listenToScroll = debounce(e => {
-      const target = e.target;
-      setScrollY(target?.scrollTop || 0);
-      setScrollHeight(target?.scrollHeight || 0);
-      setClientHeight(target?.clientHeight || 0);
-    }, 10);
+  const isScrolling = useCallback((direction: ScrollDirection) => {
+    switch (direction) {
+      case 'up':
+        return (
+          previousScrollY.current !== undefined &&
+          scrollY.current !== undefined &&
+          previousScrollY.current > scrollY.current
+        );
+      case 'down':
+        return (
+          previousScrollY.current !== undefined &&
+          scrollY.current !== undefined &&
+          previousScrollY.current < scrollY.current
+        );
+      case 'left':
+        return (
+          previousScrollX.current !== undefined &&
+          scrollX.current !== undefined &&
+          previousScrollX.current > scrollX.current
+        );
+      case 'right':
+        return (
+          previousScrollX.current !== undefined &&
+          scrollX.current !== undefined &&
+          previousScrollX.current < scrollX.current
+        );
+      default:
+        return false;
+    }
+  }, []);
 
-    window.addEventListener('scroll', listenToScroll, {
+  useEffect(() => {
+    if (isDisabled) {
+      return;
+    }
+
+    const listenToScroll = debounce((e: Event) => {
+      const target = e.target;
+      if (target instanceof Element) {
+        previousScrollX.current = scrollX.current;
+        scrollX.current = target.scrollLeft;
+        scrollHeight.current = target.scrollHeight;
+
+        previousScrollY.current = scrollY.current;
+        scrollY.current = target.scrollTop;
+        clientHeight.current = target.clientHeight;
+
+        const currentScrollX = scrollX.current;
+        const currentScrollY = scrollY.current;
+
+        if (currentScrollX !== undefined && currentScrollY !== undefined) {
+          listeners.current.forEach(listener =>
+            listener({
+              scrollX: currentScrollX,
+              scrollY: currentScrollY,
+              isScrolling,
+              hasScrolledToEnd,
+            }),
+          );
+        }
+      }
+    }, delayMs);
+
+    const ref = scrollableRef.current;
+
+    ref?.addEventListener('scroll', listenToScroll, {
       capture: true,
       passive: true,
     });
     return () =>
-      window.removeEventListener('scroll', listenToScroll, {
+      ref?.removeEventListener('scroll', listenToScroll, {
         capture: true,
       });
-  }, []);
+  }, [delayMs, hasScrolledToEnd, isDisabled, isScrolling, scrollableRef]);
+
+  const registerScrollListener: RegisterScrollListener = useCallback(
+    listener => {
+      listeners.current.push(listener);
+
+      return () => {
+        listeners.current = listeners.current.filter(l => l !== listener);
+      };
+    },
+    [],
+  );
 
   return (
-    <ScrollContext.Provider value={{ scrollY, hasScrolledToBottom }}>
+    <ScrollContext.Provider value={{ registerScrollListener }}>
       {children}
     </ScrollContext.Provider>
   );
 }
 
-export function useScroll(): IScrollContext {
-  return useContext(ScrollContext);
+export function useScrollListener(listener: ScrollListener) {
+  const context = useContext(ScrollContext);
+  if (!context) {
+    throw new Error('useScrollListener must be used within a ScrollProvider');
+  }
+
+  const { registerScrollListener } = context;
+
+  useEffect(() => {
+    return registerScrollListener(listener);
+  }, [listener, registerScrollListener]);
 }

packages/desktop-client/src/components/mobile/MobileNavTabs.tsx

@@ -1,7 +1,7 @@
 // @ts-strict-ignore
 import React, {
+  useCallback,
   type ComponentType,
-  useEffect,
   type CSSProperties,
 } from 'react';
 import { NavLink } from 'react-router-dom';
@@ -9,7 +9,6 @@ import { useSpring, animated, config } from 'react-spring';
 
 import { useDrag } from '@use-gesture/react';
 
-import { usePrevious } from '../../hooks/usePrevious';
 import {
   SvgAdd,
   SvgCog,
@@ -23,16 +22,20 @@ import { SvgCalendar } from '../../icons/v2';
 import { theme, styles } from '../../style';
 import { View } from '../common/View';
 import { useResponsive } from '../responsive/ResponsiveProvider';
-import { useScroll } from '../ScrollProvider';
+import { useScrollListener } from '../ScrollProvider';
 
 const COLUMN_COUNT = 3;
 const PILL_HEIGHT = 15;
 const ROW_HEIGHT = 70;
+const TOTAL_HEIGHT = ROW_HEIGHT * COLUMN_COUNT;
+const OPEN_FULL_Y = 1;
+const OPEN_DEFAULT_Y = TOTAL_HEIGHT - ROW_HEIGHT;
+const HIDDEN_Y = TOTAL_HEIGHT;
+
 export const MOBILE_NAV_HEIGHT = ROW_HEIGHT + PILL_HEIGHT;
 
 export function MobileNavTabs() {
   const { isNarrowWidth } = useResponsive();
-  const { scrollY } = useScroll();
 
   const navTabStyle = {
     flex: `1 1 ${100 / COLUMN_COUNT}%`,
@@ -96,53 +99,50 @@ export function MobileNavTabs() {
     <div key={idx} style={navTabStyle} />
   ));
 
-  const totalHeight = ROW_HEIGHT * COLUMN_COUNT;
-  const openY = 0;
-  const closeY = totalHeight - ROW_HEIGHT;
-  const hiddenY = totalHeight;
+  const [{ y }, api] = useSpring(() => ({ y: OPEN_DEFAULT_Y }));
 
-  const [{ y }, api] = useSpring(() => ({ y: totalHeight }));
+  const openFull = useCallback(
+    ({ canceled }) => {
+      // when cancel is true, it means that the user passed the upwards threshold
+      // so we change the spring config to create a nice wobbly effect
+      api.start({
+        y: OPEN_FULL_Y,
+        immediate: false,
+        config: canceled ? config.wobbly : config.stiff,
+      });
+    },
+    [api, OPEN_FULL_Y],
+  );
 
-  const open = ({ canceled }) => {
-    // when cancel is true, it means that the user passed the upwards threshold
-    // so we change the spring config to create a nice wobbly effect
-    api.start({
-      y: openY,
-      immediate: false,
-      config: canceled ? config.wobbly : config.stiff,
-    });
-  };
+  const openDefault = useCallback(
+    (velocity = 0) => {
+      api.start({
+        y: OPEN_DEFAULT_Y,
+        immediate: false,
+        config: { ...config.stiff, velocity },
+      });
+    },
+    [api, OPEN_DEFAULT_Y],
+  );
 
-  const close = (velocity = 0) => {
-    api.start({
-      y: closeY,
-      immediate: false,
-      config: { ...config.stiff, velocity },
-    });
-  };
+  const hide = useCallback(
+    (velocity = 0) => {
+      api.start({
+        y: HIDDEN_Y,
+        immediate: false,
+        config: { ...config.stiff, velocity },
+      });
+    },
+    [api, HIDDEN_Y],
+  );
 
-  const hide = (velocity = 0) => {
-    api.start({
-      y: hiddenY,
-      immediate: false,
-      config: { ...config.stiff, velocity },
-    });
-  };
-
-  const previousScrollY = usePrevious(scrollY);
-
-  useEffect(() => {
-    if (
-      scrollY &&
-      previousScrollY &&
-      scrollY > previousScrollY &&
-      previousScrollY !== 0
-    ) {
+  useScrollListener(({ isScrolling }) => {
+    if (isScrolling('down')) {
       hide();
-    } else {
-      close();
+    } else if (isScrolling('up')) {
+      openDefault();
     }
-  }, [scrollY]);
+  });
 
   const bind = useDrag(
     ({
@@ -163,9 +163,9 @@ export function MobileNavTabs() {
       // the threshold for it to close, or if we reset it to its open position
       if (last) {
         if (oy > ROW_HEIGHT * 0.5 || (vy > 0.5 && dy > 0)) {
-          close(vy);
+          openDefault(vy);
         } else {
-          open({ canceled });
+          openFull({ canceled });
         }
       } else {
         // when the user keeps dragging, we just move the sheet according to
@@ -176,7 +176,7 @@ export function MobileNavTabs() {
     {
       from: () => [0, y.get()],
       filterTaps: true,
-      bounds: { top: -totalHeight, bottom: totalHeight - ROW_HEIGHT },
+      bounds: { top: -TOTAL_HEIGHT, bottom: TOTAL_HEIGHT - ROW_HEIGHT },
       axis: 'y',
       rubberband: true,
     },
@@ -192,7 +192,7 @@ export function MobileNavTabs() {
         backgroundColor: theme.mobileNavBackground,
         borderTop: `1px solid ${theme.menuBorder}`,
         ...styles.shadow,
-        height: totalHeight + PILL_HEIGHT,
+        height: TOTAL_HEIGHT + PILL_HEIGHT,
         width: '100%',
         position: 'fixed',
         zIndex: 100,
@@ -216,7 +216,7 @@ export function MobileNavTabs() {
           style={{
             flexDirection: 'row',
             flexWrap: 'wrap',
-            height: totalHeight,
+            height: TOTAL_HEIGHT,
             width: '100%',
           }}
         >

packages/desktop-client/src/components/mobile/transactions/ListBox.jsx

@@ -2,8 +2,7 @@ import React, { useRef } from 'react';
 import { useListBox } from 'react-aria';
 import { useListState } from 'react-stately';
 
-import { usePrevious } from '../../../hooks/usePrevious';
-import { useScroll } from '../../ScrollProvider';
+import { useScrollListener } from '../../ScrollProvider';
 
 import { ListBoxSection } from './ListBoxSection';
 
@@ -13,13 +12,12 @@ export function ListBox(props) {
   const { listBoxProps, labelProps } = useListBox(props, state, listBoxRef);
   const { loadMore } = props;
 
-  const { hasScrolledToBottom } = useScroll();
-  const scrolledToBottom = hasScrolledToBottom(5);
-  const prevScrolledToBottom = usePrevious(scrolledToBottom);
-
-  if (!prevScrolledToBottom && scrolledToBottom) {
-    loadMore?.();
-  }
+  useScrollListener(({ hasScrolledToEnd }) => {
+    const scrolledToBottom = hasScrolledToEnd('down', 5);
+    if (scrolledToBottom) {
+      loadMore?.();
+    }
+  });
 
   return (
     <>

upcoming-release-notes/3731.md

@@ -0,0 +1,6 @@
+---
+category: Bugfix
+authors: [joel-jeremy]
+---
+
+Fix mobile navigation tabs scrolling when scrolling anywhere in the app e.g. scrolling through category/group notes in mobile budget view.