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

[Navigator] width is not updated when orientation is changed, so switch-animation stops halfway and component vanishes #2111

Closed
joeyvandijk opened this issue Jul 24, 2015 · 11 comments
Labels
Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Resolution: Locked This issue was locked by the bot.

Comments

@joeyvandijk
Copy link

I found a bug when you create a simple app that uses only the <Navigator /> component with iOS when you change the orientation from portrait to landscape. You will see that the back-animation (when switching states) keeps it old target-x-position to animate to.

out

I expect that the back-animation will stop at the far end/border of the landscape-screen ("outside" the view).

While trying to set the width/height to the view & navigator, which not solved it, I suspect that because it only happens in landscape mode the width of the navigator view is not updated when the orientation changes so the old target-value (x-position) is used to animate the old state to.

You can test it following these steps:

  • react-native init ORIENTATION
  • copy this example into index.ios.js

(will fail for React-Native 0.7.1 & 0.8.0-rc.2)

Anyone have an idea where to look? I am not an native iOS developer, but any tips/fixes are welcome.

@brentvatne
Copy link
Collaborator

Great, thanks for the detailed report @joeyvandijk!

https://github.com/facebook/react-native/blob/master/Libraries/CustomComponents/Navigator/Navigator.js#L53-L57

Notice that here it is just grabbing the screen width at load time, and so if that changes this won't be updated. We'll need to monitor orientation changes and update the screen width / height values as orientation changes. cc @hedgerwang @ericvicenti

@ericvicenti
Copy link
Contributor

We are planning on refactoring the animations soon (upgrading to Animated), and I think we can stop depending on the absolute width. But in the meantime, somebody could go in and use onLayout in the Navigator to use the current width instead of the Dimensions hack.

@brentvatne brentvatne added Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Community Responsibility labels Jul 24, 2015
@brentvatne
Copy link
Collaborator

Thanks @ericvicenti! Good task for interested contributors ^

@joeyvandijk
Copy link
Author

Thanks @ericvicenti and @brentvatne for the input, but I am having trouble with the #goodfirsttask tag ;), while it is not so simple to change. The problem lies not so much in listening to the layout event and updating the props/state, but more in the sceneConfigs. See https://github.com/facebook/react-native/blob/master/Libraries/CustomComponents/Navigator/NavigatorSceneConfigs.js#L223-L224 to be exact for this error. Doubling (#hack) the value at line 223 will provide a quick solution, but is far from ideal: no bug on landscape but a too quick animation at portrait mode.

All the properties of NavigatorSceneConfigs variables are extended from each other to import into Navigator as statics which provides me the choice what next step would be most wise?

Could you provide any tips concerning how to fix it when Navigator is already initiated during runtime, the orientation changed and all values in NavigatorSceneConfigs are incorrect, but also the instantiated / configured objects/scenes imported into Navigator?
I am wondering if NavigatorSceneConfigs should be a function-export that has 2 properties (width/height) and could be re-require(d) (which is a no-go for performance) or just rewrite it that it keeps the option to always be able to change the values, but double work is not handy if the Animated approach is already planned/developing.

Any (small) insights are appreciated, because I think the issue it is bigger than I thought at first, right?

@ericvicenti
Copy link
Contributor

Yep, because of the SceneConfig complication, it is a bigger issue than I realized this morning.

Even if the sceneConfig was dynamically generated for the current screen size, it would get stale. To get it working, you could hack into Navigator and make sure it re-generates the scene config when dimensions change.

I want to transition away from SceneConfigs altogether and let the scenes animate themselves. (An Animated.Value would be passed in to each scene from the navigator). We might need to wait for that change in order to get this fully fixed.

@joeyvandijk
Copy link
Author

Thanks @ericvicenti for the input. I had time to investigate but not for a full rewrite, but also not when it is not fully clear what direction is needed, but I like your input about scenes that animate themselves. Will look into it again when I have more time.

@brentvatne
Copy link
Collaborator

Hi there! This issue is being closed because it has been inactive for a while.

But don't worry, it will live on with ProductPains! Check out it's new home: https://productpains.com/post/react-native/navigator-width-is-not-updated-when-orientation-is-changed-so-switch-animation-stops-halfway-and-component-vanishes

@grittathh
Copy link

I managed to hack together something that works. Basically I pass in the width and the height from the parent view of the Navigator using the style property. Then, in Navigator.js, I removed the maincontainer style, and replaced all instances of SCREEN_WIDTH, SCREEN_HEIGHT, SCENE_DISABLED_NATIVE_PROPS, and disabledScene, with equivalent objects containing props.style.width and props.style.height.

Props.style.width and props.style.height are set in the parent view (in my case, index.js) and get reset by onLayout.

This completely doesn't address SceneConfig issues, but I'll live with this for now.

@ufon
Copy link

ufon commented Mar 23, 2016

@grittathh please, can u describe more clear with code and comments?

@antseburova
Copy link

antseburova commented Sep 14, 2016

There is a way to get it working properly without a need to change the Navigator's source code. It doesn't look very nice, but, in my opinion, it's not a bad temporary workaround.

So, there are two issues which you need to deal with.

Proper resizing of scene views

In order to get your scenes properly resized after a device orientation has been changed, you can create custom scene configurations, where you could use a proper app layout width. Once your device orientation is changed, you can reset every scene with a new route stack and render the scenes using updated scene configs.

To do so, you can copy all of the styles you need for your scene transitions from the NavigatorSceneConfigs.js file (https://github.com/facebook/react-native/blob/master/Libraries/CustomComponents/Navigator/NavigatorSceneConfigs.js), paste them into your custom scene configs, and replace all of the SCREEN_WIDTH and Dimensions.get('window').width occurrences with a passed width value. I preferred to keep all of the custom scene config related stuff in a separate file. You may have something like this if you would like to use HorizontalSwipeJump config option:

import {Navigator, PixelRatio} from 'react-native';
import buildStyleInterpolator from 'buildStyleInterpolator';

export default function(appLayoutWidth) {
  const BaseConfig = Navigator.SceneConfigs.HorizontalSwipeJump;
  const windowWidth = appLayoutWidth;
  const stillCompletionRatio = 1 / 10;

  const FromTheRight = {
    opacity: {
      value: 1.0,
      type: 'constant',
    },

    transformTranslate: {
      from: {x: appLayoutWidth, y: 0, z: 0},
      to: {x: 0, y: 0, z: 0},
      min: 0,
      max: 1,
      type: 'linear',
      extrapolate: true,
      round: PixelRatio.get(),
    },

    translateX: {
      from: appLayoutWidth,
      to: 0,
      min: 0,
      max: 1,
      type: 'linear',
      extrapolate: true,
      round: PixelRatio.get(),
    },

    scaleX: {
      value: 1,
      type: 'constant',
    },
    scaleY: {
      value: 1,
      type: 'constant',
    },
  };

  const ToTheLeft = {
    transformTranslate: {
      from: {x: 0, y: 0, z: 0},
      to: {x: -appLayoutWidth, y: 0, z: 0},
      min: 0,
      max: 1,
      type: 'linear',
      extrapolate: true,
      round: PixelRatio.get(),
    },
    opacity: {
      value: 1.0,
      type: 'constant',
    },

    translateX: {
      from: 0,
      to: -appLayoutWidth,
      min: 0,
      max: 1,
      type: 'linear',
      extrapolate: true,
      round: PixelRatio.get(),
    },
  };

  const CustomSceneConfig = {
    ...BaseConfig,
    gestures: {
      jumpBack: {
        ...BaseConfig.gestures.jumpBack,
        edgeHitWidth: windowWidth,
        stillCompletionRatio: stillCompletionRatio,
      },

      jumpForward: {
        ...BaseConfig.gestures.jumpForward,
        edgeHitWidth: windowWidth,
        stillCompletionRatio: stillCompletionRatio,
      },
    },

    animationInterpolators: {
      into: buildStyleInterpolator(FromTheRight),
      out: buildStyleInterpolator(ToTheLeft),
    },
  };

  return CustomSceneConfig;
}

Then, you need to call the specified above function within the configureScene property of your navigator:

<Navigator 
  configureScene={() => customNavigatorSceneConfig(this.props.appLayout.width)} 
  ref="navigator" 
/>

this.props.appLayout.width should contain a relevant value of your layout width.

If you use Redux to update the layout width, you can catch the moment of an orientation being changed and use the following code to reset every scene with a new stack of routes. In the following example, just one scene is added to the stack:

componentWillReceiveProps(nextProps) {
    if (this.props.appLayout.width !== nextProps.appLayout.width) {
      this.refs.navigator.immediatelyResetRouteStack([currentRoute]);
    }
  }

Next, we’ll fix the second issue which relates to styling disabled scenes.

Applying proper styles to disabled scenes

If you have a few routes in your stack, you will probably notice a rendering issue when you switch from a landscape view to portrait. A part of your next scene will be visible at the bottom of your current scene. The issue occurs due to the fact that all of the scenes, except for the current one,
are hidden with CSS (styles.disabledScene, https://github.com/facebook/react-native/blob/master/Libraries/CustomComponents/Navigator/Navigator.js#L114) which uses a layout height, that was evaluated just once in Navigator.js
The indicated CSS is used within the _renderScene function. So, you may consider overriding it with your code which uses an up-to-date layout height. To do so, you can extend the Navigator component with your own:

import {Navigator} from 'react-native';
import sceneNavigatorOverride from '../lib/sceneNavigatorOverride';

export default class extends Navigator {

  constructor(props) {
    super(props);
    this._renderScene = sceneNavigatorOverride;
  }
}

Within your sceneNavigatorOverride.js file, you will need to have something like this:

import React from 'react';
import {View, StyleSheet} from 'react-native';

export default function(route, i) {
  let disabledSceneStyle = null;
  let disabledScenePointerEvents = 'auto';
  if (i !== this.state.presentedIndex) {
    disabledSceneStyle = {
      top: this.props.appLayout.height * 2,
      bottom: -this.props.appLayout.height * 2,
    };
    disabledScenePointerEvents = 'none';
  }

  return (
    <View
      key={'scene_' + _getRouteID(route)}
      ref={'scene_' + i}
      onStartShouldSetResponderCapture={() => {
        return (this.state.transitionFromIndex != null) || (this.state.transitionFromIndex != null);
      }}
      pointerEvents={disabledScenePointerEvents}
      style={[styles.baseScene, this.props.sceneStyle, disabledSceneStyle]}>
      {this.props.renderScene(
        route,
        this
      )}
    </View>
  );
}

function _getRouteID(route) {
  if (route === null || typeof route !== 'object') {
    return String(route);
  }

  const key = '__navigatorRouteID';

  if (!route.hasOwnProperty(key)) {
    Object.defineProperty(route, key, {
      enumerable: false,
      configurable: false,
      writable: false,
      value: getuid(),
    });
  }
  return route[key];
}


let __uid = 0;

function getuid() {
  return __uid++;
}


const styles = StyleSheet.create({
  baseScene: {
    position: 'absolute',
    overflow: 'hidden',
    left: 0,
    right: 0,
    bottom: 0,
    top: 0,
  },
});

That's it! I hope it was helpful.

Please let me know if you have any questions.

Thanks @lazarmat and @zharley for cooperation and great job in figuring out the described above solution.

@ksloan
Copy link

ksloan commented Sep 25, 2016

@antseburova's fix worked for me.

This part tripped me up at first...

this.refs.navigator.immediatelyResetRouteStack([currentRoute]);

but you can just use

this.refs.navigator.immediatelyResetRouteStack(this.refs.navigator.state.routeStack)

to refresh the stack.

Hopefully this helps someone, thanks @antseburova!

@facebook facebook locked as resolved and limited conversation to collaborators Jul 22, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

8 participants