-
-
Notifications
You must be signed in to change notification settings - Fork 587
feat(iOS, Tabs): add bottomAccessory support #3288
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't finished yet, flushing the 1st part
void adopt(ShadowNode &shadowNode) const override { | ||
react_native_assert( | ||
dynamic_cast<RNSBottomTabsAccessoryShadowNode *>(&shadowNode)); | ||
auto &bottomTabsAccessoryShadowNode = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: might be considered as const
|
||
namespace facebook::react { | ||
|
||
using namespace yoga; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I prefer using yoga::
instead of via using namespace
- yoga classes and other definitions have pretty common names, so it may lead to some conflicts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw it's not used
@@ -0,0 +1,18 @@ | |||
#include "RNSBottomTabsAccessoryShadowNode.h" | |||
|
|||
#include <yoga/Yoga.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not used
const Size frameSize{}; | ||
const Point offset{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like transitive dependencies, can we import these classes explicitly?
- (void)didMoveToWindow | ||
{ | ||
if (self.window != nil) { | ||
[_helper registerForAccessoryFrameChanges]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need some cleanup counterpart for this method?
/** | ||
* If not null, the bottom tabs host view that this accessory component view belongs to. | ||
*/ | ||
@property (nonatomic, weak, nullable) RNSBottomTabsHostComponentView *reactSuperview; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe reactSuperview -> bottomTabsHostView ?
if (@available(iOS 26, *)) { | ||
_helper = [[RNSBottomAccessoryHelper alloc] initWithBottomAccessoryView:self]; | ||
} | ||
_reactEventEmitter = [RNSBottomTabsAccessoryEventEmitter new]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this under the compile-time check, but not under runtime check?
- (void)invalidateController | ||
{ | ||
#if RNS_IPHONE_OS_VERSION_AVAILABLE(26_0) && !TARGET_OS_TV && !TARGET_OS_VISION | ||
[_helper invalidate]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we check if helper is not nil
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, leaving some more comments, but overall the code is looking very clear and most of them are nitpicks
switch (environment) { | ||
case UITabAccessoryEnvironmentRegular: | ||
payloadEnvironment = react::RNSBottomTabsAccessoryEventEmitter::OnEnvironmentChangeEnvironment::Regular; | ||
break; | ||
case UITabAccessoryEnvironmentInline: | ||
payloadEnvironment = react::RNSBottomTabsAccessoryEventEmitter::OnEnvironmentChangeEnvironment::Inline; | ||
break; | ||
default: | ||
return NO; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering whether this conversion can be extracted
{ | ||
if (self = [super init]) { | ||
#if RCT_NEW_ARCH_ENABLED | ||
_reactEventEmitter = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will be nullptr by default, we don't need to initialize it explicitly
NSString *environmentString; | ||
switch (environment) { | ||
case UITabAccessoryEnvironmentRegular: | ||
environmentString = @"regular"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd consider defining these strings as static variables
RNSBottomTabsAccessoryComponentView *bottomAccessory = nil; | ||
for (UIView *childView in _reactSubviews) { | ||
if ([childView isKindOfClass:[RNSBottomTabsScreenComponentView class]]) { | ||
RNSBottomTabsScreenComponentView *childScreen = static_cast<RNSBottomTabsScreenComponentView *>(childView); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: by obj-c convention, should we use static_cast or c-style cast here?
- (void)insertReactSubview:(UIView *)subview atIndex:(NSInteger)index | ||
{ | ||
[super insertReactSubview:subview atIndex:index]; | ||
BOOL isValidBottomAccessory = [subview isKindOfClass:[RNSBottomTabsAccessoryComponentView class]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isValidBottomAccessory -> isBottomAccessory ?
[super removeReactSubview:subview]; | ||
BOOL isValidBottomAccessory = [subview isKindOfClass:[RNSBottomTabsAccessoryComponentView class]]; | ||
BOOL isTabsScreen = [subview isKindOfClass:[RNSBottomTabsScreenComponentView class]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is repeated across some functions, I'd consider extracting it
<BottomTabsAccessoryNativeComponent | ||
{...props} | ||
collapsable={false} | ||
style={[props.style, styles.container]}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be reversed? container might override some user-defined styles
{...props} | ||
collapsable={false} | ||
style={[props.style, styles.container]}> | ||
{props.children} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shorter version <BottomTabsAccessoryNativeComponent ... /> should work well
Description
Adds support for
botttomAccessory
in Bottom Tabs starting from iOS 26.Synchronization with ShadowTree
When bottom accessory transitions between
regular
andinline
environments (when tab bar is minimized), we need to update the position and size of the bottom accessory. Our approach is different for RN < 0.82 and RN >= 0.82.Legacy architecture & New architecture prior to
[email protected]
In versions prior to RN 0.82, we need use
DisplayLink
and presentation layer frames to get intermediate frames during the transition. This approach however has a major drawback - we are always at least one frame behind the current state as we're observing what is currently presented. When the difference in size/origin between frames is significant, you can see the content "jumping". In the case of bottom accessory, this is especially visible when using non-translucent background and transitioning frominline
toregular
environment (pay attention to the right edge of the accessory).Simulator.Screen.Recording.-.iPhone.17.Pro.-.2025-10-13.at.08.31.39.mov
Introduction of synchronous state updates in RN 0.82 (more details below) does not improve the situation when using this approach as we are still going to be at least one frame behind the animation.
[email protected]
and higherThanks to introduction of synchronous state updates in RN 0.82, we can rely fully on native mechanisms for handling the transition. Bottom accessory only receives the final frame of the transition and thanks to synchronous state updates, we can immediately recalculate the layout in the Shadow Tree and update the Host Tree. This allows Core Animation framework to make the transition smooth. Details of how we think this works are available here.
Unfortunately, when using
react-native
, the situation is a little bit more complicated.Text
Text component behaves differently to the native platform. During the transition, it immediately adapts to the final frame size and then it is stretched. In bare UIKit app, the text adapt to new frame size at the end of the transition.
react-native
UIKit
Simulator.Screen.Recording.-.iPhone.17.Pro.-.2025-10-13.at.10.16.33.mov
Simulator.Screen.Recording.-.iPhone.17.Pro.-.2025-10-13.at.10.22.22.mov
This requires more investigation and potentially changes in
react-native
.Borders
CoreAnimation
does not support non-uniform borders soreact-native
handles them in a custom way that does not seem to be compatible with the transition mechanism.Simulator.Screen.Recording.-.iPhone.17.Pro.-.2025-10-13.at.10.28.39.mov
Simulator.Screen.Recording.-.iPhone.17.Pro.-.2025-10-13.at.10.32.17.mov
This requires more investigation and potentially changes in
react-native
.Images
Similar problem (in a way it looks, not the exact mechanism of the bug) happens when using images e.g. with
width: 100%
.Simulator.Screen.Recording.-.iPhone.17.Pro.-.2025-10-13.at.11.08.09.mov
This requires more investigation and potentially changes in
react-native
.Mounting/unmounting views during transition
While state updates are performed synchronously, any changes to React Element Tree in reaction to environment change are handled asynchronously. We think that this is why the transition handled by
CoreAnimation
breaks when trying to mount/unmount components on environment change.Simulator.Screen.Recording.-.iPhone.17.Pro.-.2025-10-13.at.08.16.12.mov
Here, we try to remove the note icon. Unfortunately, the rest of the layout does not adapt. You can also observe the text stretching as mentioned 2 sections above.
Changes
BottomTabsAccessory
JS component and use it inBottomTabs
,BottomTabsAccessoryComponentView
,BottomTabsAccessoryEventEmitter
,BottomTabsAccessoryComponentViewManager
,BottomAccessoryHelper
to synchronize state between Host and ShadowTree,BottomTabsHost
to accept 2 types of children (Screen
andAccessory
),Test code and steps to reproduce
Run
Test3288
.Checklist