Open garrettmoon opened 7 years ago
From @nlutsenko on August 11, 2016 19:56
What we ended up doing in Instant Articles is literally increasing the bounding box for the text node by 3.0
for ascender and 0.5
for descender, but I have a feeling that this is not the correct approach.
After looking around for similar issues - looks like not a whole lot of people encountered this, but there are absolutely reports about boundingRectForGlyphRange:inTextContainer:
and usedRectForTextContainer:
returning different sizes compared to CoreText.
Would love to hear your thoughts guys about the approach that we should take here to fix it once and for all for everyone.
From @ocrickard on August 11, 2016 20:49
I tested this with CKTextComponent/LabelComponent, and I'm not seeing the same issues. There must be a difference in the sizing or in the origin positioning of the layout manager.
Here's an example of 30pt HelveticaNeue in the ComponentKit demo project:
From @nlutsenko on August 11, 2016 22:23
@ocrickard, here you go:
A good repro of text being cut off from ComponentKit.
From @nlutsenko on August 11, 2016 22:23
^ The only change in above is replacing all the quotes with gggg
and replacing the font on the frosted component with HelveticaNeue
.
From @nlutsenko on August 11, 2016 23:2
Here is another one, just for reference - this is using
HelveticaNeue-Italic
From @hannahmbanana on August 12, 2016 3:49
@garrettmoon: this appears similar to the other issue you were investigating?
Similar but slightly different, in the italicized case, UILable will clip too. In the original case here (helveticaneue, 30pt, ggggg), UILabel doesn't clip it but ASTextNode does :/
From @ocrickard on August 12, 2016 17:54
Uggh, that's too bad. We're just using vanilla TextKit to determine the size of the glyphs I believe? We've had the same troubles internally with CoreText for years.
From @nlutsenko on August 12, 2016 17:56
Yup, looks like it's an issue with TextKit internals. What do you guys think should be the best way to fix this?
From @ocrickard on August 12, 2016 18:14
Looks to me like UILabel is still rendered and sized using NSStringDrawing, so I'm not surprised at the difference. I'd compare to UITextView.
Hmm, good point, I'll try that out real quick
@ocrickard UITextView also appears to work correctly… hmmm
From @ocrickard on August 12, 2016 18:24
As for how to fix:
@ocrickard there's a PR up for adding padding: #2062
From @ocrickard on August 12, 2016 18:27
@nlutsenko what OS version are you testing on for ComponentKit? I can't repro with the same strings and font sizes for the descender clipping. The italic clipping issue is likely a separate issue.
From @nlutsenko on August 12, 2016 18:27
iPhone 6s simulator running from inside Xcode 7.3.1. Also can repro the same thing on Xcode 8b5.
From @nlutsenko on August 12, 2016 18:29
From my investigation down the rabbit hole:
UILabel
is using NSAttributedString - boundingRectWithSize:options:context:
UIFoundation
, and the implementation doesn't seem to use TextKitFrom @ocrickard on August 12, 2016 18:46
I'm not really sure where all these things are in ASDK, but can you try removing this line: https://github.com/facebook/AsyncDisplayKit/blob/aba05a747c9222de91de2a65e601393a7a284943/AsyncDisplayKit/TextKit/ASTextKitContext.mm#L56 ? I think that should just leave the default line fragment padding, which may help.
From @ocrickard on August 12, 2016 18:47
(I think that may help with the italic clipping issue, though at the cost of slightly larger padding around the text container for normal glyphs)
From @nlutsenko on August 12, 2016 18:48
That helps with italic clipping issue, indeed, by adding extra padding around the text. Doesn't help with descender/ascender stuff though...
From @ocrickard on August 12, 2016 18:53
Yeah, looking at the code, the sizing looks correct to me. Let's add the text container inset, and pad. It's sad, but it seems like the simplest solution here.
From @ocrickard on August 12, 2016 18:53
Also, I'd be in favor of killing that lineFragmentPadding = 0. PR welcome.
From @nlutsenko on August 11, 2016 19:50
Looks like
ASTextNode
while performing layout and drawing can potentially cut-off a single pixel (half a point) in the descenders of some fonts. From my investigation - the underestimation happens inside TextKit NSLayoutManager, where it returns theboundingRectForGlyphRange:inTextContainer:
andusedRectForTextContainer:
that is slightly off.This was reported and attempted to be fixed in #637
Repro:
Kittens
from theexamples
folder@[ @"gggggg" ]
HelveticaNeue
with30.0
point sizeWhilst the proper one should be:![simulator screen shot aug 11 2016 12 50 21 pm](https://cloud.githubusercontent.com/assets/606991/17602604/34bf9f9e-5fc2-11e6-9318-0901cacadff9.png)
Copied from original issue: facebookarchive/AsyncDisplayKit#2061