Asyncdisplaykit: [ASTextNode] Truncation issue in text with lineSpacing

[yxztj]

@hannahmbanana Both latest master branch and 1.9.81 are tested.
Repro steps:

1. Create ASTextNode with maximumNumberOfLines of 3
2. Set attributedText with lineSpacing > 0 (won't repro if = 0)
3. Set truncation string with the same attributes
   At last we would get a wrong number of lines.
   I created a repo to demo: https://github.com/yxztj/ASTextNodeTest

With ASDK master branch(demo project branch "master"):
Expected:

Actual:

With ASDK 1.9.81(demo project branch "1.9.81"):
At first number of lines are correct. But when scrolling down and up again(text node re-rendered), the issue appears just as in ASDK master branch above.
Please let me know if additional information is required.

[yxztj]

@maicki Since it's a production issue, resolving it is important to us. Do you think we can have a quick workaround?

[yxztj]

@appleguy Is there any updates on this issue？

[yxztj]

@appleguy After spending some time diving into ASTextKitRenderer, here's what I've found:

Suppose we have 10 lines of text, 5px height for text and 2px lineSpacing, setting maximumNumberOfLines to 3. During the time, [ASTextKitRenderer _calculateSize] is called several times.

First pass:

Creating [ASTextKitRenderer context] with unlimited constrainedSize in height.
Truncate textStorage into 3 lines. Set node's _calculatedSize height to the height of 3 lines, which is 19px(5px * 3 + 2px * 2)

Second pass:

The context is re-created with constrained size(19px height) and original 10 lines of text.
Now the truncator uses height of 19px in [layoutManager glyphRangeForBoundingRect]. This is enough for the text after truncation(3 lines of text), however the rect is not enough for clipping 10 lines of text into 3 lines, which needs at least 21px height(5 * 3 + 2 * 3, the lineSpacing after 3rd line is also counted. I guess both the text height and lineSpacing below it are needed for layoutManager to calculate glyph range) So we only got 2 lines finally.

Hope we can get a fix based on information above :)

[appleguy]

@yxztj great info, thanks! The results above — were they tested with master, or 1.9.90? Master does have some changes in this area.

The initial height calculation — is the value for the hight coming from TextKit? I'm thinking it should return the necessary height to draw...if it is not, then this is a TextKit bug. It is something we may should be able to work around, but this is a bit tricky, because the line spacing property could change in different ranges in the attributed string...and Apple may fix the sizing bug in a future iOS version.

If it's not a TextKit bug and is only an ASDK bug, that's certainly great news!

Relatedly, we currently have an ASTextKitRenderer caching mechanism in-place, but disabled. It's intended that the second pass here not be necessary, and that would seemingly fix the issue too. However we should make sure the result is consistently the same even if caching were to fix it, since a cache shouldn't be required for correctness.

Thanks again, looking forward to fixing this. We made some key breakthroughs adopting the new layout API version in Pinterest today, and will be intensely focused on it this week, but the test case and debugging you provided will make it much more practical for me to try and fix it this weekend

[yxztj]

@appleguy Thanks for the reply. Now I found some more information. All test is based on latest master branch.

1. In ASTextNode calculateSizeThatFits, textRenderer is required to calculate its size. At this time constrainedSize height is infinite.
2. After size is calculated(using [layoutManager usedRectForTextContainer]), in ASTextNode calculatedLayoutDidChange, it sets the result as new constrainedSize for renderer. Since the size is changed, textRenderer discard its context and reset all variables.
3. In ASTextNode drawRect, the renderer needs to re-calculate its size based on new constrainedSize, which is only enough for a total of 3 lines(2 lineSpacing count, so I cannot say it is a wrong size), but not good for clipping 10 lines to 3 with lineSpacing > 0, as the 3rd lineSpacing is also needed for TextKit to render the 3rd line(not documented, maybe it's an Apple bug, maybe it's by design).

For ASDK, setting constrainedSize on the renderer leads to discarding its current context(with correct truncation result), triggering another size calculation during drawRect. Due to short in height, truncation result is different than first pass.

Is it necessary to reset textRender's context on calculatedLayoutDidChange? Anyway its expensive for the text size to be calculate twice(in both layout and drawRect, even if with no truncation), as we already have the correct size for truncated text.
If there needs to be 2 passes of size calculation, then we might need to figure out how to give enough space for TextKit to calculate the correct character range in truncation in 2nd pass.

[yxztj]

OK I've submitted a bugreport to apple. It shouldn't be required to give extra space for lineSpacing below last line to get it rendered. But I doubt Apple is going to look at it soon..
Can we turn on textRenderer caching to work around this issue now?

[yxztj]

Yeah as we expected Apple is not replying on radar. Maybe we can find a workaround? @appleguy

[appleguy]

@yxztj thanks again for your patience - we're getting closer to being able to investigate this. Curious if you've tried with the new optimized sizing + display codepaths. One PR still hasn't been merged (see the one from Adlai about drawing fast path), but the sizing fast path may actually fix this in cases that don't use a custom .truncationAttributedString (which is the common case).

[yxztj]

@appleguy I just tested with latest master(commit: 4dca05767bf7f5d555746d8cb9e444de51bd458b) in the demo project, the issue is still there(no matter truncationAttributedString is set) :(

[appleguy]

@Adlai-Holler - since you looked at ASTextNode recently, do the symptoms described here sound familiar in any sense?

@yxztj Thanks for your patience! We are almost done adopting all of the 2.0 APIs in Pinterest, so we should be able to investigate this in the next couple weeks.

[maicki]

@Adlai-Holler @appleguy Just looked into that a bit with master. So apparently this works now reliable, unfortunately reliable wrong. Setting the maximumNumberOfLines to 3 (no matter truncationAttributedString is set) the text node will show only 2 lines of text but the text node is in fact the right size.

[appleguy]

@yxztj quick update here. The team is now focused on ASTextNode improvements. @maicki did a variety of investigation here, and @Adlai-Holler has been working on some significant changes to address an issue discovered in the truncation algorithm.

It will be some difficult work, but we currently expect to have this issue fixed in the next couple weeks. We are sprinting to try to have it addressed for the Dec 1st event. Feel free to discuss with Adlai on Slack (note however he is currently working from Japan timezone)

[yxztj]

@appleguy Just tested with 2.0 release. The issue is still there..

[yxztj]

@maicki @Adlai-Holler Not sure if #2643 is related, but the PR didn't fix this issue. Tested with latest master branch.

[bawn]

+1

[yxztj]

@appleguy @hannahmbanana Happy new year! Sorry to ping you again but not being able to truncate text with line spacing has been bothering us for over half a year. I know it involves some major improvements in the whole async text rendering/layout process, and that's why we weren't trying to fix it ourselves.

Do you think we can get a workaround soon? Or maybe currently I could use ASDisplayView to wrap a UILabel instead?

[stevenojo]

I'm seeing a similar issue in my project with ASTextNode. Not sure if they are realted, but figured it might be worth commenting here:

With custom lineSpacing of 8 on the attributed string, when I set maximumNumberOfLines = 2, I'm only seeing one line displayed on certain strings that are 3 or 4 lines. Compare the difference in the "National Lampoon.." with these two settings, and note the "All Dogs Go To Heaven" TextNode being unaffected.

When maximumNumberOfLines is set to 0 (default):

With maximumNumberOfLinesSet to 2:

The expectation, is that the ASTextNode would truncate text on the second line. Is there a workaround for this, or a fix I could make that could resolve this?

[stevenojo]

Found a workaround for my own issue:

In my case it was as simple as changing truncationMode to .byTruncatingTail instead of the default option and the problem ceased. Does that work for anyone else by chance?

[Shafichariri]

Hi, Any updates on this issue?

[xilin]

@stevenojo Thanks for the tip, it works for me.

[garrettmoon]

This issue was moved to TextureGroup/Texture#140