Fix override user interface style

[IvanChan]

Just move https://github.com/TextureGroup/Texture/pull/1798 here For further detail & discuss check out the old mr

Before

UIView & UIViewController both has a callback when traitCollection changed: - (void)traitCollectionDidChange:(UITraitCollection *)previousTraitCollection

ASDK depends on it to switch between different user interface style

Let's see why ASViewController works

When u r using UIViewController Case 0:

1. init a UIViewController
2. set UIViewController overrideUserInterfaceStyle
3. You get callback from UIViewController that traitCollectionDidChange

When overrideUserInterfaceStyle is set, UIViewController will get this callback , thus ASViewController get a chance to update TraitCollection to right the one

Also ASViewController update TraitCollection in several different callback like viewWillLayoutSubviews. So clearly ASViewController has more opportunity to sync TraitCollection from UIKit

But we just miss that chance when just using Node instead of ViewController

Actually ASViewController also has same problem

If you check out Demo22.zip

You will find out problem still there even though using ASViewController, when ViewController has child viewController, user interface style is NOT pass through

And why UIKit causing this problem

When you r using UIView Case 1:

1. init a UIView
2. set UIView overrideUserInterfaceStyle
3. You get callback from UIView that traitCollectionDidChange

Case 2:

1. init a UIViewController & set UIViewController overrideUserInterfaceStyle
2. add a UIView to UIViewController
3. This UIView won‘t get the same callback, deep inside UIView (I don't know whether is apple's bug), UIView just take it as initial value, no callback of traitCollectionDidChange

We now are facing Case 2, and got to find a way to sync TraitCollection from its initial value, because there is no callback tell you that TraitCollection is changed.

---

Now the problem as I described at the beginning:

ASDisplayNode init with userInterfaceStyle = .unspecific, and only update userInterfaceStyle when UIView call traitCollectionDidChange, it will miss UIView's default value of traitCollection.userInterfaceStyle

Now review what I did:

• Sync TraitCollection when node's view is loaded, make sure the initial value is correct
  • Ignored when superNode != nil, because we get other way to propagate from super to children
• Resolve color depends on TraitCollection to make sure we r using right color
  • Because a color var is independent, it just don't know who owned it & how to use it to render what, and not connected to owner's traitCollection, so it's loyal to system's traitCollection.

I don't think init Node's traitCollection with correct value is strange synchronization logic, otherwise, init with unspecific witch is NOT the same with it's view, that's strange.

I have tried to make minimum changes to fix it, and my project runs well for now. If there are better solutions, it would be great.

[IvanChan]

Review 1

UIColor *backgroundColor = _backgroundColor;
          if (@available(iOS 13.0, *)) {
             UITraitCollection *tempTraitCollection = [UITraitCollection traitCollectionWithUserInterfaceStyle:_primitiveTraitCollection.userInterfaceStyle];
             backgroundColor = [backgroundColor resolvedColorWithTraitCollection:tempTraitCollection];

Might be a stupid question, but why can't we resolve the color using _primitiveTraitCollection here? What makes it different from a new one created from its user interface style and why?

resolvedColorWithTraitCollection need a UITraitCollection class, I just create one from _primitiveTraitCollection, other case in ASImageNodeContentsKey only has UIUserInterfaceStyle

Review 2

    if ([self supernode] == nil) {

If I understand correctly, what you want to check here is whether this node is the root node within a view controller. If that's the case, you can simply check _flags.isViewControllerRoot here. This flag is set automatically by ASViewController. If you don't use ASViewController, you need to set it yourselves.

In some way, I use UIViewController and add subnodes later, these nodes will face the same problem, thus _flags.isViewControllerRoot might not be the best choice.

Change the ivar directly because I wanted less logic during _locked_loadViewOrLayer which I afraid there might have side effect for new code. And I debug it find out here is quite early like init, and this value updated will propagate down later.

setPrimitiveTraitCollection also works and it's little bit heavy I think.

Review 3

Essentially, the root view of the UIViewController has a different value rather than UIUserInterfaceStyleUnspecified and its node doesn't pick that up. That is a valid issue to me. There are 2 solutions I can think of: In your code, set the override interface style to the node's view right after it's loaded (i.e in -viewDidLoad).

I think it's unpractical, If I added 10 sub nodes to my UIViewController's View, I have to find them all out and set override interface style to them. Everytime & every VC that I used overrideInterfaceStyle

Review 4

Can you please confirm that the issue doesn't exist with UIViewController? That is, if the parent and child view controllers are UIViewControllers, the overrideUserInterfaceStyle of the parent VC will be passed through to the child VC and its view?

In demo, I added both UIView & ASDisplayNode， and UIKit just works well

Review 5

As for this pull request, I have a few asks: Please break the changes in this pull request into multiple ones. This pull request should only be about the issue described above. That way, it'll be easier for us to review, as well as manage after it's merged. Please consider adding a new unit test to ensure the issue won't occur again in the future. The steps you described above are very clear and the test can just follow them.

Sorry again for messing this merge request before. And I will check out the unit test code & find out how to add test code. Thanks alot.

@nguyenhuy

[IvanChan]

I debug these more, and found the reason

view-based node

If view is NOT loaded

ASPendingState.mm
- (void)applyToView:(UIView *)view withSpecialPropertiesHandling:(BOOL)specialPropertiesHandling {

  if (flags.setBackgroundColor) {
    view.backgroundColor = backgroundColor;
    layer.backgroundColor = backgroundColor.CGColor;
  }
}

If View is loaded

- (void)setBackgroundColor:(UIColor *)newBackgroundColor
{
...
      _view.backgroundColor = _backgroundColor;
      // Gather the CGColorRef from the view incase there are any changes it might apply to which CGColorRef is returned for dynamic colors
      _layer.backgroundColor = _view.backgroundColor.CGColor;
}

_layer is actually _view.layer, at this point, seting layer's backgroundColor is totally unnecessary when node is view-based rendering, it's find to just let UIKit do its work.

Debug it and found the magic

• In UIKit, when you set UIView.backgroundColor = .red
• UIView.layer.backgroundColor is still nil
• UIView.layer.content is a red color CATintedImage

So overriding the layer's backgroundColor would be the reason why ASDisplayNode has different color with UIView when overrideUserInterfaceStyle is set at the beginning.

So we can just remove 2 line code will fix view-based node's problem, but I don't know why needed them before.

layerbacked mode

Because we store backgroundColor in node-level and user feel free to set in order to change node's background color(actually is layer's background color), so layer should resolve color according to node's traitCollection.

If layer is NOT loaded

ASPendingState.mm
- (void)applyToLayer:(CALayer *)layer {
  if (flags.setBackgroundColor)
    layer.backgroundColor = backgroundColor.CGColor;
}

If Layer is loaded

- (void)setBackgroundColor:(UIColor *)newBackgroundColor
{
 if (_flags.layerBacked) {
      _layer.backgroundColor = _backgroundColor.CGColor;
    } 
}

So here should changed to resolved color with UIUserInterfaceStyle to make sure we are using the right color for rendering layer-backed node. Thus we should make sure we have the right trait collection.

• As long as we fix view-based node display, its children layer-backed node will display correctly also.
• If using layer-backed node without a view-based node as parent, like using CALayer directly in UIKit, I think it's ok to miss the dark mode feature.

[IvanChan]

Just mark

I have tried to sync TraitCollection in addSubnode , ASTraitCollectionPropagateDown to all child nodes. just like ASTraitCollectionPropagateDown is called in _setSupernode.

And its NOT working because:

• UIViewController. overrideUserInterfaceStyle = .dark doesn't means UIViewController.view.userInterfaceStyle == .dark, view's userInterfaceStyle will update later.
• - [UIView addSubview: ASDisplayNode.view] is NOT covered

[IvanChan]

Added test case to compare backgroundColor display when overrideUserInterfaceStyle is set, which fail in master & succeed in this branch

I am not sure setting layer's backgroundColor is somehow useful or not, so I just move them back and resolve with trait collection to make sure color is right.

@nguyenhuy

[volt-iner]

Any news?

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[bdolman]

Any movement on this? I've just run into this issue myself and came to the same conclusion as @IvanChan: When pending state is applied, it should be using the node's trait collection.