TextLayout: take full width if text wrapped #47435

s77rt · 2024-11-05T20:06:18Z

Summary:

Problem

The calculated width for a multiline text is based on the longest line. However it does not account for text that wraps.

Example if numberOfLines=1 and the text wraps

+---------------------------+
This is a long text that
will wrap
+---------------------------+

The TextView will render

+---------------------------+
This is a long text t...
+---------------------------+

because the calculatedWidth took the width of the first line.

Also see #41770 (comment) for additional context.

Solution

If the text wraps, take the whole width.

+---------------------------+
This is a long text that w...
+---------------------------+

Fixes #39722
Fixes facebook/yoga#1730

Changelog:

[GENERAL] [FIXED] - Fix text not taking full width

Test Plan:

        <Text
          numberOfLines={1}
          style={{
            backgroundColor: 'red',
            alignSelf: 'flex-start',
            color: 'white',
            fontSize: 34,
          }}>
          {'This is a long text that will wrap.'}
        </Text>
        <Text
          numberOfLines={3}
          style={{
            backgroundColor: 'red',
            alignSelf: 'flex-start',
            color: 'white',
            fontSize: 34,
          }}>
          {
            '1\n\ntest\nThis is a long text that will wrap.This is a long text that will wrap.This is a long text that will wrap.\n'
          }
        </Text>
        <Text
          numberOfLines={3}
          style={{
            backgroundColor: 'red',
            alignSelf: 'flex-start',
            color: 'white',
            fontSize: 34,
          }}>
          {
            '1\n\nThis is a long text that will wrap.This is a long text that will wrap.This is a long text that will wrap.\n'
          }
        </Text>

Verify that the first and third text take full width
Verify that the second text does not take full width

Before	After

s77rt · 2024-11-05T20:14:00Z

cc @cipolleschi or @realsoelynn You may want to review this since you have the context from #41770

facebook-github-bot · 2024-11-06T05:41:43Z

@realsoelynn has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

NickGerleman · 2024-11-06T20:27:31Z

That also likely fixes issue reported here. facebook/yoga#1730

I think we may want this behavior regardless of max number of lines. Wrapping when we have AtMost constraint should be changed on iOS/Android to take full available width.

facebook-github-bot · 2024-11-06T20:48:19Z

@realsoelynn has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

s77rt · 2024-11-07T13:45:05Z

@NickGerleman It won't fix the other issue. That one is in yoga. I will look into it as well

s77rt · 2024-11-07T13:45:43Z

@realsoelynn I see a failing internal test. Does this require any action from my side?

NickGerleman · 2024-11-08T00:33:11Z

@s77rt @realsoelynn the other issue was reported to Yoga, but I highly suspect it is RN measure function issue related to what this change is addressing.

s77rt · 2024-11-08T08:54:19Z

Making this a draft, will work on what @NickGerleman suggested. It should fix both this issue and the other one

s77rt · 2024-11-08T13:42:32Z

iOS	Android

s77rt · 2024-11-08T13:42:50Z

Ready for review. cc @realsoelynn @NickGerleman

cipolleschi · 2024-11-11T16:07:53Z

FYI: @NickGerleman is on PTO this week. He should be back next week

facebook-github-bot · 2024-11-11T21:30:14Z

@realsoelynn has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

NickGerleman

Back from PTO! Took a look, and I think we might need to handle max-content differently.

NickGerleman · 2024-11-20T01:08:55Z

...react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java

+        if (!endsWithNewLine && lineIndex + 1 < layout.getLineCount()) {
+          calculatedWidth = width;
+          break;
+        }


I think we may also reach this path when widthYogaMeasureMode is YogaMeasureMode.UNDEFINED, in which case we are asking for max-content size, and the width is NaN.

I think on L604, we could replace:

if (widthYogaMeasureMode == YogaMeasureMode.AT_MOST && calculatedWidth > width)

with

if (widthYogaMeasureMode == YogaMeasureMode.AT_MOST && (calculatedWidth > width || layout.getLineCount() > 1))

I think we may also reach this path when widthYogaMeasureMode is YogaMeasureMode.UNDEFINED

In this case, should we keep the calculated width as the max width of the lines?

I think on L604, we could replace

We set calculatedWidth = width only if the text wraps due to overflow. If the user write 2 short lines we still use the width of the maximum line

NickGerleman · 2024-11-20T01:21:10Z

...erer/textlayoutmanager/platform/ios/react/renderer/textlayoutmanager/RCTTextLayoutManager.mm

  CGSize size = [layoutManager usedRectForTextContainer:textContainer].size;

+  if (textDidWrap) {
+    size.width = textContainer.size.width;


I think we may have similar case to the above, for measuring max-content, where we don't want to set to size. I think in this case, it looks like that might be represented as Infinity layout constraint which makes its way to the NSTextContainer.

NickGerleman · 2024-11-20T01:31:11Z

...erer/textlayoutmanager/platform/ios/react/renderer/textlayoutmanager/RCTTextLayoutManager.mm

+                                 NSUInteger lastCharacterIndex = range.location + range.length - 1;
+                                 BOOL endsWithNewLine =
+                                     [textStorage.string characterAtIndex:lastCharacterIndex] == '\n';
+                                 if (!endsWithNewLine && textStorage.string.length > lastCharacterIndex + 1) {


Would this correctly detect wrapping for something like the below?

Hello\n World\n

I think we may be able to just count lines. https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/TextLayout/Tasks/CountLines.html#//apple_ref/doc/uid/20001810-CJBGBIBB

No, that's not considered text wrap. In this case we want the current behaviour (text width) otherwise we'd break case 2 from #47435 (comment)

NickGerleman

Ah, you are correct about only wanting to do this when we need to break due to wrapping.

That also solves the max-content case, since we will never wrap in this case since we have unconstrained width.

So I think this is good!

facebook-github-bot · 2024-11-20T20:11:07Z

@realsoelynn has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

react-native-bot · 2024-11-21T11:25:49Z

This pull request was successfully merged by @s77rt in 550b0c0

^{When will my fix make it into a release? | How to file a pick request?}

facebook-github-bot · 2024-11-21T11:25:51Z

@realsoelynn merged this pull request in 550b0c0.

Summary: When we create a layout from measure constraints, we do some processing of the width, to return a different one, potentially smaller than the layout width, sometimes using line width, and sometimes using the container width. This logic has gotten spooooky over time, and after a series of changes, and bugfixes, now effectively does nothing! 1. Way back in 2020, yungsters made D21056031 introducing this logic to "shrink wrap" text which is wrapped. 2. "Shrink wrapping" is not how web works when text is wrapped, (though it is how it works when there is explicit newline), and facebook#47435 later undid this change 3. facebook#37790 made changes specific to the case of trailing newline, because the logic to "shrink wrap" did not handle correctly. After D74366936, which changes width used for layout creation to correctly respect `Layout.desiredWidth`, we should be back to multiline layouts, with no paragraph whose lines take up more than container width, being "shrink wrapped", while not doing so when there is wrapping or ellipsization, like current behavior. The desired width also excludes the non-visible trailing whitespace. It means we can remove all of this logic, while preserving the same behavior. Mismatched measure widths from those used in the intermediate layout may also result in issues for Facsimile (see example in last diff of stack). Changelog: [Internal] Differential Revision: D74368513

Summary: Pull Request resolved: facebook#51206 When we create a layout from measure constraints, we do some processing of the width, to return a different one, potentially smaller than the layout width, sometimes using line width, and sometimes using the container width. This logic has gotten spooooky over time, and after a series of changes, and bugfixes, now effectively does nothing! 1. Way back in 2020, yungsters made D21056031 introducing this logic to "shrink wrap" text which is wrapped. 2. "Shrink wrapping" is not how web works when text is wrapped, (though it is how it works when there is explicit newline), and facebook#47435 later undid this change 3. facebook#37790 made changes specific to the case of trailing newline, because the logic to "shrink wrap" did not handle correctly. After D74366936, which changes width used for layout creation to correctly respect `Layout.desiredWidth`, we should be back to multiline layouts, with no paragraph whose lines take up more than container width, being "shrink wrapped", while not doing so when there is wrapping or ellipsization, like current behavior. The desired width also excludes the non-visible trailing whitespace. It means we can remove all of this logic, while preserving the same behavior. Mismatched measure widths from those used in the intermediate layout may also result in issues for Facsimile (see example in last diff of stack). Changelog: [Internal] Differential Revision: D74368513

Summary: Pull Request resolved: #51206 When we create a layout from measure constraints, we do some processing of the width, to return a different one, potentially smaller than the layout width, sometimes using line width, and sometimes using the container width. This logic has gotten spooooky over time, and after a series of changes, and bugfixes, now effectively does nothing! 1. Way back in 2020, yungsters made D21056031 introducing this logic to "shrink wrap" text which is wrapped. 2. "Shrink wrapping" is not how web works when text is wrapped, (though it is how it works when there is explicit newline), and #47435 later undid this change 3. #37790 made changes specific to the case of trailing newline, because the logic to "shrink wrap" did not handle correctly. After D74366936, which changes width used for layout creation to correctly respect `Layout.desiredWidth`, we should be back to multiline layouts, with no paragraph whose lines take up more than container width, being "shrink wrapped", while not doing so when there is wrapping or ellipsization, like current behavior. The desired width also excludes the non-visible trailing whitespace. It means we can remove all of this logic, while preserving the same behavior. Mismatched measure widths from those used in the intermediate layout may also result in issues for Facsimile (see example in last diff of stack). Changelog: [Internal] Reviewed By: javache Differential Revision: D74368513 fbshipit-source-id: df5d7b773ad1888ebca1966ee4020a5c2ce7fd64

TextLayout: take full width if text wrapped beyond numberOfLines

6f9d2c8

facebook-github-bot added 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. labels Nov 5, 2024

linter

7fbdf03

s77rt marked this pull request as draft November 8, 2024 08:53

use full width if text wraps

4e30c8b

s77rt marked this pull request as ready for review November 8, 2024 13:39

s77rt changed the title ~~TextLayout: take full width if text wrapped beyond numberOfLines~~ TextLayout: take full width if text wrapped Nov 8, 2024

NickGerleman requested changes Nov 20, 2024

View reviewed changes

NickGerleman approved these changes Nov 20, 2024

View reviewed changes

facebook-github-bot closed this in 550b0c0 Nov 21, 2024

react-native-bot added the Merged This PR has been merged. label Nov 21, 2024

s77rt mentioned this pull request Nov 21, 2024

[HOLD for payment 2024-12-12] [$2000] Android - Title is not rendered fully in the available space. Expensify/App#21219

Closed

6 tasks

s77rt mentioned this pull request Dec 2, 2024

(RN Patch) TextLayout: take full width if text wrapped Expensify/App#53388

Merged

50 tasks

fabOnReact mentioned this pull request Mar 5, 2025

Width measures of Text components using a custom font are wrong #25481

Closed

s77rt mentioned this pull request Mar 26, 2025

Inconsistent text wrap behavior with flexbox facebook/yoga#1730

Closed

1 task

NickGerleman mentioned this pull request May 8, 2025

Directly Trust Text Layout Width #51206

Closed

TextLayout: take full width if text wrapped #47435

TextLayout: take full width if text wrapped #47435

Uh oh!

Conversation

s77rt commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary:

Problem

Solution

Changelog:

Test Plan:

Uh oh!

s77rt commented Nov 5, 2024

Uh oh!

facebook-github-bot commented Nov 6, 2024

Uh oh!

NickGerleman commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Nov 6, 2024

Uh oh!

s77rt commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s77rt commented Nov 7, 2024

Uh oh!

NickGerleman commented Nov 8, 2024

Uh oh!

s77rt commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s77rt commented Nov 8, 2024

Uh oh!

s77rt commented Nov 8, 2024

Uh oh!

cipolleschi commented Nov 11, 2024

Uh oh!

facebook-github-bot commented Nov 11, 2024

Uh oh!

NickGerleman left a comment

Choose a reason for hiding this comment

Uh oh!

NickGerleman Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

NickGerleman Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

s77rt Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

NickGerleman Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

NickGerleman Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

s77rt Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

NickGerleman left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Nov 20, 2024

Uh oh!

react-native-bot commented Nov 21, 2024

Uh oh!

facebook-github-bot commented Nov 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

s77rt commented Nov 5, 2024 •

edited

Loading

NickGerleman commented Nov 6, 2024 •

edited

Loading

s77rt commented Nov 7, 2024 •

edited

Loading

s77rt commented Nov 8, 2024 •

edited

Loading