fix(iOS): non-interactive screen while switching between bottom-tab and native-stack navigators

[tboba]

Description

Currently, while switching between BottomTabNavigator and NativeStackNavigator, user cannot interact with screens from BottomTabNavigator. That's because while we're coming back to the previous screen, original screen frame has frame (0, 0, 0, 0). I've investigated that even if we're calling updateLayoutMetrics on a view, it's not being updated, since the RNSScreen implementation of that method firstly checks if parentVC is not RNSNavigationContainer and if it's not nil -> Because it's always nil during the insertion (as we're calling for screen updates too late), we're never updating layout metrics.

Fixes #2252.

Changes

• Moved calling for parent updates before updating layout metrics.

Screenshots / GIFs

Before

Screen.Recording.2024-07-22.at.17.17.57.mov

After

Screen.Recording.2024-07-22.at.17.15.58.mov

Test code and steps to reproduce

You can test Test2252.tsx to check these changes.

Checklist

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

[kkafar]

Great catch! I've few questions that require answering before we can proceed, and possibly few objections to proposed solution.

[...] I've investigated that even if we're calling updateLayoutMetrics on a view, it's not being updated, since the RNSScreen implementation of that method firstly checks if parentVC is not RNSNavigationContainer and if it's not nil -> Because it's always nil during the insertion (as we're calling for screen updates too late), we're never updating layout metrics.

I do not understand a couple of things here and need some explanation.

Let me present you with my thought process first:

1. On Fabric, children are initialised first with props, frame, state, etc. and mounted to the parent view component before the parent is updated with its frame & prop information.
2. In - [RNSScreenView updateLayoutMetrics: newLayoutMetrics:], thus when receiving Yoga frame, we're setting the frame if and only if the RNSScreenView's RNSScreen view controller has parent view controller and it is not a stack view controller.
3. Points (1) & (2) & the fact that it is responsibility of screen container to attach RNSScreen into view controller hierarchy imply that the initial frame for a RNSScreenView under a screen container is always ignored, because at the time when the initial frame is set, the RNSScreenView is not yet mounted under RNSScreenContainerView. This is interesting fact on it's own & I think it is not intended or at least I do not know the reason to ignore initial frame in screen container. We should definitely look into this at some later point.
4. Just few calls later after ignoring initial frame for RNSScreenView because it's view controller has not yet been mounted under screen container, we finally mount the RNSScreenView under RNSScreenContainerView & in the mountChildComponentView:index: method we try to set RNSScreenView's frame before we effectively mount it by calling - markUpdated, which fails exactly due to the same reasoning as in point (3).

Do I get that correctly?

Now, provided that we do not set the screen's frame in any other place after the screen is mounted it might be likely that bugs happens cause we did not effectively set a frame for a screen. However we do it in - [RNScreenContainerView layoutSubviews] method <- thus here comes my next question: doesn't the system trigger the layout once the child view of container is mounted? Cause if it does not trigger the layout, then we would not set the screen's frame ever 😅 How does it happen then?

Another questions relate to the #2252 issue description. How does the presence of SafeAreaProvider or headerShown: false on stack navigators impact the behaviour of the screen stack? Why there is no bug when SafeAreaProvider is not present?

And at last I have a remark to the current solution, as mounting the child before it has a frame set could possibly lead to reintroducing some bugs. I can't point to what exactly could break, but we break assumption we make in - [RNSScreenContainerView attachScreen: atIndex:] method. Thus my suggestion would be to remain with current calling order but, for example create a separate method - [RNSScreenView updateLayoutMetrics: oldLayoutMetrics: force:] which would set the frame forcibly if force == YES or apply - [RNSScreenView updateLayoutMetrics: oldLayoutMetrics] otherwise. You could also try to modify condition in - [RNSScreenView updateLayoutMetrics: oldLayoutMetrics], so that when parent is nil we allow for setting the frame.

[tboba]

Hey hey @kkafar, about your reasoning - yep, you're absolutely right! 😄
So, about your question - we're indeed updating the layout metrics inside the layoutSubviews method, but unfortunately, subviews are being layouted only on the initial render of views from BottomTabNavigator (at least in this scenario, with conditional rendering) - this means that after switching the state of what should be rendered conditionally, original component (here, view with BottomTabNavigator) is being recycled and after coming back again to this view, it is taken from recycling pool (I just confirmed that). So one option is to disable recycling here (I'm going to do this in a separate PR) and remove updating layout metrics from mountChildComponentView, but for now I'm against it, since I'm worried about content jumping while layouting subviews and not knowing initial values from updateLayoutMetrics in mountChildComponentView 🤔

Frankly, I don't know why having SafeAreaProvider impacts the layout 😅 But without SafeAreaProvider or/and with headerShown set to true, layoutSubviews method is being called -> really hard to say what calls this layout, since it comes from UIView from iOS natives. Possibly overriden method of layoutSubviews in RNCSafeAreaProviderCompat blocks further method calls (even if it has [super layoutSubviews] method called there?).

Sure, I'll think about another approach at today's morning!

[kkafar]

I've played around with this diff on different examples & haven't seen any issue it could have caused, so I think we're good to go here. Great job!