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

Remove view flattening error for Text component #3338

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

Conversation

m-bert
Copy link
Contributor

@m-bert m-bert commented Jan 16, 2025

Description

Right now our Text components causes error that GestureDetector received view that may be view flattened. Adding collapsable into Text is not possible, so we decided to remove the error.

Android check is different because using dynamic_pointer_cast yields false, even though in debugger shadowNode can be seen as TextShadowNode.

Also, I've decided to use Android Studio formatter, to make sure that everyone in the team will have the same results.

Test plan

Tested on Nested Text example.

Comment on lines 111 to 117
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);
const char *componentName = shadowNode->getComponentName();
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

android/src/main/jni/cpp-adapter.cpp Show resolved Hide resolved
@m-bert
Copy link
Contributor Author

m-bert commented Jan 21, 2025

Okay, I've managed to find out what's going on on both platform, hopefully this will answer your questions, @j-piasecki 😅

First, these are values of isFormsStackingContext:

Android iOS
ParagraphShadowNode false true
TextShadowNode false false

in case of single Text component, only one ShadowNode is checked, that is ParagraphShadowNode.

When it comes to nested Text, two ShadowNodes are checked (ParagraphShadowNode and TextShadowNode).

Given those information, on iOS we can omit checking for ParagraphShadowNode as this function will return true anyway. On the other hand, on Android we have to handle both cases.

@j-piasecki
Copy link
Member

Given those information, on iOS we can omit checking for ParagraphShadowNode as this function will return true anyway. On the other hand, on Android we have to handle both cases.

I think we can leave checks for both everywhere. The difference would be confusing in the future with no context.

@m-bert
Copy link
Contributor Author

m-bert commented Jan 23, 2025

I agree, done in 39cf55b

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.

2 participants