introduce gracefully degrading abstractions for dark mode for ios and…

[motocodeltd]

… macos and use them to draw text

Issue Link 🔗

#4133
#3742

Goals ⚽

Introduce dark mode for label and gracefully degrade to black for the older operating systems

Implementation Details 🚧

Introduce labelOrBlack in NSUIColor. Set it to either NSUIColor.label or black depending on os versions. use it in charts

Testing Details 🔍

decided against it as it's colours that in some cases are not configurable and hence easily testable

[liuxuan30]

hmm, I need some background here.

I'm feeling the ChartsDemo is just a demo. But you guys want the library itself, to support dark mode? More specific, only the label supporting dark mode, or all default colors?

Besides, I don't think we should support backward compatibility, since it's only started on iOS13/macOS 10.14?

[motocodeltd]

Hi,

I'll have as look at the macos failures when I get home (it built fine for me :-)

With the current release version of Charts, when displaying a chart in a dark mode app, the chart itself is perfectly visible (and dark) but all labels and legend etc isn't because they are dark text on top of a dark background. This is solved in iOS13 and Mojave by providing a colour called label, that automatically changes depending on if we're in dark mode or light mode.

The graceful degradation is required because {UIColor/NSColor}.label is only available on mojave, ios13 and not before.

I'll also attach a before and after screenshot later on.

[motocodeltd]

I'm attaching a couple of screenshots of our app, CycleMaps. Both are of iPhones (simulator) running iOS13 in dark mode. The first one shows the chart without the patch, the second one with the patch. They're using a line chart provided by the Charts library (thanks!). As you can see, without the patch the axis and legend are invinsible.

[liuxuan30]

ah amazing. I will take a deep look at this topic and review. The change seems pretty straight forward to merge, so if nothing special likely it can be merged soon.

[motocodeltd]

that's great, thanks

[liuxuan30]

@motocodeltd the change is fine to me. Just a quick question, you mentioned

when displaying a chart in a dark mode app, the chart itself is perfectly visible (and dark)

In my understanding, the current chart color/background color setting like

    internal func initialize()
    {
        #if os(iOS)
        self.backgroundColor = NSUIColor.clear
        #endif

is not getting in your way right? So only the text color need update, the rest of colors are pure user customization.

[liuxuan30]

@jjatie any feedback? If nothing special I would merge this week, and maybe a minor 3.4.1 release for dark mode

[jjatie]

I think this is a much more elegant solution as I mentioned in #3766:

extension UIColor {
    @available(iOS, introduced: 9.0, obsoleted: 13.0)
    @available(tvOS, introduced: 9.0, obsoleted: 13.0)
    static var label: UIColor { .black }
}

and doing the same for NSColor

extension NSColor {
    @available(macOS, introduced: 10.11, obsoleted: 10.14)
    static var label: UIColor { .black }
}

We can simply delete this code altogether without changing anything else once Charts' deployment target reaches the appropriate versions.

@liuxuan30 Note the introduced is only as far back as the framework supports

[motocodeltd]

Hi @liuxuan30 - yes, clear works just fine for me, apologies for the delay.

[motocodeltd]

@jjatie this unfortunately does not work for me (I tried it) on my iOS13 devices. The colour comes back as .black.

[liuxuan30]

I tested #4175, seems not working for axis labels, @jjatie

I think we'd either merge this or #4175, but given that there are more code refactor coming after #4175, I'm suggesting fixing #4175 either like this PR, or we fix the @available(tvOS, introduced: 9.0, obsoleted: 13.0) and merge #4175.

[motocodeltd]

@liuxuan30 @jjatie
The solution in #3766 does not work for me.

extension UIColor {
    @available(iOS, introduced: 9.0, obsoleted: 13.0)
    @available(tvOS, introduced: 9.0, obsoleted: 13.0)
    static var label: UIColor { .black }
}

The snippet above always gives me black on iOS13 devices and simulators, which is why I implemented it differently in this PR. In terms of elegance, I'm personally not a big fan of extensions either because they make finding out what the API is slightly harder, but this is more of a matter of personal preference.

[liuxuan30]

I'm feeling it's Apple's bug? I have filed a radar today to see what's their opinion.

[motocodeltd]

Hi @liuxuan30 - thanks for this. I'm wondering if it's worth merging this, because it gives dark mode in iOS13 and macos mojave & catalina. It can be refactored if and when Apple respond and also allows us not to depend on a branch too :-)

[liuxuan30]

@motocodeltd thanks for your patience. I think both PR is ok. But I just want to hear from @jjatie if we merge this, what would be for the following PRs from him, because they are based on #4175.
However seems @jjatie is busy right now. So have to wait :( sorry.

If we merged this and we fixed @available and refactored again, it seems a waste?