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

fix(iOS): stuttering back arrow animation when using headerLargeTitle #2014

Closed
wants to merge 1 commit into from

Conversation

dylancom
Copy link
Contributor

@dylancom dylancom commented Jan 17, 2024

Description

When navigating to a screen that has a large header title and the back title hidden, the back arrow animation bugs (kind of bounces in / stuttery animation).
I noticed that when setting the title to a string, it animates normally.
I found that using a space instead of a complete empty string, the title of the previous screen animates nicely, instead of disappearing instantly.
(Tested on iPhone 15 Pro, iOS 17.0)

EDIT:
I also just noticed that on an iPhone 8 plus (iOS 14.0) the back button completely disappears after the stuttery animation.
It seems that setting it to nil caused multiple bugs...

Demo (bug)

arrow.jumping.demo.mp4

Demo (fix)

fix.mp4

Reproduction

<Stack.Screen
  name="Home"
  component={HomeScreen}
  options={{ headerLargeTitle: true }}
/>
<Stack.Screen
  name="Details"
  component={DetailsScreen}
  options={{
    headerLargeTitle: true,
    headerBackTitleVisible: false,
  }}
/>

From Home Screen: navigation.navigate('Details')

@tboba
Copy link
Member

tboba commented Jan 18, 2024

Hi @dylancom, thanks for creating this PR! 🥳
Hm, that's strange 🤔 do you know on which iPhone and which iOS does this back button stutter? I'm seeing this behavior the first time!

@dylancom
Copy link
Contributor Author

dylancom commented Jan 18, 2024

Hi @tboba, you're welcome!
I have tested this on an iPhone 15 Pro, iOS 17.0.

EDIT:
I am seeing a bigger bug on an iPhone 8 Plus, running iOS 14.0.
The back button disappears...
It animates in the same stuttery way and then disappears.

back.button.gone.mp4

This PR also fixes this.

@tboba
Copy link
Member

tboba commented Jan 18, 2024

@dylancom Yeah, something's definitely wrong there 🤔
Do you know if previously in RNScreens this bug was also happening for you? Maybe we've made a wrong change between versions

@dylancom
Copy link
Contributor Author

@tboba I have no idea, I haven't been using headerLargeTitle in this way before.

@dylancom
Copy link
Contributor Author

@tboba anything more needed to see this merged?

@focux
Copy link

focux commented Jan 20, 2024

I'm experiencing this same issue. I'm using an iPhone 15 Pro with iOS 17.2 and react-native-screens ^3.27.0. In my case, I'm navigating from a screen with a large header (Screen One) to a screen without a large header (Screen Two), and the headerBackTitleVisible: false.

      <Stack.Screen
        name="ScreenOne"
        component={ScreenOne}
        options={{
          title: 'Screen One',
          headerLargeTitle: true,
          headerShadowVisible: false,
          headerLargeStyle: {backgroundColor: '#fff'},
          headerStyle: {backgroundColor: '#fff'},
        }}
      />
      <Stack.Screen
        name="ScreenTwo"
        component={ScreenTwo}
        options={{
          headerBackTitleVisible: false,
        }}
      />

@tboba
Copy link
Member

tboba commented Jan 20, 2024

@dylancom not at the moment, thanks! Currently I'm struggling with fixing main branch, as it's quite broken, but I'll take a look on that in upcoming days 🙌
@focux thanks for confirming!

@tboba
Copy link
Member

tboba commented Jan 22, 2024

@dylancom I've just tested this PR and it looks like this change fixes the bug with the title, but unfortunately it breaks the animation of appearing title after pressing back button 😢

Without the change:

Screen.Recording.2024-01-22.at.11.47.12.mov

With the change (notice that right now the title is being scaled instead of appearing from the side):

Screen.Recording.2024-01-22.at.11.48.11.mov

Do you know if there's any other solution for fixing that back button item?

@dylancom
Copy link
Contributor Author

If you remove the space and just use an empty string, it doesn't scale.
[backBarButtonItem setTitle:@""];

@tboba
Copy link
Member

tboba commented Jan 22, 2024

@dylancom Yeah, actually I was trying to set the title of the button to an empty string and it breaks the animation - hence I see that you've added an empty space to the title. Sorry for misleading you!

@@ -528,7 +528,7 @@ + (void)updateViewController:(UIViewController *)vc

// When backBarButtonItem's title is null, back menu will use value
// of backButtonTitle
[backBarButtonItem setTitle:nil];
[backBarButtonItem setTitle:@" "];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this change still breaks how the hint for the previous screens looks like. With the changed title, it is being set to empty field, which is unacceptable. Can we think about the better approach or "hack" the hints in other way?

Screen.Recording.2024-01-22.at.12.07.33.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The solutions that I found were setting an empty string or space. Others suggested to set a transparent color on the text. See: https://stackoverflow.com/questions/33025239/remove-text-from-back-button-keeping-the-icon

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dylancom thanks! I've tried some methods mentioned in StackOverflow, but unfortunately setting the text color to Clear doesn't fix that bug. Also, I've tried to create a custom menu that will contain the title of the previous screen, but somehow iOS doesn't replace it after calling setMenu (I also don't want to steer the menu manually). Hence I'm afraid I won't merge this for now - I don't want to sacrifice one functionality for another, but of course you can patch screens with this change on your own.

@tboba
Copy link
Member

tboba commented Jan 24, 2024

@dylancom as I said in the subcomment, because of the bug that comes with this change, I won't merge this PR for now (I also can't find any other solution for fixing this issue). If you have any suggestion how this can be fixed, don't hesitate to re-open this PR! Otherwise, can I ask you for filling a new issue with the bug described here? You can link this PR to know the additional context of our attempts. Thanks!

@tboba tboba closed this Jan 24, 2024
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

Successfully merging this pull request may close these issues.

3 participants