Fix no scroll behavior on iOS (#3131)

taneliang · web-flow · commit 95f7b3e0454d · 2021-01-15T00:13:34.000+08:00
diff --git a/website/package.json b/website/package.json
@@ -40,14 +40,14 @@
     "@testing-library/jest-dom": "5.11.8",
     "@testing-library/react": "11.2.3",
     "@testing-library/user-event": "12.6.0",
+    "@types/body-scroll-lock": "2.6.1",
     "@types/classnames": "2.2.11",
     "@types/enzyme": "3.10.8",
     "@types/jest": "26.0.20",
     "@types/json2mq": "0.2.0",
     "@types/leaflet": "1.5.19",
     "@types/lodash": "4.14.167",
     "@types/mousetrap": "1.6.5",
-    "@types/no-scroll": "2.1.0",
     "@types/query-string": "5.0.0",
     "@types/react": "17.0.0",
     "@types/react-beautiful-dnd": "13.0.0",
@@ -129,6 +129,7 @@
     "@sentry/tracing": "5.29.2",
     "@tippy.js/react": "3.1.1",
     "axios": "0.21.1",
+    "body-scroll-lock": "3.1.5",
     "bootstrap": "4.5.3",
     "bowser": "2.11.0",
     "browserslist-config-nusmods": "file:../packages/browserslist-config-nusmods",
@@ -143,7 +144,6 @@
     "leaflet-gesture-handling": "1.2.1",
     "lodash": "4.17.20",
     "mousetrap": "1.6.5",
-    "no-scroll": "2.1.1",
     "nusmoderator": "3.0.0",
     "query-string": "5.0.0",
     "react": "17.0.1",
diff --git a/website/src/styles/components/loading-spinner.scss b/website/src/styles/components/loading-spinner.scss
diff --git a/website/src/styles/layout/site.scss b/website/src/styles/layout/site.scss
@@ -16,10 +16,6 @@ body {
   @include media-breakpoint-down(sm) {
     padding-top: $navbar-height;
     font-size: 1rem * $sm-font-size-ratio;
-
-    &.no-scroll {
-      padding-top: $navbar-height + $navtab-height;
-    }
   }
 }
 
@@ -36,7 +32,8 @@ a {
 
 .main-container {
   flex: 1 auto;
-  padding: env(safe-area-inset-top) env(safe-area-inset-right) env(safe-area-inset-bottom) env(safe-area-inset-left);
+  padding: env(safe-area-inset-top) env(safe-area-inset-right) env(safe-area-inset-bottom)
+    env(safe-area-inset-left);
 
   @include media-breakpoint-down(sm) {
     .main-content {
diff --git a/website/src/styles/main.scss b/website/src/styles/main.scss
@@ -21,8 +21,6 @@
 @import 'layout/site';
 
 // Components
-@import 'components/loading-spinner';
-@import 'components/search-panel';
 @import 'components/sentry';
 @import 'components/buttons';
 @import 'components/kbd';
diff --git a/website/src/utils/disableScrolling.test.ts b/website/src/utils/disableScrolling.test.ts
diff --git a/website/src/utils/disableScrolling.ts b/website/src/utils/disableScrolling.ts
diff --git a/website/src/views/components/Modal.tsx b/website/src/views/components/Modal.tsx
@@ -1,55 +1,62 @@
-import * as React from 'react';
+import { FC, useCallback, useLayoutEffect, useState } from 'react';
 import ReactModal, { Props as ModalProps } from 'react-modal';
+import { disableBodyScroll, enableBodyScroll } from 'body-scroll-lock';
 import classnames from 'classnames';
 
-import disableScrolling from 'utils/disableScrolling';
 import styles from './Modal.scss';
 
 type Props = ModalProps & {
   isOpen: boolean;
   overlayClassName?: string;
   className?: string;
-  children?: React.ReactNode;
-  fullscreen: boolean;
+  fullscreen?: boolean;
   animate?: boolean;
 };
 
-export default class Modal extends React.Component<Props> {
-  static defaultProps = {
-    fullscreen: false,
-  };
-
-  componentDidMount() {
-    disableScrolling(this.props.isOpen);
-  }
-
-  componentDidUpdate(prevProps: Props) {
-    if (this.props.isOpen !== prevProps.isOpen) {
-      disableScrolling(this.props.isOpen);
+const Modal: FC<Props> = ({
+  isOpen,
+  overlayClassName,
+  className,
+  fullscreen = false,
+  animate,
+  children,
+  ...otherModalProps
+}) => {
+  // Because ReactModal's contentRef is only provided after all effects have
+  // executed, in order for `Modal` to react to the setting/unsetting of
+  // `contentRef`, `contentRef` needs to be stored in component state, even if
+  // this causes additional renders.
+  const [modalContent, setModalContent] = useState<HTMLDivElement | undefined>();
+  const contentRefCallback = useCallback((node) => setModalContent(node), []);
+
+  // Disable body scrolling if modal is open, but allow modal to scroll.
+  useLayoutEffect(() => {
+    if (!modalContent) {
+      return undefined;
     }
-  }
-
-  componentWillUnmount() {
-    // Ensure disableScrolling is disabled if the component is
-    // unmounted without the modal closing
-    disableScrolling(false);
-  }
-
-  render() {
-    const { className, overlayClassName, children, fullscreen, animate, ...rest } = this.props;
+    if (isOpen) {
+      disableBodyScroll(modalContent);
+      return () => enableBodyScroll(modalContent);
+    }
+    enableBodyScroll(modalContent);
+    return undefined;
+  }, [isOpen, modalContent]);
+
+  return (
+    <ReactModal
+      overlayClassName={classnames(styles.overlay, overlayClassName)}
+      className={classnames(styles.modal, className, {
+        [styles.fullscreen]: fullscreen,
+        [styles.animated]: animate,
+      })}
+      closeTimeoutMS={animate ? 150 : 0}
+      isOpen={isOpen}
+      contentRef={contentRefCallback}
+      {...otherModalProps}
+    >
+      {children}
+    </ReactModal>
+  );
+};
 
-    return (
-      <ReactModal
-        overlayClassName={classnames(styles.overlay, overlayClassName)}
-        className={classnames(styles.modal, className, {
-          [styles.fullscreen]: fullscreen,
-          [styles.animated]: animate,
-        })}
-        closeTimeoutMS={animate ? 150 : 0}
-        {...rest}
-      >
-        {children}
-      </ReactModal>
-    );
-  }
-}
+export default Modal;
diff --git a/website/src/views/components/SearchkitSearchBox.scss b/website/src/views/components/SearchkitSearchBox.scss
@@ -1,4 +1,6 @@
-.search-panel {
+@import '~styles/utils/modules-entry';
+
+.searchBox {
   position: sticky;
   top: $navbar-height;
   z-index: $module-finder-search-z-index-md;
@@ -22,11 +24,3 @@
     );
   }
 }
-
-.no-scroll .search-panel {
-  position: fixed;
-  top: $navbar-height;
-  right: 0;
-  left: 0;
-  margin: 0;
-}
diff --git a/website/src/views/components/SearchkitSearchBox.tsx b/website/src/views/components/SearchkitSearchBox.tsx
@@ -8,6 +8,7 @@ import classnames from 'classnames';
 
 import elements from 'views/elements';
 import SearchBox from 'views/components/SearchBox';
+import styles from './SearchkitSearchBox.scss';
 
 // The default URL query string key used for the search term
 const DEFAULT_SEARCH_QUERY_KEY = 'q';
@@ -99,7 +100,7 @@ export default class SearchkitSearchBox extends SearchkitComponent<Props, State>
     if (!this.queryAccessor()) return null;
     return (
       <SearchBox
-        className={classnames(elements.moduleFinderSearchBox, 'search-panel')}
+        className={classnames(styles.searchBox, elements.moduleFinderSearchBox)}
         throttle={this.props.throttle}
         useInstantSearch
         isLoading={this.isLoading()}
diff --git a/website/src/views/components/SideMenu.scss b/website/src/views/components/SideMenu.scss
@@ -1,4 +1,4 @@
-@import "~styles/utils/modules-entry";
+@import '~styles/utils/modules-entry';
 
 $header-margin-top: 1rem;
 $animation-duration: 0.3s;
@@ -60,7 +60,7 @@ $offset-top: $navbar-height + 1rem;
     $closed-menu-translation: calc(100% + 120px);
 
     width: 100%;
-    padding: 1rem 0 3rem;
+    padding: 3rem 0 3rem;
     background: var(--body-bg);
 
     // Entry/exit animation
diff --git a/website/src/views/components/SideMenu.tsx b/website/src/views/components/SideMenu.tsx
@@ -1,9 +1,9 @@
-import { FC, memo, useEffect } from 'react';
+import { FC, memo, useLayoutEffect, useRef } from 'react';
 import classnames from 'classnames';
+import { disableBodyScroll, enableBodyScroll } from 'body-scroll-lock';
 
 import { Menu, X as Close } from 'react-feather';
 import useMediaQuery from 'views/hooks/useMediaQuery';
-import disableScrolling from 'utils/disableScrolling';
 import { breakpointUp } from 'utils/css';
 import Fab from './Fab';
 
@@ -31,13 +31,22 @@ export const SideMenuComponent: FC<Props> = ({
   children,
 }) => {
   const matchBreakpoint = useMediaQuery(breakpointUp('md'));
-
   const isSideMenuShown = isOpen && !matchBreakpoint;
 
-  useEffect(() => {
-    disableScrolling(isSideMenuShown);
-    return () => disableScrolling(false);
-  });
+  // Disable body scrolling if side menu is open, but allow side menu to scroll.
+  const scrollableRef = useRef<HTMLDivElement>(null);
+  useLayoutEffect(() => {
+    const scrollable = scrollableRef.current;
+    if (!scrollable) {
+      return undefined;
+    }
+    if (isSideMenuShown) {
+      disableBodyScroll(scrollable);
+      return () => enableBodyScroll(scrollable);
+    }
+    enableBodyScroll(scrollable);
+    return undefined;
+  }, [isSideMenuShown]);
 
   return (
     <>
@@ -54,13 +63,18 @@ export const SideMenuComponent: FC<Props> = ({
       {/* boundaryContainer defines the top and bottom boundaries which sideMenu can extend to */}
       <div className={styles.boundaryContainer}>
         {/*
-            sideMenu is the scrollable menu element. On mobile, it expands
-            from the bottom of the screen to the top boundary of the
-            container; i.e. if the menu's content is shorter than the
-            container, it'll appear as a little action sheet rising from the
-            bottom of the screen.
-          */}
-        <div className={classnames(styles.sideMenu, { [styles.isOpen]: isOpen })}>{children}</div>
+          sideMenu is the scrollable menu element. On mobile, it expands
+          from the bottom of the screen to the top boundary of the
+          container; i.e. if the menu's content is shorter than the
+          container, it'll appear as a little action sheet rising from the
+          bottom of the screen.
+        */}
+        <div
+          className={classnames(styles.sideMenu, { [styles.isOpen]: isOpen })}
+          ref={scrollableRef}
+        >
+          {children}
+        </div>
       </div>
     </>
   );
diff --git a/website/src/views/layout/Navtabs.scss b/website/src/views/layout/Navtabs.scss
@@ -47,13 +47,6 @@
   }
 }
 
-@include media-breakpoint-down(sm) {
-  // position sticky doesn't work with no-scroll is active
-  :global(.no-scroll) .nav {
-    position: fixed;
-  }
-}
-
 .link {
   position: relative;
   flex: 1 0 auto;
diff --git a/website/yarn.lock b/website/yarn.lock
