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(Android,Fabric): prevent another infinite state update loop for header subviews with zero size #2696

Merged

Conversation

kkafar
Copy link
Member

@kkafar kkafar commented Feb 13, 2025

Description

Closes #2675

Error mechanism

This one was tricky to figure out.

Few concurrent facts first:

  1. When statusBarTranslucent is true we apply paddingTop to native Toolbar to heighten the header.
  2. Since 4.6.0 we update frames of header subviews in ShadowTree to the values from HostTree, everytime the frame changes in HostTree.
  3. We had a check in header subview shadow node that would prevent node size update if the frame was (0, 0).

Header subview that represents the search bar has no content. Therefore native layout sets it frame to exactly (0, 0), but with origin (x, toolbar.paddingTop).
This frame - ((x, toolbar.paddingTop), (0, 0)) is send to shadow node, where it is ignored, but the layout is triggered by subsequent header config update.
Therefore Yoga resolves height of the subview to full height of the parent (usually 154 px = 40 dip). New layout metrics are sent to HostTree, where next native toolbar layout
determines toolbar size to be its content height + padding == subview height + paddingTop. This gets send to ShadowTree, then back to HostTree, native layout is triggered and another
paddingTop value is added to the overall header height, and so on.

This is a regression introduced in 4.6.0 and limited to Fabric.

The fix is simple - accept the (0, 0) size for the subview in ShadowTree. We just need to use more reasonable value to denote the uninitialized frame - {-1, -1} seems like a better choice as
it is an invalid frame.

Changes

  • Use different initial value in state so that we can unambiguously distinguish between un- and initialized state

Test code and steps to reproduce

Test2675

Tested on all combinations:

  • Android SDK 34 (w/o edge-to-edge) and 35 (w/ edge-to-edge enabled),
  • statusBarTranslucent: true and false,
  • search bar present, not present
  • other header elements present / not present.

Also tested iOS on the same example, because the code changes affect shared C++ code.

Checklist

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

Copy link
Member Author

@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.

Currently blocked by regression detected in testing

@@ -32,12 +33,11 @@ class RNSScreenStackHeaderSubviewComponentDescriptor final

auto state = std::static_pointer_cast<
const RNSScreenStackHeaderSubviewShadowNode::ConcreteState>(
shadowNode.getState());
Copy link
Member Author

Choose a reason for hiding this comment

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

The bug I've reported in earlier comment was caused by accessing mounted state and not the most recent one.

@kkafar kkafar merged commit 31c7035 into main Feb 17, 2025
8 checks passed
@kkafar kkafar deleted the @kkafar/weird-header-behaviour-with-search-bar-present-android-2 branch February 17, 2025 21:30
@marcel-happyfloat
Copy link

marcel-happyfloat commented Feb 18, 2025

@kkafar Thanks for the quick fix, unfortunately I can't install v4.7.0 on IOS now.
Getting this error (after running npx expo prebuild --clean and npx expo run:ios --no-build-cache) :

⚠️ ld: ignoring duplicate libraries: '-lc++'
❌ ld: framework 'RNScreens' not found

Android works now, but it looks like it is adding some safe area insets again (as a black bar) to the bottom of my screen, when I'm navigating (default presentation) to some other root screen.

modal and screens within the current Tab Stack are working fine...

Just to let you quickly know...

@kkafar
Copy link
Member Author

kkafar commented Feb 19, 2025

@marcel-happyfloat thanks for the report, however I do need more details to investigate any of the issues you mentioned.

Could you provide a recording of the bug and preferably a snippet / detailed description of navigation hierarchy where I can reproduce the issue?

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.

Android Header expands unexpectedly when headerSearchBarOptions are set in v4.6.0+
2 participants