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

rahul-malik · 2019-07-17T22:26:40Z

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.

ghost · 2019-07-17T22:30:16Z

🚫 CI failed with log

nguyenhuy

Makes sense to me to always use a UIColor whenever the node is view-backed now that UIColor has (light & dark) magic!

ghost · 2019-07-17T22:51:39Z

🚫 CI failed with log

ghost · 2019-07-17T23:05:38Z

🚫 CI failed with log

ghost · 2019-07-17T23:46:54Z

🚫 CI failed with log

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.

ghost · 2019-07-18T04:03:55Z

🚫 CI failed with log

ghost · 2019-07-18T06:24:19Z

🚫 CI failed with log

linlinyao1 · 2019-07-18T11:51:17Z

Dynamic capability is lost during conversion between UIColor and CGColor. Thus a UIColor reference is required in this case

nguyenhuy · 2019-07-18T18:13:05Z

Source/Private/ASDisplayNode+UIViewBridge.mm

@@ -727,7 +734,14 @@ - (void)setContentMode:(UIViewContentMode)contentMode
 - (UIColor *)backgroundColor
 {
  _bridge_prologue_read;
-  return [UIColor colorWithCGColor:_getFromLayer(backgroundColor)];
+  if (_loaded(self)) {
+    if (_view != nil) {


Use _flags.layerBacked to be consistent with other methods?

Source/Private/ASDisplayNode+UIViewBridge.mm

ghost · 2019-07-18T20:43:10Z

🚫 CI failed with log

ghost · 2019-07-18T22:17:05Z

🚫 CI failed with log

maicki · 2019-07-18T23:33:43Z

Podfile.lock

@@ -52,4 +52,4 @@ SPEC CHECKSUMS:

 PODFILE CHECKSUM: 445046ac151568c694ff286684322273f0b597d6

-COCOAPODS: 1.6.0
+COCOAPODS: 1.6.1


We should not update the cocoapods version with this commit imho

I'll undo this

maicki · 2019-07-18T23:34:09Z

Source/Private/ASDisplayNode+UIViewBridge.mm

  BOOL shouldApply = ASDisplayNodeShouldApplyBridgedWriteToView(self);

  if (shouldApply) {
-    BOOL oldOpaque = _layer.opaque;
+    BOOL oldOpaque;
+    oldOpaque = _layer.opaque;


Could you please elaborate why we are making this change here?

The values of opaque differ depending on if it's a view or layer backed.

maicki · 2019-07-18T23:35:38Z

Tests/ASDisplayNodeTests.mm

@@ -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];


Could you elaborate why this test is failing now?

This was just for testing to see if clearColor was changing the interpretation of isOpaque on CALayer

maicki · 2019-07-18T23:37:08Z

Source/Private/_ASPendingState.mm

-      // 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];


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.

@maicki - We tested this today and surprisingly, the layer is not updated (at least not immediately)

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.

maicki · 2019-07-18T23:39:24Z

Source/Private/ASDisplayNode+UIViewBridge.mm

    } else {
-        _layer.backgroundColor = newBackgroundCGColor;
+      _view.backgroundColor = newBackgroundColor;
+      _layer.backgroundColor = _view.backgroundColor.CGColor;


Same as below if you update the view I don't think you need to update the layer.

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

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

opaque to false

Source/Private/ASDisplayNode+UIViewBridge.mm

nguyenhuy · 2019-07-19T17:25:23Z

Tests/ASDisplayNodeTests.mm

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


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

Nit: indentation is off.

gah 2 spaces

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

nguyenhuy · 2019-07-19T17:26:25Z

Tests/ASDisplayNodeTests.mm


  node.backgroundColor = [UIColor clearColor];
+//  [node.view setNeedsDisplay];


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.

i meant to remove this!

nguyenhuy · 2019-07-19T17:34:58Z

Source/Private/_ASPendingState.mm

-      // 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];


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.

nguyenhuy

LGTM!

nguyenhuy · 2019-07-19T18:49:35Z

I'm going to merge this PR once CI passes. We want to get this in before our sync this week so we can test it immediately.

@maicki @Adlai-Holler @appleguy Feel free to do post-reviews. @rahul-malik and I will follow up on any of your concerns in new diffs.

ghost · 2019-07-19T19:59:47Z

🚫 CI failed with log

ghost · 2019-07-19T20:28:02Z

🚫 CI failed with log

rahul-malik requested review from appleguy and nguyenhuy July 17, 2019 22:26

rahul-malik changed the title ~~Set background color directly to view or layer depending on if the node~~ Set background color directly to view if not layer backed Jul 17, 2019

nguyenhuy approved these changes Jul 17, 2019

View reviewed changes

nguyenhuy requested a review from maicki July 17, 2019 22:32

rahul-malik force-pushed the rmalik/fix-uiview-bridge-backgroundcolor branch from e56ebc0 to ebc8831 Compare July 18, 2019 03:59

Updates to UIViewBridge to fix tests

62705e7

nguyenhuy reviewed Jul 18, 2019

View reviewed changes

rahul-malik added 2 commits July 18, 2019 13:00

Remove opaque

d26f517

Test fixes

67ae90e

Revert change to test checker method

f889231

maicki reviewed Jul 18, 2019

View reviewed changes

nguyenhuy self-requested a review July 19, 2019 01:57

Fix strange issue where setting UIView backgroundColor sets the layer to

3d2d5d4

opaque to false

rahul-malik force-pushed the rmalik/fix-uiview-bridge-backgroundcolor branch from 75fa029 to 3d2d5d4 Compare July 19, 2019 03:13

rahul-malik mentioned this pull request Jul 19, 2019

Update ASPendingState backgroundColor type to UIColor #1592

Merged

nguyenhuy requested changes Jul 19, 2019

View reviewed changes

rahul-malik added 2 commits July 19, 2019 11:04

Address Huy comments

6f20d49

Address last comment

0e0b37c

nguyenhuy approved these changes Jul 19, 2019

View reviewed changes

nguyenhuy merged commit d4d6897 into master Jul 20, 2019

nguyenhuy added the iOS 13 label Jul 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rahul-malik commented Jul 17, 2019 •

edited

Loading

ghost commented Jul 17, 2019

nguyenhuy left a comment

ghost commented Jul 17, 2019

ghost commented Jul 17, 2019

ghost commented Jul 17, 2019

ghost commented Jul 18, 2019

ghost commented Jul 18, 2019

linlinyao1 commented Jul 18, 2019

nguyenhuy Jul 18, 2019

ghost commented Jul 18, 2019

ghost commented Jul 18, 2019

maicki Jul 18, 2019

rahul-malik Jul 19, 2019

maicki Jul 18, 2019 •

edited

Loading

rahul-malik Jul 19, 2019

maicki Jul 18, 2019

rahul-malik Jul 19, 2019

maicki Jul 18, 2019

rahul-malik Jul 19, 2019

nguyenhuy Jul 19, 2019 •

edited

Loading

maicki Jul 18, 2019

rahul-malik Jul 19, 2019

rahul-malik Jul 19, 2019

nguyenhuy Jul 19, 2019

rahul-malik Jul 19, 2019

rahul-malik Jul 19, 2019

nguyenhuy Jul 19, 2019

rahul-malik Jul 19, 2019

nguyenhuy Jul 19, 2019 •

edited

Loading

nguyenhuy left a comment

nguyenhuy commented Jul 19, 2019

ghost commented Jul 19, 2019

ghost commented Jul 19, 2019


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

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

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

Conversation

rahul-malik commented Jul 17, 2019 • edited Loading

ghost commented Jul 17, 2019

nguyenhuy left a comment

Choose a reason for hiding this comment

ghost commented Jul 17, 2019

ghost commented Jul 17, 2019

ghost commented Jul 17, 2019

ghost commented Jul 18, 2019

ghost commented Jul 18, 2019

linlinyao1 commented Jul 18, 2019

Choose a reason for hiding this comment

ghost commented Jul 18, 2019

ghost commented Jul 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maicki Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy Jul 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy Jul 19, 2019 • edited Loading

Choose a reason for hiding this comment

nguyenhuy left a comment

Choose a reason for hiding this comment

nguyenhuy commented Jul 19, 2019

ghost commented Jul 19, 2019

ghost commented Jul 19, 2019

rahul-malik commented Jul 17, 2019 •

edited

Loading

maicki Jul 18, 2019 •

edited

Loading

nguyenhuy Jul 19, 2019 •

edited

Loading

nguyenhuy Jul 19, 2019 •

edited

Loading