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.

Sign up for GitHub

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
merged 3 commits into from
Apr 29, 2024
Merged

fix(iOS): restore native behavior of auto shortening back button title #2105

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee3ad3b
fix(iOS): restore native behavior of auto shortening back button title
zetavg Apr 14, 2024
07c7e1e
chore: add comment to explain why comparing backTitleFontFamily again…
zetavg Apr 17, 2024
4272ec4
Apply suggestions from code review
zetavg Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions ios/RNSScreenStackHeaderConfig.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,8 +511,17 @@ + (void)updateViewController:(UIViewController *)vc
action:nil];
[backBarButtonItem setMenuHidden:config.disableBackButtonMenu];

auto isBackButtonCustomized = !isBackTitleBlank || config.disableBackButtonMenu;

if (config.isBackTitleVisible) {
if (config.backTitleFontFamily || config.backTitleFontSize) {
if ((config.backTitleFontFamily &&
// While being used by react-navigation, the `backTitleFontFamily` will
// be set to "System" by default - which is the system default font.
// To avoid always considering the font as customized, we need to have an additional check.
// See: https://github.com/software-mansion/react-native-screens/pull/2105#discussion_r1565222738
![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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

config.backTitleFontSize) {
isBackButtonCustomized = YES;
NSMutableDictionary *attrs = [NSMutableDictionary new];
NSNumber *size = config.backTitleFontSize ?: @17;
if (config.backTitleFontFamily) {
Expand All @@ -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. 🎉

prevItem.backButtonTitle = resolvedBackTitle;
}
prevItem.backBarButtonItem = backBarButtonItem;

// Prevent unnecessary assignment of backBarButtonItem if it is not customized,
// as assigning one will override the native behavior of automatically shortening
// the title to "Back" or hide the back title if there's not enough space.
// See: https://github.com/software-mansion/react-native-screens/issues/1589
if (isBackButtonCustomized) {
prevItem.backBarButtonItem = backBarButtonItem;
}

if (@available(iOS 11.0, *)) {
if (config.largeTitle) {
Expand Down
Loading