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

Crash when rendering Text within ShadowView within Text #17

Open
Titozzz opened this issue Aug 3, 2023 · 4 comments
Open

Crash when rendering Text within ShadowView within Text #17

Titozzz opened this issue Aug 3, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@Titozzz
Copy link

Titozzz commented Aug 3, 2023

Describe the bug

When rendering something like

<Text>
  <ShadowView>
    <Text>"Foo"</Text> // This will be RCTVirtualizedText and crash
  </ShadowView>
</Text>

This is because in react-native, Views import TextAncestor context and set it to false (https://github.com/facebook/react-native/blob/0fb71630c7d4eff5758f7148ae2d9b1f09bc9d2c/packages/react-native/Libraries/Components/View/View.js#L130)

To reproduce

See above

Expected behavior

No crash

Your environment

N/Applicable.


TLDR: this crash is well identified and I've started a discussion with Meta engineers to know how we are supposed to proceed as importing from 'react-native/Libraries/Text/TextAncestor' isn't safe, this is more to let other people having this issue that it's being looked at.

@Titozzz Titozzz added the bug Something isn't working label Aug 3, 2023
@WadhahEssam
Copy link

@Titozzz why is importing it directly from react native isn't safe?

@Titozzz
Copy link
Author

Titozzz commented Aug 9, 2023

@WadhahEssam well this isn't a public API and could break at any release. If you are fine with that point, you can totally use it. (We do)

@simontreny
Copy link
Collaborator

Hi @Titozzz,

To be sure I understand correctly, could you tell me more about what you're trying to achieve?

  • If you'd like to apply a shadow directly on the inner text element, you shouldn't use ShadowedView (or even View) but rely instead on the textShadowColor/textShadowRadius/textShadowOffset style props (doc). They work well on both iOS and Android and will perform better on iOS than using a view for this.
  • If instead you'd like to apply a shadow on the inline ShadowedView element (I assume it would at least have a background color in this case), then indeed, we'll have to add a TextAncestor context in ShadowedView for it to work. I'm ok to import it even though it's not a public API as we've been relying on it internally for our design system for a while now and it didn't break for many React Native releases. In the meantime, you can probably fix the crash by just wrapping the ShadowedView with a regular View to get the context without having to import the file yourself. Also, note that inline views in Text comes with some downsides: text within the inline view won't be wrapped, you might need to adjust vertical alignment of text and you might get different results between iOS and Android.

@Titozzz
Copy link
Author

Titozzz commented Aug 19, 2023

Not trying to achieve anything special, usually we avoid nesting view within Text for the reasons listed above, but for some reason there were a few that slipped through. And when I replaced all our Views with ShadowedViews it crashed.

It's not a huge deal since I now only replace Views if there are shadow styles + I removed the nested View as they should not have existed. Also just to be sure, I patched the lib so that it wouldn't crash.

This issue was more a FYI than anything else!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants