diff --git a/Source/Private/ASDisplayNode+UIViewBridge.mm b/Source/Private/ASDisplayNode+UIViewBridge.mm index 87bc05ee4..0aef83fc4 100644 --- a/Source/Private/ASDisplayNode+UIViewBridge.mm +++ b/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]; } diff --git a/Source/Private/_ASPendingState.mm b/Source/Private/_ASPendingState.mm index 9ec44ca68..2663955a5 100644 --- a/Source/Private/_ASPendingState.mm +++ b/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; diff --git a/Tests/ASDisplayNodeTests.mm b/Tests/ASDisplayNodeTests.mm index 522a26d74..802b9ee32 100644 --- a/Tests/ASDisplayNodeTests.mm +++ b/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