-
-
Couldn't load subscription status.
- Fork 1k
Fix android ripple color bug on fabric #3369
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
Conversation
| ? 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
| 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 toRawButtonand it's inheritors are passed throughprocessColor, which is crucial onPaper, and broken onFabric.This PR disables usage of
processColor, when running onFabric.Note:
isFabriccannot be moved out of the body of thePressable, as it likely won't be initialised before thePressablestarts 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