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

fireEvent seems to always trigger event (on disabled TouchableOpacity) #28

Closed
fwal opened this issue Oct 12, 2018 · 31 comments
Closed

fireEvent seems to always trigger event (on disabled TouchableOpacity) #28

fwal opened this issue Oct 12, 2018 · 31 comments

Comments

@fwal
Copy link

fwal commented Oct 12, 2018

I'm trying to figure out why it seems like the fireEvent.press function always seem to trigger a callback even though it shouldn't be registered. My use case is that I'm trying to write a test that makes sure that onPress isn't called if the component is "disabled".

Trying to mimic the behaviour below...

  const MyComponent = _props => (
    <View>
      <TouchableHighlight testID={'touchable'}>
        <Text>{'Foo'}</Text>
      </TouchableHighlight>
    </View>
  )

  const pressedCallback = jest.fn()
  const { getByTestId } = render(<MyComponent onPress={() => pressedCallback('bar')} />)
  fireEvent.press(getByTestId('touchable'))
  expect(pressedCallback).not.toBeCalled() //Expected mock function not to be called but it was called with: ["bar"]
@fwal fwal changed the title fireEvent seems to alw fireEvent seems to always trigger event Oct 12, 2018
@fwal
Copy link
Author

fwal commented Oct 12, 2018

Hmm seems like fireEvent traverses up the tree and call the function if found, that would make my use-case a bit trickier... https://github.com/callstack/react-native-testing-library/blob/master/src/fireEvent.js#L18

@thymikee
Copy link
Member

Yup, that's a desired behavior – user may press a text, which in fact may trigger a parent press event handler.

@fwal
Copy link
Author

fwal commented Oct 12, 2018

Any idea on how I could test my behaviour? I basically don't set onPress on the touchable component if my prop disabled is true...

@thymikee
Copy link
Member

cc @Esemesek

@thymikee
Copy link
Member

thymikee commented Oct 12, 2018

I think it makes sense to stop bubbling just before root component, that would prevent this and seems like a sane solution, but haven't thought it through yet

@fwal
Copy link
Author

fwal commented Oct 12, 2018

Ah I could probably should swallow the onPress event in the TouchableHighlight if disabled is true rather than omitting to set the prop... 🤔

@fwal
Copy link
Author

fwal commented Oct 12, 2018

For example this works:

  const MyComponent = ({ disabled, onPress }) => (
    <View>
      <TouchableHighlight onPress={disabled ? () => {} : onPress} testID={'touchable'}>
        <Text>{'Foo'}</Text>
      </TouchableHighlight>
    </View>
  )

But then again I'm changing implementation to satisfy the test 😅

@thymikee
Copy link
Member

thymikee commented Oct 12, 2018

I usually do both just in case, but I guess we could detect the "disabled" prop on the Core RN components like Touchable* and native Button.

@fwal
Copy link
Author

fwal commented Oct 12, 2018

There's probably more cases like this, like onScroll and scrollEnabled in ScrollView. However In my button case I'll probably leave it and rely on that the core components are properly tested.

@thymikee
Copy link
Member

Yup, we have an idea on how to make it more generic without changing implementation of fireEvent. Bare with us while we're fixing this.

@thymikee thymikee added bug Something isn't working enhancement New feature or request labels Oct 12, 2018
@thymikee
Copy link
Member

Just merged #30 so the bug is fixed now (not released yet). As for disabled prop, this is something that should be fixed in RN mocks tbh. We'll need to think about simulating native events capturing as close as possible though (e.g. including capturing phase).

@thymikee thymikee removed the enhancement New feature or request label Oct 15, 2018
@will-stone
Copy link

Hi @thymikee, thanks for all the hard work on the lib. I know this issue is closed and was a while ago but what was the conclusion on how to deal with a disabled prop? I'm struggling to get my test to pass.

@thymikee
Copy link
Member

thymikee commented Jan 9, 2019

For test purposes you'll need to make sure by yourself that your events won't fire when disabled prop is passed, there's not really other way until the mocks are fixed.

We may, however, consider some special treatment for Touchable* components when in RN context (usual), as these are special and e.g. cancel-out event bubbling, unlike one the web where events propagate up freely unless stopped explicitly by the user.

@will-stone
Copy link

So this...

<Button onPress={() => {
  if(thing && otherThing) {
    this.props.onButtonPress()
  }
}} disabled={!thing || !otherThing}>
  Press Me
</Button>

is the way to go for now?

@thymikee
Copy link
Member

thymikee commented Jan 9, 2019

I'd extract disabled to a variable since it's reused, but yes.

@reaktivo
Copy link

reaktivo commented Jun 18, 2019

Hi @thymikee, I just bumped into this same issue myself. Is there any newer workaround that doesn't involve modifying the component itself? Can this ticket be reopened?

@thymikee
Copy link
Member

You can mock the TouchableOpacity and pass it default implementation plus disabled prop set correctly.

We may introduce some special handling to disabled prop in touchable elements, e.g. as an option to fireEvent, but this is clearly a hack and we don't want to bake it into the library for good.

This is unfortunately a shortcoming of our event triggering system and we'd like to create something better, but on the other hand not scoped to React Native internals – and this is gonna be hard, provided no documentation of the event/touch system and platform inconsistencies. I'm however looking forward to what the React Flare project brings. If successful, would make it way easier for us to implement robust and cross-platform React event handling.

@thymikee thymikee reopened this Jun 18, 2019
@thymikee thymikee changed the title fireEvent seems to always trigger event fireEvent seems to always trigger event (on disabled TouchableOpacity) Jun 18, 2019
@reaktivo
Copy link

Thanks for the quick reply, your response is quite reasonable. For now as a temporary workaround I'm first checking the element's disabled prop and running my expectations based on that.

@alexborton
Copy link

We also just came across this issues. I have added a mock in the setup.js to keep it tucked away and ignore moving forward 🙄

jest.mock('TouchableOpacity', () => {
  const RealComponent = require.requireActual('TouchableOpacity')
  const MockTouchableOpacity = props => {
    const { children, disabled, onPress } = props
    return React.createElement(
      'TouchableOpacity',
      { ...props, onPress: disabled ? () => {} : onPress },
      children,
    )
  }
  MockTouchableOpacity.propTypes = RealComponent.propTypes
  return MockTouchableOpacity
})

@swingywc
Copy link

We also just came across this issues. I have added a mock in the setup.js to keep it tucked away and ignore moving forward 🙄

jest.mock('TouchableOpacity', () => {
  const RealComponent = require.requireActual('TouchableOpacity')
  const MockTouchableOpacity = props => {
    const { children, disabled, onPress } = props
    return React.createElement(
      'TouchableOpacity',
      { ...props, onPress: disabled ? () => {} : onPress },
      children,
    )
  }
  MockTouchableOpacity.propTypes = RealComponent.propTypes
  return MockTouchableOpacity
})

In my point of view, I don't think testing should override any component, and should not change the implementation to satisfy the test.
@thymikee Is there any update on the disabled property in the fireEvent? Thank you.

@thymikee
Copy link
Member

@swingywc no updates yet.

In my point of view, I don't think testing should override any component, and should not change the implementation to satisfy the test.

I see your point, but I have to disagree here. Sometimes, an inability to write a test for a piece of code indicates an accessibility issue with it – e.g. it's not usable by someone using a screen reader.

@MattAgn
Copy link
Collaborator

MattAgn commented Jan 14, 2020

I stumbled upon the same issue and the best workaround I found was to use this jest extension for react native. It extends the jest "expect" function with helpers like .toBeDisabled, toBeEmpty, toHaveProp etc... Very useful!

@thymikee thymikee added upstream bug and removed bug Something isn't working labels May 25, 2020
@sanjeevyadavIT
Copy link
Contributor

Yup, we have an idea on how to make it more generic without changing implementation of fireEvent. Bare with us while we're fixing this.

@thymikee Are you working on this bug???

I see your point, but I have to disagree here. Sometimes, an inability to write a test for a piece of code indicates an accessibility issue with it – e.g. it's not usable by someone using a screen reader.

I disagree with you, I don't understand how this falls under accessibility issue you are talking about, sending different function when disabled, is literally changing code just to pass my test, not because it is requirement, because I expect onPress to be disabled when pass disabled prop on Touch listener provided my react-native. ☺️

@thymikee
Copy link
Member

@alexakasanjeev we're not currently working on this. But the proper place to fix it is the Touchable* components in RN core. I won't patch the library to work around specific logic that lives on the native side.

For example, there's a new Pressable component coming up, which may or may not have this issue. Shall we patch it then as well? What about other components? I hope you see my point. This is an upstream "bug" (or limitation) and I only leave it open for folks to be aware.

I can only advise to always wrap RN touchable components into custom components which you can control. This will also make touchables unified across your app, so it's worth doing anyway. A sample implementation: https://github.com/react-native-community/react-native-platform-touchable/blob/master/PlatformTouchable.js of such primitive

@TDAK1509
Copy link

I test the disable status by using snapshot to check the style. (When disabled, my button has a different color)

    // Default is disabled
    it("matches snapshot", () => {
      expect(wrapper).toMatchSnapshot();
    });

    // Button is enabled when an image is selected
    it("matches snapshot when image is selected", () => {
      const { getAllByTestId } = wrapper;
      const checkBox = getAllByTestId("checkbox");
      fireEvent.press(checkBox[0]);
      expect(wrapper).toMatchSnapshot();
    });

After doing this, in snapshot file there will be 2 cases for the button, which will be disabled state and enabled state with different colors.

<View
    accessible={true}
    focusable={true}
    onClick={[Function]}
    onResponderGrant={[Function]}
    onResponderMove={[Function]}
    onResponderRelease={[Function]}
    onResponderTerminate={[Function]}
    onResponderTerminationRequest={[Function]}
    onStartShouldSetResponder={[Function]}
    style={
      Object {
        "alignItems": "center",
        "borderRadius": 50,
        "bottom": 80,
        "justifyContent": "center",
        "opacity": 1,
        "position": "absolute",
        "right": 30,
        "zIndex": 10,
      }
    }
    testID="deleteButton"
  >
    <Text
      allowFontScaling={false}
      style={
        Array [
          Object {
            "color": "#948f8f", <--- grey for disabled state
            "fontSize": 40,
          },
          undefined,
          Object {
            "fontFamily": "anticon",
            "fontStyle": "normal",
            "fontWeight": "normal",
          },
          Object {},
        ]
      }
    ></Text>
  </View>
<View
    accessible={true}
    focusable={true}
    onClick={[Function]}
    onResponderGrant={[Function]}
    onResponderMove={[Function]}
    onResponderRelease={[Function]}
    onResponderTerminate={[Function]}
    onResponderTerminationRequest={[Function]}
    onStartShouldSetResponder={[Function]}
    style={
      Object {
        "alignItems": "center",
        "borderRadius": 50,
        "bottom": 80,
        "justifyContent": "center",
        "opacity": 1,
        "position": "absolute",
        "right": 30,
        "zIndex": 10,
      }
    }
    testID="deleteButton"
  >
    <Text
      allowFontScaling={false}
      style={
        Array [
          Object {
            "color": "#000", <--- black for enabled state
            "fontSize": 40,
          },
          undefined,
          Object {
            "fontFamily": "anticon",
            "fontStyle": "normal",
            "fontWeight": "normal",
          },
          Object {},
        ]
      }
    ></Text>
  </View>

@thymikee
Copy link
Member

@TDAK1509 I encourage you to use https://github.com/jest-community/snapshot-diff in such cases :)

@thymikee
Copy link
Member

Good news, we're fixing that in the next major release (which will involve rename to @testing-library/react-native).

Closed via #460

@ManuViola77
Copy link

ManuViola77 commented Jun 16, 2022

could it be this bug is back again? Because Im testing with the extended version that is disabled like this:

expect(something).toBeDisabled();

and this passes correctly but then I do this:

fireEvent.press(something);
expect(function).toHaveBeenCalledTimes(0);

and this fails saying it was called 1 time 😢

Im using "@testing-library/react-native": "^9.1.0"

@Med-Amine-Fredj
Copy link

Med-Amine-Fredj commented Jan 18, 2023

Any idea on how I could test my behaviour? I basically don't set onPress on the touchable component if my prop disabled is true...

I was asking my self the same question , so for me i tried to test the prop it self if it is passed correctly passed :

it("should be disabled when the disabled prop is true", () => {
    const { getByTestId } = render(<AppButton title="Test" disabled={true} />);
    const button = getByTestId("AppButton-test");
    expect(button.props.accessibilityState.disabled).toBeTruthy();
  });

this is my AppButton Component :

const AppButton: React.FC<AppButtonProps> = ({
  title = "",
  onPress = () => {},
  disabled = false,
}) => {
  return (
    <TouchableOpacity
      testID="AppButton-test"
      disabled={disabled}
      onPress={onPress}>
      <AppText>
        {title}
      </AppText>
    </TouchableOpacity>
  );
};

@mdjastrzebski
Copy link
Member

@Med-Amine-Fredj
I would suggest using Jest Native matchers to improve the test code:

it("should be disabled when the disabled prop is true", () => {
    const { getByTestId } = render(<AppButton title="Test" disabled={true} />);
    const button = getByTestId("AppButton-test");
    expect(button).toBeDisabled();
  });

@robertwt7
Copy link

This is still happening for me when using Pressable component? I am on version 11.4.0

How is everyone testing their disabled component now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests