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): jumping content on fabric #2313

Merged

Conversation

alduzy
Copy link
Member

@alduzy alduzy commented Aug 22, 2024

Description

This PR applies modifications to a previous fix: #2169 for fabric only, which has stopped working since RN 0.75.

In RN 0.74 the adopt in RNSScreenComponentDescriptior.h was once called without stateData but with children and we could then check if the ScreenStackHeaderConfig is present among them and make adjustments based on it.

When working on #2292 it became clear that the fix does not work anymore. Now the adopt is called either with no children and no stateData or with both.
The solution is to move the code to appendChild in RNSScreenShadowNode.cpp so we can perform the adjustments as soon as the children append.

Changes

  • moved code from adopt in RNSScreenComponentDescriptior.h to newly added appendChild override in RNSScreenShadowNode.cpp

Screenshots / GIFs

Before

Screen.Recording.2024-08-22.at.17.45.41.mov

After

Screen.Recording.2024-08-22.at.17.48.01.mov

Test code and steps to reproduce

  • Use TestHeader to test this change

Checklist

@alduzy alduzy requested a review from WoLewicki August 22, 2024 15:50
@alduzy alduzy changed the base branch from main to @kkafar/fabric-pressables August 22, 2024 15:51
@alduzy alduzy changed the title fix(Android): jumping content on fabric fix(Android): jumping content on fabric and paper! Aug 22, 2024
@alduzy alduzy requested review from tboba and kkafar August 22, 2024 17:16
@alduzy alduzy changed the title fix(Android): jumping content on fabric and paper! fix(Android): jumping content on fabric Aug 22, 2024
@tboba
Copy link
Member

tboba commented Aug 23, 2024

Why do we want to merge this into @kkafar/fabric-pressables? Is this PR some part of ongoing another one?

@WoLewicki
Copy link
Member

Is there a reason you removed the checks for if the calculated values are the same as the ones returned by the system later? It is important source of information for if we do the calculations right.

@alduzy alduzy force-pushed the @alduzy/android-fabric-jumping-content-fix branch from a69ae08 to 87097ea Compare August 23, 2024 08:34
@alduzy alduzy changed the base branch from @kkafar/fabric-pressables to main August 23, 2024 08:35
@alduzy
Copy link
Member Author

alduzy commented Aug 23, 2024

@tboba You're right, we can merge it independently.

@alduzy alduzy force-pushed the @alduzy/android-fabric-jumping-content-fix branch from 87097ea to f9a647c Compare August 23, 2024 09:29
@tboba

This comment was marked as outdated.

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! Just one remark about method ordering 😄

Comment on lines 18 to 85
std::optional<std::reference_wrapper<const ShadowNode::Shared>>
findHeaderConfigChild(
const YogaLayoutableShadowNode &screenShadowNode) {
for (const ShadowNode::Shared &child : screenShadowNode.getChildren()) {
if (std::strcmp(
child->getComponentName(), "RNSScreenStackHeaderConfig") == 0) {
return {std::cref(child)};
}
}
return {};
}

static constexpr const char *kScreenDummyLayoutHelperClass =
"com/swmansion/rnscreens/utils/ScreenDummyLayoutHelper";

#ifdef ANDROID
std::optional<float> findHeaderHeight(
const int fontSize,
const bool isTitleEmpty) {
JNIEnv *env = facebook::jni::Environment::current();

if (env == nullptr) {
LOG(ERROR) << "[RNScreens] Failed to retrieve env\n";
return {};
}

jclass layoutHelperClass = env->FindClass(kScreenDummyLayoutHelperClass);

if (layoutHelperClass == nullptr) {
LOG(ERROR) << "[RNScreens] Failed to find class with id "
<< kScreenDummyLayoutHelperClass;
return {};
}

jmethodID computeDummyLayoutID =
env->GetMethodID(layoutHelperClass, "computeDummyLayout", "(IZ)F");

if (computeDummyLayoutID == nullptr) {
LOG(ERROR)
<< "[RNScreens] Failed to retrieve computeDummyLayout method ID";
return {};
}

jmethodID getInstanceMethodID = env->GetStaticMethodID(
layoutHelperClass,
"getInstance",
"()Lcom/swmansion/rnscreens/utils/ScreenDummyLayoutHelper;");

if (getInstanceMethodID == nullptr) {
LOG(ERROR) << "[RNScreens] Failed to retrieve getInstanceMethodID";
return {};
}

jobject packageInstance =
env->CallStaticObjectMethod(layoutHelperClass, getInstanceMethodID);

if (packageInstance == nullptr) {
LOG(ERROR)
<< "[RNScreens] Failed to retrieve packageInstance or the package instance was null on JVM side";
return {};
}

jfloat headerHeight = env->CallFloatMethod(
packageInstance, computeDummyLayoutID, fontSize, isTitleEmpty);

return {headerHeight};
}
#endif // ANDROID
Copy link
Member

Choose a reason for hiding this comment

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

Let's move these methods below appendChild

Copy link
Member Author

Choose a reason for hiding this comment

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

@tboba I'm afraid those methods have to be declared before they're used in cpp

Copy link
Member

Choose a reason for hiding this comment

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

@alduzy But didn't we achieve different order, but in RNSScreenComponentDescriptor.h? 😄
It's not a big deal to have them here though, so if it's really not possible, I think we can proceed with what we have right now.

@alduzy alduzy force-pushed the @alduzy/android-fabric-jumping-content-fix branch from fdb1834 to aee2a1f Compare August 23, 2024 10:21
@alduzy alduzy changed the base branch from main to @kkafar/fabric-pressables August 23, 2024 10:21
@alduzy
Copy link
Member Author

alduzy commented Aug 23, 2024

@tboba EDIT: the correct layout is actually achieved based on the changes made in #2292 so I had to rebase back to it.

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.

Looks good! Thanks!

@kkafar kkafar merged commit 204bef3 into @kkafar/fabric-pressables Aug 27, 2024
2 checks passed
@kkafar kkafar deleted the @alduzy/android-fabric-jumping-content-fix branch August 27, 2024 09:19
@kkafar kkafar restored the @alduzy/android-fabric-jumping-content-fix branch August 27, 2024 09:21
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.

4 participants