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

Substr measurements expensify #56

Conversation

azimgd
Copy link

@azimgd azimgd commented May 18, 2023

Upstream PR Link

facebook#37397

Summary

I would like to apply styling to a substring of text, such as a background color, padding, and rounded corners. This functionality could be used for:

  • Username mention highlighting (similar to Slack): Happy birthday @username
  • Inline code block (similar to GitHub): Lorem Ipsum is simply dummy text of the printing and typesetting industry. Lorem Ipsum has been the industry's standard dummy text ever since the 1500s, when an unknown printer took a galley of type and scrambled it to make a type specimen book.
Screen Shot 2023-05-16 at 14 42 00

The problem is that the Text element in RN behaves like a block element rather than an inline element when it is multiline. Additionally, it does not allow you to have borders or rounded corners. This makes it unsuitable for implementing a multiline inline code block.

Adding a new prop called textLayoutRegions which accepts an array of regions (substring positions) could be used to solve the problem. Passing this prop will calculate the regions [[start, end], [start, end] ...] position and pass the response into onTextLayout callback. Knowing exact substring position and dimensions will allow implementing custom styled View behind the text.

<View>
  {regions.map((region, index) => (
    <View style={{width: region.width, height: region.height, top: region.y, left: region.x, backgroundColor: 'red', position: 'absolute'}} />
  ))}

  <Text style={{lineHeight: 20}} onTextLayout={event => setRegions(event.nativeEvent.regions)} textLayoutRegions={[[12, 242]]}>
    Lorem Ipsum  is simply dummy text of the printing and typesetting industry. Lorem Ipsum has been the industry's standard dummy text ever since the 1500s, when an unknown printer took a galley of type and scrambled it to make a type specimen  book.
  </Text>
</View>

// onTextLayout event
{
  lines: [...],
  regions: [
  {
    "height": 20,
    "line": 0,
    "region": 0,
    "text": " is simply dummy text of the printing and typesetting ",
    "width": 325.71429443359375,
    "x": 85.42857360839844,
    "y": 0
  },
  {
    "height": 20,
    "line": 1,
    "region": 0,
    "text": "industry. Lorem Ipsum has been the industry's standard dummy ",
    "width": 393.71429443359375,
    "x": 0,
    "y": 20
  },
  {
    "height": 20,
    "line": 2,
    "region": 0,
    "text": "text ever since the 1500s, when an unknown printer took a galley ",
    "width": 400.28570556640625,
    "x": 0,
    "y": 40
  },
  {
    "height": 20,
    "line": 3,
    "region": 0,
    "text": "of type and scrambled it to make a type specimen  ",
    "width": 313.1428527832031,
    "x": 0,
    "y": 60
  }
]
}

Changelog

[GENERAL] [ADDED] - New textLayoutRegions prop for Text component that accepts an array of substring regions. Each region's layout position will be appended into onTextLayout event callback.
[GENERAL] [ADDED] - New regions props for onTextLayout event callback.

Test Plan

  • Run RNTester app
  • Go to APIs → Layout Events page
  • Ensure substring has a red background color around selected region

@azimgd
Copy link
Author

azimgd commented May 18, 2023

@Szymon20000 could you have a look please?

@parasharrajat
Copy link
Member

onTextLayout={event => setRegions(event.nativeEvent.regions)} textLayoutRegions={[[12, 242]]}

Instead of textLayoutRegions can the same purpose be achieved with nested Text element? So users don't have to pass these regions manually instead we access them from the content inside the text itself. Each Text will share the region details on its own onTextlayout event.

@azimgd
Copy link
Author

azimgd commented May 25, 2023

https://reactnative.dev/docs/text#nested-text

Behind the scenes, React Native converts this to a flat NSAttributedString or SpannableString that contains the following information:

"I am bold and red"
0-9: bold
9-17: bold, red

Nested text with style is essentially just a string and not a view component, therefore this implementation isn't straightforward to implement.

@Szymon20000
Copy link

Szymon20000 commented May 29, 2023

Taking a look now. Wouldn't be cleaner to implement onLayout for nested Text components? We could then wrap fragment that has to be highlighted into nested text.

<Text> sdfnsidjfnsidf sdfuisdfisndfisjdnfisn <Text onLayout={setViewBehind}> HighlightedCode (can be multiline then we send lines) </Text> </Text>

@azimgd
Copy link
Author

azimgd commented May 29, 2023

It would, however please take a look at the comment above.

Nested text with style is essentially just a string and not a view component, therefore this implementation isn't straightforward to implement.

@Szymon20000
Copy link

It doesn't seems to be that hard. At least just checked it on iOS Fabric.
We can add onLayoutProp to TextComponent (currently it's only set for paragraph).
Then build regions from fragments (if corresponding TextShadowNode has onLayout prop set ).
Then instead of calling Paragraph Event Emitter we can call TextEventEmitter (it's not hard to create it).

@Szymon20000
Copy link

BaseTextShadowNode::buildAttributedString <- This method creates fragments. It could also create regions

@Szymon20000
Copy link

Otherwise the code seems to be correct to me :)

@Szymon20000
Copy link

We could also add regionID prop for nested text instead. Then paragraphEventEmitter would return map {regionID: RegionMeasurment}

@parasharrajat
Copy link
Member

@azimgd Any thoughts on @Szymon20000's comments?

@parasharrajat
Copy link
Member

parasharrajat commented Jun 2, 2023

Started testing the Rn_tester app with Hermes. Faced this issue. Is this related?

Screenshot 2023-06-03 at 3 42 07 AM

@puneetlath puneetlath self-requested a review June 2, 2023 23:34
@parasharrajat
Copy link
Member

parasharrajat commented Jun 5, 2023

@azimgd Any help with the above issue?

@perunt
Copy link

perunt commented Jun 6, 2023

@parasharrajat this PR fix this issue

@parasharrajat
Copy link
Member

Awesome. Thanks @perunt. I'll watch it.

@puneetlath
Copy link

@azimgd thoughts on the feedback?

@azimgd
Copy link
Author

azimgd commented Jun 15, 2023

I'm not entirely certain about the approach we should take here since it seems that the upstream PR will not be merged, and the expensify/RN fork is being abandoned.
I will attempt to develop my own library that handles text measurement and background drawing separately from the RN core. The main challenge lies in accessing the LayoutManager of the children elements. I am familiar with how to accomplish this using Fabric, but currently investigating how to do the same with the old-arch.

I'm actively working on it, but unfortunately cannot provide an estimate for when it will be completed. Let me know any thoughts or suggestions you may have, and I will keep you updated once I have smth.

@parasharrajat
Copy link
Member

@azimgd We haven't finalized the decision of dropping the fork yet. If that is blocking you move forward, I will suggest we wait for the final verdict on it before you start spending time on a custom implementation.

Also, it will be great to show us the plan before implementing it.

@Szymon20000
Copy link

I'm not entirely certain about the approach we should take here since it seems that the upstream PR will not be merged, and the expensify/RN fork is being abandoned. I will attempt to develop my own library that handles text measurement and background drawing separately from the RN core. The main challenge lies in accessing the LayoutManager of the children elements. I am familiar with how to accomplish this using Fabric, but currently investigating how to do the same with the old-arch.

I'm actively working on it, but unfortunately cannot provide an estimate for when it will be completed. Let me know any thoughts or suggestions you may have, and I will keep you updated once I have smth.

I think the upstream pr could be merged. We just need to change the API a bit.

@puneetlath
Copy link

Hey @azimgd. Is this still something you want to work on or shall we ask someone else to try one of the alternative approaches?

@azimgd
Copy link
Author

azimgd commented Jul 6, 2023

Hey @puneetlath, I'm still exploring alternative approach.

@puneetlath
Copy link

Ok cool. Can we help in any way?

@puneetlath
Copy link

Let's go ahead and close this out since we're no longer taking this approach. Thanks for the effort everyone!

@puneetlath puneetlath closed this Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants