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

Add substring measurements for Text #37397

Closed
wants to merge 19 commits into from

Conversation

azimgd
Copy link

@azimgd azimgd commented May 11, 2023

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

@facebook-github-bot
Copy link
Contributor

Hi @azimgd!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@azimgd azimgd marked this pull request as draft May 11, 2023 20:55
@azimgd
Copy link
Author

azimgd commented May 11, 2023

I have been first trying to solve the problem by adding a new prop called textCodeBlockStyle to . This prop accepts a background color, border radius, etc. Text renders a background rect natively using NSLayoutManager#drawBackgroundForGlyphRange for iOS and LineBackgroundSpan#drawBackground for Android.
However my approach had following drawbacks:

  • No Padding + Margin support as you can't draw anything past line height/width for text.
  • Unreliable behavior for iOS / Android

implementation: Expensify#43
context: Expensify/App#4733

Would you be willing to help me validate my idea @NickGerleman?

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 11, 2023
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@NickGerleman
Copy link
Contributor

I would like to apply styling to a substring of text, such as a background color, padding, and rounded corners.

Are you looking for this? https://reactnative.dev/docs/text#nested-text

@azimgd
Copy link
Author

azimgd commented May 12, 2023

Nested Text won't let you draw rounded borders with a background, margins don't work as well.

I would like to implement exactly this in RN:
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. It has survived not only five centuries, but also the leap into electronic typesetting, remaining essentially unchanged.

@analysis-bot
Copy link

analysis-bot commented May 15, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,732,275 -7,106
android hermes armeabi-v7a 8,042,840 -7,434
android hermes x86 9,220,576 -8,993
android hermes x86_64 9,074,227 -7,170
android jsc arm64-v8a 9,296,420 -6,304
android jsc armeabi-v7a 8,484,532 -6,667
android jsc x86 9,356,134 -7,845
android jsc x86_64 9,613,456 -6,463

Base commit: 1671247
Branch: main

@azimgd
Copy link
Author

azimgd commented May 16, 2023

Fully functioning textLayoutRegions prop on iOS and Android is pushed, see PR description for a screenshot and event payload. Will work on a CR fixes and old arch support soon.

@azimgd
Copy link
Author

azimgd commented May 17, 2023

RNTester:
Screen Shot 2023-05-17 at 16 21 37

@azimgd
Copy link
Author

azimgd commented May 17, 2023

@NickGerleman may I ask for a code-review please.

@azimgd azimgd marked this pull request as ready for review May 17, 2023 11:23
@cortinico cortinico requested a review from NickGerleman May 17, 2023 16:20
Copy link
Contributor

@NickGerleman NickGerleman left a comment

Choose a reason for hiding this comment

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

We shouldn't be adding a new API for marking spans of text. If we want to be able to do more with spans of text, we should built it into the existing facilities.

Note that we do also allow Views inline in text already.

@azimgd
Copy link
Author

azimgd commented May 17, 2023

This particular PR enables tracking layout metrics for a substring of text, by extending onTextLayout event callback we can get more granular metrics. Curious to know the downsides of this.

If we want to be able to do more with spans of text, we should built it into the existing facilities.

I have previously tried to implement that on a native layer here: Expensify#43. Main problem was that you can't go beyond Text component's line-height with styling. That said drawing a background layer with 30px is not possible on a line which has 20px.

I would appreciate any help and suggestions on what would be the best way to move forward. @NickGerleman @cortinico

@NickGerleman
Copy link
Contributor

To reiterate, the API in its current form is not something I think we are interested in.

I think it might be possible to reconcile this with the existing nested text APIs, to expand the APIs React Native already offer in a way that is cohesive. The path there I think would be to start with a proposal of the new API and get feedback on that first.

I think the faster way to achieve the goal you are looking for might be a custom native component.

@NickGerleman
Copy link
Contributor

To better explain what I mean by reconciling the APIs, we already have a way for users to segment texts, like a span. So adding a “regions” prop for this one particular separate use case is not cohesive with the current API.

As a start of an API to explore, what if “onTextLayout” was set on a specific nested text span to get its information? But these sorts of major APIs usually take some back and forth to refine.

If there gets to be agreement on an API (The Discussions and Proposals Repo would be a great place for this), the next steps would be a a series of smaller, reviewable PRs, building it out in increment. I.e. not all the platforms and logic at once, to help make the change reviewable.

@Szymon20000
Copy link

I think it's a great opportunity to add onLayout support for nested text. Expensify#56 (comment) I would like to help with implementing it. Pretty sure I know how to do it.

@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Jul 20, 2023
Copy link

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Jan 17, 2024
Copy link

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants