Conversation
| // as a background previously. This will not work correctly as the drawable callback logic is | ||
| // messed up in AOSP | ||
|
|
||
| Drawable background = null; |
There was a problem hiding this comment.
This is where the logic flaw lives -- this changes how background is calculated. With this change it is only calculated from a background set via setNativeBackground. However, there are other ways that the background could be set.
Specifically, in ReactViewGroup#getOrCreateReactViewBackground() the background property is being set through a means other than setNativeBackground. Thus, views that just setABackgroundColor, or a borderRadius, without setting a nativeBackground, will not see those properties take effect.
|
While integrating the border-radius ripple changes in 0.38.101 to our main app it became clear that this implementation breaks how react native renders various view properties, such as borderRadius, on views that do not have a ripple drawable. The inline comment explains why this approach is flawed -- this is likely a similar reasons as to why the original commit in React core was reverted. To not block #8 I am reverting this change first. The options I see moving forward is to fix the logic error in ReactViewGroup#setTranslucentBackground to acknowledge there are two potential sources of backgrounds and follow the similar pattern of using a layer list. But, I also want to investigate the possibility of just exposing a mask field on the RippleDrawable binding, to avoid as much heavy changes to these core view classes. |
|
LGTM |
|
@BenSchwab can you walk me through the bug and how it's breaking things and then we can talk about potential fixes? I feel that this should "just work" in the eyes of the user and that it might be worth some extra rigamarole to get it working |
|
Spoke offline - moving forward with a fix to the original diff. Going to merge this in the meantime as I don't like this branch having such a big bug. |
This reverts b59f99b and fe00e2d.
The approach to add touchable ripple to backgrounds with border-radius
breaks other React Native backgrounds on Android. Specifically,
any background that is not set through setNativeBackground will
not be rendered.