React-native-windows: Reconcile AppTheme's high contrast APIs with Lean Core Appearance APIs

[kikisaints]

Align AppTheme API with Lean Core Appearance API

We have a Windows-only API module called AppTheme. Which enables you to listen to the dark and light theme state changes for Windows.

As of this commit, React Native core has an iOS and Android ones. It operates very similarly to how ours does, but there are some API name changes.

This proposal is to align the names with the Lean Core implementation, so that we can push up the Windows theme changed event work up to React Native to sit along side the iOS and Android implementations.

Summary

We must make sure our Windows implementation of the theme changed event maps to the React Native Appearance and the getColorScheme() function, instead of AppTheme and the currentTheme string property.

We should also separately PR the highContrastedChanged work into the Appearance API to enable support for that type of theme change.

Motivation

In order for our Windows implementation of theme changed events to work properly in React Native's lean core, we must align with their implementation of theme changed events.

This Windows alignment is happening on the JS-layer only.

The JS API to align with can be found in the Libraries/Utilities/Appearance.js file on the React Native repo.

Basic example

import { Appearance } from 'react-native'

  render() {
    return (
      <Button title='click me' color={Appearance.getColorScheme() === 'dark' ? 'grey' : 'orange'}></Button>
    );
  }

Contributing Back

As mentioned above, it's important that during this alignment that we do not lose the highContrastedChanged implemented within AppTheme. This is an important feature for windows users and something we should contribute back to React Native core with.

[chrisglein]

We should align our API to the one coming so we don't incur a breaking change later.

[kikisaints]

@chrisglein that was the intent behind this issue - is that not clear?

[chrisglein]

@chrisglein that was the intent behind this issue - is that not clear?

@kikisaints Looking at it now, yes. Not sure why we felt the need to restate that while we were going through triage last week :)

[ddalp]

@kikisaints @chrisglein Few questions about this issue:

1. Do we need to completely remove AppTheme and replace it with Appearance? Is there risk of breaking App-Compat?
2. How about highContrastChanged event in current AppTheme module? This does not have corresponding concept/event in Appearance.

[kikisaints]

@kikisaints @chrisglein Few questions about this issue:

1. Do we need to completely remove AppTheme and replace it with Appearance? Is there risk of breaking App-Compat?
2. How about highContrastChanged event in current AppTheme module? This does not have corresponding concept/event in Appearance.

There may have to be some breaking on back-compat when we updated to use Appearance instead of AppTheme.

for the highContrastChanged - I like it, I'll add it to this issue spec. I think it would be a great thing to contribute back to the community too - @chrisglein thoughts?

[chrisglein]

@kikisaints contributing that back sounds like a great idea, although that sounds like we'd want to pair that with changing over from AppTheme to Appearance so that we don't leave a functionality gap, correct? We currently have this tagged to complete this month so that we can tie it to the vnext/61 roll-out. As a breaking change it'd be good to tie to a version number bump, but we're also not going to get to all lean core moves in 61 (there's always 62...). Does this seem high priority enough to push for this month?

[kikisaints]

I would call it a P1/2. It's not the top of the list, but decently important to have for developers coming from the windows side wishing to use React Native to target UWP.

[ddalp]

Moving this issue to M5 as the FaceBook RN change is targeted for v0.62.0-rc.0.

[chrisglein]

Note that in #4322 the AppearanceModule is implemented. But we still need to reconcile that with what we already had. See notes in that issue.

[chrisglein]

High contrast support doesn't existing in Appearance, so we need to keep AppTheme around temporarily just for that, until we upstream it somehow.

https://microsoft.github.io/react-native-windows/docs/apptheme-api

• getCurrentTheme()
• isHighContrast()
• currentHighContrastColors()
• appThemeChanged()
• highContrastChanged()

Sounds like we get rid of getCurrentTheme() and appThemeChanged(). But the rest remain.
The docs already say “this thing is deprecated”, which will be mostly true.

Also need to update this doc:
https://microsoft.github.io/react-native-windows/docs/windowsbrush-and-theme

[NickGerleman]

Moving issues to 0.64 since Chris is out this week, and we're in the last week of stabilizing 0.63. This seems like major feature/design work that shouldn't block the release. Please let me know if you disagree.