Changes fragment commit to synchronous

[Ubax]

Description

Changes fragment commit to synchronous
This PR is based on #1006 (java code rewritten to Kotlin)

Checklist

• Included code example that can be used to test this change
• Ensured that CI passes

[cristianoccazinsp]

@Ubax I'm not sure if you are aware, but this change also fixes an issue where the app's background may be visible for half a second, causing an ugly flash. This is great!

However, it does seem to still lag under a new case, which didn't happen before. Please take a look at the last video from this comment: #1020 (comment)

[kmagiera]

I'm fine with this change as long as it does not break anything :)

One comment I left is about method naming. I think that we shold not leave the old naming in place as the names were chosen to reflect the nature of updates being delayed. I think that now we should call performUpdate directly instead of markUpdated and make it such that performUpdate calls into onUpdate (not the other way around as it is now). Then allow for onUpdate to be overridden ("onSth" is typically used for things that subclasses can override)

[kmagiera]

I'd suggest we get rid of this naming policy now that all updtes are executed immediately. There is no need to a method named "markUpdated" if it does not in fact mark anything and just executes the update. What I think would make the most sense is if we remove markUpdated and updateIfNeeded methods, then instead call performUpdate directly. In turn, performUpdate should do mAttached and other checks (that are currently in updateIfNeeded and all the logic from performUpdate should be moved to onUpdate that can be overridden by native stack implementation hence allow for different transactions to be instantiated etc.

[kacperkapusciak]

This PR breaks animations on Android. Check Animations playground in Example/ app.

[cristianoccazinsp]

This PR breaks animations on Android. Check Animations playground in Example/ app.

That's too bad! I was really looking forward this getting merged as it fixed a bunch of odd navigation/animation issues! Does it not work with native navigators? Can something still be done in order to make this work, or is it a dead end?

[WoLewicki]

@cristianoccazinsp we are working on fixing it right now.

[WoLewicki]

@cristianoccazinsp can you check if adding this new commit does not introduce any new problems?

[cristianoccazinsp]

@WoLewicki it seems to still be working fine (fixes the original bugs). However, the pop animation seems to have changed a bit. I don't know what exactly, but if you look at the top bar, it animates slightly different (and worse, like little flashes).

Before (no changes at all). Look at the header, it transitions very smootly.

before.mp4

With this branch: the animation speed and header animates slightly different, the text seems to scramble. Not really sure if visible on the video:

after.mp4

Also, the patch I proposed here (react-navigation/react-navigation#9760) still applies to help with large views and animations lagging.

Without my PR (and with this PR):
https://user-images.githubusercontent.com/48869228/131887085-14c690fb-b575-4936-b2dd-c384ac837715.mp4

With my PR (and with this PR):
https://user-images.githubusercontent.com/48869228/131887097-568a13d9-9472-40e1-b056-33dc9836cc3a.mp4

You can see that without my PR, the animation still lags for half a second and shows a black piece on the left. Either way, this normally happens only with very long screens. For regular use, the fixes in this PR are enough.

So ideally, this PR and the react-navigation PR should be included to minimize animation lag.

[cristianoccazinsp]

I'm using this for the animation config, for both open and close:

const animationConfig = {
  animation: 'spring',
  config: {
    stiffness: 1200,
    damping: 100,
    mass: 3,
    overshootClamping: true,
    restDisplacementThreshold: 0.01,
    restSpeedThreshold: 0.01,
    useNativeDriver: true,
  },
};

[WoLewicki]

@cristianoccazinsp I tried to check the video frame by frame, but I couldn't see anything specific regarding the header. Can you provide the whole reproduction in the form of minimal needed code in e.g. a snack?

[cristianoccazinsp]

@WoLewicki I will try to build a repro with the example code, but it's very time consuming so it may take me a bit.

On the other hand, if you have run the examples and everything seems normal, it's probably an issue on my side with how I've set up the animations. Now that they behave a bit better, perhaps an existing issue is now more noticeable.

Lastly, what about the PR for react-navigation? Would that kind of change also help or have you guys discarded it entirely? I believe the two last videos show the issue very clearly.

Update: Just tried to reproduce it using the example code and I could not see anything strange with the animations, even when trying to use the same setup, so I'm not sure exactly, perhaps just a visual glitch due to the header content that's barely visible.

[cristianoccazinsp]

Using the following animation config and StackNavigator (not native stack navigator), I ended up in this situation when combining push and replace (screen cut in half to fit git size):

screen-20210903-155918_3.mp4

const animationConfig = {
  animation: 'spring',
  config: {
    stiffness: 200,
    damping: 100,
    mass: 5,
    overshootClamping: true,
    restDisplacementThreshold: 0.01,
    restSpeedThreshold: 0.01,
    useNativeDriver: true,
  },
};

I think the odd animation I'm seeing is that sometimes the screen appears in front and sometimes behind

[cristianoccazinsp]

Making the animation even slower, you can see how the first time the screen pops, the previous screen animates very quickly (ignoring the settings, or appearing almost instantly?). Further popping the screen, animates as expected.

small.mov

const animationConfig = {
  animation: 'spring',
  config: {
    stiffness: 100,
    damping: 100,
    mass: 30,
    overshootClamping: true,
    restDisplacementThreshold: 0.1,
    restSpeedThreshold: 0.1,
    useNativeDriver: true,
  },
};

UPDATE: Looks like the screen appearing already almost revealed is due to the use of CardStyleInterpolators.forHorizontalIOS from react-navigation, defined as follows:

From https://reactnavigation.org/docs/stack-navigator/#animations

const forSlide = ({ current, next, inverted, layouts: { screen } }) => {
  const progress = Animated.add(
    current.progress.interpolate({
      inputRange: [0, 1],
      outputRange: [0, 1],
      extrapolate: 'clamp',
    }),
    next
      ? next.progress.interpolate({
          inputRange: [0, 1],
          outputRange: [0, 1],
          extrapolate: 'clamp',
        })
      : 0
  );

  return {
    cardStyle: {
      transform: [
        {
          translateX: Animated.multiply(
            progress.interpolate({
              inputRange: [0, 1, 2],
              outputRange: [
                screen.width, // Focused, but offscreen in the beginning
                0, // Fully focused
                screen.width * -0.3, // Fully unfocused
              ],
              extrapolate: 'clamp',
            }),
            inverted
          ),
        },
      ],
    },
  };
};

Changing screen.width * -0.3 to something like -0.9 makes it look more natural. So the animations are not an issue with these changes but rather how I configured them!

[WoLewicki]

Lastly, what about the PR for react-navigation? Would that kind of change also help or have you guys discarded it entirely? I believe the two last videos show the issue very clearly.

PR in react-navigation is not really in the scope of this PR's discussion thread. It would need to be tested very deeply in the context of proper events and gestures dispatching. I think it would be best if you just used patch-package with that change until one of the members have time to look deeper at it.

[kmagiera]

After the recent changes the old naming makes more sense as we are actually marking the view to take into the account all the updates later on. I suggest that we restructure the naming as follows:

1. performUpdate rename to onScreenChanged
2. ScreenContainer implementation of onScreenChanged will perform all the fragment manager magic (things that are currently in performUpdate and in onUpdate)
3. We drop onUpdate and performUpdate completely
4. in ScreenStack we override onScreenChanged and set mNeedUpdate there (what we now do in performUpdate)
5. We move logic from onUpdate in ScreenStack to a new method performUpdates (we can add "s" at the end of the method name which will suggest that there are a number of changes that can be reflected all at once). Now this method should do all the stuff that's curerntly in performUpdate and onUpdate in ScreenStack.
6. Rename performUpdateImmediately to performanUpdatesNow (note there is "s" after "updates")

I think the rest look ok 👍

[cristianoccazinsp]

@WoLewicki , I think this has introduced a bug with react navigation when detachPreviousScreen=false is used on Android.

I'm using the animation config from here https://reactnavigation.org/docs/stack-navigator to simulate a nice slide animation.

const animationConfig = {
  animation: 'spring',
  config: {
    stiffness: 1100,
    damping: 100,
    mass: 3,
    overshootClamping: true,
    restDisplacementThreshold: 0.01,
    restSpeedThreshold: 0.01,
    useNativeDriver: true,
  },
};

//let animationInterp = CardStyleInterpolators.forHorizontalIOS; // use iOS slide as well on Android, looks cool
// https://reactnavigation.org/docs/stack-navigator/#animations
// IOS default one is slightly bugged, re-implement it here
const animationInterp = ({current, next, inverted, layouts: {screen}}) => {
  const progress = Animated.add(
    current.progress.interpolate({
      inputRange: [0, 1],
      outputRange: [0, 1],
      extrapolate: 'clamp',
    }),
    next
      ? next.progress.interpolate({
          inputRange: [0, 1],
          outputRange: [0, 1],
          extrapolate: 'clamp',
        })
      : 0,
  );

  return {
    cardStyle: {
      transform: [
        {
          translateX: Animated.multiply(
            progress.interpolate({
              inputRange: [0, 1, 2],
              outputRange: [
                screen.width, // Focused, but offscreen in the beginning
                0, // Fully focused
                screen.width * -0.3, // Fully unfocused
              ],
              extrapolate: 'clamp',
            }),
            inverted,
          ),
        },
      ],
    },
  };
};

Note that the screen that is technically visible, is also not interactive at all as it looks like the one behind (not de-attached) blocks it regardless of the animation config.

Steps to reproduce:

• Navigate to a screen with detachPreviousScreen: false, at this point the previous screen will already animate oddly (cover the screen while the animation is happening), but will go away after the animation ends
• Push another screen (without detachPreviousScreen)
• Pop, Pop
• After getting back to the screen that was never detached, the above capture happens.

Sorry I couldn't catch this earlier, but the use of detachPreviousScreen was a new addition in our project for a very specific use case.

Also, the screen that overlaps is not the one that was not detached, but rather the one before it.

I'm also using

    "@react-navigation/compat": "5.3.20",
    "@react-navigation/native": "6.0.2",
    "@react-navigation/stack": "6.0.7",

[WoLewicki]

Could you provide the full reproduction of it in a form of a snack in an issue so everyone can see it and elaborate?

[cristianoccazinsp]

Any good starting snack I can use that has all these libs for navigation and screens already set up?

[WoLewicki]

you can use snack to provide the minimal reproduction code or make a branch with reproduction test in our TestsExample project in the repo.

[cristianoccazinsp]

I cannot seem to reproduce the issue with the Example app, still trying to debug to see what's the exact cause of the issue, but since it doesn't happen with the example app, it has to be either a configuration or a conflict with another library.

[cristianoccazinsp]

@WoLewicki this is really strange. I'm using the exact same code (including Android SDK, libs versions, customizations to java code, disabling hermes, and a bunch of other changes) and I cannot reproduce the bug with the example app, even with nearly 90% of an identical setup. Still debugging, but does something come to mind of what could it be?

UPDATE: Finally found it, our project uses appcompat version 1.3.x instead of the default 1.1 from this and other libraries. With appcompat 1.3, the bug happens right away. Will open an issue soon.

[cristianoccazinsp]

@WoLewicki sorry I keep bothering, but I keep finding tiny issues with this.

Did this code change anything that would cause surface events to fire twice? For instance, react-native-camera now fires a surface changed event right after the surface is destroyed when the screen is popped and the component unmounted: https://github.com/react-native-camera/react-native-camera/blob/master/android/src/main/java/com/google/android/cameraview/SurfaceViewPreview.java

[hippiejuice]

@WoLewicki does this fix this issue: #17 ?

[WoLewicki]

@hippiejuice it does not change the behavior seen in #17.

[mimo-10]

for me, i had to create a work around by playing with the animation configs. and i could say it did work perfectly, but i had to stick with only one type of animation.