[$250] [Dev][StrictMode] Popover menu has no background smallscreen

[m-natarajan]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number:
Reproducible in staging?: DEV only
Reproducible in production?: DEV only
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @ishpaul777
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1720819445275459

Action Performed:

Long press on any report in LHN

Expected Result:

Popover menus haves translucent background

Actual Result:

Popover menu dont have background

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Screen.Recording.2024-07-13.at.2.49.31.AM.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01ff564d543a9355a6
• Upwork Job ID: 1812925525309666872
• Last Price Increase: 2024-07-29
• Automatic offers:
• dominictb | Contributor | 103339816

Issue Owner

Current Issue Owner: @allroundexperts

[melvin-bot]

Triggered auto assignment to @lschurr (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[MelvinBot]

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01ff564d543a9355a6

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @allroundexperts (External)

[lschurr]

I'm not able to test since I don't work in a DEV environment. @allroundexperts could you try reproducing this?

[allroundexperts]

It's easily reproducible @lschurr.

Screen.Recording.2024-07-16.at.12.05.09.AM.mov

[isogit123]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Popover menu has no background in small screens in the development environment when using the strict mode.

What is the root cause of that problem?

ReactNativeModal isn't showing the backdrop background and the opacity of the background is set to zero when the strict mode is enabled. The background appears as expected if the strict mode is disabled. As dominictb mentioned in their proposal, as React renders components twice in the strict mode, the backdrop animatable.View component is initialized with the correct state in the first render, but in the second render, the state is reset so the opacity value is lost. This can be verified by logging the state of the animatable.View component in the componentDidMount event with and without strict mode.

What changes do you think we should make in order to solve the problem?

Using the ReactNativeModal ref, we can set the opacity to the desired value so the background appears as expected by adding the following code snippet:

const modalRef = useRef(null);
try{
    modalRef.current.backdropRef.ref.style.opacity = !shouldUseCustomBackdrop && hideBackdrop ? 0 : variables.overlayOpacity;
}
catch {}

in line 209 at:

App/src/components/Modal/BaseModal.tsx

Lines 205 to 218 in 105dce5

	activeModalType: isVisible ? type : undefined,
	}),
	[isVisible, type],
	);
	return (
	<ModalContext.Provider value={modalContextValue}>
	<View
	// this is a workaround for modal not being visible on the new arch in some cases
	// it's necessary to have a non-collapseable view as a parent of the modal to prevent
	// a conflict between RN core and Reanimated shadow tree operations
	// position absolute is needed to prevent the view from interfering with flex layout
	collapsable={false}
	style={[styles.pAbsolute, {zIndex: 1}]}

then bind the ref to the ReactNativeModal at line 224 in the following code snippet:

App/src/components/Modal/BaseModal.tsx

Lines 220 to 230 in 105dce5

	<ReactNativeModal
	// Prevent the parent element to capture a click. This is useful when the modal component is put inside a pressable.
	onClick={(e) => e.stopPropagation()}
	onBackdropPress={handleBackdropPress}
	// Note: Escape key on web/desktop will trigger onBackButtonPress callback
	// eslint-disable-next-line react/jsx-props-no-multi-spaces
	onBackButtonPress={Modal.closeTop}
	onModalShow={handleShowModal}
	propagateSwipe={propagateSwipe}
	onModalHide={hideModal}
	onModalWillShow={saveFocusState}

Below is a screenshot after applying the change:

What alternative solutions did you explore? (Optional)

The last commit to the React Native Modal library used in the Popover menu is 2 years old and is looking for maintainers, which is mentioned in https://github.com/react-native-modal/react-native-modal?tab=readme-ov-file#announcements, so we can use the React Native built-in Modal component or any other wrapper library that is compatible with the strict mode and maintained.

[dominictb]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Popover menu don't have background when strict mode is enabled in dev env

What is the root cause of that problem?

We have 2 problems here in dev strict mode, and I'll try to fix both:

• The opacity is not there for popover menu
• The appearance transition of the menu is not smooth.

For first problem, the issue is because in strict mode, React will try to simulate mount -> unmount -> remount the component, and during that simulation, the state of the component is reset. So, when this is called during during the first mounting phase, the state of the backdrop animatable.View component is initialized correctly, but after the unmount/remount simulation phase, that state is being refreshed, hence we lost the opacity in the final computed style of the backdrop (check this)

The second problem has the same root cause, the difference is that the final computed style is correct, but this animation trigger doesn't run on the remount simulation, the context menu seems to pop-up right away instead of a smooth transition

What changes do you think we should make in order to solve the problem?

We either can patch or contribute to the original package, but the idea is that:

• Provide a props onMounted for createAnimatableComponent HOC

 onMounted: PropTypes.func,

 componentDidMount() {
      const {
        animation,
        duration,
        delay,
        onAnimationBegin,
        iterationDelay,
        onMounted,
      } = this.props;
      if (animation) {
        const startAnimation = () => {
          onAnimationBegin();
          this.startAnimation(duration, 0, iterationDelay, endState =>
            this.props.onAnimationEnd(endState),
          );
          this.delayTimer = null;
        };
        if (delay) {
          this.delayTimer = setTimeout(startAnimation, delay);
        } else {
          startAnimation();
        }
      }

      if (onMounted) {
        onMounted();
      }
    }

• In react-native-modal, make sure that the backdropRef and contentRef will trigger the relevant transition and animation if the respective component is mounted (or remounted), as at this point their states have been reset already

this.onBackdropMounted = () => {
            if (this.isTransitioning) {
                return;
            }
            if (this.backdropRef) {
                this.backdropRef.transitionTo({ opacity: this.props.backdropOpacity }, this.props.backdropTransitionInTiming);
            }
        }

        this.contentContainerMounted = () => {
            if (this.isTransitioning) {
                return;
            }
            if (this.contentRef) {
                this.props.onModalWillShow && this.props.onModalWillShow();
                if (this.interactionHandle == null) {
                    this.interactionHandle = InteractionManager.createInteractionHandle();
                }
                this.contentRef
                    .animate(this.animationIn, this.props.animationInTiming)
                    .then(() => {
                    this.isTransitioning = false;
                    if (this.interactionHandle) {
                        InteractionManager.clearInteractionHandle(this.interactionHandle);
                        this.interactionHandle = null;
                    }
                    if (!this.props.isVisible) {
                        this.close();
                    }
                    else {
                        this.props.onModalShow();
                    }
                });
            }
        }
        
        ....
       const backdropWrapper = (React.createElement(animatable.View, { ref: ref => (this.backdropRef = ref), useNativeDriver: useNativeDriverForBackdrop !== undefined
                    ? useNativeDriverForBackdrop
                    : useNativeDriver, style: [styles.backdrop, backdropComputedStyle], onMounted: this.onBackdropMounted.bind(this) }, hasCustomBackdrop && customBackdrop));

     ....
     const containerView = (React.createElement(animatable.View, Object.assign({}, panHandlers, { ref: ref => (this.contentRef = ref), style: [panPosition, computedStyle], pointerEvents: "box-none", useNativeDriver: useNativeDriver, onMounted: this.contentContainerMounted.bind(this) }, containerProps), _children));

What alternative solutions did you explore? (Optional)

We can consider using a custom backdrop to manage the opacity transition ourselves, which allow us to provide a robust implementation that satisifies the strict mode. However, the second problem remains (in StrictMode dev env only), but if we consider it non-important, we can go with this approach.

[allroundexperts]

Thanks for the proposal @dominictb. Your RCA seems to be accurate, but I'm not sure if patching / editing the package is the correct way to proceed here. Software mansion maintains the react-native-reanimated library, and I think it would be great if we can consult about this with them in the open source channel. Can you spin up a message and tag me? Thanks!

[allroundexperts]

@isogit123 Your RCA is incomplete. Please make sure to include proper root cause in order for the proposal to be accepted.

[dominictb]

Open a discussion in Slack https://expensify.slack.com/archives/C01GTK53T8Q/p1721299326263619

[isogit123]

Proposal updated

[dominictb]

@allroundexperts sorry but I think we are referring to 2 different libraries here. The package mentioned in my proposal is react-native-animatable, not react-native-reanimated. I don't think we maintain the react-native-animatable right?

[lschurr]

Any update on this one @allroundexperts?

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[allroundexperts]

@allroundexperts sorry but I think we are referring to 2 different libraries here. The package mentioned in my proposal is react-native-animatable, not react-native-reanimated. I don't think we maintain the react-native-animatable right?

Yes, my bad. Thinking about it now, can we just use react-native-reanimated only here?

[dominictb]

@allroundexperts react-native-animatable is used inside react-native-modal, and as we are dealing with 2 problems here:

• backdrop blurring is not happening: This can be solved with the idea of not touching react-native-animatable by using a custom backdrop, so we can control the animation/transition there. We can use react-native-reanimated, but I still need to confirm if react-native-reanimated 3.8.1 works fine with strict mode: On web, with React's strict mode, animations don't work on first render software-mansion/react-native-reanimated#6264

• The appearance animation of the menu is not smooth: The animation is applied directly to the menu container but react-native-modal doesn't allow us to control that (unlike backdrop, where we could define a custom one). So either we need to replace the react-native-modal library or doing some patches as I mentioned.

[melvin-bot]

@allroundexperts, @lschurr Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[lschurr]

Bump on this @allroundexperts

[dominictb]

@allroundexperts we got 1 patch for react-native-modal, but none for react-native-animatable

We are using react-native-animatable in the codebase though, for SignInPageLayout/BackgroundImage.tsx

[allroundexperts]

Okay. Can we somehow not use react-native-animatable in our codebase and just use react-native-reanimated instead?

[dominictb]

We still need to patch react-native-animatable to fix the second issue mentioned here: #45385 (comment)

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot]

@allroundexperts @lschurr this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[allroundexperts]

We still need to patch react-native-animatable to fix the second issue mentioned here: #45385 (comment)

Ah, I see. I guess that's the best option then.

[allroundexperts]

@dominictb's proposal looks good to me. Let's create a patch for the package.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @flodnv, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[melvin-bot]

📣 @dominictb 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[dominictb]

On it today.

[dominictb]

PR should be up by today.

[lschurr]

Any update @dominictb?

[melvin-bot]

@flodnv, @allroundexperts, @lschurr, @dominictb Huh... This is 4 days overdue. Who can take care of this?

[flodnv]

Could you please review the PR @allroundexperts ?

[allroundexperts]

Reviewed!

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

This issue has not been updated in over 15 days. @flodnv, @allroundexperts, @lschurr, @dominictb eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!