Introduces TextVisitor to consolidate text traversal algorithms #8454
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We have 3 different features that require a DFS traversal of the text tree: 1. The textTransform algorithm needs to recurse the undefined paths of the Text sub-tree where the prop value changes to update. 2. The backgroundColor algorithm needs to recurse the entire text tree to re-calculate highlighters any time the text changes or backgroundColor / foregroundColor props change within the tree. 3. The WIP hit test algorithm needs to recurse the text tree to identify which span is pressed. Additionally, we have 4 "upward" traversal algorithms: 1. When a RCTRawText changes it's text prop, we need to send an accessibility notification 2. When a RCTRawText changes, we need to re-apply text transforms. 3. When a background or foregroundColor changes, we need to notify each parent that it may require a highlighter for an optimization that skips calculating highlighters for text trees without foreground or background colors. 4. In the WIP hit test algorithm, we need to notify parent nodes that a nested Text component is pressable for the optimization that selectively skips traversal of non-pressable sub-trees Each of these algorithms follow the same pattern, so reduce the potential for bugs, I'd like to consolidate these algorithms to use a visitor pattern.
7 tasks
rozele
commented
Aug 23, 2021
vnext/Microsoft.ReactNative/Microsoft.ReactNative.vcxproj.filters
Outdated
Show resolved
Hide resolved
Previously, if the child of a virtual text node with undefined textTransform was appended, and that virtual text node had a parent with a defined textTransform, the textTransform value would not inherit properly. Similarly, if a virtual text node switched from defined to undefined for textTransform prop, it would not inherit the correct value and update children accordingly either. Fixes microsoft#8460
rozele
changed the title
…ext or highlights are updated
ghost
added
the
rectified95
reviewed
Aug 24, 2021
rectified95
reviewed
Aug 24, 2021
vnext/Microsoft.ReactNative/Views/Text/TextPropertyChangedParentVisitor.h
Show resolved
Hide resolved
rectified95
reviewed
Aug 24, 2021
rozele
force-pushed
the
textVisitor
branch
3 times, most recently
from
a0fab31 to
2e4403c
Compare
rozele
commented
Aug 24, 2021
vnext/Microsoft.ReactNative/Views/Text/TextPropertyChangedParentVisitor.cpp
Outdated
Show resolved
Hide resolved
5 tasks
There was a problem hiding this comment.
Looks good to me, but will have to wait on another dev to look over the architecture decisions overall. One q, we filed an issue recently because textTransform style prop changes haven't been updating during Fast Refresh. With this overhaul change to Text, does that behavior now work?
acoates-ms
approved these changes
Sep 17, 2021
This was referenced Sep 17, 2021
chrisglein
pushed a commit
to chrisglein/react-native-windows
that referenced
this pull request
Sep 22, 2021
…osoft#8454) * Initial implementation of TextVisitor We have 3 different features that require a DFS traversal of the text tree: 1. The textTransform algorithm needs to recurse the undefined paths of the Text sub-tree where the prop value changes to update. 2. The backgroundColor algorithm needs to recurse the entire text tree to re-calculate highlighters any time the text changes or backgroundColor / foregroundColor props change within the tree. 3. The WIP hit test algorithm needs to recurse the text tree to identify which span is pressed. Additionally, we have 4 "upward" traversal algorithms: 1. When a RCTRawText changes it's text prop, we need to send an accessibility notification 2. When a RCTRawText changes, we need to re-apply text transforms. 3. When a background or foregroundColor changes, we need to notify each parent that it may require a highlighter for an optimization that skips calculating highlighters for text trees without foreground or background colors. 4. In the WIP hit test algorithm, we need to notify parent nodes that a nested Text component is pressable for the optimization that selectively skips traversal of non-pressable sub-trees Each of these algorithms follow the same pattern, so reduce the potential for bugs, I'd like to consolidate these algorithms to use a visitor pattern. * Change files * Move highlighter logic to visitor pattern * Fixes a couple bugs and adds VisitChildren method to share logic * Split text visitors into individual files * Fixes issue with inherited text transforms Previously, if the child of a virtual text node with undefined textTransform was appended, and that virtual text node had a parent with a defined textTransform, the textTransform value would not inherit properly. Similarly, if a virtual text node switched from defined to undefined for textTransform prop, it would not inherit the correct value and update children accordingly either. Fixes microsoft#8460 * Adds TextPropertyChangedParentVisitor for Text tree traversals when text or highlights are updated * Removes irrelevant headers and fixes namespaces * Refactor TextVisitor to eliminate need for `VisitExtensionText'
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The main motivation for this PR is to ensure each of the tree traversal algorithms we use inside RN Core Text supports 3rd party TextElements (e.g.,
Hyperlink). Right now, the logic is spread across a number of functions and each function would need to be checked individually to ensure it supports recursion through non-core TextElement view managers. Using visitor patterns, we can ensure that the base class appropriately handles recursion.We have 3 different features that require a DFS traversal of the text tree:
Additionally, we have 4 "upward" traversal algorithms:
Each of these algorithms follow the same pattern, so to reduce the potential for bugs, I'd like to consolidate these algorithms to use a visitor pattern.
While working on this diff, I discovered a bug in the
textTransformlogic where when we update the prop tonull/undefinedor add a child to a RCTVirtualText node withnull/undefined, it should still attempt to inherit from ancestors.Fixes #8460
Microsoft Reviewers: Open in CodeFlow