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

Fixes issues with Text backgroundColor #8408

Merged
merged 11 commits into from
Aug 19, 2021
Merged

Conversation

rozele
Copy link
Collaborator

@rozele rozele commented Aug 11, 2021

There were two issues with the Text backgroundColor prop:

  1. Changes to the backgroundColor prop on virtual text nodes did not update their corresponding TextHighlighters
  2. Changes to the underlying text did not update the TextHighlighter ranges

This change forces a refresh of all TextHighlighters any time:

  1. A raw text node changes text value
  2. A virtual text node is added or removed from the text tree
  3. A virtual text or root text node updates its backgroundColor prop

TODOs:

  • Pass inherited foreground color down so virtual text highlighters apply the correct foreground color
  • Update snapshots since highlighter approach has changed
  • Produce a before and after video to ensure RNTester TextExample still works as expected
  • Add examples to RNTester to toggle background colors on RCTText and RCTVirtualText to see highlighter updates working
  • Add examples to RNTester to remove RCTRawText and RCTVirtualText nodes to see highlighter updates working

Fixes #7568
Fixes #7569
Fixes #8407

Microsoft Reviewers: Open in CodeFlow

There were two issues with the Text backgroundColor prop:
1. Changes to the backgroundColor prop on virtual text nodes did not
update their corresponding TextHighlighters
2. Changes to the underlying text did not update the TextHighlighter
ranges

This change forces a refresh of all TextHighlighters any time:
1. A raw text node changes text value
2. A virtual text node is added or removed from the text tree
3. A virtual text or root text node updates its backgroundColor prop

Fixes microsoft#7569
Fixes microsoft#8407
@rozele rozele requested a review from a team as a code owner August 11, 2021 15:52
@ghost ghost added the Area: Text label Aug 11, 2021
@rozele
Copy link
Collaborator Author

rozele commented Aug 11, 2021

This is not very efficient. It means we do a DFS on the text tree every time the text changes to recalculate the TextHightlighters. One optimization we could consider is to maintain state in the text shadow nodes to signal whether the DFS is required. We could do this in one of two ways:

  1. Maintain a per node count of children with backgroundColor set
  2. Maintain a boolean flag at the root text node that signals whether any children have backgroundColor.

My preference would be the latter. I initially thought that having counts at each node would allow us to prune which parts of the tree we walk, but we always need to do a complete DFS to get correct TextRange values for the highlighter.

To go along with the latter approach, the idea would be that any time a RCTText or RCTVirtualText node has a backgroundColor, the RCTText node boolean flag would get set to true. When an RCTVirtualText node removes it's background color, we need to do a DFS to update the TextHighlighters anyway, so at that point we could conditionally reset the flag to false if no highlighters are created.

Taking this a step further, if only the root has a backgroundColor set, we can actually avoid the DFS using the Text property on TextBlock.

@rozele
Copy link
Collaborator Author

rozele commented Aug 11, 2021

Another thing to consider is to keep the highlighters and just update the ranges, but its less memory efficient to maintain a map from virtual text nodes to their corresponding TextHighlighters than to just recompute the set of required highlighters. Also, recalculating the highlighters from scratch feels a bit less bug prone (there are more edge cases to account for). I'm open to reconsidering this option if someone feels strongly about it.

@chrisglein
Copy link
Member

Maybe related to the issue @chiaramooney has been looking at (#8395)?

@rozele
Copy link
Collaborator Author

rozele commented Aug 11, 2021

Maybe related to the issue @chiaramooney has been looking at (#8395)?

@chrisglein Quite possibly. I believe fast refresh does not re-render the entire tree, just updates props that have changed. If that's the case, I believe fast refresh would suffer the same limitations that programmatically updating a prop through a state change faces.

@rozele
Copy link
Collaborator Author

rozele commented Aug 11, 2021

Here is a side beside comparison of the before (left) and after (right) for the RNTester TextExample:

2021-08-11.14-34-58.mp4

Note that nothing has changed in the UX, though I believe the snapshots will have changed slightly since this simplifies the way highlights are applied and applies highlights to the entire span of text (rather than just the parts that don't have descendant values for highlights).

@rozele
Copy link
Collaborator Author

rozele commented Aug 12, 2021

Here's a before and after with this change:
Before:

React.Native.Playground.Win32.2021-08-11.22-06-36.mp4

After:

React.Native.Playground.Win32.2021-08-11.22-26-12.mp4

@rozele rozele force-pushed the issue7569 branch 2 times, most recently from 2d61372 to 65f8730 Compare August 14, 2021 01:48
@rozele rozele force-pushed the issue7569 branch 2 times, most recently from 4b35582 to 7b00898 Compare August 14, 2021 14:32
@NickGerleman
Copy link
Collaborator

@rozele is this ready for merge?

@rozele
Copy link
Collaborator Author

rozele commented Aug 19, 2021

@NickGerleman yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants