-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Comments
Please note, this repros on DPI / scale settings of 150%, which I believe is important because it affects rounding errors when checking cached measurements. |
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? |
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):
Here's some things we should probably look into:
|
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.movThat 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. |
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.movIdeally 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. |
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 |
Edit: This may not be true, I mistakenly thought I was on 125% when I was on 150%. Still TBD. |
@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:
|
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. |
I think adding this code before the call to element.Measure in 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 |
@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: In this branch with the fix the text is no longer truncated: 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. |
@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. |
For a deterministic repro of this bug in Yoga: facebook/yoga#1161 |
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
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
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
* 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 <[email protected]>
Re-opening, as this is not a complete fix. |
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
…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
* 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
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
2022-01-07.17-14-24.mp4
Expected Results
Text should never be truncated.
CLI version
npx react-native --version
Environment
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.
The text was updated successfully, but these errors were encountered: