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

React Native occasionally truncates text #9343

Open
rozele opened this issue Jan 7, 2022 · 14 comments · Fixed by #10661
Open

React Native occasionally truncates text #9343

rozele opened this issue Jan 7, 2022 · 14 comments · Fixed by #10661
Labels
Area: Layout Area: Text bug Partner: Facebook Workstream: Component Parity Close the parity gap between RNW and RN for core RN components and their supporting APIs.
Milestone

Comments

@rozele
Copy link
Collaborator

rozele commented Jan 7, 2022

Problem Description

Filing this here, since I can only repro on react-native-windows right now, but I found a very peculiar case where text gets truncated when it should wrap. I believe this is a Yoga caching issue, but react-native-windows may be the best place to debug.

Steps To Reproduce

  1. Copy app from Gist here: https://gist.github.com/rozele/7a8704a69ff548613407c0f9fb72257b
  2. Resize window on the boundary of where the text would break from one line to two (very slowly and carefully, as shown in the video):
2022-01-07.17-14-24.mp4
  1. Notice that, if the stars align (and presumably the Yoga node measurement caches are in the right state), the text will be truncated.

Expected Results

Text should never be truncated.

CLI version

npx react-native --version

Environment

npx react-native info

Target Platform Version

No response

Target Device(s)

No response

Visual Studio Version

No response

Build Configuration

No response

Snack, code example, screenshot, or link to a repository

See video and Gist above for a minimal-ish repro.

@rozele rozele added the bug label Jan 7, 2022
@ghost ghost added Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) Partner: Facebook labels Jan 7, 2022
@rozele
Copy link
Collaborator Author

rozele commented Jan 7, 2022

Please note, this repros on DPI / scale settings of 150%, which I believe is important because it affects rounding errors when checking cached measurements.

@chrisglein chrisglein added Area: Layout Area: TextInput and removed Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) labels Jan 13, 2022
@chrisglein
Copy link
Member

Primary question is whether this some sort of DPI rounding issue in UWP, or whether it's the interaction between Yoga and the quirky UWP rounding that's causing the issue.

@rozele outside of the reduced repro (thanks for that), does this impact your product on a static layout, just during resize, or any other flavor you can offer?

@chrisglein chrisglein added this to the Backlog milestone Jan 13, 2022
@rozele
Copy link
Collaborator Author

rozele commented Jan 20, 2022

We've had bug reports of this occurring in static layout for message bubbles in Messenger (without resizing the window), which makes me think this could be a DPI rounding issue in UWP / XAML.

Here's what I know about Yoga - there definitely seems to be an occasional bug where a cached flex-measured ancestor of text pulls a height from cache that is smaller than the measured text height, and switches to exact measurements rather than undefined / flexible measurements for the text node. This is likely the issue that emerges when resizing the window.

However, it may be the case that there is non-determinism in XAML where it occasionally reports that it can squeeze the text into one less line and the cache is actually correct, so the root cause may be XAML in both the window resize and static layout case.

Here's what I know about layout for text on react-native-windows (today):

  1. We do not customize the scale factor in the YogaContext, so we use a point scale factor of 1 regardless of DPI settings. This is probably a good thing, because apps may have multiple windows residing on different screens with different DPI settings, and we'd need to refactor react-native-windows to create a per-root YogaContext that is applied to children and also add APIs to track/update for DPI changes for UWP / XAML Islands / WinAppSDK. Here's where we'd need to wire all this up:
  2. I have experimented with explicitly specifying the point scale factor on the YGContextRef to match the OS settings for DPI and still reproduced the bug (though only while resizing the window). I think the sequence might look like the following:

Text line height is 20, based on constraints, measures or pulls from cache 80w x 40h logical pixels
Ancestor pulls from cache from previous measurement where text fit in 100w x 20h
When reconciling the parent constraint height of 20p and available width of 100p, text gets constrained to 80x20.

Here's some things we should probably look into:

  1. Add logging so understand the circumstances where the "bad" height gets cached when resizing the window - can we confirm in this case that the text node measured to the same height, and if so was XAML actually able to render the text into one line at that point?
  2. Is XAML text measurement 100% deterministic? Given the same constraints, will it always return the same desired size?
  3. Perhaps this could occur due to XAMLs arrange pass. I haven't investigated this yet, but given we round to whole pixels, perhaps rounding to a whole pixel for layout position truncates the space available when XAML does it's own rounding when mapping logical to physical pixels. E.g., consider the following:

Imagine you have 150% scaling, and you have text that measures to 30 logical pixels. This maps to 45 physical pixels. If the left position of the view gets mapped to 1 logical pixel (which would sit physical pixel 1.5), where does XAML actually position the view, and is there any way that XAML ends up forcing the view to fit into less than 30 logical / 45 physical pixels depending on where the left position gets rounded?

@lyahdav
Copy link
Collaborator

lyahdav commented May 10, 2022

I'm investigating this since we've received a lot of reports of this issue. In order to debug this better, I created a branch where I can reproduce the issue like @rozele detailed above. I also put in a hack fix which I think will be the foundation for a PR soon.

Here's a video of me using the hack to fix the bug (after I resized the window to repro the bug):

Screen.Recording.2022-05-10.at.11.55.53.AM.mov

That menu item just marks all Text nodes as dirty (the menu item name is misleading, it doesn't dirty just the last one). Next my plan is to subscribe to the TextBlock.IsTextTrimmedChanged Event and dirtying the Text node there.

@lyahdav
Copy link
Collaborator

lyahdav commented May 10, 2022

Looks like the IsTextTrimmedChanged works. Here's a branch in my fork with the WIP fix: https://github.com/lyahdav/react-native-windows/tree/yoga-text-wrap-bug-with-fix. Here's a video showing how the bug momentarily appears and then fixes itself:

Screen.Recording.2022-05-10.at.1.25.43.PM.mov

Ideally the Text wouldn't look truncated momentarily, but I'm not sure how to do so and I think this is a good enough fix for now.

As a next step I'll work on a PR.

@lyahdav
Copy link
Collaborator

lyahdav commented Jun 2, 2022

I'm still trying to root cause this as the PR I created above may lead to other issues (e.g. a layout cycle). For posterity, this branch in my fork is in a good state for reproducing the issue now as it has a bunch of logging from Yoga: https://github.com/lyahdav/react-native-windows/tree/yoga-text-bug-root-causing

@lyahdav
Copy link
Collaborator

lyahdav commented Jun 2, 2022

Note also I can repro on display scale of 125% about as often as I could on 150%.

Edit: This may not be true, I mistakenly thought I was on 125% when I was on 150%. Still TBD.

@lyahdav
Copy link
Collaborator

lyahdav commented Jun 3, 2022

@acoates-ms and I are still debugging this issue. The latest theory is a bug in XAML since we see that when the text is truncated the XAML debugger shows that the ActualWidth property doesn't match the Width property. Also by inspecting the view's properties with the XAML debugger it fixes the bug, somehow triggering a layout and fixing the mismatch between the Width and ActualWidth properties.

One thing I might try next is seeing if I can repro this outside of RNW.

Also, we found that there were previous PRs and issues in this repo about related issues, though none of them gave a clue to how we should fix this issue:

Here's a branch in my fork where the bug happens on launch: https://github.com/lyahdav/react-native-windows/tree/yoga-text-bug-repros-on-launch-at-150-scale. We were also able to reduce the JS code in the test case while still reproducing the bug.

Other things we tried that don't fix this issue:

@acoates-ms
Copy link
Contributor

Ok, I think I understand what is happening.

In the specific example in the branch above, yoga is providing 60% of the width to the text. The window is 1252 wide, so 60% is 751.2px (Or 500.799988 dip).

When calling measure on the TextBlock with that available width.
DWrite determines the text metrics to be: {width=500.734863 height=19.9511719 }
Then XAML pixel aligns the value to be: {width=501.333344 height=20.0000000 } [Note this size is now larger than the original available]
Then XAML clips the desired size to the available size, since the desired shouldn't be larger than the available, which trims the width to 500.799988 again - but note this doesn't increase the height as it should if its wrapping text.
Then XAML pixel aligns the value again, giving us a desired size of: {width=500.666656 height=20.0000000 }, which is smaller than it should be.

@acoates-ms
Copy link
Contributor

I think adding this code before the call to element.Measure in DefaultYogaSelfMeasureFunc should fix the issue.

    if (auto text = view.try_as<xaml::Controls::TextBlock>()) {
      if (auto xamlRoot = text.XamlRoot()) {
        auto scale = static_cast<float>(xamlRoot.RasterizationScale());
        availableSpace.Width = floor(availableSpace.Width * scale) / scale;
        availableSpace.Height = floor(availableSpace.Height * scale) / scale;
      }
    }

And please remove the hack added previously in GetConstrainedResult while you are at it.

@lyahdav
Copy link
Collaborator

lyahdav commented Jun 9, 2022

@acoates-ms I can confirm that at least fixes the scenario we reproduced, thanks!

In this branch without the fix you can see the text is truncated:
image

In this branch with the fix the text is no longer truncated:
image

As a next step I'll try the fix in that scenario where the bug was only reproducing while resizing the window and report back.

@lyahdav
Copy link
Collaborator

lyahdav commented Jul 11, 2022

@acoates-ms Turns out your proposed fix doesn't resolve this bug in all cases. @rozele is going to take this over as he says he's close to having a fix.

@rozele
Copy link
Collaborator Author

rozele commented Sep 29, 2022

For a deterministic repro of this bug in Yoga: facebook/yoga#1161

rozele added a commit to rozele/react-native-windows that referenced this issue Sep 30, 2022
By default, Yoga will cache flex basis values to avoid expensive
recomputation. However, there are some scenarios, particularly when
resizing the window, where cached layout values from Yoga combined with
cached flex basis values can produce incorrect results.

This change adds a new QuirkSetting to opt-in to the
YGExperimentalFeatureWebFlexBasis option in Yoga for apps where re-use
of this cached flex basis value is problematic. We'll keep this opt-in
for now, as it is likely to impact performance of Yoga layout.

Resolves microsoft#9343
rozele added a commit to rozele/react-native-windows that referenced this issue Oct 4, 2022
In order to enable native layout updates, we need to expose an interface
to mark Yoga nodes as dirty, and also to trigger native layout.

While we could just blindly invoke `onBatchComplete` on the
NativeUIManager, it is safer to also expose an interface to test whether
the UIManager is currently processing a batch of UIManager operations
so we can piggy back on the current onBatchComplete call that will
occur at the end of a batch if one is in progress. This is also likely
to prevent UI tearing. Today, it is unlikely that an call from the UI
thread to invoke Yoga and XAML layout will interrupt a batch, if we
eventually implement something like eager view creation, this may no
longer be the case.

Also, rather than calling `onBatchComplete`, which may have the
side-effect of calling UpdateLayout on nodes that specify
`NeedsForceLayout`, native UI updates will not involve the creation of
new nodes, so we can skip this step, thus the addition of a new API to
call YGNodeCalculateLayout for a specific root (and skip layout for
other roots where it may not be needed). The LayoutService can also
conditionally apply layout to all roots, a specific root with unbound
constraints, or a specific root with known constraints.

We will be using this service for a number of features, including
re-entrant Yoga layout for views that are self-measuring with React
Native descendants dependent on Yoga layout, as well as to work around a
text truncation bug caused by Yoga caching to force the Text to
re-measure and re-layout when truncation is detected (is narrow use
cases).

Resolves microsoft#9988
Towards microsoft#7601
Workaround for microsoft#9343
rozele added a commit to rozele/react-native-windows that referenced this issue Oct 4, 2022
In order to enable native layout updates, we need to expose an interface
to mark Yoga nodes as dirty, and also to trigger native layout.

While we could just blindly invoke `onBatchComplete` on the
NativeUIManager, it is safer to also expose an interface to test whether
the UIManager is currently processing a batch of UIManager operations
so we can piggy back on the current onBatchComplete call that will
occur at the end of a batch if one is in progress. This is also likely
to prevent UI tearing. Today, it is unlikely that an call from the UI
thread to invoke Yoga and XAML layout will interrupt a batch, if we
eventually implement something like eager view creation, this may no
longer be the case.

Also, rather than calling `onBatchComplete`, which may have the
side-effect of calling UpdateLayout on nodes that specify
`NeedsForceLayout`, native UI updates will not involve the creation of
new nodes, so we can skip this step, thus the addition of a new API to
call YGNodeCalculateLayout for a specific root (and skip layout for
other roots where it may not be needed). The LayoutService can also
conditionally apply layout to all roots, a specific root with unbound
constraints, or a specific root with known constraints.

We will be using this service for a number of features, including
re-entrant Yoga layout for views that are self-measuring with React
Native descendants dependent on Yoga layout, as well as to work around a
text truncation bug caused by Yoga caching to force the Text to
re-measure and re-layout when truncation is detected (is narrow use
cases).

Resolves microsoft#9988
Towards microsoft#7601
Workaround for microsoft#9343
@ghost ghost closed this as completed in #10661 Oct 6, 2022
ghost pushed a commit that referenced this issue Oct 6, 2022
* Adds QuirkSetting to use YGExperimentalFeatureWebFlexBasis

By default, Yoga will cache flex basis values to avoid expensive
recomputation. However, there are some scenarios, particularly when
resizing the window, where cached layout values from Yoga combined with
cached flex basis values can produce incorrect results.

This change adds a new QuirkSetting to opt-in to the
YGExperimentalFeatureWebFlexBasis option in Yoga for apps where re-use
of this cached flex basis value is problematic. We'll keep this opt-in
for now, as it is likely to impact performance of Yoga layout.

Resolves #9343

* Change files

* Update vnext/Microsoft.ReactNative/QuirkSettings.cpp

Co-authored-by: Jon Thysell <thysell@gmail.com>
@rozele
Copy link
Collaborator Author

rozele commented Oct 7, 2022

Re-opening, as this is not a complete fix.

@rozele rozele reopened this Oct 7, 2022
@ghost ghost added the Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) label Oct 7, 2022
@jonthysell jonthysell removed the Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) label Oct 10, 2022
rozele added a commit to rozele/react-native-windows that referenced this issue Oct 22, 2022
In order to enable native layout updates, we need to expose an interface
to mark Yoga nodes as dirty, and also to trigger native layout.

While we could just blindly invoke `onBatchComplete` on the
NativeUIManager, it is safer to also expose an interface to test whether
the UIManager is currently processing a batch of UIManager operations
so we can piggy back on the current onBatchComplete call that will
occur at the end of a batch if one is in progress. This is also likely
to prevent UI tearing. Today, it is unlikely that an call from the UI
thread to invoke Yoga and XAML layout will interrupt a batch, if we
eventually implement something like eager view creation, this may no
longer be the case.

Also, rather than calling `onBatchComplete`, which may have the
side-effect of calling UpdateLayout on nodes that specify
`NeedsForceLayout`, native UI updates will not involve the creation of
new nodes, so we can skip this step, thus the addition of a new API to
call YGNodeCalculateLayout for a specific root (and skip layout for
other roots where it may not be needed). The LayoutService can also
conditionally apply layout to all roots, a specific root with unbound
constraints, or a specific root with known constraints.

We will be using this service for a number of features, including
re-entrant Yoga layout for views that are self-measuring with React
Native descendants dependent on Yoga layout, as well as to work around a
text truncation bug caused by Yoga caching to force the Text to
re-measure and re-layout when truncation is detected (is narrow use
cases).

Resolves microsoft#9988
Towards microsoft#7601
Workaround for microsoft#9343
rozele added a commit to rozele/react-native-windows that referenced this issue Oct 23, 2022
…rosoft#10671)

Summary:
In order to enable native layout updates, we need to expose an interface
to mark Yoga nodes as dirty, and also to trigger native layout.

While we could just blindly invoke `onBatchComplete` on the
NativeUIManager, it is safer to also expose an interface to test whether
the UIManager is currently processing a batch of UIManager operations
so we can piggy back on the current onBatchComplete call that will
occur at the end of a batch if one is in progress. This is also likely
to prevent UI tearing. Today, it is unlikely that an call from the UI
thread to invoke Yoga and XAML layout will interrupt a batch, if we
eventually implement something like eager view creation, this may no
longer be the case.

Also, rather than calling `onBatchComplete`, which may have the
side-effect of calling UpdateLayout on nodes that specify
`NeedsForceLayout`, native UI updates will not involve the creation of
new nodes, so we can skip this step, thus the addition of a new API to
call YGNodeCalculateLayout for a specific root (and skip layout for
other roots where it may not be needed). The LayoutService can also
conditionally apply layout to all roots, a specific root with unbound
constraints, or a specific root with known constraints.

We will be using this service for a number of features, including
re-entrant Yoga layout for views that are self-measuring with React
Native descendants dependent on Yoga layout, as well as to work around a
text truncation bug caused by Yoga caching to force the Text to
re-measure and re-layout when truncation is detected (is narrow use
cases).

Resolves microsoft#9988
Towards microsoft#7601
Workaround for microsoft#9343

Test Plan: See test plan in D40066331

Reviewers: lyahdav

Reviewed By: lyahdav

Differential Revision: https://phabricator.intern.facebook.com/D40064613

Tasks: T95569365
chiaramooney pushed a commit that referenced this issue Nov 3, 2022
* Adds LayoutService interface to allow native layout updates

In order to enable native layout updates, we need to expose an interface
to mark Yoga nodes as dirty, and also to trigger native layout.

While we could just blindly invoke `onBatchComplete` on the
NativeUIManager, it is safer to also expose an interface to test whether
the UIManager is currently processing a batch of UIManager operations
so we can piggy back on the current onBatchComplete call that will
occur at the end of a batch if one is in progress. This is also likely
to prevent UI tearing. Today, it is unlikely that an call from the UI
thread to invoke Yoga and XAML layout will interrupt a batch, if we
eventually implement something like eager view creation, this may no
longer be the case.

Also, rather than calling `onBatchComplete`, which may have the
side-effect of calling UpdateLayout on nodes that specify
`NeedsForceLayout`, native UI updates will not involve the creation of
new nodes, so we can skip this step, thus the addition of a new API to
call YGNodeCalculateLayout for a specific root (and skip layout for
other roots where it may not be needed). The LayoutService can also
conditionally apply layout to all roots, a specific root with unbound
constraints, or a specific root with known constraints.

We will be using this service for a number of features, including
re-entrant Yoga layout for views that are self-measuring with React
Native descendants dependent on Yoga layout, as well as to work around a
text truncation bug caused by Yoga caching to force the Text to
re-measure and re-layout when truncation is detected (is narrow use
cases).

Resolves #9988
Towards #7601
Workaround for #9343

* Change files

* Removes unnecessary ApplyLayout overload and makes semantics of methods more clear in the method name and IDL doc strings.

* Moves IsInBatch to IDL property getter
@chrisglein chrisglein modified the milestones: Backlog, 0.72 Feb 25, 2023
@TatianaKapos TatianaKapos added the Recommend: Backlog Recommend that issue should be given Backlog milestone. label Aug 31, 2023
@chiaramooney chiaramooney removed the Recommend: Backlog Recommend that issue should be given Backlog milestone. label Sep 25, 2023
@chrisglein chrisglein modified the milestones: 0.72, Backlog Apr 30, 2024
@chiaramooney chiaramooney added the Workstream: Component Parity Close the parity gap between RNW and RN for core RN components and their supporting APIs. label Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Layout Area: Text bug Partner: Facebook Workstream: Component Parity Close the parity gap between RNW and RN for core RN components and their supporting APIs.
Projects
None yet
7 participants