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

Remove view flattening error for Text component #3338

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Remove view flattening error for Text component #3338

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
353f41a
Add check for text on android
m-bert Jan 16, 2025
c2c0f58
Change variable name
m-bert Jan 16, 2025
9426805
Check for text on iOS
m-bert Jan 16, 2025
e8cdbe5
Merge branch 'main' into @mbert/remove-flattening-warning-with-text
m-bert Jan 16, 2025
72e78cf
Update android/src/main/jni/cpp-adapter.cpp
m-bert Jan 21, 2025
393b377
Simplify iOS
m-bert Jan 21, 2025
b6b6765
Remove unused imports
m-bert Jan 21, 2025
411ad67
Merge branch 'main' into @mbert/remove-flattening-warning-with-text
m-bert Jan 21, 2025
39cf55b
Bring back Paragraph check on iOS
m-bert Jan 23, 2025
a16dfd4
Merge branch 'main' into @mbert/remove-flattening-warning-with-text
m-bert Jan 23, 2025
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
25 changes: 16 additions & 9 deletions android/src/main/jni/cpp-adapter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
#include <jsi/jsi.h>

#include <react/renderer/uimanager/primitives.h>
#include <react/renderer/components/text/ParagraphShadowNode.h>
#include <react/renderer/components/text/TextShadowNode.h>

using namespace facebook;
using namespace react;
Expand All @@ -20,21 +22,26 @@ void decorateRuntime(jsi::Runtime &runtime) {
}

auto shadowNode = shadowNodeFromValue(runtime, arguments[0]);
bool isFormsStackingContext = shadowNode->getTraits().check(ShadowNodeTraits::FormsStackingContext);
bool isFormsStackingContext = shadowNode->getTraits().check(
ShadowNodeTraits::FormsStackingContext);

return jsi::Value(isFormsStackingContext);
const char *componentName = shadowNode->getComponentName();
m-bert marked this conversation as resolved.
Show resolved Hide resolved
bool isTextComponent = strcmp(componentName, "Paragraph") == 0 ||
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, does this happen in both cases or only for Text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Answered here

strcmp(componentName, "Text") == 0;

return jsi::Value(isFormsStackingContext || isTextComponent);
});
runtime.global().setProperty(
runtime, "isFormsStackingContext", std::move(isFormsStackingContext));
}

extern "C" JNIEXPORT void JNICALL
Java_com_swmansion_gesturehandler_react_RNGestureHandlerModule_decorateRuntime(
JNIEnv *env,
jobject clazz,
jlong jsiPtr) {
jsi::Runtime *runtime = reinterpret_cast<jsi::Runtime *>(jsiPtr);
if (runtime) {
decorateRuntime(*runtime);
}
JNIEnv *env,
jobject clazz,
jlong jsiPtr) {
jsi::Runtime *runtime = reinterpret_cast<jsi::Runtime *>(jsiPtr);
if (runtime) {
decorateRuntime(*runtime);
}
}
17 changes: 16 additions & 1 deletion apple/RNGestureHandlerModule.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
#import <ReactCommon/CallInvoker.h>
#import <ReactCommon/RCTTurboModule.h>

#import <react/renderer/components/text/ParagraphShadowNode.h>
#import <react/renderer/components/text/TextShadowNode.h>
#import <react/renderer/uimanager/primitives.h>
#endif // RCT_NEW_ARCH_ENABLED

Expand All @@ -27,6 +29,8 @@

#import <React/RCTJSThread.h>

#import <cstring>

#ifdef RCT_NEW_ARCH_ENABLED
using namespace facebook;
using namespace react;
Expand Down Expand Up @@ -101,7 +105,18 @@ void decorateRuntime(jsi::Runtime &runtime)
}
auto shadowNode = shadowNodeFromValue(runtime, arguments[0]);
bool isFormsStackingContext = shadowNode->getTraits().check(ShadowNodeTraits::FormsStackingContext);
return jsi::Value(isFormsStackingContext);

bool isTextComponent = false;

if (auto v = dynamic_pointer_cast<const ParagraphShadowNode>(shadowNode); v != nullptr) {
isTextComponent = true;
}

if (auto v = dynamic_pointer_cast<const TextShadowNode>(shadowNode); v != nullptr) {
isTextComponent = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

  1. Is isFormsStackingContext false in both cases or only in the second one?
  2. If in both, can this be a single if (v != nullptr is not necessary, nullptr is falsy)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like it is true in first case and false in second. Also, this if statement is adapted from docs, but I can change it.

What do you think about returning jsi::Value(true), instead of assigning to another variable? It could be cleaner, but less readable at the same time so I'm not sure.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like it is true in first case and false in second

In which case the error is thrown then?

Also, this if statement is adapted from docs, but I can change it.

I'd change it to stay consistent with RN and other libraries.

What do you think about returning jsi::Value(true), instead of assigning to another variable? It could be cleaner, but less readable at the same time so I'm not sure.

That works, we can also consider changing the name of the function at this point, as it does a wider check than the name suggests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In which case the error is thrown then?

It is thrown when we have nested Text components, i.e. we have TextShadowNode. We can remove if with ParagraphShadowNode as error is not thrown in that case.

I'd change it to stay consistent with RN and other libraries.

Sure, I'll do it

That works, we can also consider changing the name of the function at this point, as it does a wider check than the name suggests.

What about canBeViewFlattened, since this is the reason why we perform this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Partially done in 393b377

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More info in this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about canBeViewFlattened, since this is the reason why we perform this check?

Okay, now when I think about it it doesn't work, as it returns exactly the opposite. What about one of these:

  • disallowsViewFlattening
  • isFlatteningDisabled


return jsi::Value(isFormsStackingContext || isTextComponent);
});
runtime.global().setProperty(runtime, "isFormsStackingContext", std::move(isFormsStackingContext));
}
Expand Down
Loading