Skip to content
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

Merged
merged 15 commits into from
Feb 3, 2025

Conversation

latekvo
Copy link
Contributor

@latekvo latekvo commented Jan 31, 2025

Description

Android ripple currently does not work on Fabric, as all values passed to RawButton and it's inheritors are passed through processColor, which is crucial on Paper, and broken on Fabric.

This PR disables usage of processColor, when running on Fabric.

Note: isFabric cannot be moved out of the body of the Pressable, as it likely won't be initialised before the Pressable 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
import React from 'react';
import { Pressable as RNPressable, StyleSheet, Text } from 'react-native';
import {
  BaseButton,
  GestureHandlerRootView,
  RectButton,
  Pressable,
} from 'react-native-gesture-handler';

const App = () => {
  return (
    <GestureHandlerRootView>
      <RectButton style={styles.wrapperCustom} rippleColor={'blue'}>
        <Text style={styles.text}>RectButton</Text>
      </RectButton>
      <BaseButton style={styles.wrapperCustom} rippleColor={'blue'}>
        <Text style={styles.text}>BaseButton</Text>
      </BaseButton>
      <Pressable
        style={styles.wrapperCustom}
        android_ripple={{ color: 'blue' }}>
        {({ pressed }) => (
          <Text style={styles.text}>
            {pressed ? 'Pressed!' : 'Pressable from react-native'}
          </Text>
        )}
      </Pressable>
      <RNPressable
        style={styles.wrapperCustom}
        android_ripple={{ color: 'blue' }}>
        {({ pressed }) => (
          <Text style={styles.text}>
            {pressed ? 'Pressed!' : 'Pressable from react-native'}
          </Text>
        )}
      </RNPressable>
      <Pressable
        style={styles.wrapperCustom}
        android_ripple={{ color: '#00f' }}>
        {({ pressed }) => (
          <Text style={styles.text}>
            {pressed ? 'Pressed!' : 'Pressable from react-native'}
          </Text>
        )}
      </Pressable>
      <RNPressable
        style={styles.wrapperCustom}
        android_ripple={{ color: '#00f' }}>
        {({ pressed }) => (
          <Text style={styles.text}>
            {pressed ? 'Pressed!' : 'Pressable from react-native'}
          </Text>
        )}
      </RNPressable>
    </GestureHandlerRootView>
  );
};

const styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: 'center',
    gap: 16,
    padding: 16,
  },
  text: {
    fontSize: 32,
  },
  wrapperCustom: {
    borderRadius: 8,
    padding: 6,
    flex: 1,
    backgroundColor: 'papayawhip',
    borderColor: 'red',
    borderWidth: 2,
  },
  logBox: {
    flex: 1,
    padding: 20,
    margin: 10,
    borderWidth: StyleSheet.hairlineWidth,
    borderColor: '#f0f0f0',
    backgroundColor: '#f9f9f9',
  },
});

export default App;

@latekvo latekvo marked this pull request as draft January 31, 2025 13:12
@@ -380,6 +378,18 @@ export default function Pressable(props: PressableProps) {
? children({ pressed: pressedState })
: children;

const defaultRippleColor = useMemo(

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

Copy link
Contributor Author

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

@latekvo latekvo marked this pull request as ready for review January 31, 2025 14:18
@latekvo latekvo requested review from j-piasecki and m-bert January 31, 2025 14:18
Comment on lines +382 to +383
const defaultRippleColor = android_ripple ? undefined : 'transparent';
const unprocessedRippleColor = android_ripple?.color ?? defaultRippleColor;
Copy link
Contributor

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.

Copy link
Contributor Author

@latekvo latekvo Feb 3, 2025

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

@@ -122,10 +123,14 @@ class InnerBaseButton extends React.Component<BaseButtonWithRefProps> {
render() {
const { rippleColor, style, ...rest } = this.props;

const processedRippleColor = isFabric()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about maybeProcessedRippleColor?

Copy link
Contributor

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?

Copy link
Contributor Author

@latekvo latekvo Feb 3, 2025

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

Copy link
Contributor

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 👍

Copy link
Contributor Author

@latekvo latekvo Feb 3, 2025

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.

Copy link
Contributor Author

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

Copy link
Contributor

@m-bert m-bert left a 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? 😅

@latekvo
Copy link
Contributor Author

latekvo commented Feb 3, 2025

What about using the same IS_FABRIC trick in Pressable?

Done in f8b2de5

@latekvo latekvo merged commit 7f90209 into main Feb 3, 2025
1 check passed
@latekvo latekvo deleted the @latekvo/fix-ripple-color-bug-on-fabric branch February 3, 2025 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Pressable] android_ripple / color doesn't work correctly
3 participants