fix: fix Modal flickering & cleanup

Author: BogiKay

src/components/Modal.tsx

@@ -112,27 +112,13 @@ function Modal({
   testID = 'modal',
 }: Props) {
   const theme = useInternalTheme(themeOverrides);
-  const visibleRef = React.useRef(visible);
-
-  React.useEffect(() => {
-    visibleRef.current = visible;
-  });
-
   const onDismissCallback = useLatestCallback(onDismiss);
-
   const { scale } = theme.animation;
-
   const { top, bottom } = useSafeAreaInsets();
-
   const opacity = useAnimatedValue(visible ? 1 : 0);
+  const [visibleInternal, setVisibleInternal] = React.useState(visible);
 
-  const [rendered, setRendered] = React.useState(visible);
-
-  if (visible && !rendered) {
-    setRendered(true);
-  }
-
-  const showModal = React.useCallback(() => {
+  const showModalAnimation = React.useCallback(() => {
     Animated.timing(opacity, {
       toValue: 1,
       duration: scale * DEFAULT_DURATION,
@@ -141,7 +127,7 @@ function Modal({
     }).start();
   }, [opacity, scale]);
 
-  const hideModal = React.useCallback(() => {
+  const hideModalAnimation = React.useCallback(() => {
     Animated.timing(opacity, {
       toValue: 0,
       duration: scale * DEFAULT_DURATION,
@@ -152,17 +138,24 @@ function Modal({
         return;
       }
 
-      if (visible) {
-        onDismissCallback();
-      }
-
-      if (visibleRef.current) {
-        showModal();
-      } else {
-        setRendered(false);
-      }
+      setVisibleInternal(false);
     });
-  }, [onDismissCallback, opacity, scale, showModal, visible]);
+  }, [opacity, scale]);
+
+  React.useEffect(() => {
+    if (visibleInternal === visible) {
+      return;
+    }
+
+    if (!visibleInternal && visible) {
+      setVisibleInternal(true);
+      return showModalAnimation();
+    }
+
+    if (visibleInternal && !visible) {
+      return hideModalAnimation();
+    }
+  }, [visible, showModalAnimation, hideModalAnimation, visibleInternal]);
 
   React.useEffect(() => {
     if (!visible) {
@@ -171,7 +164,7 @@ function Modal({
 
     const onHardwareBackPress = () => {
       if (dismissable || dismissableBackButton) {
-        hideModal();
+        onDismissCallback();
       }
 
       return true;
@@ -183,37 +176,26 @@ function Modal({
       onHardwareBackPress
     );
     return () => subscription.remove();
-  }, [dismissable, dismissableBackButton, hideModal, visible]);
-
-  const prevVisible = React.useRef<boolean | null>(null);
+  }, [dismissable, dismissableBackButton, onDismissCallback, visible]);
 
-  React.useEffect(() => {
-    if (prevVisible.current !== visible) {
-      if (visible) {
-        showModal();
-      } else {
-        hideModal();
-      }
-    }
-    prevVisible.current = visible;
-  });
-
-  if (!rendered) return null;
+  if (!visibleInternal) {
+    return null;
+  }
 
   return (
     <Animated.View
       pointerEvents={visible ? 'auto' : 'none'}
       accessibilityViewIsModal
       accessibilityLiveRegion="polite"
       style={StyleSheet.absoluteFill}
-      onAccessibilityEscape={hideModal}
+      onAccessibilityEscape={onDismissCallback}
       testID={testID}
     >
       <AnimatedPressable
         accessibilityLabel={overlayAccessibilityLabel}
         accessibilityRole="button"
         disabled={!dismissable}
-        onPress={dismissable ? hideModal : undefined}
+        onPress={dismissable ? onDismissCallback : undefined}
         importantForAccessibility="no"
         style={[
           styles.backdrop,

src/components/__tests__/Modal.test.tsx

@@ -98,22 +98,23 @@ describe('Modal', () => {
     });
   });
   describe('when open', () => {
-    describe('if closed via touching backdrop', () => {
-      it('will run the animation but not fade out', async () => {
+    describe('if backdrop touched', () => {
+      it('should invoke the onDismiss function immediately', () => {
+        const onDismiss = jest.fn();
         const { getByTestId } = render(
-          <Modal testID="modal" visible onDismiss={() => {}}>
+          <Modal testID="modal" visible onDismiss={onDismiss}>
             {null}
           </Modal>
         );
 
-        expect(getByTestId('modal-surface-outer-layer')).toHaveStyle({
-          opacity: 1,
-        });
+        expect(onDismiss).not.toHaveBeenCalled();
 
         act(() => {
           fireEvent.press(getByTestId('modal-backdrop'));
         });
 
+        expect(onDismiss).toHaveBeenCalled();
+
         expect(getByTestId('modal-surface-outer-layer')).toHaveStyle({
           opacity: 1,
         });
@@ -129,36 +130,58 @@ describe('Modal', () => {
         expect(getByTestId('modal-surface-outer-layer')).toHaveStyle({
           opacity: 1,
         });
+
+        expect(onDismiss).toHaveBeenCalledTimes(1);
       });
+    });
 
-      it('should invoke the onDismiss function after the animation', () => {
-        const onDismiss = jest.fn();
-        const { getByTestId } = render(
-          <Modal testID="modal" visible onDismiss={onDismiss}>
-            {null}
-          </Modal>
-        );
+    it('runs the closing animation if visible toggled', () => {
+      const { getByTestId, queryByTestId, rerender } = render(
+        <Modal testID="modal" visible onDismiss={() => {}}>
+          {null}
+        </Modal>
+      );
 
-        expect(onDismiss).not.toHaveBeenCalled();
+      expect(getByTestId('modal-surface-outer-layer')).toHaveStyle({
+        opacity: 1,
+      });
 
-        act(() => {
-          fireEvent.press(getByTestId('modal-backdrop'));
-        });
+      act(() => {
+        fireEvent.press(getByTestId('modal-backdrop'));
+      });
 
-        expect(onDismiss).not.toHaveBeenCalled();
+      rerender(
+        <Modal testID="modal" visible={false} onDismiss={() => {}}>
+          {null}
+        </Modal>
+      );
 
-        act(() => {
-          jest.runAllTimers();
-        });
+      expect(getByTestId('modal-surface-outer-layer')).toHaveStyle({
+        opacity: 1,
+      });
 
-        expect(onDismiss).toHaveBeenCalledTimes(1);
+      expect(getByTestId('modal-backdrop')).toHaveStyle({
+        opacity: 1,
+      });
+
+      expect(getByTestId('modal-surface-outer-layer')).toHaveStyle({
+        opacity: 1,
       });
+
+      act(() => {
+        jest.runAllTimers();
+      });
+
+      expect(queryByTestId('modal-surface-outer-layer')).not.toBeOnTheScreen();
+
+      expect(queryByTestId('modal-backdrop')).not.toBeOnTheScreen();
     });
 
     describe('if closed via Android back button', () => {
-      it('will run the animation but not fade out', async () => {
+      it('invokes onDismiss', async () => {
+        const onDismiss = jest.fn();
         const { getByTestId } = render(
-          <Modal testID="modal" visible onDismiss={() => {}}>
+          <Modal testID="modal" visible onDismiss={onDismiss}>
             {null}
           </Modal>
         );
@@ -186,28 +209,6 @@ describe('Modal', () => {
         expect(getByTestId('modal-surface-outer-layer')).toHaveStyle({
           opacity: 1,
         });
-      });
-
-      it('should invoke the onDismiss function after the animation', () => {
-        const onDismiss = jest.fn();
-
-        render(
-          <Modal testID="modal" visible onDismiss={onDismiss}>
-            {null}
-          </Modal>
-        );
-
-        expect(onDismiss).not.toHaveBeenCalled();
-
-        act(() => {
-          BackHandler.mockPressBack();
-        });
-
-        expect(onDismiss).not.toHaveBeenCalled();
-
-        act(() => {
-          jest.runAllTimers();
-        });
 
         expect(onDismiss).toHaveBeenCalledTimes(1);
       });