iOS FRNAppearanceAdditions: add support for multiple trait collections #2675

lyzhan7 · 2023-03-02T01:21:20Z

Platforms Impacted

Description of changes

Before this change, the FRNAppearanceAdditions native module assumed that there was only one window/root view at a time, i.e. one trait collection at a time. This assumption doesn't hold for iPad multiwindow scenarios where there can be more than one window at a time, each having different trait collections.

In order to get the native module working with multiple trait collections, the size class/user interface level/accessibility contrast option properties in the native module (both the JS side and the native side) needed to be refactored so we can make each property specific to a given window/root view- this was done by replacing the properties with dictionaries. All the methods between JS/native were refactored accordingly.

One of the main decisions made in this PR was determining what the key of the dictionaries should be. It needed to be something that both the native and JS side of the native module could figure out independently:

At boot, the the native side (objective c code) gets initialized before the JS side is initialized. Without knowing anything about the JS side, the native side needs to be able to initialize the trait collection dictionaries properly
At runtime, FURN sometimes makes requests for certain trait collections, qne in order to make those requests FURN needs to know which root view/window to ask the native side for.

The react native rootTag identifier returns the react tag of the root view. I found that this is accessible from the native side by going through a window's rootViewController's view's reactTag. More documentation about rootTag here

I considered attempting to making the native module stateless, but decided against it because we would end up firing notifications for any trait collection change in any window, including ones we don't care about.

Notes:

Technically only the size class and user interface level traits need dictionaries - accessibilityContrast is a system wide setting, so every value in that dictionary will always be the same.
I ended up creating a dictionary per trait on both js/native side of the native module, but another option is to make a single dictionary
From the rootTag documentation

With the new React Native architecture progressing, there will be future iterations to RootTag, with the intention to keep the RootTag type opaque and prevent thrash in React Native codebases. Please do not rely on the fact that RootTag currently aliases to a number! If your app relies on RootTags, keep an eye on our version change logs, which you can find here.
- I think I've added some dependencies on rootTag being a number, I'll think about ways to not depend on this (maybe just changing the type to be more generic) but not sure how much

State of the traits we support:

Accessibility contrast (normal vs. high)
1. Already worked, since it is a system wide setting/not specific to a window
Appearance size class (compact vs. regular)
1. Requires support for multiple trait collections - now works after this PR. See before & after videos below
User interface level (base vs. elevated)
1. Requires support for multiple trait collections (this PR)
2. Also requires support for multiple themes (out of scope for this PR)

Verification

Before (can't have one side be compact mode, other side be regular mode - both windows will render the same way, which means that one side is sometimes incorrect)

multitraits_before.mov

After (each window will render in compact more or regular mode correctly - both at boot, and at runtime)

multitraitcollection_after.mov

Pull request checklist

This PR has considered (when applicable):

packages/components/Notification/src/Notification.tsx

Saadnajmi · 2023-03-02T02:37:26Z

packages/experimental/AppearanceAdditions/ios/FRNAppearanceAdditions.m

-            _userInterfaceLevel = userInterfaceLevel;
-            _accessibilityContrastOption = accessibilityContrastOption;
+
+        NSNumber *rootTag = [[[notification object] contentView] reactTag];


I think we should guard that the selector exists here, or else we might get a runtime crash.

I prefer class verification, since it verifies that the selectors will also return the types you think (while pure duck-typed selector sniffing does not):

if (![[notification object] isKindOfClass:[RCTRootView class]]) { return; } RCTRootView *rootView = [notification object]; NSNumber *rootTag = [rootView rootTag];

Just updated - I thought that the verification for trait collection and rootTag should be the same, so I also changed the behaviour so that if the verification for trait collection fails we just return as well.

Some questions:

I'm unclear what's happening in the UITraitCollection *traitCollection = [[notification userInfo] valueForKey:...] line. At this point, if the right side doesn't return a UITraitCollection pointer, shouldn't this cause some kind of error instead of at the check on the next line where we explicitly check the class? Thinking about it more I would have expected this to result in a compiler error. Testing locally, if I change UITraitCollection *traitCollection to something clearly wrong like UIView *traitCollection everything actually still works

I'm wondering if we should also check that contentView and reactTag are both non-nil

Saadnajmi · 2023-03-02T02:37:55Z

packages/experimental/AppearanceAdditions/src/NativeAppearanceAdditions.ts

-    return 'regular' as SizeClass;
+    return {};
  },
  userInterfaceLevel: () => {
    console.warn('NativeAppearanceAdditions is only available on iOS');
-    return 'base' as UserInterfaceLevel;
+    return {};
  },
  accessibilityContrastOption: () => {
    console.warn('NativeAppearanceAdditions is only available on iOS');
-    return 'normal' as AccessibilityContrastOption;
+    return {};


Why these changes?

Hmm so this file is just stubs for the other platforms. These methods changed from returning strings to returning dictionaries, so I changed it to return an empty dictionary. I'll double check on macOS to make sure other platforms aren't affected

Tested by just running the macOS tester and don't see any issues. I realized on macOS though we don't seem to support multiple windows either...at least I haven't figure out how to open another FluentTester window. Will look into what's needed to support this

Saadnajmi · 2023-03-02T02:39:14Z

packages/experimental/AppearanceAdditions/src/NativeAppearanceAdditions.types.ts

+  horizontalSizeClassForRootTag(rootTag: number): SizeClass;
+  userInterfaceLevelForRootTag(rootTag: number): UserInterfaceLevel;
+  accessibilityContrastOptionForRootTag(rootTag: number): AccessibilityContrastOption;


The docs say not to rely on Root tag being a number. Can we use a different type here? Something more specific that comes from the RN Types?
https://reactnative.dev/docs/roottag#future-plans

Yeah I had put this as a concern in the description. Can you explain more what you mean by "something more specific that comes from the RN Types"? I had actually thought I would need to make the type more generic

Use the RootTag type from Typescript: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react-native/Libraries/ReactNative/RootTag.d.ts

Right now that's just a number... but in the future if that becomes, let's say.. a string, then our JS code would still run.

I also left a comment of what we can use inside our Objective-C type for the same purpose, in the absence of a formal type from RNCore (id<NSCopying>).

Thanks Saad - didn't realize there was type in React Native I could use. Just updated to use import type { RootTag } from 'react-native'; - let me know if this is what you were thinking. The link you posted is from "DefinitelyTyped" - were you expecting the import to be from that?

~~@mischreiber I tried id but got this error when I tried to use it~~ never mind realized I needed to also get rid of the *

Saadnajmi

Random thought: Are we sure root tags are guaranteed to be unique? And what happens when you, say, close one window, and create a new one? Will the old root tag (and it's map of values) clash with the newly created window?

packages/experimental/AppearanceAdditions/ios/FRNAppearanceAdditions.m

mischreiber · 2023-03-06T20:56:18Z

packages/experimental/AppearanceAdditions/src/appearanceAdditions.ios.ts

    });
  }
+
+  horizontalSizeClassForRootTag(rootTag: number): SizeClass {


Will these functions ever be called before the eventEmitter above has been invoked? If so, what are they returning?

Hmm I don't think that can happen since those functions need to be called on an instance of AppearanceAdditions- so the constructor which initializes the eventEmitter would always be called first. Doing a very brief check, this seems to be the case. Were you thinking of a specific scenario?

lyzhan7 · 2023-03-09T20:54:51Z

Random thought: Are we sure root tags are guaranteed to be unique? And what happens when you, say, close one window, and create a new one? Will the old root tag (and it's map of values) clash with the newly created window?

Update on what I'm currently looking into: so when I tried to double check what was happening here I started noticing some other strange behavior. I thought that the native module was a singleton, and it would only be created once during the whole duration of the app - which I thought was while at least one window was open. I actually think now that every FluentTester window that gets opened will create a new FRNAppearanceAdditions instance. The same notification gets sent for every window that's open. Still trying to think through what this means for this PR.

At some point I thought that this PR wouldn't work as expected when opening new windows, since it didn't look like we were updating the native dictionary anywhere - but I realized when new windows are opened, we actually do update the dictionary once appearanceChanged is called from that window. While testing, opening new windows does work as expected (with caveats described above ex. extra notifications sent)

I also realized, if a user closes a window, we never remove the dictionary entry. I'm not sure yet if this is a problem - it actually seems like once any window is closed, the native module gets reset...still looking into this.

Also the PR checks are failing because one of the places where we call the native module is in createAppleTheme, but at that point we don't have a view to get the rootTag from. I'm trying to figure out what the best thing to do there is, maybe just return the first entry in the dictionary.

Overall, I think I need to come up with a more detailed test plan/set of test cases that should work as expected. Might need to take a step back and draw up a more detailed engineering plan.

lyzhan7 · 2023-03-13T19:13:55Z

Closing this PR in favor of #2697

The approach in this PR was made under the assumption that all native modules, including FRNAppearanceAdditions, are singletons - turns out this is not true. There is one native module created per bridge, and there is one bridge per scene. So every scene has its own instance of FRNAppearanceAdditions - which means we shouldn't need to maintain a dictionary of roottags to trait collections, we just need to modify the existing behavior so that each instance of FRNAppearanceAdditions communicates the right information to the JS side of each scene.

lyzhan7 added 4 commits March 1, 2023 16:43

Support multiple trait collections on native side

ade150b

Support multiple trait collections on js side

55135d1

Update stubs for non-ios

4369133

Update usages of size class to pass in root tag

669796f

lyzhan7 requested a review from a team as a code owner March 2, 2023 01:21

Change files

f644ad2

Saadnajmi reviewed Mar 2, 2023

View reviewed changes

packages/components/Notification/src/Notification.tsx Outdated Show resolved Hide resolved

Saadnajmi reviewed Mar 2, 2023

View reviewed changes

mischreiber reviewed Mar 6, 2023

View reviewed changes

packages/experimental/AppearanceAdditions/ios/FRNAppearanceAdditions.m Outdated Show resolved Hide resolved

mischreiber reviewed Mar 6, 2023

View reviewed changes

packages/experimental/AppearanceAdditions/ios/FRNAppearanceAdditions.m Outdated Show resolved Hide resolved

mischreiber reviewed Mar 6, 2023

View reviewed changes

lyzhan7 added 7 commits March 8, 2023 12:39

Rename some trait collection methods to reflect change to return a map

c2fe804

Move rootTag context call into useHorizontalSizeClass hook

630d97b

Merge branch 'main' into support-multiple-trait-collections

708f9d1

Add class verification for rootview

042c058

Update rootTag type from number to RootTag

3ee6ac4

Move curly braces up (not on new line)

98349f7

Replace type of rootTag, from NSNumber to id<NSCopying>

487275a

lyzhan7 marked this pull request as draft March 10, 2023 20:26

lyzhan7 changed the title ~~iOS FRNAppearanceAdditions native module: add support for multiple trait collections~~ iOS FRNAppearanceAdditions: add support for multiple trait collections Mar 10, 2023

lyzhan7 closed this Mar 13, 2023

lyzhan7 deleted the support-multiple-trait-collections branch March 15, 2023 17:27

lyzhan7 mentioned this pull request Mar 29, 2023

iOS FRNAppearanceAdditions: support for multiple trait collections #2697

Merged

10 tasks

iOS FRNAppearanceAdditions: add support for multiple trait collections #2675

iOS FRNAppearanceAdditions: add support for multiple trait collections #2675

Uh oh!

Conversation

lyzhan7 commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Platforms Impacted

Description of changes

Verification

Pull request checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyzhan7 Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Saadnajmi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyzhan7 commented Mar 9, 2023

Uh oh!

lyzhan7 commented Mar 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lyzhan7 commented Mar 2, 2023 •

edited

Loading

lyzhan7 Mar 8, 2023 •

edited

Loading