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 backgroundColor not being applied to RNGestureHandlerButton #3368

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

latekvo
Copy link
Contributor

@latekvo latekvo commented Jan 30, 2025

Description

RNGestureHandlerButton consists of 2 nested ViewGroups - ViewGroupManager and ButtonViewGroup

Before React Native 0.77, backgroundColor was applied to RNGestureHandlerButton by ButtonViewGroup.setBackgroundColor(color), which was called by unknown React Native mechanism.

In React Native 0.77 something changed, and ButtonViewGroup.setBackgroundColor(color) stopped being called.

This PR overrides the setter for backgroundColor on ViewGroupManager, and forwards the argument to ButtonViewGroup.setBackgroundColor(color).

I have not found the root cause (RN commit) of this issue, and i have no idea what are the consequences of calling both the BackgroundStyleApplicator.setBackgroundColor() and ButtonViewGroup.setBackgroundColor(color), but it seems like those two functions were already called sequentially before 0.77, as BackgroundStyleApplicator was introduced in RN 0.76 and most likely called ButtonViewGroup.setBackgroundColor(color) itself.

Fixes #3367

Test plan

Repro code
import React from 'react';
import { Pressable as RNPressable, StyleSheet, Text } from 'react-native';
import {
  BaseButton,
  GestureHandlerRootView,
  PureNativeButton,
  RawButton,
  RectButton,
} from 'react-native-gesture-handler';

const App = () => {
  return (
    <GestureHandlerRootView>
      <RectButton style={styles.wrapperCustom}>
        <Text style={styles.text}>RectButton</Text>
      </RectButton>
      <BaseButton style={styles.wrapperCustom}>
        <Text style={styles.text}>BaseButton</Text>
      </BaseButton>
      <RawButton style={styles.wrapperCustom}>
        <Text style={styles.text}>RawButton</Text>
      </RawButton>
      <PureNativeButton style={styles.wrapperCustom}>
        <Text style={styles.text}>PureNativeButton</Text>
      </PureNativeButton>
      <RNPressable style={styles.wrapperCustom}>
        {({ 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',
    borderWidth: 2,
  },
  logBox: {
    flex: 1,
    padding: 20,
    margin: 10,
    borderWidth: StyleSheet.hairlineWidth,
    borderColor: '#f0f0f0',
    backgroundColor: '#f9f9f9',
  },
});

export default App;

@@ -55,6 +55,12 @@ class RNGestureHandlerButtonViewManager : ViewGroupManager<ButtonViewGroup>(), R
view.useDrawableOnForeground = useDrawableOnForeground
}

@ReactProp(name = "backgroundColor")
override fun setBackgroundColor(view: ButtonViewGroup, backgroundColor: Int) {
super.setBackgroundColor(view, backgroundColor)
Copy link
Member

Choose a reason for hiding this comment

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

Is the super call needed? Isn't the next line effectively overriding whatever is set here?

Copy link
Contributor Author

@latekvo latekvo Jan 30, 2025

Choose a reason for hiding this comment

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

I think you're right. I previously had my concerns because of this experimental feature introduced in 0.77, which changes the background updating system, but i re-checked, and updateBackgroundColor() works independently of that feature.

Another concern, which is not ruled out, is that the aforementioned NewBackgroundAndBorderDrawables feature may deprecate LayerDrawable method used in updateBackgroundColor(), but since there isn't any documentation regarding that feature yet, i think it'd make the most sense to add a comment mentioning this possibility, in case something breaks in the future, and move on. I suggest the following:

@ReactProp(name = "backgroundColor")
override fun setBackgroundColor(view: ButtonViewGroup, backgroundColor: Int) {
  // view.setBackgroundColor may become deprecated in future releases of React Native
  // https://github.com/facebook/react-native/commit/cc23aea2fc95947b19dbd28061f748c13ba6344b
  view.setBackgroundColor(backgroundColor)
}

I'm not sure if we should be finding a way to support enableNewBackgroundAndBorderDrawables right now, I personally think it's a low-priority change. I'm not sure if feature flags are even easily accessible to people other than core React Native developers.

Besides those things, super.setBackgroundColor() (implementation) seems to do more or less the same things as updateBackgroundColor() (implementation).

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 won't show background color
2 participants