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

Use CALayers to draw text #24387

Closed
wants to merge 2 commits into from

Conversation

@janicduplessis
Copy link
Collaborator

commented Apr 10, 2019

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
[layoutManager drawBackgroundForGlyphRange:glyphRange atPoint:_contentFrame.origin];
[layoutManager drawGlyphsForGlyphRange:glyphRange atPoint:_contentFrame.origin];

__block UIBezierPath *highlightPath = nil;

This comment has been minimized.

Copy link
@janicduplessis

janicduplessis Apr 10, 2019

Author Collaborator

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

@janicduplessis janicduplessis requested a review from shergin Apr 10, 2019

@janicduplessis

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 10, 2019

TODO: Update RNTester screenshot tests

@shergin

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

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

cc @rigdern

@RubenSandwich

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

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

@janicduplessis

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 11, 2019

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

@shergin
Copy link
Contributor

left a comment

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

_contentFrame = contentFrame;
}

- (void)drawLayer:(CALayer *)layer

This comment has been minimized.

Copy link
@shergin

shergin Apr 12, 2019

Contributor

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?

This comment has been minimized.

Copy link
@janicduplessis

janicduplessis Apr 12, 2019

Author Collaborator

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.

This comment has been minimized.

Copy link
@shergin

shergin Apr 21, 2019

Contributor

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?

This comment has been minimized.

Copy link
@janicduplessis

janicduplessis Apr 29, 2019

Author Collaborator

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

  1. 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.

  1. Yea I didn't end up doing this, I moved it back to RCTTextView
@mikelovesrobots

This comment has been minimized.

Copy link

commented Apr 12, 2019

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
Screenshot 2019-04-12 09 57 13 Screenshot 2019-04-12 09 22 58 Screenshot 2019-04-12 14 30 27

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

@janicduplessis

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 13, 2019

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

@janicduplessis

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 13, 2019

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.):

image

image

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

This comment has been minimized.

Copy link
Collaborator

commented Apr 17, 2019

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!

@janicduplessis janicduplessis force-pushed the janicduplessis:rcttextview branch from 218ab11 to ab84933 Apr 23, 2019

@shergin

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

@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
Copy link
Contributor

left a comment

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

@sammy-SC

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

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

@jeanregisser

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 17, 2019

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

@janicduplessis janicduplessis force-pushed the janicduplessis:rcttextview branch from ab84933 to aa3aa0e Jun 17, 2019

@janicduplessis

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 17, 2019

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

@janicduplessis janicduplessis force-pushed the janicduplessis:rcttextview branch from b415ac6 to 09405c0 Jun 18, 2019

@facebook-github-bot
Copy link

left a comment

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

@react-native-bot

This comment has been minimized.

Copy link
Collaborator

commented Jun 24, 2019

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

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

kelset added a commit that referenced this pull request Jun 28, 2019

Use CALayers to draw text (#24387)
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.