fix: non-editable wrapped TextInput events

[TMaszko]

Summary

[Description by @mdjastrzebski]
This PR modifies the fireEvent handling for TextInputs by checking the editable prop status of the nearest touch responder of given element. The nearest touch responder is the nearest descendant element that is both host element as well as advertises as touch responder. One such element is TextInput.

The previous version of the code, checked the editable prop status on the element itself, which could lead to situations when TextInput wrappers would execute events, e.g. changeText, focus, etc on non-editable TextInput, because the check has been limited to host TextInput and wrapping it composite TextInput coming from RN package. Any additional TextInput wrapper would not have it's editable prop checked and would refer to host TextInput editable prop.

During experiments conducted on iOS and Android, I've discovered that most of the events are blocked by editable={false} prop, but some events like layout, contentSizeChange, etc are being called anyway. This PR includes special exclusion for such events.

Fixes #1384
Fixes #1261

Test plan

Green tests

[pierrezimmermannbam]

I'm not too sure about this. It will fix this issue indeed but saying that no event should be fired if at any point we pass through a non editable TextInput is not really correct either. For instance if I did a fireEvent.press on a non editable TextInput that's wrapped by a Pressable the press would not be run while it should. This may be a lesser evil still but there may be some other problems that we haven't thought of

[pierrezimmermannbam]

In the end fireEvent is just a low level API that calls props and we try to do too much with it so it has many unexpected behavior. How I see it long term is to fix this kind of issues using the new API userEvent but it's still at its early stage. It's not ideal that we can change text of non editable TextInputs but maybe to test those cases the easiest would be to use a custom matcher instead

[TMaszko]

If we want to call fireEvent.press on a non editable TextInput that's wrapped by a Pressable the press handler shouldn't be called (at least that's the behaviour that user will experience and we already have in the browser). If touch responder isn't able to handle the event I think that's enough because it means that we base on the host component props in the first place and that should match the end user experience.

[thymikee]

@pierrezimmermannbam could you post some test cases that you suspect wouldn't work so that @TMaszko could include in this PR? With fireEvent and handling Responder API we kinda reverse-engineer the event system of RN, which may actually change depending on the platform (especially web), so expect edge cases that would be great to understand and document

[pierrezimmermannbam]

The following test will fail

test('can pres pressable that wraps non editable textinput', () => {
    const mockOnPress = jest.fn();

    const { getByPlaceholderText } = render(
      <Pressable onPress={mockOnPress}>
        <TextInput placeholder="hello" editable={false} />
      </Pressable>
    );

    fireEvent.press(getByPlaceholderText('hello'));

    expect(mockOnPress).toHaveBeenCalled();
  });

We consider all text inputs to be touch responders, so when we do a press targeting a TextInput, it is considered to be the touch responder, even though it may be non editable. So we fix a bug but introduce another, possibly more. I'm fine with this pr being merged, it looks like a lesser evil but I feel like we're trying to extend too much the fireEvent API so it will always be flawed and it should in my opinion be considered as just an API that calls a prop (in fact I think it should be renamed to fireProp). For the long term i believe we need the userEvent API to really fix those issues and keep the fireEvent API only as a way to call custom props.

[pierrezimmermannbam]

The main issue with fireEvent is that is has almost the same behavior no matter what prop we call, so we disable all events on non editable text inputs for instance, but we should still be able to call onPressIn

test('can trigger onPressIn on disabled textinput', () => {
    const mockOnPressIn = jest.fn();

    const { getByPlaceholderText } = render(
      <TextInput
        placeholder="hello"
        editable={false}
        onPressIn={mockOnPressIn}
      />
    );

    fireEvent(getByPlaceholderText('hello'), 'pressIn');

    expect(mockOnPressIn).toHaveBeenCalled();
  });

This test fails as well but it shouldn't

[TMaszko]

I don't get why this test should pass. Isn't it a proper behaviour to block events when component is basically disabled?

[pierrezimmermannbam]

The editable prop controls wether the value of the input can be modified or not but it still responds to touch events when it's not editable. You can try to press a non editable text input, the onPressIn prop is called in both cases

[mdjastrzebski]

@TMaszko, @pierrezimmermannbam I've been curious to get the understanding of how the TexInput events actually work when running in a regular RN app. I've build a small experiments app to test it, and submitted it as PR #1391.

The relevant experiment code is here: https://github.com/callstack/react-native-testing-library/pull/1391/files#diff-800ebe5060dbf16ec52e1cf9c65e7c2fa0f0cd86af32dcfdf913e13576831a70

For editable: false scenario both onPressIn and onPressOut events are being triggered on iOS:

 LOG  Event: pressIn {"changedTouches": [[Circular]], "identifier": 1, "locationX": 57.33332824707031, "locationY": 35, "pageX": 77.33332824707031, "pageY": 146, "target": 117, "timestamp": 707054389.9047917, "touches": [[Circular]]}
 LOG  Event: pressOut {"changedTouches": [[Circular]], "identifier": 1, "locationX": 57.33332824707031, "locationY": 35, "pageX": 77.33332824707031, "pageY": 146, "target": 117, "timestamp": 707054477.4037917, "touches": []}

But they are not being called on Android(!).

As for original @TMaszko issue #1384, it is correct in the respect that non-editable TextInput does not rise the focus/blur events.

I'm yet to prepare experiments involving Pressable.

[mdjastrzebski]

I've also created a second test case, when non-editable TextInput is wrapped in Pressable:

<Pressable
  onPress={handlePressablePress}
  onPressIn={handlePressablePressIn}
  onPressOut={handlePressablePressOut}
  style={styles.pressable}
>
  <TextInput
    style={styles.textInput}
    value={value}
    onChangeText={handleChangeText}
    editable={false}
    onPressIn={handlePressIn}
    onPressOut={handlePressOut}
  />
</Pressable>

In this cases, when I press on the TextInput I get following events on iOS:

 LOG  Event: pressIn {"changedTouches": [[Circular]], "identifier": 1, "locationX": 165.66665649414062, "locationY": 16.333328247070312, "pageX": 205.66665649414062, "pageY": 147.3333282470703, "target": 65, "timestamp": 717273074.8629583, "touches": [[Circular]]}
 LOG  Event: pressOut {"changedTouches": [[Circular]], "identifier": 1, "locationX": 165.66665649414062, "locationY": 16.333328247070312, "pageX": 205.66665649414062, "pageY": 147.3333282470703, "target": 65, "timestamp": 717273155.2179583, "touches": []}

So TextInputs pressIn/pressOunt event are being invoked.

On the other hand on Android I get following events:

 LOG  Event: Pressable.pressIn {"changedTouches": [[Circular]], "identifier": 0, "locationX": 146.18182373046875, "locationY": 60.727272033691406, "pageX": 146.18182373046875, "pageY": 140.72727966308594, "target": 795, "targetSurface": -1, "timestamp": 514525, "touches": [[Circular]]}
 LOG  Event: Pressable.press {"changedTouches": [[Circular]], "identifier": 0, "locationX": 146.18182373046875, "locationY": 60.727272033691406, "pageX": 146.18182373046875, "pageY": 140.72727966308594, "target": 795, "targetSurface": -1, "timestamp": 514619, "touches": []}
 LOG  Event: Pressable.pressOut {"changedTouches": [[Circular]], "identifier": 0, "locationX": 146.18182373046875, "locationY": 60.727272033691406, "pageX": 146.18182373046875, "pageY": 140.72727966308594, "target": 795, "targetSurface": -1, "timestamp": 514619, "touches": []}

So now it's Pressabless pressIn/pressOut event are being invoked.

There is a surprising and significant difference the platforms.

[mdjastrzebski]

Based on the experiments described here, and here I've came to the conclusion that there is a difference between iOS and Android in terms of handling the editable prop. On Android, making TextInput non-editable seems to prevent all events (incl. pressIn) but on iOS it does seem to prevent some events (e.g. focus) but not others (e.g. pressIn).

I hope that will help us focus the review discussion. Regarding the idea of checking touch responder editable prop in case of touch responder being TextInput that could still be a valid case but deserves more consideration, and probably some more tests.

As per @pierrezimmermannbam comment, the touch responder variable is being reset when walking up the element tree a new touch responder is encountered. So in the Pressable example given by Pierre, the host View rendered by Pressable would become the nearest touch responder when found. However, as shown above, this does not happen because even non-editable TextInput is still a valid touch responder for pressIn event.

[mdjastrzebski]

I think we need to figure out what to do with the discrepancy between iOS and Android platforms mentioned above. Both approaches seems to have a reasoning behind them, but they are not compatibile with each other. At the moment we do not support separate simulations for iOS and Android, we have one unified event processing.

The good part is that the mentioned events (pressIn,pressOut) are relatively minor for TextInput, and the major events are being ignored. The bad thing is that the event propagation also differs, as on iOS "consumes" the event preventing further propagation, while on Android the event is not "consumed" and it looks for another responder.

@pierrezimmermannbam, @TMaszko I am curious your thoughts on this topic.

[mdjastrzebski]

@TMaszko I've added tests based on #1261 that I believe should be part of this PR. They basically check if focus, changeText, submitEditing events are prevented on non-editable TextInput while layout event is allowed. We would need to add some kind of inclusion / exclusions list of events blocked by editable={false}.

Note: the new test are failing, specifically because layout event is being blocked.

[pierrezimmermannbam]

@mdjastrzebski this is very interesting, I hadn't done tests on Android, it's quite surprising indeed! I'm wondering if we want to simulate all of this behavior, including the difference between platforms. I feel like we're hitting the limitations of testing in a node environment here, specially with TextInput component being mocked in React Native preset and it feels like we'd need to reimplement the TextInput for everything to work as expected so I'm not really sure it's worth it. Maybe we should just recommend using matchers from jest-native to test disabled pressables / text inputs? I guess the issue is that fireEvent already partially handles non editable text inputs or disabled pressables which gives the impression it handles all cases which is not true. Simplifying fireEvent could be an option but it would be a huge breaking change and it may not be understood so this may not be the best option. I think it would be better to implement all of this in userEvent instead and then keep fireEvent as just an API that calls props

[mdjastrzebski]

@pierrezimmermannbam I am coming to the conclusion that if editable + touch propagation differs between the platforms, then the users should not rely on it working either iOS or Android way, as that would work only on as single platform. Therefore, we should be able to simulate it either way. It seems like going with Android approach would just be simpler, as we could shutdown all touch-related events when editable == false.

In the real world of RN apps, putting Pressable (or any other touch consuming container) around TextInput is rather rare, so we should not worry too much about this.

As for this PR, it seems to be improving our simulation by basing the editable check on nearest touch responder (i.e. host TextInput), rather than composite or host element. Let's add necessary tests, and perform necessary research using the new experiments app, but otherwise it seems like the right direction.

[mdjastrzebski]

I've prepared the PR for review:

• rebased on current main
• moved text-input related test to separate file, as they started to grow
• added support for allowing events not affected by editable prop to be called, e.g. layout
• refactored the TextInput event check to take into account only nearest touch responder instead of checking the element itself
• cleaned up the code

The subject is quite complex, so I am looking forward for a challenging review @pierrezimmermannbam, @MattAgn, @AugustinLF, @thymikee, @TMaszko :-)

[AugustinLF]

It seems like going with Android approach would just be simpler, as we could shutdown all touch-related events when editable == false.

That sounds like a fair tradeoff to me, we should however make sure that we document it.

Overall I agree that it seems like this PR makes it better. It's not perfect, but it's going to be a bit better.

[pierrezimmermannbam]

It seems like going with Android approach would just be simpler, as we could shutdown all touch-related events when editable == false.

That sounds like a fair tradeoff to me, we should however make sure that we document it.

Overall I agree that it seems like this PR makes it better. It's not perfect, but it's going to be a bit better.

I agree, I think this is a good tradeoff for now!

[mdjastrzebski]

@pierrezimmermannbam @AugustinLF @MattAgn From my perspective this PR is ready for merging. I'm looking forward for code review approval or further change request for this PR.

[mdjastrzebski]

LGTM but I've got quite involved in the PR, so looking for more objective reviewers.

[AugustinLF]

Left a small clarification regarding the test name/comment, but nothing that prevents from merging. And since I don't have a good suggestion (I'm not sure what exactly this test is testing), it's good to be merged for me :)

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (674b2f9) 96.85% compared to head (ab70817) 96.85%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1385   +/-   ##
=======================================
  Coverage   96.85%   96.85%           
=======================================
  Files          51       51           
  Lines        3398     3399    +1     
  Branches      506      507    +1     
=======================================
+ Hits         3291     3292    +1     
  Misses        107      107           

Impacted Files	Coverage Δ
src/fireEvent.ts	100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.