Skip to content
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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jul 20, 2019
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ebc8831
Set background color directly to view or layer depending on if the node
rahul-malik Jul 17, 2019
62705e7
Updates to UIViewBridge to fix tests
rahul-malik Jul 18, 2019
d26f517
Remove opaque
rahul-malik Jul 18, 2019
67ae90e
Test fixes
rahul-malik Jul 18, 2019
f889231
Revert change to test checker method
rahul-malik Jul 18, 2019
3d2d5d4
Fix strange issue where setting UIView backgroundColor sets the layer to
rahul-malik Jul 19, 2019
6f20d49
Address Huy comments
rahul-malik Jul 19, 2019
0e0b37c
Address last comment
rahul-malik Jul 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions Source/Private/ASDisplayNode+UIViewBridge.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,17 +511,20 @@ - (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) {
BOOL oldOpaque = _layer.opaque;
nguyenhuy marked this conversation as resolved.
Show resolved Hide resolved
if (!_flags.layerBacked) {
_view.opaque = newOpaque;
}
_layer.opaque = newOpaque;
if (oldOpaque != newOpaque) {
[self setNeedsDisplay];
Expand Down Expand Up @@ -727,7 +730,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
Expand All @@ -740,13 +750,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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 _layer.backgroundColor to nil when setting the views background color.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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];
}
Expand Down
13 changes: 5 additions & 8 deletions Source/Private/_ASPendingState.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)

Copy link
Member

@nguyenhuy nguyenhuy Jul 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The 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 nil background color by the time the test takes a snapshot using ASSnapshotVerifyNode.

An alternative approach would be updating ASSnapshotVerifyNode to use the view if the node is view-backed. But by doing so, we're accepting the risk that there is a time window in which the view and the layer disagree. And this time window is undocumented by UIKit.

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)
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;
Expand Down
18 changes: 12 additions & 6 deletions Tests/ASDisplayNodeTests.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -1922,18 +1923,23 @@ - (void)checkBackgroundColorOpaqueRelationshipWithViewLoaded:(BOOL)loaded layerB
}

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");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? At the beginning the view and the layer should agree with each other?

Nit: indentation is off.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gah 2 spaces

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so they don't agree with each other in practice. i'll add a comment here


node.backgroundColor = [UIColor clearColor];
// [node.view setNeedsDisplay];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering the reason behind having this line in the first place. Would that be because doing this causes the view to sync its states (opaque and background color) to the layer? Even if that's true, I think we still shouldn't rely on it because of what I said in the preview comment.

Plus, we should just remove this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i meant to remove this!


// 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];
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");
}

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
Expand Down