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): restore native behavior of auto shortening back button title #2105

Merged

Conversation

zetavg
Copy link
Contributor

@zetavg zetavg commented Apr 14, 2024

Description

Restore the iOS native behavior of automatically shorting the title of the header back button to "Back" if there is not enough space, which is documented here1 but does not behave as expected since v3.21.

Fixes #1589.

Changes

  • Assign to backBarButtonItem only if actual customizations of the back button are being made.

Screenshots / GIFs

Before After
Broken 1 Working 1

Test code and steps to reproduce

Checklist

Footnotes

  1. According to the document, 'On iOS this includes a label next to the button, which shows the title of the previous screen when the title fits in the available space, otherwise it says "Back".'

Copy link
Member

@tboba tboba left a comment

Choose a reason for hiding this comment

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

Hey @zetavg, thanks for this PR! ❤️
Just one small comment from me 😄

if (config.isBackTitleVisible) {
if (config.backTitleFontFamily || config.backTitleFontSize) {
if ((config.backTitleFontFamily &&
![config.backTitleFontFamily isEqual:@"System"]) ||
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to check the font family here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because due to the current implementation of @react-navigation/native-stack, backTitleFontFamily will always be assigned to the font family of fonts.regular, which will be "System" if the default theme is used.

It seems that the "System" font family is already the default on iOS, as setting the font family to "System" does not change the appearance, but in this case, will make config.backTitleFontFamily always resolve to YES.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Do you know, if we could rely on the native component that represents the title of back button, instead of comparing config.backTitleFontFamily?

Copy link
Member

@kkafar kkafar Apr 15, 2024

Choose a reason for hiding this comment

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

Thanks for the PR, this is great!

Can we have this comment in the code please? @zetavg

Copy link
Contributor Author

@zetavg zetavg Apr 15, 2024

Choose a reason for hiding this comment

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

Thanks for the comment @tboba, but I'm sorry that due to my limited knowledge, I can't quite understand what you mean. Do you suggest we can handle this in something like the getter/setter of RNSScreenStackHeaderConfig, or the JS code in react-navigation?

@kkafar Sure, I'll add comments to explain this besides related code once we finalize the solution.

Copy link
Member

@tboba tboba Apr 16, 2024

Choose a reason for hiding this comment

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

@zetavg I'm wondering if it would be possible to get title component of back title and check if it has default font set, but after further reflections I'm afraid this could lead to some flickers (setting the font and other properties -> checking the font of the native component -> setting system back 😄), so I guess we can stay with the current solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for explaining, I think I got your point! I do think the current solution of comparing the font against "System" isn't perfect, one way is that there may be different ways to use the same system font (maybe ".AppleSystemUIFont"? I'm not sure), and if react-navigation changes to also assign a default value to config.backTitleFontSize this will break again.

Your solution inspired me to think of a possible way to get this native behavior to work while still customizing the back button (such as changing the font or customizing the shortened "Back" text) - measure the width of the headerTitle and headerRight to see if there's enough space on layout, and replace the backButtonTitle with the shortened text or hide it. Well, flickering may be a problem.

Copy link
Member

@tboba tboba left a comment

Choose a reason for hiding this comment

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

LGTM! Let's just wait for the comment 🚀

@zetavg
Copy link
Contributor Author

zetavg commented Apr 17, 2024

Comment added! Please check if it's suitable 😃

@tboba
Copy link
Member

tboba commented Apr 18, 2024

Yeah, I think for now it's a pass 🎉

Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

Just one last thing

ios/RNSScreenStackHeaderConfig.mm Outdated Show resolved Hide resolved
Co-authored-by: Kacper Kafara <kacperkafara@gmail.com>
@zetavg
Copy link
Contributor Author

zetavg commented Apr 29, 2024

Hi @kkafar, just a ping to let you know I have applied your suggestion (thanks for that!) for the change you requested. If there's anything I can do to get this PR merged, please let me know.

Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

I think we're all good here.

I'll retrigger CI before merging.

@kkafar kkafar merged commit cee947e into software-mansion:main Apr 29, 2024
5 checks passed
@@ -535,9 +544,17 @@ + (void)updateViewController:(UIViewController *)vc
// When backBarButtonItem's title is null, back menu will use value
// of backButtonTitle
[backBarButtonItem setTitle:nil];
isBackButtonCustomized = YES;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @zetavg, I know that this PR is already merged, but I wonder why do you set isBackButtonCustomized to true here. My understanding is that this else block is for the case when we hide back button, but want to preserve it in back menu. So, by default when we have navigationItem hidden we don't display anything here - nothing is customized, when we want to custom hide it from back manu then we set config.disableBackButtonMenu which is in initial condition anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quick reply - I think it's because this isBackButtonCustomized is used to determine whether we need to assign the backBarButtonItem to prevItem.backBarButtonItem later on line 556.

Since backBarButtonItem has been changed here ([backBarButtonItem setTitle:nil];), my understanding is that the changed backBarButtonItem has to be assigned to prevItem.backBarButtonItem for the changes to take effect. So it should be considered customized - or changed. Maybe it's better to rename isBackButtonCustomized to isBackBarButtonItemChanged so that it will be more clear (or use another way to track if we have changed anything on backBarButtonItem).

My guess is that if we do not set isBackButtonCustomized to true here, the back title won't be hidden as expected. I may need to verify it though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it's better to rename isBackButtonCustomized to isBackBarButtonItemChanged ...

Oh, I see that you had already done this in #2123. 🎉

maciekstosio added a commit that referenced this pull request May 16, 2024
## Description

~This PR improves upon #2105. #2105 allowed to use iOS 14 default back
button behavior when label is not provided. This PR allows to modify the
behavior by allowing to provide UINavigationButtonBackButtonDisplayMode
and enables it for custom text (without style modifications). The main
problem is that we used to provide backButtonItem in most of the cases
which
[disables](https://developer.apple.com/documentation/uikit/uinavigationitem/3656350-backbuttondisplaymode)
backButtonDisplayMode.~

This PR adds possibility to customize default behavior of back button
using `backButtonDisplayMode`
([UINavigationBackButtonDisplayMode](https://developer.apple.com/documentation/uikit/uinavigationitem/backbuttondisplaymode))
for iOS.

:warning: **This modifies only default back button**, when any
customization is added (including headerBackTitle) in native part we
create custom `RNSUIBarButtonItem` and set it as `backButtonItem`, which
[disables](https://developer.apple.com/documentation/uikit/uinavigationitem/3656350-backbuttondisplaymode)
`backButtonDisplayMode` behavior.

I tried to make it work together with custom label (`headerBackTitle`)
using `prevItem.backButtonTitle`, but due to iOS limitations it is not
viable option. It influences also back button menu - changes the label
of previous screen - which is not the behavior we want.

To sum up, `backButtonDisplayMode` work when none of:
- `headerBackTitleStyle.fontFamily`
- `headerBackTitleStyle.fontSize`
- `headerBackTitle`
- `disableBackButtonMenu`

are set. 

## Screenshots / GIFs

|Paper|Fabric|
|-|-|
|<video
src="https://github.com/software-mansion/react-native-screens/assets/11800297/c6aa7697-4331-4cb4-a81d-7f77f128513d"
/>|<video
src="https://github.com/software-mansion/react-native-screens/assets/11800297/fa0edd92-1aa2-45e5-a466-516c0ec120d2"
/>|

<details>
<summary>Example component used in tests:</summary>

```jsx
import * as React from 'react';
import { Button, View, Text, StyleSheet } from 'react-native';
import { NavigationContainer, ParamListBase } from '@react-navigation/native';
import { createNativeStackNavigator } from '@react-navigation/native-stack';
import { NativeStackNavigationProp } from '@react-navigation/native-stack';

const Stack = createNativeStackNavigator();

type NavProp = {
  navigation: NativeStackNavigationProp<ParamListBase>;
};

export default function App() {
  return (
    <NavigationContainer>
      <Stack.Navigator>
        <Stack.Screen
          name="screenA"
          component={ScreenA}
          options={{ headerTitle: 'A: Home' }}
        />
        <Stack.Screen
          name="screenB"
          component={ScreenB}
          options={{
            headerTitle: 'B: default',
            backButtonDisplayMode: 'default',
          }}
        />
        <Stack.Screen
          name="screenC"
          component={ScreenC}
          options={{
            headerTitle: 'C: generic',
            backButtonDisplayMode: 'generic',
          }}
        />
        <Stack.Screen
          name="screenD"
          component={ScreenD}
          options={{
            headerTitle: 'D: minimal',
            backButtonDisplayMode: 'minimal',
          }}
        />
        <Stack.Screen
          name="screenE"
          component={ScreenE}
          options={{
            headerTitle: 'E: custom',
            headerBackTitle: 'Back Title',
            backButtonDisplayMode: 'minimal',
          }}
        />
      </Stack.Navigator>
    </NavigationContainer>
  );
}

const ScreenA = ({ navigation }: NavProp) => (
  <View style={styles.container}>
    <Text>Screen A</Text>
    <Button
      onPress={() => navigation.navigate('screenB')}
      title="Go to screen B"
    />
  </View>
);

const ScreenB = ({ navigation }: NavProp) => (
  <View style={styles.container}>
    <Text>Screen B</Text>
    <Text>backButtonDisplayMode: default</Text>
    <Button
      onPress={() => navigation.navigate('screenC')}
      title="Go to screen C"
    />
  </View>
);

const ScreenC = ({ navigation }: NavProp) => (
  <View style={{ flex: 1, paddingTop: 50 }}>
    <Text>Screen C</Text>
    <Text>backButtonDisplayMode: generic</Text>
    <Button
      onPress={() => navigation.navigate('screenD')}
      title="Go to screen D"
    />
  </View>
);

const ScreenD = ({ navigation }: NavProp) => (
  <View style={styles.container}>
    <Text>Screen D</Text>
    <Text>backButtonDisplayMode: minimal</Text>
    <Button
      onPress={() => navigation.navigate('screenE')}
      title="Go to screen E"
    />
  </View>
);

const ScreenE = (_props: NavProp) => (
  <View style={styles.container}>
    <Text>Screen E</Text>
    <Text>backButtonDisplayMode omitted because of the headerBackTitle</Text>
  </View>
);

const styles = StyleSheet.create({
  container: { flex: 1, alignItems: 'center', justifyContent: 'space-around' },
});
```

</details>

## Checklist

- [x] Included code example that can be used to test this change
- [x] Updated TS types
- [x] Updated documentation: <!-- For adding new props to native-stack
-->
- [x]
https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md
- [x]
https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md
- [x]
https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx
- [x]
https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx
- [x] Ensured that CI passes

Tested #1864: Paper ✅ Fabric ✅
Tested #1646: Paper ❌ Fabric ❌ - but it does not work on main too, could
now be achieved using `backButtonDisplayMode: ‘minimal’`

---------

Co-authored-by: Kacper Kafara <kacper.kafara@swmansion.com>
High5Apps added a commit to High5Apps/organize-rn that referenced this pull request Jun 25, 2024
High5Apps added a commit to High5Apps/organize-rn that referenced this pull request Jun 26, 2024
ja1ns pushed a commit to WiseOwlTech/react-native-screens that referenced this pull request Oct 9, 2024
software-mansion#2105)

## Description

Restore the iOS native behavior of automatically shorting the title of
the header back button to "Back" if there is not enough space, which is
documented
[here](https://reactnavigation.org/docs/header-buttons#customizing-the-back-button)[^1]
but does not behave as expected since v3.21.

Fixes software-mansion#1589.


## Changes

* Assign to `backBarButtonItem` only if actual customizations of the
back button are being made.


## Screenshots / GIFs

| Before | After |
|--------|------|
| ![Broken
1](https://github.com/software-mansion/react-native-screens/assets/3784687/880eaecb-54d9-48d3-95bd-5f8e6cd7b066)
| ![Working
1](https://github.com/software-mansion/react-native-screens/assets/3784687/201e8006-544d-43ee-95e3-308e2f926566)
|


## Test code and steps to reproduce

<!--
Please include code that can be used to test this change and short
description how this example should work.
This snippet should be as minimal as possible and ready to be pasted
into editor (don't exclude exports or remove "not important" parts of
reproduction example)
-->

## Checklist

- [ ] Included code example that can be used to test this change
- [ ] Updated TS types
- [ ] Updated documentation: <!-- For adding new props to native-stack
-->
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx
- [ ] Ensured that CI passes

[^1]: According to the document, 'On iOS this includes a label next to
the button, which shows the title of the previous screen when the title
fits in the available space, otherwise it says "Back".'

---------

Co-authored-by: Kacper Kafara <kacperkafara@gmail.com>
ja1ns pushed a commit to WiseOwlTech/react-native-screens that referenced this pull request Oct 9, 2024
…e-mansion#2123)

## Description

~This PR improves upon software-mansion#2105. software-mansion#2105 allowed to use iOS 14 default back
button behavior when label is not provided. This PR allows to modify the
behavior by allowing to provide UINavigationButtonBackButtonDisplayMode
and enables it for custom text (without style modifications). The main
problem is that we used to provide backButtonItem in most of the cases
which
[disables](https://developer.apple.com/documentation/uikit/uinavigationitem/3656350-backbuttondisplaymode)
backButtonDisplayMode.~

This PR adds possibility to customize default behavior of back button
using `backButtonDisplayMode`
([UINavigationBackButtonDisplayMode](https://developer.apple.com/documentation/uikit/uinavigationitem/backbuttondisplaymode))
for iOS.

:warning: **This modifies only default back button**, when any
customization is added (including headerBackTitle) in native part we
create custom `RNSUIBarButtonItem` and set it as `backButtonItem`, which
[disables](https://developer.apple.com/documentation/uikit/uinavigationitem/3656350-backbuttondisplaymode)
`backButtonDisplayMode` behavior.

I tried to make it work together with custom label (`headerBackTitle`)
using `prevItem.backButtonTitle`, but due to iOS limitations it is not
viable option. It influences also back button menu - changes the label
of previous screen - which is not the behavior we want.

To sum up, `backButtonDisplayMode` work when none of:
- `headerBackTitleStyle.fontFamily`
- `headerBackTitleStyle.fontSize`
- `headerBackTitle`
- `disableBackButtonMenu`

are set. 

## Screenshots / GIFs

|Paper|Fabric|
|-|-|
|<video
src="https://github.com/software-mansion/react-native-screens/assets/11800297/c6aa7697-4331-4cb4-a81d-7f77f128513d"
/>|<video
src="https://github.com/software-mansion/react-native-screens/assets/11800297/fa0edd92-1aa2-45e5-a466-516c0ec120d2"
/>|

<details>
<summary>Example component used in tests:</summary>

```jsx
import * as React from 'react';
import { Button, View, Text, StyleSheet } from 'react-native';
import { NavigationContainer, ParamListBase } from '@react-navigation/native';
import { createNativeStackNavigator } from '@react-navigation/native-stack';
import { NativeStackNavigationProp } from '@react-navigation/native-stack';

const Stack = createNativeStackNavigator();

type NavProp = {
  navigation: NativeStackNavigationProp<ParamListBase>;
};

export default function App() {
  return (
    <NavigationContainer>
      <Stack.Navigator>
        <Stack.Screen
          name="screenA"
          component={ScreenA}
          options={{ headerTitle: 'A: Home' }}
        />
        <Stack.Screen
          name="screenB"
          component={ScreenB}
          options={{
            headerTitle: 'B: default',
            backButtonDisplayMode: 'default',
          }}
        />
        <Stack.Screen
          name="screenC"
          component={ScreenC}
          options={{
            headerTitle: 'C: generic',
            backButtonDisplayMode: 'generic',
          }}
        />
        <Stack.Screen
          name="screenD"
          component={ScreenD}
          options={{
            headerTitle: 'D: minimal',
            backButtonDisplayMode: 'minimal',
          }}
        />
        <Stack.Screen
          name="screenE"
          component={ScreenE}
          options={{
            headerTitle: 'E: custom',
            headerBackTitle: 'Back Title',
            backButtonDisplayMode: 'minimal',
          }}
        />
      </Stack.Navigator>
    </NavigationContainer>
  );
}

const ScreenA = ({ navigation }: NavProp) => (
  <View style={styles.container}>
    <Text>Screen A</Text>
    <Button
      onPress={() => navigation.navigate('screenB')}
      title="Go to screen B"
    />
  </View>
);

const ScreenB = ({ navigation }: NavProp) => (
  <View style={styles.container}>
    <Text>Screen B</Text>
    <Text>backButtonDisplayMode: default</Text>
    <Button
      onPress={() => navigation.navigate('screenC')}
      title="Go to screen C"
    />
  </View>
);

const ScreenC = ({ navigation }: NavProp) => (
  <View style={{ flex: 1, paddingTop: 50 }}>
    <Text>Screen C</Text>
    <Text>backButtonDisplayMode: generic</Text>
    <Button
      onPress={() => navigation.navigate('screenD')}
      title="Go to screen D"
    />
  </View>
);

const ScreenD = ({ navigation }: NavProp) => (
  <View style={styles.container}>
    <Text>Screen D</Text>
    <Text>backButtonDisplayMode: minimal</Text>
    <Button
      onPress={() => navigation.navigate('screenE')}
      title="Go to screen E"
    />
  </View>
);

const ScreenE = (_props: NavProp) => (
  <View style={styles.container}>
    <Text>Screen E</Text>
    <Text>backButtonDisplayMode omitted because of the headerBackTitle</Text>
  </View>
);

const styles = StyleSheet.create({
  container: { flex: 1, alignItems: 'center', justifyContent: 'space-around' },
});
```

</details>

## Checklist

- [x] Included code example that can be used to test this change
- [x] Updated TS types
- [x] Updated documentation: <!-- For adding new props to native-stack
-->
- [x]
https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md
- [x]
https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md
- [x]
https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx
- [x]
https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx
- [x] Ensured that CI passes

Tested software-mansion#1864: Paper ✅ Fabric ✅
Tested software-mansion#1646: Paper ❌ Fabric ❌ - but it does not work on main too, could
now be achieved using `backButtonDisplayMode: ‘minimal’`

---------

Co-authored-by: Kacper Kafara <kacper.kafara@swmansion.com>
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.

Header back button does not shorten to 'Back' on iOS if there's not enough space
4 participants