#1673 - Re-render Clipping Corners when User Interface Style Changes #1674
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shamanskyh
commented
Sep 13, 2019
| if (!CGColorEqualToColor(_layer.backgroundColor, cgBackgroundColor)) { | ||
| _layer.backgroundColor = cgBackgroundColor; | ||
| BOOL needsClippingCornerUpdate = NO; | ||
| CGFloat cornerRadius = _cornerRadius; |
There was a problem hiding this comment.
Capturing these values up here so they're behind the lock, but in scope when we call _updateClipCornerLayerContentsWithRadius on line 466
shamanskyh
commented
Sep 13, 2019
| @@ -61,18 +61,50 @@ - (void)testPrecompositedCornerRounding | |||
|
|
|||
| - (void)testClippingCornerRounding | |||
| { | |||
| self.recordMode = YES; | |||
| #if AS_AT_LEAST_IOS13 | |||
There was a problem hiding this comment.
Looking for feedback on this inline approach. Should I delete the reference images for older Xcode/iOS versions?
There was a problem hiding this comment.
I personally think this is fine for now. I'm assuming you tested both with & without iOS13?
ernestmama
approved these changes
Sep 16, 2019
bolsinga
approved these changes
Sep 16, 2019
rahul-malik
reviewed
Sep 16, 2019
| BOOL needsClippingCornerUpdate = NO; | ||
| CGFloat cornerRadius = _cornerRadius; | ||
| ASCornerRoundingType cornerRoundingType = _cornerRoundingType; | ||
| UIColor *backgroundColor = self.backgroundColor; |
There was a problem hiding this comment.
any reason we're using self.backgroundColor vs _backgroundColor
There was a problem hiding this comment.
self.backgroundColor has a getter that pulls from the layer or the view. _backgroundColor should only be used if layer backed.
Source/ASDisplayNode.mm
Outdated
| // Background colors do not dynamically update for layer backed nodes since they utilize CGColorRef | ||
| // instead of UIColor. We utilize the _backgroundColor instance variable to track the full dynamic color | ||
| // and apply any changes here when trait collection updates occur. | ||
| CGColorRef cgBackgroundColor = _backgroundColor.CGColor; |
There was a problem hiding this comment.
use should use the new local variable here
| node.maskedCorners = c; | ||
| node.cornerRadius = 15; | ||
| // A layout pass is required, because that's where we lay out the clip layers. | ||
| [node.layer layoutIfNeeded]; | ||
|
|
||
| #if AS_AT_LEAST_IOS13 |
There was a problem hiding this comment.
We should probably write a macro for light/dark testing if we're doing this a lot
…clipping corner layer doesn't get re-rendered when going from a dark to light user interface style. The fix is to add code in the `-asyncTraitCollectionDidChangeWithPreviousTraitCollection:` method that calls through to `-_updateClipCornerLayerContentsWithRadius:backgroundColor:` if the user interface style changes. I've also added snapshot tests to verify that using clipping corners with a dynamic background color updates when the user interface style changes. Turn off record mode, remove errant newline This PR addresses an issue I raised with TextureGroup#1673 where the clipping corner layer doesn't get re-rendered when going from a dark to light user interface style. The fix is to add code in the `-asyncTraitCollectionDidChangeWithPreviousTraitCollection:` method that calls through to `-_updateClipCornerLayerContentsWithRadius:backgroundColor:` if the user interface style changes. I've also added snapshot tests to verify that using clipping corners with a dynamic background color updates when the user interface style changes. Turn off record mode, remove errant newline
shamanskyh
force-pushed
the
dynamic-clipping-corners
branch
from
a6167b2
to
a03e077
Compare
This was referenced Sep 16, 2019
rahul-malik
added a commit
that referenced
this pull request
Sep 18, 2019
…nDidChange traitCollectionDidChange isn't necessarily called on Main, but our self.backgroundColor property access must be done on main once the view is loaded. I originally opted to use self.backgroundColor rather than _backgroundColor in #1674, but I think we should use the ivar here. This could get out-of-sync if someone modified the UIView's background color instead of updating the node, but I think that's unlikely, so we should be safe to use the ivar here.
rahul-malik
added a commit
that referenced
this pull request
Sep 18, 2019
…nDidChange (#1678) traitCollectionDidChange isn't necessarily called on Main, but our self.backgroundColor property access must be done on main once the view is loaded. I originally opted to use self.backgroundColor rather than _backgroundColor in #1674, but I think we should use the ivar here. This could get out-of-sync if someone modified the UIView's background color instead of updating the node, but I think that's unlikely, so we should be safe to use the ivar here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses an issue I raised with #1673 where the clipping corner layer doesn't get re-rendered when going from a dark to light user interface style. The fix is to add code in the
-asyncTraitCollectionDidChangeWithPreviousTraitCollection:method that calls through to-_updateClipCornerLayerContentsWithRadius:backgroundColor:if the user interface style changes.I've also added snapshot tests to verify that using clipping corners with a dynamic background color updates when the user interface style changes.