Skip to content
This repository has been archived by the owner on Sep 27, 2024. It is now read-only.

iOS: Replace NBSP with ZWSP to allow block rendering #520

Merged
merged 12 commits into from
Jan 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,70 @@ extension DTHTMLElement {
}
}
}

func clearTrailingAndLeadingNewlinesInCodeblocks() {
guard let childNodes = childNodes as? [DTHTMLElement] else {
return
}

if name == "pre",
childNodes.count == 1,
let child = childNodes.first as? DTTextHTMLElement,
var text = child.text(),
text != .nbsp {
var leadingDiscardableElement: DiscardableTextHTMLElement?
var trailingDiscardableElement: DiscardableTextHTMLElement?
var shouldReplaceNodes = false

if text.hasPrefix("\(Character.nbsp)") {
shouldReplaceNodes = true
text.removeFirst()
leadingDiscardableElement = createDiscardableElement()
}

if text.hasSuffix("\(Character.nbsp)") {
shouldReplaceNodes = true
text.removeLast()
trailingDiscardableElement = createDiscardableElement()
}

if shouldReplaceNodes {
Comment on lines +49 to +65
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this could be replaced by

let hasLeadingNbsp = text.hasPrefix(...)
let hasTrailingNbsp = text.hasSuffix(...)

if hasLeadingNbsp || hasTrailingNbsp {
    removeAllChildNodes() 
    
    // Prepare nodes and push them, we only need to create the new nodes at this point
    // Don't need to work with these optionals.
    [...] 
}

removeAllChildNodes()

if let leadingDiscardableElement = leadingDiscardableElement {
addChildNode(leadingDiscardableElement)
addChildNode(createLineBreak())
}

let newTextNode = DTTextHTMLElement()
newTextNode.inheritAttributes(from: self)
newTextNode.interpretAttributes()
newTextNode.setText(text)
addChildNode(newTextNode)
Comment on lines +73 to +77
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you might be able to re-use the same text node after detaching it instead of creating a new one (no need to re-inherit/interpret attributes then)


if let trailingDiscardableElement = trailingDiscardableElement {
addChildNode(createLineBreak())
addChildNode(trailingDiscardableElement)
}
}
} else {
for childNode in childNodes {
childNode.clearTrailingAndLeadingNewlinesInCodeblocks()
}
}
}

private func createDiscardableElement() -> DiscardableTextHTMLElement {
let discardableElement = DiscardableTextHTMLElement()
discardableElement.inheritAttributes(from: self)
discardableElement.interpretAttributes()
return discardableElement
}

private func createLineBreak() -> DTBreakHTMLElement {
let lineBreakElement = DTBreakHTMLElement()
lineBreakElement.inheritAttributes(from: self)
lineBreakElement.interpretAttributes()
return lineBreakElement
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ final class DiscardableTextHTMLElement: DTTextHTMLElement {
setText(textNode.text())
}

override init() {
super.init()
setText(.nbsp)
}

override func attributesForAttributedStringRepresentation() -> [AnyHashable: Any]! {
var dict = super.attributesForAttributedStringRepresentation() ?? [AnyHashable: Any]()
// Insert a key to mark this as discardable post-parsing.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,20 @@ extension NSAttributedString {
return ranges
}

func discardableTextRanges(in range: NSRange? = nil) -> [NSRange] {
let enumRange = range ?? .init(location: 0, length: length)
var ranges = [NSRange]()

enumerateAttribute(.discardableText,
in: enumRange) { (attr: Any?, range: NSRange, _) in
if attr != nil {
ranges.append(range)
}
}

return ranges
}

/// Computes index inside the HTML raw text from the index
/// inside the attributed representation.
///
Expand All @@ -95,9 +109,17 @@ extension NSAttributedString {
.outOfBoundsAttributedIndex(index: attributedIndex)
}

let discardableTextRanges = discardableTextRanges()
var actualIndex = attributedIndex

for discardableTextRange in discardableTextRanges {
if discardableTextRange.upperBound <= attributedIndex {
actualIndex -= discardableTextRange.length
}
}
Comment on lines +115 to +119
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think this raises a Swiftlint warning, I'm guessing we can e.g. write something like:

actualIndex -= discardableTextRanges
    .filter { $0.upperBound <= attributedIndex }
    .reduce(0) { $1 + $2.length }


let prefixes = listPrefixesRanges()
var actualIndex: Int = attributedIndex


for listPrefix in prefixes {
if listPrefix.upperBound <= attributedIndex {
actualIndex -= listPrefix.length
Expand All @@ -117,8 +139,16 @@ extension NSAttributedString {
/// - htmlIndex: the index inside the HTML raw text
/// - Returns: the index inside the attributed representation
func attributedPosition(at htmlIndex: Int) throws -> Int {
let discardableTextRanges = discardableTextRanges()
var actualIndex = htmlIndex

for discardableTextRange in discardableTextRanges {
if discardableTextRange.location < actualIndex {
actualIndex += discardableTextRange.length
}
}
Comment on lines +145 to +149
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

but more generally I'm sure we can globally improve the code here, and perhaps merge list prefixes and discardable text into the same Array so we don't have multiple arrays that achieve same kind of computation

Copy link
Member Author

@Velin92 Velin92 Jan 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is actually something missing in my mapping, because when I delete the last character in the last newline, the rendering is lost for that line, I think that I need just like for the list, create some kind of exception based on the cursor position, because deleting the last character probably is also deleting the ZWSP character and removing the rendering, would be nice to be able to keep it, and removing it alongside the newline

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah not sure what happens in that case, technically on backspace we do replace the HTML so we should be re-parsing and adding that ZWSP.


let prefixes = listPrefixesRanges()
var actualIndex: Int = htmlIndex

for listPrefix in prefixes {
if listPrefix.location < actualIndex {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,18 @@ extension NSMutableAttributedString {
}
}

/// Removes any text that has been marked as discardable.
func removeDiscardableText() {
/// Finds any text that has been marked as discardable
/// and either replaces it with ZWSP if contained overlaps with text marked with a background style
/// or removes it otherwise
func replaceOrDeleteDiscardableText() {
enumerateTypedAttribute(.discardableText) { (discardable: Bool, range: NSRange, _) in
guard discardable == true else { return }

self.deleteCharacters(in: range)
let attributes = self.attributes(at: range.location, effectiveRange: nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can directly get the backgroundStyle alone here and avoid looking up the entire dictionary ourselves

if attributes[.backgroundStyle] != nil {
self.replaceCharacters(in: range, with: String.zwsp)
} else {
self.deleteCharacters(in: range)
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@

extension String {
static let nbsp = "\(Character.nbsp)"
static let zwsp = "\(Character.zwsp)"
}

public extension Character {
static let nbsp = Character("\u{00A0}")
static let zwsp = Character("\u{200B}")
}
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ public final class HTMLParser {
// Removing NBSP character from <p>&nbsp;</p> since it is only used to
// make DTCoreText able to easily parse new lines.
element.clearNbspNodes()
element.clearTrailingAndLeadingNewlinesInCodeblocks()
}

guard let attributedString = builder.generatedAttributedString() else {
Expand All @@ -114,7 +115,7 @@ public final class HTMLParser {

mutableAttributedString.applyBackgroundStyles(style: style)
mutableAttributedString.applyInlineCodeBackgroundStyle(codeBackgroundColor: style.codeBackgroundColor)
mutableAttributedString.removeDiscardableText()
mutableAttributedString.replaceOrDeleteDiscardableText()
mutableAttributedString.removeParagraphVerticalSpacing()

removeTrailingNewlineIfNeeded(from: mutableAttributedString, given: html)
Expand All @@ -124,14 +125,17 @@ public final class HTMLParser {
private static func removeTrailingNewlineIfNeeded(from mutableAttributedString: NSMutableAttributedString, given html: String) {
// DTCoreText always adds a \n at the end of the document, which we need to remove
// however it does not add it if </code> </a> are the last nodes.
// Also we don't want to remove it if a codeblock contains that newline
// and is not empty, because DTCoreText does not add a newline if these blocks
// contain one at the end.
// It should give also issues with codeblock and blockquote when they contain newlines
// but the usage of nbsp and zwsp solves that
if mutableAttributedString.string.last == "\n",
!html.hasSuffix("</code>"),
!html.hasSuffix("</a>"),
!html.hasSuffix("\n</pre>") {
mutableAttributedString.deleteCharacters(in: NSRange(location: mutableAttributedString.length - 1, length: 1))
!html.hasSuffix("</a>") {
mutableAttributedString.deleteCharacters(
in: NSRange(
location: mutableAttributedString.length - 1,
length: 1
)
)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ private struct StringDiff {
private extension String {
/// Converts all whitespaces to NBSP to avoid diffs caused by HTML translations.
var withNBSP: String {
String(map { $0.isWhitespace ? Character.nbsp : $0 })
String(map { $0.isWhitespace ? Character.nbsp : $0 }).trimmingCharacters(in: .whitespacesAndNewlines)
}

/// Computes the diff from provided string to self. Outputs UTF16 locations and lengths.
Expand Down