React-native-web: Issue with opening modal after updating version to '0.13.1' from '0.12.0'

[anujraghuvanshi]

The problem
Issue with package: 'modal-enhanced-react-native-web' .
I am using this package for Modal. In v0.12.0, It was working fine. But I have to update react-native-web package due to these issues. But after updating, it is breaking modal functionalities. I am able to open Modal once, after that it doesn't open (In 0.13.1). And different behaviours are there in each version above 0.13.0

How to reproduce

1. Fork it - https://codesandbox.io/s/6lx6ql1w5r
2. Add package modal-enhanced-react-native-web
3. Add code to open modal
4. Final Sandbox URL - https://codesandbox.io/s/2c4y1

Note: It has different behaviour in all version above v0.12.0 of react-native-web. I want modal behaviour same as its above 0.12.0

Steps to reproduce:

1. Open https://codesandbox.io/s/2c4y1
2. Click on button added to open modal
3. You will see modal opened.
4. Click anywhere to close modal. Now it can be seen that modal fluctuates and then closes. In my use case, It closes opens modal when it is clicked anywhere on modal. (It has different and random behaviour on each version ^0.13.0).

Expected behavior

• It should work same as it works in v0.12.0 in the latest version also (v0.13.3)

Environment (include versions). Did this work in earlier versions?

• React Native for Web (version): Issue on versions above 0.12.0
• React (version): 16.12.0
• Browser: Chrome@latest

[tajinder-logiciel]

facing the same issue

[Anuj-logiciel]

@necolas - Could you please confirm for an earlier update or resolution of this bug?

[SimonVanneste]

I had the same problem, resolved it to a problem with TouchableWithoutFeedback: https://github.com/necolas/react-native-web/issues/1663
https://github.com/Dekoruma/react-native-web-modal/blob/316619e5e3f13c9e28e00da399d7ffc4eb23f22b/packages/modal-enhanced-react-native-web/src/index.js#L480

[necolas]

This could be a bug in that modal package. It hasn't been updated for 2 years and has a peer dependency on react-native-web 0.9.

Any time React calls the ref function, the package tries to re-open the modal:

https://github.com/Dekoruma/react-native-web-modal/blob/316619e5e3f13c9e28e00da399d7ffc4eb23f22b/packages/modal-enhanced-react-native-web/src/index.js#L328-L333

And every time it renders, it creates a new copy of the event handlers:

https://github.com/Dekoruma/react-native-web-modal/blob/316619e5e3f13c9e28e00da399d7ffc4eb23f22b/packages/modal-enhanced-react-native-web/src/index.js#L444

[anujraghuvanshi]

Thanks @necolas! Your response helped me to get that. As this is not issue of this package. Hence Closing now.

[necolas]

It could be a bug in this library. The behavior has definitely changed. I need help from someone who wants to investigate what is going on, because I don't have time to look into an issue like this unless it's a regression in a maintained package.

[tajinder-logiciel]

@necolas I am facing one issue onLayout event is not working after 0.12.3 version on ios devices
@Anuj-Raghuvanshi

[necolas]

@tajinder-logiciel that's not relevant to this issue. If you read the release notes you will see that you must include a resize observer polyfill for 0.13

[tajinder-logiciel]

@necolas okay thanks

[imnotjames]

Did some research on this issue.

TLDR: In 0.13+ when everything was refactored to functional components, View calls setAndForwardRef's function any time the props change which causes the Modal to transition from open to open then close.

Class based components only seemed to call the underlying function when the ref has changed.

If this is something we do want to correct we would need to ensure that the function returned by setAndForwardRef is only called the underlying function when the ref has changed.

---

The standard package, modal-react-native-web that's also in that repository works just fine with animation & react-native-web 0.13.12 - so the issue is between the enhanced only & something to do with a change between 0.12.3 & 0.13.3

In 0.12.3 it does indeed work as expected.
In 0.13.0 it stops working nearly entirely because the TouchableWithoutFeedback lost the ability to set the ref.
In 0.13.2 the issue with TouchableWithoutFeedback was fixed but this issue started occurring.
In 0.13.12 the issue still occurs.

The animation is two animations that are layered to create the intended effect. The animation for the content slides out of view & the backdrop animates until it has an opacity of 0.

The backdrop animation works as expected. Inspecting the animation values yields values that make sense. Looking at how the animations for the slide work.. well, that's a bit easier to determine once we look further both at the source (and something Necolas brought up) --

Any time react calls the set ref function the package will attempt to reopen the modal. The close fires off, changing the visible prop, which then updates the View that the modal uses to wrap everything which fires off the content ref handler - the open function is called so the animation for opening fires off yet again.. and THEN the close fires off.

All of this uses react-native-animatable.

For the backdrop it works so well because it uses transitionTo - so it takes the current state & transitions it to the desired state. In this case for opacity it goes from 0.7 to 0.7 - no change.

However, for the content to slide it uses the helper slideUp from react-native-animatable. This resets it back to the bottom & slides the content up. Only then will the close handler be called & then slide it back down.

In 0.12 this wasn't a problem because we didn't use setAndForwardRef

[imnotjames]

One possibility for why the result of setAndForwardRef is constantly called is because it always returns a new function meaning that the ref property passed to View is always changing, causing it to fire off.

I tested this via a small experiment where I updated View to use the following:

  const setRef = useMemo(
    () => setAndForwardRef({
      getForwardedRef: () => forwardedRef,
      setLocalRef: hostNode => {
        hostRef.current = hostNode;
      },
    }),
    []
  );

This prevented the function returned fromsetAndForwardRef` from being called on every prop change.

Given the functions that are passed to setAndForwardRef - we might want to wrap the calls to it in useMemo in each place where it's being set. Thoughts, @necolas ? It wouldn't be too bad to add some test cases to View for this & verify it via those as well.

Side Benefit to this is that it would help prevent unnecessary re-renders. :)

[imnotjames]

I've added test to a branch in that exhibit this behavior. The tests create a mock and passes it to the ref prop & counts the number of times it's called - ensuring it's only 1 during re-renders. With the useMemo call as above the test passes :green_apple: . Without it, the test fails :red_square: .

Thing is, the issue applies to every component that uses setAndForwardRef. setAndForwardRef isn't a hook (at least, it's not named as such) so I can't refactor it to use useMemo internally. Even if I did, the functions that get passed into it would change on every render - thus negating the memo's efficacy.

I can update all components that use this to fix it, but it does feel a bit messy.

[imnotjames]

Pressable & TouchableWithFeedback do not exhibit this issue - except they refuses to ever call ref again after the first time, even if the ref function changes. I found this when trying to create test cases for them as well.

Edit: Seems it was some weird interactions between View and TouchableWithFeedback

[d4rky-pl]

The problem still persists on the reproducible example from this ticket on the latest version of RNW:

https://codesandbox.io/s/quizzical-sea-xu5xu

There's also another bug that seems to have started triggering around 0.13 - whenever the children props change, the modal is also re-rendered and animation retriggered. This did not happen before:

https://codesandbox.io/s/awesome-flower-t15sg

[d4rky-pl]

This issue should be reopened :)

[imnotjames]

The issue was fixed for the .14 release, wasn't it?

[necolas]

Yeah I had to revert the fix because it broke react native paper. The 3rd party modal package is just buggy because it assumes refs are only called on mount, which is not a guarantee in react. So that package needs to be patched

[d4rky-pl]

@necolas ooooh, so that's what it is! I guess it's forking time 😄 Thanks for letting us know!

[necolas]

I'm hoping that the next patch release (try it here: 0.0.0-583e16fa8) will fix this issue without causing problems for react native paper (#1749). Both the codesandbox test cases work with 0.0.0-583e16fa8. Give it a try and lmk