Fixed boundingRect(with:options:) miscalculation of MessageLabel like text Ꮚ˘̴͈́ꈊ˘̴͈̀Ꮚ⋆✩

[zhongwuzw]

Fixes #812 #816 .
I think it's a bug for boundingRect(with:options:), it would calculates a less size when text like Ꮚ˘̴͈́ꈊ˘̴͈̀Ꮚ⋆✩ or Tomorrow is ....

[SD10]

@zhongwuzw I have some concerns about this because 5 extra points is just a magic number. I’ll dig deeper to see what caused this if there’s a bug

_{Sent with GitHawk}

[zhongwuzw]

@SD10 Yeah, it's a magic number, I tried JSQ, and it also has this bug 😂 , and it also has a magic number. 🤔 See size-calculation-of-JSQ

[SD10]

@zhongwuzw I have seen that before but why did you suggest 5pt if they use 2pt at JSQMVC?

[zhongwuzw]

@SD10 Emm, only greater than 5pt can cover these situations.

[Sherlouk]

What is the number dependent on though? Does it change based on the length of the string? If I insert 100 of that style of emoticon, will 5pt still be accurate?

[zhongwuzw]

@Sherlouk Good opinion 👍 , but I think it's not just simply based on length, I think we can think it as the biggest margin of error. I don't know wether 5pt is enough, it's just a value which can handle all these bugs we faced. 😂

PS. I test multiple text as you said, and it works currently.

[Sherlouk]

Yea I mean just in general magic numbers that no one understands is really bad code smell. 5pt might be fine for this specific string or use case, but I sense there will be others.

Has a radar been raised explaining this issue? I would say at bare minimum we should make sure that's done and reference it with a comment as well as a link to the discussion so people can get involved and update it later.

[SD10]

I'm curious if this is a bug in UIKit or MessageKit. I'd like to see what the resulting frame of boundingRect is for this text 🤔

[Sherlouk]

Might be a good idea to try and create a very basic sample app with the sole purpose of checking the bounding box and overlaying a view to see if it's a reasonable size?

[yokochi]

https://github.com/yokochi/MessageKit/tree/test-emoticon-string
yokochi@f0bd392
Please use it if you want. An emoticon pattern was added to the sample application.

[Sherlouk]

Very basic playground setup, experimented with a few variations (With and without extra text, etc) and the red overlay covered the text every time. Interesting 🤔

I've also just cloned master and added this 5pts to the same calculation, but the issue is still present inside of the example project unless I'm majorly messing something up here

// Edit 1
Okay so mild backtrack, I made some modifications to the example project to make this easier to test. Red test is attributed, Black text is normal "text". With the 5pts some of the red ones work, some don't. Without it basically non of them work. This means the 5pt is at least partially based on fonts as that's the only deviation here. Will continue to investigate

// Edit 2
Red Text - Doesn't Work UIFont.preferredFont(forTextStyle: .body)
Green Text - Does Work UIFont.boldSystemFont(ofSize: UIFont.systemFontSize)
Blue Text - Does Work UIFont.monospacedDigitSystemFont(ofSize: UIFont.systemFontSize, weight: UIFont.Weight.bold)
Purple Text - Doesn't Work UIFont.italicSystemFont(ofSize: UIFont.systemFontSize)

Again this is all with the 5pt "fix".

[Sherlouk]

Okay so I've narrowed down to where it's going wrong, but still need to work out how to actually fix it.

This is using the same code, but different texts. I've updated the MessageLabel to show both our way of rendering text but also Apple's. You can see in normal text they're more or less the exact same, therefore MessageKit doesn't have an issue. With the different characters as per this bug, they're not the same.

Now the reason it's getting truncated is fairly easy, the textContainer size is too small so it can't fit. Because the lineBreakMode is set to split using wordWrapping this breaks it where it does. Changing this to use byClipping makes it so you can see the whole string but there's less padding on the right vs normal strings. Acceptable for most people, but not amazing.

The solution in this PR affects all strings, meaning even working/valid strings will have more padding on the right. An inconsistency I'm not a fan of.

We need to find out what's wrong with the way we render text, that means the two solutions don't overlay. Interestingly, the first character seems to be fine - so maybe some weird letter spacing? I don't know - still investigating.

[SD10]

@Sherlouk Thanks for all that debugging you did. I only have time for a short response --

I'm not a fan of the 5pt magic number because it breaks Strings that are currently being formatted properly. I'm also not a fan of .byClipping as the line break mode because it will reduce the amount of padding on the trailing side. Both of these "fixes" feel too much like hacks to me.

There must be an issue with calculating the text container size when applying the insets:

https://github.com/MessageKit/MessageKit/blob/master/Sources/Views/MessageLabel.swift#L172-L182

Fwiw, I would like to move away from MessageLabel and use StyledTextKit in the future

[Sherlouk]

So yea I narrowed the problem down to the same block of code (as we spoke briefly about on Slack) but the textContainer.size seemed to have no impact on the actual issue at hand in the limited testing I did. That size was actually perfect and as expected. Sure you can add +5 and achieve the same fix, but not what we want.

It just seems to be the drawGlyphs call is drawing it slightly different to what happens if you call super.draw - not sure why though.

Haven't tried using these sorts of emoticons in StyledTextKit, but probably worth trying a basic experiment first before going all in.

[zhongwuzw]

I have just a glimpse, not deep in, StyledTextView not provide auto calculation size of attributedString?

On StackOverflow, seems boundingRect actually not do well itself.

[Sherlouk]

I've not looked into either StyledTextView nor that StackOverflow post in detail but I can tell you that the size calculation is working perfectly fine. If you look at the screenshots I posted above, when you let Apple draw the string using our existing size calculations (not with the 5pt hack) it works perfectly fine. It's only when you use our draw method that suddenly it doesn't look right.

Sizing isn't the issue here, the drawing is.

[zhongwuzw]

@Sherlouk You can try add the height value after get size of calculation. I think the reason is the width is not sufficient, leads the rest of characters positioned in new line.
JSQ uses UITextView to render text, it also has this bug, and it also use boundingRect to calculate size.

[SD10]

The issue can be in 1 of 3 places IMO:

1. The drawRect method of MessageLabel class
2. The calculation for messageContainerSize is incorrect
3. The use of boundingRect

[Sherlouk]

@zhongwuzw I think you might be missing what I'm saying though, using the size from boundingRect gives me the grey rectangle. The text does fit within that size.

Bounding rect is giving the perfect size to show that specific example of text (haven't tried it for all the other possible ones).

If you use a plain jane UILabel, set the size of that label to the output of the bounding rect calculation, and set the text. It displays fine.

I urge you to try just getting rid of the drawRect method and notice how, even with the size of the cell being calculated using boundingRect it actually displays fine. or even just add a super.drawRect call within the function and you'll notice two versions on top of each other (the screenshot I shared above) for comparison sake. Even if you make the view bigger, you're just plastering over the fact that MessageLabel is (for some reason) drawing text too big.

[zhongwuzw]

@SD10 @Sherlouk @yokochi Guys, sorry for late reply.

I removed boundingRect(with:options:), it's not accurate when calculate size of text, I use NSLayoutManager instead, and tested every emoj @yokochi listed and many texts, everything works fine.

    let emoticons = [
         "ʕ•̫͡•ʔ♡ʕ•̫͡•ʔ",
         "(✿ꈍ。 ꈍ✿)ポッ",
         "▂▅▇█▓▒(’ω’)▒▓█▇▅▂",
         "Ꮚ˘̴͈́ꈊ˘̴͈̀Ꮚ⋆✩",
         "ｷｬ━━━━(ﾟ∀ﾟ)━━━━!!",
         "✌✌(˵¯̴͒ꇴ¯̴͒˵)✌✌",
         "(ꏿ᷄౪ ꏿ᷄ ̨ )͞˭̳̳̳˭̳̳̳ˍ̿̿ˍ̿ˌ˳ˏ̇⋅∴༣",
         "φ(ﾟ-ﾟ=)",
         "✺◟(∗❛ัᴗ❛ั∗)◞✺",
         "(⸝⸝⸝ ≧ㅿ＼⸝⸝⸝)//❤\\(⸝⸝⸝°⁻̫° ⸝⸝⸝)",
         "꒰✩'ω`ૢ✩꒱",
         "ˁ˙͡˟˙ˀ",
         "ʕ•͡ɛ•͡ʼʼʔ",
         "ʕ•̼͛͡•ʕ-̺͛͡•ʔ•̮͛͡•ʔ",
         "=͟͟͞͞ʕ•̫͡•ʔ =͟͟͞͞ʕ•̫͡•ʔ =͟͟͞͞ʕ•̫͡•ʔ =͟͟͞͞ʕ•̫͡•ʔ",
         ":.ﾟ٩(๑˘ω˘๑)۶:.｡",
         "❤⃛♡꒰˘̩̩̩⌣˘̩̩̩⌗꒱",
         "┗=͟͟͞͞( ˙∀˙)=͟͟͞͞┛",
         "=͟͟͞͞👉=͟͟͞͞👉=͟͟͞͞👉=͟͟͞͞👉))ु˃̶͈̀ω˂̶͈́ )੭ु⁾⁾",
         "⁝⁞⁝⁞Ϛ⃘๑•͡ .̫•๑꒜☂⁝⁞⁝⁝"
     ]

[zhongwuzw]

Currently, we have no thread-safe issue because we do this on main thread. So we can just replace characters.

[SD10]

@zhongwuzw Thanks for looking into this! I can't wait to review it this weekend ❤️

[nathantannar4]

@zhongwuzw @SD10 whats the current status of this PR? Should it be included in the 1.1 release?

[zhongwuzw]

@nathantannar4 I think it's ready to ship it, but because it would influence all layout process, so any review back would be more appreciated. 🤔

[SD10]

@nathantannar4 As @zhongwuzw this is a pretty big change so I don't think it can make it in the next release. It needs more investigating

[nathantannar4]

#764 Would make this PR irrelevant if we scrap MessageLabel and move to StyledTextKit. @SD10 Did you still want to move in that direction?

[zhongwuzw]

@nathantannar4 Emm, even though step to StyledTextKit, seems we still need to calculate the size by myself.

[SD10]

Yeah, as @zhongwuzw said, StyledTextKit won't solve this. The reason I haven't merged this is I'm still not convinced that boundingRect(with:options:) is the problem here.

I really think this issue is related to the draw method and the layout manager in the MessageLabel class. I feel like it is far more likely that I made a mistake than the UIKit APIs

[zhongwuzw]

@SD10, Emm, JSQ uses UITextView and also has this issue.

[yokochi]

I'm using MessageKit with the app I'm in charge of. And merged this PR. Although it stood for about two weeks, I think that the application is working normally, I think that the operation in Japanese is no problem. For reference, the app has about 2,000 DAU.

[zhongwuzw]

@yokochi Thanks for information. 👍 @SD10 Any considerations? 🤔

[SD10]

@zhongwuzw This is where I left off on reviewing this PR. I was a little concerned that the values used in the textContainer wouldn't match up with the textContainer used in MessageLabel. Let's say that the user changes the MessageLabel to have a maximumNumberOfLines to 2 or lineFragmentPadding to 3. What do you think?

[zhongwuzw]

@SD10 Yeah, I also consider this issue, I prefer to let user override size calculation method.But also we can, if user change property of Label, provide maximunNumberOfLines lineFragmentPadding to the layout manager? After that we can calculate specific size for each label? 🤔

[SD10]

I don't think there's a good way for the MessageLabel and the MessageSizeCalculator to communicate within the framework. I'm really worried that this approach will lead to a large amount of undefined behavior.

We could possibly make a delegate that provides a shared MessageSizeConfiguration object to both theMessagesCollectionViewFlowLayout and the MessageLabel but blahhh 🤢

[zhongwuzw]

@SD10 Add a shared configuration between theMessagesCollectionViewFlowLayout and MessageLabel seems weird 😂 , I still think users may should override messageContainerSize(for message: MessageType)->CGSize to return size, and don't change properties of MessageLabel directly, instead to reload cell. 🤔

[marcetcheverry]

Adding a layoutManager and textStorage container adds a penalty in text size calculation over calling boundingRect. Have you tried usesDeviceMetrics?

EDIT: usesDeviceMetrics is not a 100% perfect approach. The overall direction of this PR is good, but the communication between the parts here needs to be worked out. The current sizing calculations are wrong. boundingRect is only useful for UILabel, using NSLayoutManager draws things differently, and a few pixels off here and there can create big problems in rendering.

[marcetcheverry]

Also see #1136 if it fixes this issue. I've also noticed the insetRect not being used in drawText(in rect: CGRect), I will make another PR for that.

[austinwright]

As I posted about on slack, with the release of iOS 13 the boundingRect function appears to return an unexpected value when sizing any partially attributed string. The fix I applied is similar to @marcetcheverry 's fix, adding usesDeviceMetrics to the bounding rect options. It seems that there may be some additional considerations as pointed out in #897. My fix was as follows: change MessageSizeCalculator.labelSize implementation to:

    internal func labelSize(for attributedText: NSAttributedString, considering maxWidth: CGFloat) -> CGSize {
        let constraintBox = CGSize(width: maxWidth, height: .greatestFiniteMagnitude)

        /// Workaround:
        let adjustedRect: CGRect
        if #available(iOS 13, *) {
            let fullRange = NSRange(0..<attributedText.length)
            var isPartiallyAttributed = false
            attributedText.enumerateAttributes(in: fullRange) { value, range, stop in
                if !NSEqualRanges(fullRange, range) {
                    isPartiallyAttributed = true
                    stop.pointee = true
                }
            }
            var options: NSStringDrawingOptions = [.usesLineFragmentOrigin, .usesFontLeading]
            if isPartiallyAttributed {
                options.formUnion(NSStringDrawingOptions.usesDeviceMetrics)
            }
            adjustedRect = attributedText.boundingRect(with: constraintBox,
                                                                            options: options,
                                                                            context: nil)
        } else {
            adjustedRect = attributedText.boundingRect(with: constraintBox,
                                                                            options: [.usesLineFragmentOrigin, .usesFontLeading],
                                                                            context: nil)
        }

        return adjustedRect.size
    }

If there were already issues with bounding rect, then this provides even more reason to make updates to this logic. I'm seeing a few options:

1. Refactor and use a text storage / container solution
2. add the device metrics option to both places.
3. Update to styledTextKit (and make any additional necessary adjustments)

@marcetcheverry / @zhongwuzw / @SD10 / @nathantannar4 - Any thoughts or input?

[Kaspik]

Any update on this one guys? Do we want to start digging into this again?

[austinwright]

So, my above fix causes a separate issue: A single-line, partially attributed string receives the wrong boundingRect. Here is what I think needs to happen:

1. Short-term: add logic to determine if the text will span 1 line or 2+ lines, then branch and do the old calculation without device metrics for 1 line, and with device metrics for 2 lines.
2. Long-term: Refactor and use a text storage / container solution. The caveats are:
   -- We need to allow the framework user to override the text container options with their own (such as might be the case if they are using a custom cell or if they have customized their text writing direction or paragraph style).
   -- We need to match the framework cell options that come out of the box, without tightly coupling the MessageLabel and the MessageSizeCalculator.