danielsaidi / RichTextKit

RichTextKit is a Swift SDK that helps you use rich text in Swift and SwiftUI.
MIT License
944 stars 126 forks source link

Setting rich text style on empty range causes buggy behaviour on macOS (see video) #130

Open DominikBucher12 opened 10 months ago

DominikBucher12 commented 10 months ago

This is caused by different behaviours of var richTextAttributes: RichTextAttributes inside RichTextViewComponent+Attributes.swift There are two fixes I came up with:

  1. in func setStyle(_ style: RichTextStyle, to newValue: Bool) inside RichTextCoordinator, replace the function with #ifdef like this:
func setStyle(_ style: RichTextStyle, to newValue: Bool) {
        let hasStyle = textView.richTextStyles.hasStyle(style)

        #if macOS
        // If we set value to true, we expect the style not to contain it.
        // The other way around, this doesnt quite work on macOS since we do not count in typing attributes.
        if newValue == hasStyle && newValue == true { return }
        #else
        if newValue == hasStyle { return }
        #endif

        textView.setRichTextStyle(style, to: newValue)
    }

This forces to toggle style on macOS no matter what. This feels a bit like hack to me.

  1. var richTextAttributes: RichTextAttributes behaves differently on macOS, we should either return typingAttributes when the richTextAttributes are empty or treat this the same as on iOS since there is no issue.

Note: It looks like none of the fixes above 100% works :D

https://github.com/danielsaidi/RichTextKit/assets/17381941/68e0f451-893c-47a5-b2f0-dcfbff8cbeee

DominikBucher12 commented 10 months ago

Btw you can reproduce this bug all way to 0.9.0 (didn't dare to go further) where highlighting of selected styles doesnt even work 😅 So it has been there for quite a while... I will fix this ASAP but I need to merge https://github.com/danielsaidi/RichTextKit/pull/129 and subscriptions, so patience please 🙏🏻

danielsaidi commented 10 months ago

Nice find! It seems to be something with the underline and strikethrough, which are handled differently than bold and italic:

func setRichTextStyle(
        _ style: RichTextStyle,
        to newValue: Bool
    ) {
        let value = newValue ? 1 : 0
        switch style {
        case .bold, .italic:
            let styles = richTextStyles
            guard shouldAddOrRemove(style, newValue, given: styles) else { return }
            guard let font = richTextFont else { return }
            guard let newFont = newFont(for: font, byToggling: style) else { return }
            setRichTextFont(newFont)
        case .underlined:
            setRichTextAttribute(.underlineStyle, to: value)
        case .strikethrough:
            setRichTextAttribute(.strikethroughStyle, to: value)
        }
    }

The code for setRichTextAttribute looks like this:

func setRichTextAttribute(
        _ attribute: RichTextAttribute,
        to value: Any
    ) {
        #if macOS
        setRichTextAttribute(attribute, to: value, at: selectedRange)
        typingAttributes[attribute] = value
        #else
        if hasSelectedRange {
            setRichTextAttribute(attribute, to: value, at: selectedRange)
        } else {
            typingAttributes[attribute] = value
        }
        #endif
    }

BUT it works the same, even if I remove the platform check:

func setRichTextAttribute(
        _ attribute: RichTextAttribute,
        to value: Any
    ) {
        if hasSelectedRange {
            setRichTextAttribute(attribute, to: value, at: selectedRange)
        } else {
            typingAttributes[attribute] = value
        }
    }

The behavior is however still the same for strikethrough and underline, while colors (which also use this), work great.

I'll push the platform check removal, to make this easier to fix later on.

DominikBucher12 commented 10 months ago

IMHO this will affect (or has affected in past) :D even colors and everything, I had this issue before I think but with colors but I am not sure... I think this will be in setting/unsetting Attributes, but with one fix, we can rule them all :D