fix: non-responder wrapping host elements ignore disabled prop

[adkenyon]

Summary

Effectively, wrapping a disabled Touchable with a non-responder host element (plain old View) will ignore the disabled prop. In the example below (same as spec file), fireEvent.press(screen.getByText('Trigger')); should not trigger the handlePress mock function as the TestChildTouchableComponent TouchableOpacity is disabled. Since the wrapping View in TestChildTouchableComponent is a host element it is then considered the nearestHostDescendant. Since it is not disabled then the event will eventually trigger.

The solution was to only pass host elements that are responders as nearestHostDescendant's

function TestChildTouchableComponent({ onPress, someProp }) {
  return (
    <View>
      <TouchableOpacity onPress={onPress} disabled={someProp}>
        <Text>Trigger</Text>
      </TouchableOpacity>
    </View>
  );
}

test('is not fooled by non-responder wrapping host elements', () => {
  const handlePress = jest.fn();

  const screen = render(
    <View>
      <TestChildTouchableComponent onPress={handlePress} someProp={true} />
    </View>
  );

  fireEvent.press(screen.getByText('Trigger'));
  expect(handlePress).not.toHaveBeenCalled();
});

[thymikee]

Sounds reasonable. @mdjastrzebski could you take a look?

[mdjastrzebski]

Overall I like the idea.

I have some concerns about code readability that has deteriored, because the code simply got more complex. I think that introducing some helper variables with descriptive names can help here. Also some code comments would be welcomed.

[adkenyon]

@mdjastrzebski just updated the code, hopefully it's a bit more readable. Let me know if you have an specific suggestions

[mdjastrzebski]

@adkenyon Your code comments help a lot but honestly I still need to wrap my head arounds this. I hope I'll be able to review this tomorrow.

[adkenyon]

Thanks @mdjastrzebski. I ran into this issue upgrading RNTL to the latest (we were on ~6). The example here is a bit contrived, let me know you you need more examples

[mdjastrzebski]

@adkenyon sorry for late reply, but it took me some longer time to actually wrap my head around current implmentation and your proposed changes. I've done some clean up to simplify structure and improve readability. Please review my changes.

Behavior explanation

I've diagnosed the RNTL behavior and saw that the cause why your event handler is called is not onPress prop on TouchableOpacity. This one is correctly disabled by disabled prop. What is actually called is the onPress prop on TestChildTouchableComponent. One way to check this is to rename onPress prop on TestChildTouchableComponent to onPress2 or what ever.

As you see our fireEvent implementation is quite rudimentary, as we do not try to completely emulate React Native runtime env (this is unlike React Testing Library which by using JS DOM can acutally simulate browser DOM with all it's behaviors and quirks). What our fireEvent implmentation tries to achieve is reasanable emulation of RN behavior.

Scope of the changes

This fix handles only case when composite component prop is named exactly as child host component.

• If for example we renamed composite component prop name to onTextPress and used fireEvent.textPress then it will still fire.
• But if we used the same name of onPress OR changed parents prop to e.g. onTextPress and used fireEvent.press it will not fire.

I guess that having same named event props on composite components and host components is quite common, e.g. in buttons or any kind of simple wrappers, so I think this change is a good idea.

[thymikee]

Thanks @adkenyon for your contribution ❤️ and @mdjastrzebski for cleanups!