prototype(FastText): Add option to use TextBlock for simple text #1256

rozele · 2017-08-01T18:51:02Z

Simple text, i.e., text with only a single raw text node as a child, can be transformed into a TextBlock with a Text property, instead of a RichTextBlock with a Paragraph of Inlines. This change detects in the React Native Text node has a single raw text child, and if so, uses the FastText native component. To test if your text nodes are "fast", set DebugSettings.IsTextPerformanceVisualizationEnabled to true while debugging your app; it changes optimized text color to green.

Fixes #1252

codecov-io · 2017-08-02T15:56:00Z

Codecov Report

Merging #1256 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1256      +/-   ##
==========================================
- Coverage   31.84%   31.79%   -0.05%     
==========================================
  Files         258      259       +1     
  Lines       18674    18703      +29     
  Branches     1580     1584       +4     
==========================================
  Hits         5947     5947              
- Misses      12574    12603      +29     
  Partials      153      153

Impacted Files	Coverage Δ
...ctNative.Net46/Views/Text/ReactTextCompoundView.cs	`0% <0%> (ø)`	⬆️
...ative.Net46/Views/Text/ReactFastTextViewManager.cs	`0% <0%> (ø)`
...eactNative.Net46/Views/Text/ReactTextShadowNode.cs	`0% <0%> (ø)`	⬆️
...ows/ReactNative.Shared/UIManager/RootViewHelper.cs	`15.55% <0%> (-2.87%)`	⬇️
...indows/ReactNative.Net46/Shell/MainReactPackage.cs	`0% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ededb4d...7c30286. Read the comment docs.

matthargett · 2017-08-10T20:19:20Z

ReactWindows/ReactNative.Shared/UIManager/RootViewHelper.cs

+                    return frameworkElement.Parent;
+                }
+#if !WINDOWS_UWP
+


maybe rename these two variables to just "element" with #if UWP for the different casts, and then deduplicate the if+return?

I think you're reading it wrong, we had to add the extra check for WPF because there is a difference between RichTextBlock and TextBlock in WPF.

In reply to: 132557372 [](ancestors = 132557372)

This is just a prototype.

antonkh · 2017-08-22T23:34:36Z

Libraries/Text/Text.windows.js

@@ -263,6 +275,14 @@ const Text = React.createClass({
    if (this.context.isInAParentText) {
      return <RCTVirtualText {...newProps} />;
    } else {
+      if (Platform.OS === 'windows' && typeof newProps.children === 'string') {


this.props.children can be an array of strings and in many cases this array can contain just one string. If such arrays are concatenated with .join("") this fix covers 99% of text in S4L.

This was fixed in S4L.

reseul · 2018-05-05T00:22:05Z

ReactWindows/ReactNative.Shared/UIManager/RootViewHelper.cs

+                {
+                    return frameworkContentElement.Parent;
+                }
+#endif
            }


Deferring non FrameworkElement/FrameworkContentElement to VisualTreeHelper.GetParent seems to make total sense, the helper being smart enough to understand the intricacies of various elements, etc.
Well, nope. The call throws System.Exception(E_UNEXPECTED) if passed a correctly parented/in visual tree InlineUIContainer, for example.

This is Eric Rozell's [PR](microsoft#1256) with a couple fixes to make it work with S4L. This optimization [gives](aka.ms/V3mz2) a 10% speedup and reduces memory usage by 10%. Simple text, i.e., text with only a single raw text node as a child, can be transformed into a TextBlock with a Text property, instead of a RichTextBlock with a Paragraph of Inlines. This change detects in the React Native Text node has a single raw text child, and if so, uses the FastText native component. To test if your text nodes are "fast", set DebugSettings.IsTextPerformanceVisualizationEnabled to true while debugging your app; it changes optimized text color to green. Related work items: #1164947

rozele · 2018-05-18T22:20:11Z

Libraries/Text/Text.windows.js

+    if (this.context.isInAParentText) {
+      return <RCTVirtualText {...newProps} />;
+    } else {
+      if (Platform.OS === 'windows' && typeof newProps.children === 'string') {


Platform.OS === 'windows' & [](start = 10, length = 27)

Instead of platform check, use RCTSimpleText or something else.

rozele · 2018-05-18T22:21:04Z

ReactWindows/ReactNative/Views/Text/ReactSimpleTextShadowNode.cs

+    /// <summary>
+    /// The shadow node implementation for text views.
+    /// </summary>
+    public class ReactSimpleTextShadowNode : LayoutShadowNode


ReactSimpleTextShadowNode [](start = 17, length = 25)

Revert file name to maintain diff.

reseul · 2018-05-18T22:41:42Z

ReactWindows/ReactNative.Net46/ReactNative.Net46.csproj

@@ -172,6 +172,7 @@
    <Compile Include="Views\Scroll\ScrollView.cs" />
    <Compile Include="Views\Slider\ReactSliderManager.cs" />
    <Compile Include="Views\TextInput\PlaceholderAdorner.cs" />
+    <Compile Include="Views\Text\ReactSimpleTextViewManager.cs" />
    <Compile Include="Views\Text\ReactTextCompoundView.cs" />


@rozele Where's the shadow node?

[edit]: Oh, it's a dummy implementation for WPF?

reseul

antonkh · 2018-05-18T23:31:21Z

Libraries/Text/Text.windows.js

+    if (this.context.isInAParentText) {
+      return <RCTVirtualText {...newProps} />;
+    } else {
+      if (RCTSimpleText && typeof newProps.children === 'string') {


This can be an array of strings.

@reseul - not sure if we are supposed to handle the array of strings. I don't know what the concatenation handling should be, whether we just inject a space, newline, etc. Also, I imagine this can vary based on culture/i18n. I'd like to keep this as a single space for now and force the client to specifically concatenate string arrays if they want text perf optimization.

@rozele it's ok. We've been using this fast text for some months, but I'm not sure whether we have that case. If we do. we're going to reconcile on our end.

antonkh · 2018-05-18T23:35:23Z

ReactWindows/ReactNative/Views/Text/ReactSimpleTextViewManager.cs

+        /// <returns>The view instance.</returns>
+        protected override TextBlock CreateViewInstance(ThemedReactContext reactContext)
+        {
+            var richTextBlock = new TextBlock


This text block is not exactly rich.

I'll change to var poorTextBlock :)

Simple text, i.e., text with only a single raw text node as a child, can be transformed into a `TextBlock` with a `Text` property, instead of a `RichTextBlock` with a `Paragraph` of `Inlines`. This change detects in the React Native Text node has a single raw text child, and if so, uses the FastText native component. To test if your text nodes are "fast", set `DebugSettings.IsTextPerformanceVisualizationEnabled` to true while debugging your app; it changes optimized text color to green. Fixes microsoft#1252

Also taking the latest from Text.js in [email protected].*

…ext (#1256)" This reverts commit 7fa57fc.

…ext (#1256)" (#1939) Reverts #1256. We need to update Text.windows.js to be in sync with the latest from react-native first.

…simple text (microsoft#1256)" (microsoft#1939)" This reverts commit adb4539. Updated for the latest Text.js implementation

msftclas added the cla-not-required label Aug 1, 2017

sayar previously approved these changes Aug 1, 2017

View reviewed changes

rozele dismissed sayar’s stale review via 7c30286 August 2, 2017 15:40

sayar previously approved these changes Aug 4, 2017

View reviewed changes

matthargett reviewed Aug 10, 2017

View reviewed changes

antonkh reviewed Aug 22, 2017

View reviewed changes

msftgits removed the cla-not-required label Sep 26, 2017

microsoft deleted a comment from msftclas Sep 27, 2017

rozele mentioned this pull request Oct 4, 2017

Add remaining and UWP-specific accessibility features #918

Closed

antonkh mentioned this pull request Nov 7, 2017

RX.FastText for plain text? microsoft/reactxp#347

Closed

reseul reviewed May 5, 2018

View reviewed changes

rozele force-pushed the issue1252 branch 2 times, most recently from 9566a3b to 96a49fa Compare May 18, 2018 22:18

rozele commented May 18, 2018

View reviewed changes

rozele force-pushed the issue1252 branch from 96a49fa to c2318bf Compare May 18, 2018 22:24

reseul reviewed May 18, 2018

View reviewed changes

reseul previously approved these changes May 18, 2018

View reviewed changes

antonkh previously approved these changes May 18, 2018

View reviewed changes

rozele added 6 commits August 13, 2018 19:18

fix(FastText): adds WPF support for RCTFastText

4f6d246

refactor(FastText): Renaming to RCTSimpleText

6d448f1

Also taking the latest from Text.js in [email protected].*

Change file names to SimpleText

19c2f53

Remove dependency on Platform value

84c3423

Minor fix for variable name.

51c6bdb

rozele dismissed stale reviews from antonkh and reseul via 51c6bdb August 13, 2018 23:19

rozele force-pushed the issue1252 branch from 6e6b806 to 51c6bdb Compare August 13, 2018 23:19

rozele merged commit 7fa57fc into microsoft:master Aug 14, 2018

rozele added a commit that referenced this pull request Aug 14, 2018

Revert "prototype(FastText): Add option to use TextBlock for simple t…

b282f01

…ext (#1256)" This reverts commit 7fa57fc.

rozele mentioned this pull request Aug 14, 2018

Revert "prototype(FastText): Add option to use TextBlock for simple text" #1939

Merged

rozele added a commit that referenced this pull request Aug 14, 2018

Revert "prototype(FastText): Add option to use TextBlock for simple t…

adb4539

…ext (#1256)" (#1939) Reverts #1256. We need to update Text.windows.js to be in sync with the latest from react-native first.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prototype(FastText): Add option to use TextBlock for simple text #1256

prototype(FastText): Add option to use TextBlock for simple text #1256

rozele commented Aug 1, 2017

codecov-io commented Aug 2, 2017

matthargett Aug 10, 2017

rozele Oct 19, 2017

antonkh Aug 22, 2017

antonkh Oct 13, 2017

reseul May 5, 2018

rozele May 18, 2018

rozele May 18, 2018

reseul May 18, 2018 •

edited

Loading

reseul left a comment

antonkh May 18, 2018

rozele Aug 13, 2018

reseul Aug 13, 2018

antonkh May 18, 2018

rozele May 19, 2018

prototype(FastText): Add option to use TextBlock for simple text #1256

prototype(FastText): Add option to use TextBlock for simple text #1256

Conversation

rozele commented Aug 1, 2017

codecov-io commented Aug 2, 2017

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reseul May 18, 2018 • edited Loading

Choose a reason for hiding this comment

reseul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reseul May 18, 2018 •

edited

Loading