Set background color directly to view if not layer backed (#1588)

* Set background color directly to view or layer depending on if the node
is layerBacked.

Summary:
UIColor can now be a dynamic color provider to support capabilities like Dark Mode in applications. UIKit is able to re-render views when a user enters dark mode but this dynamic capabilities is not handled at the layer level.

Currently this takes the recommendation in facebookarchive/AsyncDisplayKit#1537 (comment) to set background color on the view directly or layer depending on if the node is layer backed. This change allows UIKit to track these colors and update the UI when the UITraitCollection.userInterfaceStyle changes.

* Updates to UIViewBridge to fix tests

* Remove opaque

* Test fixes

* Revert change to test checker method

* Fix strange issue where setting UIView backgroundColor sets the layer to
opaque to false

* Address Huy comments

* Address last comment

Source/Private/ASDisplayNode+UIViewBridge.mm

@@ -511,17 +511,32 @@ - (void)layoutIfNeeded
 - (BOOL)isOpaque
 {
   _bridge_prologue_read;
-  return _getFromLayer(opaque);
+  return _getFromViewOrLayer(opaque, opaque);
 }
 
+
 - (void)setOpaque:(BOOL)newOpaque
 {
   _bridge_prologue_write;
-
   BOOL shouldApply = ASDisplayNodeShouldApplyBridgedWriteToView(self);
 
   if (shouldApply) {
+    /*
+     NOTE: The values of `opaque` can be different between a view and layer.
+
+     In debugging on Xcode 11 I saw the following in lldb:
+     - Initially for a new ASDisplayNode layer.isOpaque and _view.isOpaque are true
+     - Set the backgroundColor of the node to a valid UIColor
+     Expected: layer.isOpaque and view.isOpaque would be equal and true
+     Actual: view.isOpaque is true and layer.isOpaque is now false
+
+     This broke some unit tests for view-backed nodes so I think we need to read directly from the view and can't rely on the layers value at this point.
+     */
     BOOL oldOpaque = _layer.opaque;
+    if (!_flags.layerBacked) {
+      oldOpaque = _view.opaque;
+      _view.opaque = newOpaque;
+    }
     _layer.opaque = newOpaque;
     if (oldOpaque != newOpaque) {
       [self setNeedsDisplay];
@@ -727,26 +742,47 @@ - (void)setContentMode:(UIViewContentMode)contentMode
 - (UIColor *)backgroundColor
 {
   _bridge_prologue_read;
-  return [UIColor colorWithCGColor:_getFromLayer(backgroundColor)];
+  if (_loaded(self)) {
+    /*
+     Note: We can no longer rely simply on the layers backgroundColor value if the color is set directly on `_view`
+     There is no longer a 1:1 mapping between _view.backgroundColor and _layer.backgroundColor after testing in iOS 13 / Xcode 11 so we should prefer one or the other depending on the backing type for the node (view or layer)
+     */
+    if (!_flags.layerBacked) {
+      return _view.backgroundColor;
+    } else {
+      return [UIColor colorWithCGColor:_getFromLayer(backgroundColor)];
+    }
+  }
+  return [UIColor colorWithCGColor:ASDisplayNodeGetPendingState(self).backgroundColor];
 }
 
 - (void)setBackgroundColor:(UIColor *)newBackgroundColor
 {
   _bridge_prologue_write;
 
-  CGColorRef newBackgroundCGColor = [newBackgroundColor CGColor];
+
   BOOL shouldApply = ASDisplayNodeShouldApplyBridgedWriteToView(self);
-
+  CGColorRef newBackgroundCGColor = newBackgroundColor.CGColor;
   if (shouldApply) {
     CGColorRef oldBackgroundCGColor = _layer.backgroundColor;
 
-    BOOL specialPropertiesHandling = ASDisplayNodeNeedsSpecialPropertiesHandling(checkFlag(Synchronous), _flags.layerBacked);
-    if (specialPropertiesHandling) {
-        _view.backgroundColor = newBackgroundColor;
+    if (_flags.layerBacked) {
+      _layer.backgroundColor = newBackgroundColor.CGColor;
     } else {
-        _layer.backgroundColor = newBackgroundCGColor;
+      /*
+       NOTE: Setting to the view and layer individually is necessary.
+
+       As observed in lldb, the view does not appear to immediately propagate background color to the layer and actually clears it's value (`nil`) initially. This was caught by our snapshot tests.
+
+       Given that UIColor / UIView has dynamic capabilties now, we should set directly to the view and make sure that the layers value is consistent here.
+
+       */
+      _view.backgroundColor = newBackgroundColor;
+      _layer.backgroundColor = _view.backgroundColor.CGColor;
+      // Gather the CGColorRef from the view incase there are any changes it might apply to which CGColorRef is returned for dynamic colors
+      newBackgroundCGColor = _view.backgroundColor.CGColor;
     }
-      
+
     if (!CGColorEqualToColor(oldBackgroundCGColor, newBackgroundCGColor)) {
       [self setNeedsDisplay];
     }

Source/Private/_ASPendingState.mm

@@ -1095,20 +1095,17 @@ - (void)applyToView:(UIView *)view withSpecialPropertiesHandling:(BOOL)specialPr
     view.clipsToBounds = _flags.clipsToBounds;
 
   if (flags.setBackgroundColor) {
-    // We have to make sure certain nodes get the background color call directly set
-    if (specialPropertiesHandling) {
-      view.backgroundColor = [UIColor colorWithCGColor:backgroundColor];
-    } else {
-      // Set the background color to the layer as in the UIView bridge we use this value as background color
-      layer.backgroundColor = backgroundColor;
-    }
+    view.backgroundColor = [UIColor colorWithCGColor:backgroundColor];
+    layer.backgroundColor = backgroundColor;
   }
 
   if (flags.setTintColor)
     view.tintColor = self.tintColor;
 
-  if (flags.setOpaque)
+  if (flags.setOpaque) {
+    view.opaque = _flags.opaque;
     layer.opaque = _flags.opaque;
+  }
 
   if (flags.setHidden)
     view.hidden = _flags.hidden;

Tests/ASDisplayNodeTests.mm

@@ -719,6 +719,7 @@ - (void)checkSimpleBridgePropertiesSetPropagate:(BOOL)isLayerBacked
   // Assert that the realized view/layer have the correct values.
   [node layer];
 
+
   [self checkValuesMatchSetValues:node isLayerBacked:isLayerBacked];
 
   // As a final sanity check, change a value on the realized view and ensure it is fetched through the node.
@@ -1921,19 +1922,33 @@ - (void)checkBackgroundColorOpaqueRelationshipWithViewLoaded:(BOOL)loaded layerB
     [node layer];
   }
 
+  /*
+   NOTE: The values of `opaque` can be different between a view and layer.
+
+   In debugging on Xcode 11 I saw the following in lldb:
+   - Initially for a new ASDisplayNode layer.isOpaque and _view.isOpaque are true
+   - Set the backgroundColor of the node to a valid UIColor
+   Expected: layer.isOpaque and view.isOpaque would be equal and true
+   Actual: view.isOpaque is true and layer.isOpaque is now false
+
+   For these reasons we have the following variations of asserts depending on the backing type of the node.
+   */
   XCTAssertTrue(node.opaque, @"Node should start opaque");
-  XCTAssertTrue(node.layer.opaque, @"Node should start opaque");
+  if (isLayerBacked) {
+    XCTAssertTrue(node.layer.opaque, @"Set background color should not have made this layer not opaque");
+  } else {
+    XCTAssertTrue(node.view.opaque, @"Set background color should not have made this view not opaque");
+  }
 
   node.backgroundColor = [UIColor clearColor];
 
   // This could be debated, but at the moment we differ from UIView's behavior to change the other property in response
   XCTAssertTrue(node.opaque, @"Set background color should not have made this not opaque");
-  XCTAssertTrue(node.layer.opaque, @"Set background color should not have made this not opaque");
-
-  [node layer];
-
-  XCTAssertTrue(node.opaque, @"Set background color should not have made this not opaque");
-  XCTAssertTrue(node.layer.opaque, @"Set background color should not have made this not opaque");
+  if (isLayerBacked) {
+    XCTAssertTrue(node.layer.opaque, @"Set background color should not have made this layer not opaque");
+  } else {
+    XCTAssertTrue(node.view.opaque, @"Set background color should not have made this view not opaque");
+  }
 }
 
 - (void)testBackgroundColorOpaqueRelationshipView