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

[Attributed Strings] Unordered list items with a single line appear further indented than those with multiple lines when using a custom font #246

Closed
1 task done
mrackwitz opened this issue Feb 16, 2021 · 11 comments

Comments

@mrackwitz
Copy link

Please help prevent duplicate issues before submitting a new one:

  • I've searched other open/closed issues for duplicates before opening up this new issue.

Report

What did you do?

Render a markdown string with an unordered list with a custom font.
The custom font is used in the same size for the body as well as the listItemPrefix.
The text is rendered via a UILabel.

What did you expect to happen?

The bullet points and list items to be vertically aligned.

What happened instead?

List items with a single line appear further indented than those which have multiple lines.

More

I've looked at the code and it does seem as ListItemParagraphStyler is in charge for formatting all kinds of lists. I do not understand exactly how that is supposed to work tho for unordered lists as it doesn't seem to consider bullet points as a prefix. But I think that alone might be already problematic, in particular when a custom, non-monospaced font is used for listItemPrefix.

On a side note: I've tried to reproduce this in a snapshot test, but I've troubles getting them to run as expected in the first place. First Carthage failed to build the snapshot testing framework in a usable format, now all tests are failing. Any instructions around that?

@johnxnguyen
Copy link
Owner

Hey @mrackwitz thanks for the report, I'll take a look at this when I can.

@johnxnguyen
Copy link
Owner

johnxnguyen commented Feb 21, 2021

Hey @mrackwitz , regarding the tests: I've removed Carthage and instead have decided to use SPM to pull in the snapshot testing framework. You should be able to run the snapshot tests now on master. Use should run the tests on the iPhone 12 simulator, otherwise all tests will fail.

@mrackwitz
Copy link
Author

Yay, love it! Thanks @johnxnguyen. That's what I did locally, because it was easier for me than figuring out what was wrong. (Sorry I really could have just PRed that, but that have felt rather opinionated tbh.)

Use should run the tests on the iPhone 12 simulator, otherwise all tests will fail.

Oh that's a very important point, might make sense to document this in a section in the README – might have also just missed that.

The only update I've myself on this so far is that I've been unable to reproduce the issue with the test suite. Bullet points are correctly indented, even when not using a monospaced font. I guess it could be related to the particular font, I'm using?
What I'm still struggling to understand is how ListItemParagraphStyler and unordered lists fit together? Am I looking at the right part with this? Doesn't that need to consider the specific width of the bullet point character in the configured font?

@mrackwitz
Copy link
Author

Also one thing I've been wondering is whether using a UILabel vs a UITextView is causing this difference here. So I'd be curious if you happen to know about any limitations with UILabel when it comes to that.

@johnxnguyen
Copy link
Owner

I think the issue with Carthage is that Xcode 12 now requires xcframeworks, so you need to pass in the --use-xcframeworks when using Carthage. But I tried this and had other problems, and in the end it was just easier to switch over to SPM (which I wanted to do anyway!).

might make sense to document this in a section in the README

Yes, it's on my list to improve the README and documentation so I'll make sure to add this point.

About the indentation issue: which font are you using? I'll try to reproduce it on my side.

I've been wondering is whether using a UILabel vs a UITextView is causing this difference here.

Yes, this could be the cause. The way the list item indentation works is by inserting a tab between the prefix and the item content, then adjusting the tab width to indent the content, and adjusting the paragraph head indent to push the prefix forward so it aligns with neighboring item prefixes. (It's a neat trick but I think it's abusing the purpose of tabs and paragraph indents. I've been playing around with other ways to position text using the layout manager, but it's still a work in progress.) In the screenshot you provided, it looks like the tab get's pushed forward so the content gets indented too much, while the prefix is still fine.

It could be that this trick doesn't really work for UILabels. To be honest, I've always used a text view to render markdown. Can you try using the DownTextView to see if it solves the problem for you?

@johnxnguyen
Copy link
Owner

What I'm still struggling to understand is how ListItemParagraphStyler and unordered lists fit together? Am I looking at the right part with this? Doesn't that need to consider the specific width of the bullet point character in the configured font?

To explain this a bit further, since I didn't really answer this in the last comment. I'm pasting a bunch of code, but you only need to look at the lines that I've commented. Here is how we style a list item:

// In the vistor.
public func visit(item node: Item) -> NSMutableAttributedString {
    let s = visitChildren(of: node).joined

    // The prefix will be a bullet point. Here we just insert it into the attributed string.
    let prefix = listPrefixGenerators.last?.next() ?? ""
    let attributedPrefix = "\(prefix)\t".attributed
    styler.style(listItemPrefix: attributedPrefix)
    s.insert(attributedPrefix, at: 0)

    if node.hasSuccessor { s.append(.paragraphSeparator) }

    // Then we style of the list item (prefix included). Note that we pass
    // the length of the prefix.
    styler.style(item: s, prefixLength: (prefix as NSString).length)
    return s
}

// In the styler.
open func style(item str: NSMutableAttributedString, prefixLength: Int) {
    let paragraphRanges = str.paragraphRanges()

    guard let leadingParagraphRange = paragraphRanges.first else { return }

    // This is where we format the first paragraph of the list item (as there could be more than
    // one and they need to be handled slightly differently). Note, we pass in the prefix length here.
    indentListItemLeadingParagraph(in: str, prefixLength: prefixLength, inRange: leadingParagraphRange)

    paragraphRanges.dropFirst().forEach {
       indentListItemTrailingParagraph(in: str, inRange: $0)
    }
}

private func indentListItemLeadingParagraph(in str: NSMutableAttributedString, prefixLength: Int, inRange range: NSRange) {
    str.updateExistingAttributes(for: .paragraphStyle, in: range) { (existingStyle: NSParagraphStyle) in
        existingStyle.indented(by: itemParagraphStyler.indentation)
    }
    
    // Here we use the prefix length to get the prefix, then calculate its width.
    // The width is important for all prefix types, whether numbered or bullet.
    let attributedPrefix = str.prefix(with: prefixLength)
    let prefixWidth = attributedPrefix.size().width

    // This is where we do the paragraph styling
    let defaultStyle = itemParagraphStyler.leadingParagraphStyle(prefixWidth: prefixWidth)
    str.addAttributeInMissingRanges(for: .paragraphStyle, value: defaultStyle, within: range)
}

// In list item paragraph styler.
public func leadingParagraphStyle(prefixWidth: CGFloat) -> NSParagraphStyle {
    // This is how much the content should be indented.
    // By default we allow a max prefix width to fit 2 monospace digits. 
    // Then we add the post prefix spacing, this results in the content indentation.
    let contentIndentation = indentation

    // This is the amount to push the prefix so that its trailing edge sits flush at the prefix line (i.e the line
    // you see all other prefixes right align to).
    let prefixIndentation: CGFloat = contentIndentation - options.spacingAfterPrefix - prefixWidth

    // If the current prefix is larger that the our 2 monospace digit allowance, then we have spill.
    // This means we need to push the content a bit more so that there is equal spacing between the
    // prefix and the content.
    let prefixSpill = max(0, prefixWidth - largestPrefixWidth)
    let firstLineContentIndentation = contentIndentation + prefixSpill

    let style = baseStyle

    // The amount to push the prefix so it sits flush against the prefix line.
    style.firstLineHeadIndent = prefixIndentation

    // Specifying where the first tab should end (remember, we inserted a tab after the prefix
    // back in the attributed string visitor). This results in pushing everything after the tab (i.e the content).
    style.tabStops = [tabStop(at: firstLineContentIndentation)]
    
    // This is the indentation for all subsequent lines so it aligns with the content in the
    // fist line.
    style.headIndent = contentIndentation

    return style
}

I hope that clarifies how it works. So the ListItemParagraphStyler doesn't care about what kind of list item its. The only thing that it needs to know is how wide that prefix is. The width of the prefix is everything before the inserted tab.

@mrackwitz
Copy link
Author

About the indentation issue: which font are you using? I'll try to reproduce it on my side.

I'm using a proprietary font, Edmondsans.

It could be that this trick doesn't really work for UILabels. To be honest, I've always used a text view to render markdown. Can you try using the DownTextView to see if it solves the problem for you?

I did, but it's not working in my case as I'm using it from SwiftUI. I've to jump thru quite some hoops to make it render performant in my particular case, so I've some caching and mostly bypass the label's auto-layouting capabilities, also I have some custom logic to make hyperlinks work and have their tap recognition play nicely with SwiftUI gestures. When I'm thinking about this might all work as well with a UITextView, so I might just try that – or the other way around see if I can reproduce the issue with an UILabel from the test suite.

But in my research around whether this is generally possible or not, I did see a few examples of setting up bullet point lists with the technique you've illustrated. So I guess it should work?

You did write so one keyword in your considered alternative "using the layout manager." That's likely to break UILabel's, as UILabel doesn't expose an NSLayoutManager or really anything else about the text rendering.

I'm pasting a bunch of code, but you only need to look at the lines that I've commented.

Woah, this is great and super helpful to see all the pieces together! Thank you for doing that! 🤩 I'd have to take a little more time when I'm more awake to study this in more detail.

I hope that clarifies how it works. So the ListItemParagraphStyler doesn't care about what kind of list item its. The only thing that it needs to know is how wide that prefix is. The width of the prefix is everything before the inserted tab.

Okay, that's what I thought how it would work and now I can also see how it considers the bullet point width. 😄

@johnxnguyen
Copy link
Owner

I've only experimented a bit with SwiftUI so I'm curious what are the performance concerns of wrapping a UITextView into a UIViewRepresentable?

@johnxnguyen
Copy link
Owner

Closing due to inactivity.

benblakely added a commit to benblakely/Down that referenced this issue May 5, 2021
Insert a tab before the list item prefix (in addition to the tab already inserted after the list item prefix) so that we can set one tab stop for right aligning the list item prefixes and another tab stop for left aligning the list item content.

Resolves johnxnguyen#246
@gsbernstein
Copy link

Was there any resolution to this? @brblakley I saw you made your commit on a fork, did you make a PR into this repo?

We're considering switching libraries due to this bug.

@gsbernstein
Copy link

lists
two examples on the same screen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants