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

Support for inline code blocks #695

Open
cubuspl42 opened this issue Jul 31, 2023 · 11 comments
Open

Support for inline code blocks #695

cubuspl42 opened this issue Jul 31, 2023 · 11 comments

Comments

@cubuspl42
Copy link

cubuspl42 commented Jul 31, 2023

Introduction

Multiple existing apps (both native and web apps) have support for so-called inline code blocks. Just a few examples:

Slack

image

Discord

image

GitHub

Lorem ipsum dolor lorem ipsum

Inline code blocks can span across multiple lines if necessary.

image

Details

It's difficult or impossible to directly achieve the inline code block effect in React Native.

As I see it, there are three things in React Native that block us from achieving the discussed effect easily.

The line gap

Unlike CSS on the Web, if the inline text has a background color set, it fills the whole line. In CSS, it tightly wraps the inline text.

CSS:

image

React Native:

image

What's interesting, it seems there's no straightforward way to achieve the first effect on the second platform and vice-versa.

Such a feature exists in another W3 standard, TTML, and is named fillLineGap. It was added in 2017.

image

Figure 1 Illustrative rendition of the example immediately above with itts:fillLineGap="true" removed (left) or preserved (right). Blue lines have been added to show the before-edge and after-edge of each line area, which are coincident for successive line areas.

Also in 2017, there was some discussion about adding such a feature to CSS.

Doing just the inline-padding isn't the only place we want to do something with line areas.
... Another feature, fill-line-gap, says "draw background areas between lines".

It doesn't seem like much was done on this topic after that.

CSS behaves as if it had implicit fill-line-gap: false, and React Native behaves as if it had fill-line-gap: true.

Inline padding

In CSS, inline elements can have padding.

image

In React Native, the padding style property is ignored for inline text. As the current behavior is to fill the whole line height, it's not clear how padding should behave (or if it should be honored at all).

Borders

In CSS, inline elements can have borders, also with rounded corners.

image

The exact behavior is controlled by the box-decoration-break property.

In React Native, border styles for inline text are ignored.

Discussion points

I'd like to discuss what's necessary to support inline code blocks in React Native.

  • Is adding a boolean TTML-like fillLineGap style property to React Native reasonable?
    • The default value could be true (fill the whole gap), for backward compatibility
    • The false value would mean CSS-like behavior
  • If we supported fillLineGap, do we see any blockers from implementing these two inline text styles:
    • Padding
    • Borders

What do you think?

@cubuspl42
Copy link
Author

cubuspl42 commented Aug 7, 2023

@NickGerleman What do you think about this? I took much effort to sketch this idea in a clear way, also ensuring it fits well into the existing React Native ecosystem. I hope this proposal can be considered serious and achievable.

I'm tagging you because we already had an opportunity to work on the React Native Text subsystem and I really appreciate all the feedback you gave me then. If you don't have time right now to look at this, it would be awesome if you gave some hints on how can I get some eyes on this proposal.

@NickGerleman
Copy link

I think a prop like fillLineGap is reasonable, and extra CSS properties on nested text make sense. The most significant challenge may be that we are constrained by what is allowed by the platforms text drawing system.

Right now, these are flattened into a single stream of rich text characters drawn by the platform's underlying text system. On Android, you do get a Paint instance for text spans, where you can do some pretty custom things.

RN does allow views inline in text as well. This is akin to an element formatted with inline-flex, and any view styling can be added to it. But this will not line-break, and acts differently from the goal here.

@cubuspl42
Copy link
Author

I managed to prototype this feature on both iOS and Android. It's a proof-of-concept of respective platforms' possibilities, in tiny Swift/Kotlin native apps.

image

Both platforms resisted, but it was possible to achieve the desired effect on both of them in a surprisingly analogous manner.

To support left/right padding, it was necessary to inject artificial characters into the NSAttributedString / Spannable and later style at appropriate places to actually give them the desired bounding box (width). It's nearly for sure necessary on iOS and might be necessary on Android.

To support spanning across multiple lines, on both platforms, it was necessary to override some new (from React Native's perspective) APIs. On iOS it was necessary to subclass NSLayoutManager and NSLayoutManagerDelegate, on Android it was necessary to subclass TextView.onDraw.

@NickGerleman What's the next step here? Should I file a PR with a formal API proposal? Should I file a separate one for each new feature, or one joint one, or maybe only fillLineGap requires a proposal PR and the others can be considered "adding missing functionality"?

@NickGerleman
Copy link

Please feel free to make PRs for these, preferably as granular as possible. I have context to review the Android implementation, and can try to flag someone to look at the iOS one.

@cubuspl42
Copy link
Author

@NickGerleman I was asking about feature proposal PRs in this repo (React Native: Discussions and Proposals), did you also mean that? So far this is an issue.

@cubuspl42
Copy link
Author

@NickGerleman @mdvacca

The first PR is slowly getting closer to being merged (🎉), but it's not the end of our quest. It's just the beginning.

I want to start organizing the next steps.

First topic I would like to raise is modelling the new styles on the "core" (ReactCommon) level.

Currently, we have AttributedString, modeling a list of text fragments with denormalized style:

image

As I see it, what's necessary is to adjust it to a list of spans, which would hold styles like background color, border and padding. The rest of text styles could be kept in fragments. A span would contain a list of fragments.

image

This assumes we don't want to support span nesting, at least initially.

What do you think about this? Is it a reasonable model?

facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Jan 20, 2024
Summary:
A first step in my work on react-native-community/discussions-and-proposals#695

De-duplicate the code for creating `Spannable` on Android. I'm planning to add quite serious new features to this module. This would be really hard with the current level of code duplication.

## Changelog:

[INTERNAL] [CHANGED] - De-duplicate building `Spannable` on Android

Pull Request resolved: #39630

Test Plan: I tried to ensure that the refactored code is relatively easy to prove to be equivalent to the original duplicated one, but there's always a risk of a human mistake in this process. So far, I have been testing this by ensuring that nothing broke in the `Text` example section in RNTester.

Reviewed By: mdvacca

Differential Revision: D51016244

Pulled By: NickGerleman

fbshipit-source-id: e9f873c01b2af0685c7b0943aebea170c997d22e
@cubuspl42
Copy link
Author

@NickGerleman @cortinico I opened a few small PRs, trying (maybe naively) to ask Meta engineers who (somewhat) recently worked on the surrounding code. I hoped it could somehow parallelize the process.

This is mostly minor refactoring which makes the diff of the later changes more reviewer-friendly. I would hope to merge this in less than a month.

@cubuspl42
Copy link
Author

Also, could I get my own issue in the React Native repo for tracking this project?

@cortinico
Copy link
Member

could I get my own issue in the React Native repo for tracking this project

Sure feel free to open an issue an I can assign it to you

@cubuspl42

This comment was marked as outdated.

@cubuspl42
Copy link
Author

facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Jan 23, 2024
Summary:
Extract fragment conversions to separate functions to make refactoring easier and simplify reasoning about the code.

This code is being modified later.

This is a minor improvement in the context of my multi-PR work on react-native-community/discussions-and-proposals#695.

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[INTERNAL] [CHANGE] - Extract fragment conversions to separate functions

Pull Request resolved: #42597

Reviewed By: NickGerleman

Differential Revision: D52960655

Pulled By: robhogan

fbshipit-source-id: 0df62b9980c06a1c2fc113d645ba8b6b668fa394
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Jan 23, 2024
Summary:
Clean up the function naming in `TextMeasureCache.h`. One name was clearly a human mistake. Make the naming consistent.

This is a minor improvement in the context of my multi-PR work on react-native-community/discussions-and-proposals#695.

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[INTERNAL] [CHANGE] - Clean up the function naming in `TextMeasureCache.h`

Pull Request resolved: #42598

Reviewed By: NickGerleman

Differential Revision: D52960435

Pulled By: sammy-SC

fbshipit-source-id: 01327610446933972e8dc87e1b6e2950b7c706d2
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Jan 25, 2024
Summary:
Increase the readability of `CustomLineHeightSpan` by making the logic less stateful.

This is a minor improvement in the context of my multi-PR work on react-native-community/discussions-and-proposals#695.

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[INTERNAL] [CHANGED] - Increase the readability of `CustomLineHeightSpan`

Pull Request resolved: #42592

Test Plan:
- Prove the equivalence of the old and the new logic
- Test that the behavior of `lineHeight` doesn't change

Reviewed By: NickGerleman

Differential Revision: D53028467

Pulled By: mdvacca

fbshipit-source-id: d533bb77c8e10c29d8f2acc8cc39565d0013b03b
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Jan 25, 2024
)

Summary:
`RCTAttributedTextUtils.mm`: Split `NSAttributedString` creation to functions in preparation for adding new logic here.

This is a minor improvement in the context of my multi-PR work on react-native-community/discussions-and-proposals#695.

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[INTERNAL] [CHANGE] - Refactor `NSAttributedString` creation in `RCTAttributedTextUtils.mm`

Pull Request resolved: #42595

Reviewed By: cipolleschi

Differential Revision: D53001495

Pulled By: sammy-SC

fbshipit-source-id: 52d28e48f0a9d88d44325a73c64737fc7ac97781
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Jan 26, 2024
Summary:
Move all `ReactSpan` subclasses to a separate folder.

This is a minor improvement in the context of my multi-PR work on react-native-community/discussions-and-proposals#695.

I'm adding a new span class later, which was the direct motivation for this change.

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[INTERNAL] [CHANGE] - Move all `ReactSpan` subclasses to a separate folder

Pull Request resolved: #42594

Reviewed By: mdvacca

Differential Revision: D53123733

Pulled By: cortinico

fbshipit-source-id: 10db214a520d157c231e6f3b97948b4209a7ad4b
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Feb 20, 2024
Summary:
De-duplicate the logic for counting attachments.

This is a minor improvement in the context of my multi-PR work on react-native-community/discussions-and-proposals#695.

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[INTERNAL] [CHANGE] - De-duplicate the logic for counting attachments

Pull Request resolved: #42596

Reviewed By: rshest

Differential Revision: D53917281

Pulled By: cipolleschi

fbshipit-source-id: cdb9bc834bddd7deffc60f33578464733982fedf
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

No branches or pull requests

3 participants