Use CALayers to draw text #24387

janicduplessis · 2019-04-10T07:02:00Z

Summary

The current technique we use to draw text uses linear memory, which means that when text is too long the UIView layer is unable to draw it. This causes the issue described here. On an iOS simulator the bug happens at around 500 lines which is quite annoying. It can also happen on a real device but requires a lot more text.

To be more specific the amount of text doesn't actually matter, it is the size of the UIView that we use to draw the text. When we use [drawRect:] the view creates a bitmap to send to the gpu to render, if that bitmap is too big it cannot render.

To fix this we can use CATiledLayer which will split drawing into smaller parts, that gets executed when the content is about to be visible. This drawing is also async which means the text can seem to appear during scroll. See https://developer.apple.com/documentation/quartzcore/calayer?language=objc.

CATiledLayer also adds some overhead that we don't want when rendering small amount of text. To fix this we can use either a regular CALayer or a CATiledLayer depending on the size of the view containing the text. I picked 1024 as the threshold which is about 1 screen and a half, and is still smaller than the height needed for the bug to occur when using a regular CALayer on a iOS simulator.

Also found this which addresses the problem in a similar manner and took some inspiration from the code linked there GitHawkApp/StyledTextKit#14 (comment)

Fixes #19453

Changelog

[iOS] [Fixed] - Use CALayers to draw text, fixes rendering for long text

Test Plan

Added the example I was using to verify the fix to RNTester.
Made sure all other examples are still rendering properly.
Tested text selection

janicduplessis · 2019-04-10T07:04:17Z

Libraries/Text/Text/RCTTextRenderer.m

+  [layoutManager drawBackgroundForGlyphRange:glyphRange atPoint:_contentFrame.origin];
+  [layoutManager drawGlyphsForGlyphRange:glyphRange atPoint:_contentFrame.origin];
+
+  __block UIBezierPath *highlightPath = nil;


This part I'm not 100% sure about, what is this _highlightLayer for and how can I test it still renders properly?

https://github.com/facebook/react-native/blob/master/Libraries/Text/RCTTextAttributes.h#L53

Libraries/Text/Text/RCTTextView.m

janicduplessis · 2019-04-10T07:13:49Z

TODO: Update RNTester screenshot tests

shergin · 2019-04-10T22:05:04Z

OMG, that's so awesome and promising. Thank you!
I need some time to review this carefully.

cc @rigdern

RubenSandwich · 2019-04-11T18:39:16Z

@janicduplessis Do you know if this will have any effect on text accessibility for screen readers?

janicduplessis · 2019-04-11T19:22:22Z

@RubenSandwich I don't think so, it really only affects the drawing of the text. The accessibility part is handled by the UIView.

shergin

Love it.
(See the comment in the code.)

shergin · 2019-04-12T03:04:53Z

Libraries/Text/Text/RCTTextRenderer.m

+  _contentFrame = contentFrame;
+}
+
+- (void)drawLayer:(CALayer *)layer


I am worried about two things:

How can we deduplicate the code in this method and the similar method in the another file.

Why we should manage another _highlightLayer inside this class? Why we cannot use the original class for that?

Point 1: Not sure about this one, both methods are actually different. Here's how it works basically:

RCTTextView has 2 different layer ivars, one is a regular CALayer and the other one is a CATiledLayer subclass. The configureLayer method checks the current frame and creates an instance of the right type of layer based on it. It also removes the other layer type if an instance already exists (could happen when updating text). This means we always have one layer instanciated and the other one nil.

RCTTextRenderer is a CALayerDelegate which handles all the text drawing, this delegate is used by both the CALayer and CATiledLayer this way there is no duplication of the text drawing code.

Point 2: Yea I didn't really know what it was initially and just ported all the code inside the RCTTextRenderer class but I agree it should still be managed by the RCTTextView class. I can probably make it a CALayer subclass RCTHighlightLayer to handle the highlight drawing logic separately.

Point 1. This code draws textStorage on context, right? If so, why can't we decouple that into a static function?

Point 2. Not sure I understand why we need a subclass for that. Why can't we keep the code which renders highlight boxes where it is now?

@shergin Oups I thought I replied to this -_-

Right now text drawing is abstracted in the RCTTextRenderer class. It implements CALayerDelegate so it can be set on any CALayer. The goal of this abstraction is to avoid duplicating the text drawing code since it is used by the 2 different CALayer implementations

In this case I don't see the need to abstract text drawing further in a static function since the drawing code is already implemented only once. An alternative would be to create 2 different CALayer subclasses and extract the drawing code in a static function like you mentioned instead of using the delegate.

The core of the code is the dance between the layer types in configureLayers, not sure if we want to come up with a cleaner abstraction but since I doubt there will be more than 2 layer types it should get much more complex.

Yea I didn't end up doing this, I moved it back to RCTTextView

mikelovesrobots · 2019-04-12T22:58:43Z

Nice work! This PR definitely fixes the missing text problems we're seeing when people (who are vision-impaired) scale the system fonts up in the accessibility menu of their phone. @janicduplessis, you're a humanitarian.

For comparison:

react-native 0.59.4 @ default font scaling	react-native 0.59.4 @ max font scaling - YUCK -	this branch @ max font scaling

(Ignore the black bar, that's a video scrolled mostly off-screen)

janicduplessis · 2019-04-13T19:53:05Z

@shergin Fixed the highlight layer and tested it works properly, even when using large amounts of text.

janicduplessis · 2019-04-13T20:04:35Z

I also had to update the screenshot tests but I can't really see any difference.

Update:

Used an image diff tool to find the differences (note that the differences are very small, only detected when fuzzing is 0, even at fuzzing 1 the tool doesn't detect the changes.):

Those are the only 2 examples with differences, looks like some minor changes related to antialiasing or something. The difference isn't visible so I think this is fine.

ericlewis · 2019-04-17T22:54:48Z

This looks good to me, great job figuring this out. I also noticed that it was based around the height, but didn’t occur to me that it was due to memory problems!

shergin · 2019-04-29T16:31:29Z

@janicduplessis This is an extremely valuable and important improvement. However it also has to be perfect before we can land it. I do love this but I am still concerning about this part:
https://github.com/facebook/react-native/pull/24387/files#r277152326

I am sorry, I am probably missing something, but I also have to be sure. Text/TextInput are the most complex and used components a there is a huge maintenance (and perf) costs associated with it.

shergin

Back to Janic's queue. See my last comment. ❤️

sammy-SC · 2019-05-30T17:27:57Z

@janicduplessis this looks amazing, I will take a look at this as well.

jeanregisser · 2019-06-12T21:06:50Z

Awesome work @janicduplessis 🎉

Just wondering, is the 1024 tile size also appropriate on iPad and Apple TV? Or should it be based on the screen size so it’s always 1.5 times the max dimension?

janicduplessis · 2019-06-17T17:31:22Z

@jeanregisser Good point, 1.5 times the max dimensions sounds like a good heuristic.

janicduplessis · 2019-06-17T23:31:48Z

@shergin Any change you can have another look at this?

facebook-github-bot

@sammy-SC has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

react-native-bot · 2019-06-24T10:05:05Z

This pull request was successfully merged by @janicduplessis in 690e85d.

^{When will my fix make it into a release? | Upcoming Releases}

Summary: The current technique we use to draw text uses linear memory, which means that when text is too long the UIView layer is unable to draw it. This causes the issue described [here](#19453). On an iOS simulator the bug happens at around 500 lines which is quite annoying. It can also happen on a real device but requires a lot more text. To be more specific the amount of text doesn't actually matter, it is the size of the UIView that we use to draw the text. When we use `[drawRect:]` the view creates a bitmap to send to the gpu to render, if that bitmap is too big it cannot render. To fix this we can use `CATiledLayer` which will split drawing into smaller parts, that gets executed when the content is about to be visible. This drawing is also async which means the text can seem to appear during scroll. See https://developer.apple.com/documentation/quartzcore/calayer?language=objc. `CATiledLayer` also adds some overhead that we don't want when rendering small amount of text. To fix this we can use either a regular `CALayer` or a `CATiledLayer` depending on the size of the view containing the text. I picked 1024 as the threshold which is about 1 screen and a half, and is still smaller than the height needed for the bug to occur when using a regular `CALayer` on a iOS simulator. Also found this which addresses the problem in a similar manner and took some inspiration from the code linked there GitHawkApp/StyledTextKit#14 (comment) Fixes #19453 ## Changelog [iOS] [Fixed] - Use CALayers to draw text, fixes rendering for long text Pull Request resolved: #24387 Test Plan: - Added the example I was using to verify the fix to RNTester. - Made sure all other examples are still rendering properly. - Tested text selection Reviewed By: shergin Differential Revision: D15918277 Pulled By: sammy-SC fbshipit-source-id: c45409a8413e6e3ad272be39ba527a4e8d349e28

Summary: The current technique we use to draw text uses linear memory, which means that when text is too long the UIView layer is unable to draw it. This causes the issue described [here](facebook#19453). On an iOS simulator the bug happens at around 500 lines which is quite annoying. It can also happen on a real device but requires a lot more text. To be more specific the amount of text doesn't actually matter, it is the size of the UIView that we use to draw the text. When we use `[drawRect:]` the view creates a bitmap to send to the gpu to render, if that bitmap is too big it cannot render. To fix this we can use `CATiledLayer` which will split drawing into smaller parts, that gets executed when the content is about to be visible. This drawing is also async which means the text can seem to appear during scroll. See https://developer.apple.com/documentation/quartzcore/calayer?language=objc. `CATiledLayer` also adds some overhead that we don't want when rendering small amount of text. To fix this we can use either a regular `CALayer` or a `CATiledLayer` depending on the size of the view containing the text. I picked 1024 as the threshold which is about 1 screen and a half, and is still smaller than the height needed for the bug to occur when using a regular `CALayer` on a iOS simulator. Also found this which addresses the problem in a similar manner and took some inspiration from the code linked there GitHawkApp/StyledTextKit#14 (comment) Fixes facebook#19453 ## Changelog [iOS] [Fixed] - Use CALayers to draw text, fixes rendering for long text Pull Request resolved: facebook#24387 Test Plan: - Added the example I was using to verify the fix to RNTester. - Made sure all other examples are still rendering properly. - Tested text selection Reviewed By: shergin Differential Revision: D15918277 Pulled By: sammy-SC fbshipit-source-id: c45409a8413e6e3ad272be39ba527a4e8d349e28

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Expo Partner: Expo Partner labels Apr 10, 2019

janicduplessis commented Apr 10, 2019

View reviewed changes

react-native-bot added the Includes Changelog label Apr 10, 2019

janicduplessis commented Apr 10, 2019

View reviewed changes

Libraries/Text/Text/RCTTextView.m Show resolved Hide resolved

janicduplessis requested a review from shergin April 10, 2019 07:06

shergin suggested changes Apr 12, 2019

View reviewed changes

janicduplessis mentioned this pull request Apr 12, 2019

<Text> Component with over ~500 lines won't render [iOS] #19453

Closed

3 tasks

janicduplessis force-pushed the rcttextview branch from 218ab11 to ab84933 Compare April 23, 2019 02:41

shergin suggested changes Apr 29, 2019

View reviewed changes

react-native-bot added Bug Platform: iOS iOS applications. labels May 3, 2019

hramos removed the Includes Changelog label May 6, 2019

janicduplessis force-pushed the rcttextview branch from ab84933 to aa3aa0e Compare June 17, 2019 22:37

janicduplessis added 2 commits June 18, 2019 14:06

Use CALayers to draw text

33fa8b1

Use screen size as tiled heuristic, add comment about RCTTextRenderer

09405c0

janicduplessis force-pushed the rcttextview branch from b415ac6 to 09405c0 Compare June 18, 2019 18:07

facebook-github-bot reviewed Jun 20, 2019

View reviewed changes

facebook-github-bot closed this in 690e85d Jun 24, 2019

react-native-bot added the Merged This PR has been merged. label Jun 24, 2019

alexbrazier mentioned this pull request Mar 16, 2023

fix: Issue with response body not loading on ios on RN 0.71.x alexbrazier/react-native-network-logger#74

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use CALayers to draw text #24387

Use CALayers to draw text #24387

janicduplessis commented Apr 10, 2019 •

edited

janicduplessis Apr 10, 2019

shergin Apr 10, 2019

janicduplessis commented Apr 10, 2019

shergin commented Apr 10, 2019

RubenSandwich commented Apr 11, 2019

janicduplessis commented Apr 11, 2019

shergin left a comment

shergin Apr 12, 2019

janicduplessis Apr 12, 2019

shergin Apr 21, 2019

janicduplessis Apr 29, 2019

mikelovesrobots commented Apr 12, 2019

janicduplessis commented Apr 13, 2019

janicduplessis commented Apr 13, 2019 •

edited

ericlewis commented Apr 17, 2019

shergin commented Apr 29, 2019

shergin left a comment

sammy-SC commented May 30, 2019

jeanregisser commented Jun 12, 2019

janicduplessis commented Jun 17, 2019

janicduplessis commented Jun 17, 2019

facebook-github-bot left a comment

react-native-bot commented Jun 24, 2019

Use CALayers to draw text #24387

Use CALayers to draw text #24387

Conversation

janicduplessis commented Apr 10, 2019 • edited

Summary

Changelog

Test Plan

janicduplessis Apr 10, 2019

Choose a reason for hiding this comment

shergin Apr 10, 2019

Choose a reason for hiding this comment

janicduplessis commented Apr 10, 2019

shergin commented Apr 10, 2019

RubenSandwich commented Apr 11, 2019

janicduplessis commented Apr 11, 2019

shergin left a comment

Choose a reason for hiding this comment

shergin Apr 12, 2019

Choose a reason for hiding this comment

janicduplessis Apr 12, 2019

Choose a reason for hiding this comment

shergin Apr 21, 2019

Choose a reason for hiding this comment

janicduplessis Apr 29, 2019

Choose a reason for hiding this comment

mikelovesrobots commented Apr 12, 2019

janicduplessis commented Apr 13, 2019

janicduplessis commented Apr 13, 2019 • edited

ericlewis commented Apr 17, 2019

shergin commented Apr 29, 2019

shergin left a comment

Choose a reason for hiding this comment

sammy-SC commented May 30, 2019

jeanregisser commented Jun 12, 2019

janicduplessis commented Jun 17, 2019

janicduplessis commented Jun 17, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

react-native-bot commented Jun 24, 2019

janicduplessis commented Apr 10, 2019 •

edited

janicduplessis commented Apr 13, 2019 •

edited