Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

You cannot render into anything but top root #1502

Closed
diennguyentien opened this issue Jul 14, 2017 · 59 comments
Closed

You cannot render into anything but top root #1502

diennguyentien opened this issue Jul 14, 2017 · 59 comments
Assignees

Comments

@diennguyentien
Copy link

diennguyentien commented Jul 14, 2017

I get this error after update RN. The showLightBox is not shown


Environment

  • React Native Navigation version: 1.1.134
  • React Native version: 0.46.2
  • Platform(s) (iOS, Android, or both?): both
  • Device info (Simulator/Device? OS version? Debug/Release?): Release
@Bhullnatik
Copy link

I have somewhat the same issue, one the 2 errors come when dismissing a LightBox on Android.

Environment

  • React Native Navigation version: 1.1.134
  • React Native version: 0.46.2
  • Platform(s) (iOS, Android, or both?): Android
  • Device info (Simulator/Device? OS version? Debug/Release?): *

screenshot_1500323207
screenshot_1500323299

@lipingruan
Copy link

@diennguyentien @Bhullnatik

node_modules/react-native-navigation/android/app/src/main/java/com/reactnativenavigation/layouts/:

class SingleScreenLayout & BottomTabsLayout

rewrite function :

    @Override
    public void dismissLightBox() {
        if (lightBox != null) {
            lightBox.dismiss();
            // lightBox.hide();
            lightBox = null;
        }
    }

@diennguyentien
Copy link
Author

@lipingruan thanks you

@rgommezz
Copy link

Does @lipingruan workaround work for you guys?

@sijad
Copy link

sijad commented Jul 23, 2017

@lipingruan's workaround works.

@rgommezz
Copy link

rgommezz commented Jul 23, 2017

That will only work if you want to dismiss the lightbox from your screen, meaning by calling this.props.navigator.dismissLightBox() from your component.

However, if you wanna also close the lightbox by tapping in the background (common use case) using tapBackgroundToDismiss it will still break, i.e:

this.props.navigator.showLightBox({
  screen: screens.lightBox.sort,
  style: {
    backgroundBlur: 'dark',
    backgroundColor: 'rgba(0, 0, 0, 0.7)',
    tapBackgroundToDismiss: true,
  },
});

@lipingruan's workaround also removes entirely the animation on dismissal. My workaround works for all cases and keeps the animation, see rgommezz@c15bde4.

I am concerned if that would imply memory leaks though. @guyca do you have any thoughts on that? It'd likely break previous RN versions as well.

@lipingruan
Copy link

lipingruan commented Jul 24, 2017

@rauliyohmc
hello, I found bug is 'tag', and solved in new workaround, but it need you custom build RN.

file1: node_modules/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java :

rewrite function:

  private void detachViewFromInstance(
      ReactRootView rootView,
      CatalystInstance catalystInstance) {
    UiThreadUtil.assertOnUiThread();

    // int tag = rootView.getId();
    int tag = rootView.getRootViewTag();

    catalystInstance.getJSModule(AppRegistry.class)
        .unmountApplicationComponentAtRootTag(tag);
  }

file2: node_modules/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java :

rewrite function:

  protected synchronized void dropView(View view) {
    UiThreadUtil.assertOnUiThread();

    int tag;

    if ( view instanceof ReactRootView ) {

      tag = ((ReactRootView) view).getRootViewTag();
    } else {

      tag = view.getId();
    }

    if (!mRootTags.get( tag )) {
      // For non-root views we notify viewmanager with {@link ViewManager#onDropInstance}
      resolveViewManager( tag ).onDropViewInstance(view);
    }

    ViewManager viewManager = mTagsToViewManagers.get( tag );
    if (view instanceof ViewGroup && viewManager instanceof ViewGroupManager) {
      ViewGroup viewGroup = (ViewGroup) view;
      ViewGroupManager viewGroupManager = (ViewGroupManager) viewManager;
      for (int i = viewGroupManager.getChildCount(viewGroup) - 1; i >= 0; i--) {
        View child = viewGroupManager.getChildAt(viewGroup, i);
        if (mTagsToViews.get(child.getId()) != null) {
          dropView(child);
        }
      }
      viewGroupManager.removeAllViews(viewGroup);
    }
    mTagsToViews.remove( tag );
    mTagsToViewManagers.remove( tag );
  }

it's ok :)

detail message in :
file: node_modules/react-native/Libraries/Renderer/ReactNativeStack-dev.js: 2040
function: unmountComponentAtNode & renderComponent

@guyca
Copy link
Collaborator

guyca commented Jul 24, 2017

Hey guys
Can someone upload an example project where this issue reproduces? I understand it doesn't happen in RN 0.45

@coolerfall
Copy link

coolerfall commented Jul 31, 2017

@guyca Just change the version of react-native to 0.46.4, then run the example, this error will show when hiding light box.

@guyca guyca self-assigned this Jul 31, 2017
@yayanartha
Copy link

Any update @guyca? I'm using many lightboxes on my app mostly for loading popup indicator

@JPig
Copy link

JPig commented Aug 23, 2017

[using V2] not too sure how much help it is but i did some debugging and commenting out unmountReactApplication() in ReactContainerView.java stopped the error for me but it doesnt seem like that is a/the solution. @guyca

@hemedani
Copy link

hemedani commented Aug 30, 2017

I get this error in any way i want to close lighbox
I get error when using : tapBackgroundToDismiss: true, or this.props.navigator.dismissLightBox() even when rewrite method :

@Override
    public void dismissLightBox() {
        if (lightBox != null) {
            lightBox.dismiss();
            // lightBox.hide();
            lightBox = null;
        }
    }

my react native version : "react-native": "0.47.2"
please someone help to solve it

@hemedani
Copy link

hemedani commented Aug 30, 2017

This hacky by @rawrmaan fixed for me
change the destroy method on node_modules/react-native-navigation/android/app/src/main/java/com/reactnativenavigation/views/LightBox.java

    public void destroy() {
        // content.unmountReactView();
        dismiss();
    }

tank you man

@hemedani
Copy link

hemedani commented Sep 3, 2017

It's trow error in some devices like lenovo tab3
... please fixed this issues i needed for my app

@henrikra
Copy link
Contributor

I encounter this with light box when using Android's back button. Also using tapBackgroundToDismiss: true

@manishoo
Copy link

I'm having the same issue

@kyle-ssg
Copy link
Contributor

Also getting this on android, is this being looked at ? Basically can't use lightboxes at all at the moment.

@ODelibalta
Copy link

ODelibalta commented Sep 19, 2017

As epic as this library is, it is making my app pretty much iOS only instead of cross platform. Please !!!!! fix this issue.

@davidruisinger
Copy link

@hemedani Thanks, your workaround works quite nice. But within my lightbox I'm dependent on the componentWillUnmount method beeing called. With your workaround this method is never called for my lightbox. So currently I have a really ugly hack for this:

    if (Platform.OS === 'android') {
      setTimeout(() => {
        this.componentWillUnmount()
      }, 800)
    }

@hemedani
Copy link

beyond two month for fixing a little and irritating bug ...
please fixed.

@fi11
Copy link

fi11 commented Sep 30, 2017

+1 Please fix this issue. I can't use lightbox in my project.

@umerbaig
Copy link

umerbaig commented Oct 2, 2017

I'm having exactly same issue. tapBackgroundToDismiss: true isn't working.

@umerbaig
Copy link

umerbaig commented Oct 2, 2017

any update @guyca on this issue?

@hnvcam
Copy link

hnvcam commented Nov 18, 2017

@fliPmitKlammern yeah, there is a racing problem between the hiding animation and the native Dialog destroying.
To make it last longer, we'll need to make another fix to skip the hiding animation (SingleScreenLayout.java)

    @Override
    public void dismissLightBox() {
        if (lightBox != null) {
            // lightBox.hide();
            lightBox.dismiss();
            lightBox = null;
        }
    }

However, as my self verification, all this workaround is not stable. Sometime, RN still claims for missing view tag. So I think you should not go with this solution. Sorry for proposing this and wasting your time.

I've migrated all my lightbox needs to use my own version which employed the RN Modal component and deliver the same result without animation effects.

@nhducit
Copy link

nhducit commented Nov 18, 2017

I end up with custom lightbox which base on React native modal

@jer-sen
Copy link
Contributor

jer-sen commented Nov 27, 2017

Any update ? @pqkluan ? @guyca ?

@ODelibalta
Copy link

There is no way I am going to use this lib for any future project. All the cool features that lured me into this are all buggy. For some reason this keeps getting closed even though there is no fix

@emin93
Copy link

emin93 commented Nov 28, 2017

@ODelibalta There is no need to get emotional. This issue is still open and nobody closed it. You are probably talking about referenced issues which got closed because they are duplicates of this one.

The maintainers are doing this for free and they don't get any profit from whiny people. Like with any other open source project, feel free to fix it yourself and actually give the authors something back for their work.

@sfratini
Copy link

sfratini commented Dec 1, 2017

If some of you, like me, are tired of updating the file every time, you can use this postinstall script to apply the workaround:

let lightbox = './node_modules/react-native-navigation/android/app/src/main/java/com/reactnativenavigation/views/LightBox.java';

fs.readFile(lightbox, 'utf8', function(err, data)
{
    if (err)
    {
    } else {
        if (data.split('\n')[108].indexOf('public void destroy()') > -1 && data.split('\n')[109].indexOf('//') == -1 && data.split('\n')[109].indexOf('content.unmountReactView();') > -1){
            let aux = data.split('\n');
            aux[109] = '//'+aux[109];
            fs.writeFile(lightbox, aux.join('\n'));    
        }
        
    }
});

I am willing to debug this as soon as I have some time, because I believe with this we are causing a memory leak.

@pqkluan
Copy link
Contributor

pqkluan commented Dec 2, 2017

I confirm the current workaround is causing a memory leak.

The root container that uses to display the Lightbox is not unmounting properly. Every time you start a new Lightbox, a new container was created and keep in the root nodes list causing the leak. You could see this clearly by using this debugger feature

@hhunaid
Copy link

hhunaid commented Dec 2, 2017

Okay guys. I am not an Android guy but I downloaded Android Studio and dug around in RNN as well as RN code and I think I have figured this out. Problem is that when LightBox creates its ContentView which is a ReactRootView, RN sets a viewId and keeps its reference with it for later.
But the this line
content.setId(ViewUtils.generateViewId());
in
private void createContent(final Context context, LightBoxParams params)
creates a new viewId (a.f.a.i.k its a random integer) and assigns it again, overwriting the previous viewId RN saved to refer to it later. So when we dismiss the LightBox RN tries to deallocate but doesn't find the reference it assigned to the view and then complains that it isn't a rootView.

Commenting out said line fixes the problem for me. But I don't know about you guys. I don't even know why they needed to set an id in the first place. Maybe in RN < 0.44 ReactRootView didn't assign one? or didn't keep track of it for later deallocation. Whatever the reason was, the fix is to not set id again.

I encourage you all to try my fix and if it really fixes the bug(I am skeptical) I will submit a PR and hope @guyca comes around to merge it.

@sfratini
Copy link

sfratini commented Dec 2, 2017

Well, 0.45.1 launched around June, which is at the same time this commit was issued. I think this was a problem before, but RN was not complaining and cleaning the screen anyway. So, right now is just throwing the error. Can't confirm tho.

facebook/react-native@54f7ae1#diff-a04970ddd4a6df34e2245d37f78486e3

@dallashall
Copy link

@hhunaid's solution seems to be working for me without any other issues. Will update if I notice anything strange.

@jer-sen
Copy link
Contributor

jer-sen commented Dec 4, 2017

@hhunaid's solution seems also to work for me but sometimes lightbox content is not displayed (there is only the lightbox background that can be touched to dismiss the lightbox, but no React component).

@sfratini
Copy link

sfratini commented Dec 4, 2017

Same issue. It was happening about 1 every 10 clicks in my debug build but as soon as I made the release build, it is happening like 2 every 3 clicks. Almost renders the Lightbox not usuable, but I am not sure if this was a problem before.

@hhunaid
Copy link

hhunaid commented Dec 5, 2017

@Jay1337 @sfratini What's your RN version?
I never had this problem in dev. Didn't check in release But I am willing to try.

@fodjan-philip
Copy link

I am using RN 0.49.5 and I experience no issues with @hhunaid's solution, neither in debug nor in release mode.

@jer-sen
Copy link
Contributor

jer-sen commented Dec 5, 2017

@hhunaid :

[email protected]
[email protected]
[email protected] (patched with the fix)
Emulator Genymotion Samsung Galaxy S6 - 5.0.0 - API 21 - 1440x2560
build.gradle :
    compileSdkVersion 25
    buildToolsVersion "25.0.1"

@hhunaid
Copy link

hhunaid commented Dec 5, 2017

I guess it has something to do with that 0.50 patch then. Can you confirm that undoing my patch fixes the view not showing up issue?

@sfratini
Copy link

sfratini commented Dec 5, 2017

This is my dependencies:

@Jay1337 Do you see any of your dependencies in my list? Maybe is another module messing around.

"dependencies": {
  "clevertap-react-native": "0.1.8",
  "d3-array": "1.2.1",
  "d3-scale": "1.0.6",
  "d3-shape": "1.2.0",
  "lottie-react-native": "2.2.7",
  "prop-types": "15.6.0",
  "react": "16.0.0-alpha.12",
  "react-native": "0.48.1",
  "react-native-action-button": "2.8.0",
  "react-native-app-intro": "1.1.5",
  "react-native-collapsible": "0.9.0",
  "react-native-config": "0.8.1",
  "react-native-firebase": "3.0.5",
  "react-native-google-analytics-bridge": "5.3.3",
  "react-native-localization": "0.1.32",
  "react-native-maps": "0.17.0",
  "react-native-navigation": "1.1.210",
  "react-native-open-settings": "1.0.1",
  "react-native-permissions": "1.0.0",
  "react-native-sentry": "0.30.0",
  "react-native-storage": "0.2.2",
  "react-native-swiper": "1.5.13",
  "react-native-vector-icons": "4.3.0"
},
"devDependencies": {
  "babel-jest": "21.0.0",
  "babel-preset-react-native": "3.0.2",
  "jest": "21.0.1",
  "react-test-renderer": "16.0.0-alpha.12"
},

If I remove the patch from @hhunaid I sometimes get this and crashes my app:

12-05 09:21:17.655 2688 2725 E AndroidRuntime: com.facebook.react.uimanager.IllegalViewOperationException: View with tag 4 is not registered as a root view

But if I use the first workaround, and remove the line that unmounts the component, I still get the dimmed background and no lightbox sometimes. I am thinking the fact that the Lightbox starts a new root view is not helping.

@hhunaid
Copy link

hhunaid commented Dec 5, 2017

My patch has nothing to do with this problem then. I thought it might have been a problem with 0.50 infinite layout rendering patch but you are sitting at RN 0.48. So its definitely something else.

@jer-sen
Copy link
Contributor

jer-sen commented Dec 5, 2017

@hhunaid I've made some tests and the bug I describe (content not displayed) was already there before applying your fix.

I've opened a specific issue for that : #2288

@sfratini I can reproduce the bug with a simple project, RNN being the only installed module.

@0x5e
Copy link

0x5e commented Dec 28, 2017

@sfratini This tool is more convenient to make a local patch: https://github.com/ds300/patch-package

@ankero
Copy link

ankero commented Dec 29, 2017

Seems to be broken with react-native: 0.50.3 still. However @ywongweb 's suggestion to use background opacity works nicely. Granted, the Navigator.OS handling is a pain, but showModal in IOS with "screenBackgroundColor: transparent" doesn't seem to work.

So I opted to:

if ( Navigator.OS === "ios" ) {
Navigation.showLightBox({
        screen: "app.ContextMenu",
        passProps: {
          ...stuff
        }
      });
} else {
Navigation.showModal({
      screen: "app.ContextMenu",
      passProps: {
        ...stuff
      },
      animationType: "fade",
      navigatorStyle: {
        screenBackgroundColor: "transparent",
        navBarHidden: true
      }
    });
}

and then displaying the modal with a positioned absolute behind all other elements with onPress={closeModal}

Setting the dismissModal to display "fade" animation makes the whole thing behave smoothly:

if (Platform.OS === "ios") {
      Navigation.dismissLightBox();
    } else {
      Navigation.dismissModal({
        animationType: "fade"
      });
    }

@sfratini
Copy link

Too unstable for me, so I switched to regular RN modals.

guyca added a commit that referenced this issue Jan 1, 2018
guyca added a commit that referenced this issue Jan 1, 2018
@guyca guyca closed this as completed in 14a7952 Jan 1, 2018
@peteroid
Copy link

peteroid commented Jan 2, 2018

Thanks @guyca for fixing this 🎉🎉🎉

krystofcelba pushed a commit to krystofcelba/react-native-navigation that referenced this issue Jan 4, 2018
@jer-sen
Copy link
Contributor

jer-sen commented Jan 7, 2018

Thanks @guyca !
Could you also have a look at this issue ? #2288

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests