-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set background color directly to view if not layer backed #1588
Changes from 5 commits
ebc8831
62705e7
d26f517
67ae90e
f889231
3d2d5d4
6f20d49
0e0b37c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,4 +52,4 @@ SPEC CHECKSUMS: | |
|
||
PODFILE CHECKSUM: 445046ac151568c694ff286684322273f0b597d6 | ||
|
||
COCOAPODS: 1.6.0 | ||
COCOAPODS: 1.6.1 | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -514,15 +514,17 @@ - (BOOL)isOpaque | |
return _getFromLayer(opaque); | ||
} | ||
|
||
|
||
- (void)setOpaque:(BOOL)newOpaque | ||
{ | ||
_bridge_prologue_write; | ||
|
||
BOOL shouldApply = ASDisplayNodeShouldApplyBridgedWriteToView(self); | ||
|
||
if (shouldApply) { | ||
BOOL oldOpaque = _layer.opaque; | ||
BOOL oldOpaque; | ||
oldOpaque = _layer.opaque; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please elaborate why we are making this change here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The values of opaque differ depending on if it's a view or layer backed. |
||
_layer.opaque = newOpaque; | ||
|
||
if (oldOpaque != newOpaque) { | ||
[self setNeedsDisplay]; | ||
} | ||
|
@@ -727,7 +729,14 @@ - (void)setContentMode:(UIViewContentMode)contentMode | |
- (UIColor *)backgroundColor | ||
{ | ||
_bridge_prologue_read; | ||
return [UIColor colorWithCGColor:_getFromLayer(backgroundColor)]; | ||
if (_loaded(self)) { | ||
if (!_flags.layerBacked) { | ||
return _view.backgroundColor; | ||
} else { | ||
return [UIColor colorWithCGColor:_getFromLayer(backgroundColor)]; | ||
} | ||
} | ||
return [UIColor colorWithCGColor:ASDisplayNodeGetPendingState(self).backgroundColor]; | ||
nguyenhuy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
- (void)setBackgroundColor:(UIColor *)newBackgroundColor | ||
|
@@ -740,13 +749,13 @@ - (void)setBackgroundColor:(UIColor *)newBackgroundColor | |
if (shouldApply) { | ||
CGColorRef oldBackgroundCGColor = _layer.backgroundColor; | ||
|
||
BOOL specialPropertiesHandling = ASDisplayNodeNeedsSpecialPropertiesHandling(checkFlag(Synchronous), _flags.layerBacked); | ||
if (specialPropertiesHandling) { | ||
_view.backgroundColor = newBackgroundColor; | ||
if (_flags.layerBacked) { | ||
_layer.backgroundColor = newBackgroundCGColor; | ||
} else { | ||
_layer.backgroundColor = newBackgroundCGColor; | ||
_view.backgroundColor = newBackgroundColor; | ||
_layer.backgroundColor = _view.backgroundColor.CGColor; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as below if you update the view I don't think you need to update the layer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nguyenhuy and I were debugging and realized we needed to set both of these. UIView seems to be setting the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We assume these values are bridged but when debugging it seems like the relationship between view and layer is more complex. We're only hitting this now since we are setting backgroundColor directly on the view itself
nguyenhuy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
if (!CGColorEqualToColor(oldBackgroundCGColor, newBackgroundCGColor)) { | ||
[self setNeedsDisplay]; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1095,13 +1095,8 @@ - (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]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we set the background color to the view the layer should automatically be updated. Could you double cbeck that this is not the case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @maicki - We tested this today and surprisingly, the layer is not updated (at least not immediately) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirm what Rahul said: the view doesn't propagate the background color immediately which causes many of our tests to fail because they use the layer to render snapshots. For example this test failed because the parent node has a An alternative approach would be updating Given that now UIColor and UIView has a new dynamic capability, and that we should rely on UIView more to take advantage of this new capability, we should update the framework to set to both the view and the layer and thus eliminate the time window and its associated risks. |
||
layer.backgroundColor = backgroundColor; | ||
} | ||
|
||
if (flags.setTintColor) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -1924,16 +1925,11 @@ - (void)checkBackgroundColorOpaqueRelationshipWithViewLoaded:(BOOL)loaded layerB | |
XCTAssertTrue(node.opaque, @"Node should start opaque"); | ||
XCTAssertTrue(node.layer.opaque, @"Node should start opaque"); | ||
|
||
node.backgroundColor = [UIColor clearColor]; | ||
node.backgroundColor = [UIColor blackColor]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you elaborate why this test is failing now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was just for testing to see if |
||
|
||
// 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"); | ||
} | ||
|
||
- (void)testBackgroundColorOpaqueRelationshipView | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not update the cocoapods version with this commit imho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll undo this