-
-
Notifications
You must be signed in to change notification settings - Fork 995
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
Fix android ripple color bug on fabric #3369
Conversation
@@ -380,6 +378,18 @@ export default function Pressable(props: PressableProps) { | |||
? children({ pressed: pressedState }) | |||
: children; | |||
|
|||
const defaultRippleColor = useMemo( |
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 think both memo can be merged into only 1 ..
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.
You're right, fixed in a9d2031
const defaultRippleColor = android_ripple ? undefined : 'transparent'; | ||
const unprocessedRippleColor = android_ripple?.color ?? defaultRippleColor; |
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.
Shouldn't it be
const unprocessedRippleColor = android_ripple?.color ?? 'transparent';
?
I don't really see the point of defaultRippleColor
in that case.
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.
defaultRippleColor
is required to properly replicate the behaviour of RN Pressable
.
When android_ripple
is set, but the color
field is not provided, we want to set color
to undefined
, so that it uses the default system value (opaque whiteish).
When android_ripple
is not set, we want to remove the ripple altogether, which is done via 'transparent'
.
src/components/GestureButtons.tsx
Outdated
@@ -122,10 +123,14 @@ class InnerBaseButton extends React.Component<BaseButtonWithRefProps> { | |||
render() { | |||
const { rippleColor, style, ...rest } = this.props; | |||
|
|||
const processedRippleColor = isFabric() |
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.
What about maybeProcessedRippleColor
?
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.
Also, why don't we memoize value of isFabric
instead of calling this everytime?
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.
What about maybeProcessedRippleColor?
Not a big fan of that name, the word processed
already allows for lack of change.
But I agree that processed
may be confusing due to processColor()
function.
Maybe this will do better:
rippleColor
-> unprocessedRippleColor
processedRippleColor
-> rippleColor
?
Also, why don't we memoize value of isFabric instead of calling this everytime?
Sure, done in ff48533
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.
Sure, done in ff48533
Not exactly what I meant. I thought about storing it in a variable. Now we still call function in each render.
Maybe this will do better:
rippleColor -> unprocessedRippleColor
processedRippleColor -> rippleColor
Looks good 👍
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.
Not exactly what I meant. I thought about storing it in a variable. Now we still call function in each render.
Done for BaseButton
in d62793c
Changed useCallback
to useMemo
in 2ff4683
If you're referring to storing IS_FABRIC
outside of the Pressable
, we can't do that, isFabric()
will return incorrect data outside of the function component, as the value it reads will not be initialised before the first render.
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.
Looks good 👍
Done in 2513b9e
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.
Looks ok 👍
What about using the same IS_FABRIC
trick in Pressable
? 😅
Done in f8b2de5 |
Description
Android ripple currently does not work on
Fabric
, as all values passed toRawButton
and it's inheritors are passed throughprocessColor
, which is crucial onPaper
, and broken onFabric
.This PR disables usage of
processColor
, when running onFabric
.Note:
isFabric
cannot be moved out of the body of thePressable
, as it likely won't be initialised before thePressable
starts being rendered. More details here.Fixes #3246
Fixes #3312
Supersedes #3250
Likely could add a workaround for software-mansion/react-native-reanimated#6935
Test plan
Collapsed test code