fix: prevent non editable textinputs from being updated through getByTestID

[pierrezimmermannbam]

Summary

Related to issue #476

It is possible to modify non editable TextInputs when querying them with a testID because the testID queries return host components whose type is of type string, therefore isTextInput is always returning false. This fixes it by checking in isTextInput if the type is either the component TextInput or the string TextInput.

Test plan

Added a test case to check non editable text inputs cannot be changed when queried by testID

[AugustinLF]

Overall I think it looks good. If you could just add some comment to explain the composite vs host component issue, and how we have a different API depending on the type of query.

And if you do create the issue based on the discussion in #1080 (comment) (and my follow-up comment), It'd be nice if you could link it in the comment, so we know to clean-up afterward.

[pierrezimmermannbam]

Overall I think it looks good. If you could just add some comment to explain the composite vs host component issue, and how we have a different API depending on the type of query.

And if you do create the issue based on the discussion in #1080 (comment) (and my follow-up comment), It'd be nice if you could link it in the comment, so we know to clean-up afterward.

@AugustinLF I added the comment, I'll open an issue regarding the discussion in #1080

[AugustinLF]

Wondering if this should be handled directly in filterNodeByType? That'd require a bit more refactoring, and some form of host component -> composite element mapping. It would also add some overhead in branches that don't really care about it (all the Text based one for instance since we're not using byID.

So fine with me, but perhaps some other maintainer might feel different about that.

[pierrezimmermannbam]

You mean if filterNodeByType made the comparison for both the component and the component name ? It would involve that every query would need to filter composite components afterwards (except testId queries that already filter host components), I think it would introduce additional complexity and I don't see clearly how it could be beneficial so I'm not sure about it either

[AugustinLF]

You mean if filterNodeByType made the comparison for both the component and the component name ? It would involve that every query would need to filter composite components afterwards (except testId queries that already filter host components), I think it would introduce additional complexity and I don't see clearly how it could be beneficial so I'm not sure about it either

Yeah exactly. I was wondering if that would be the sort of abstraction we would like to move out of fireEvent. But given it's a very specific usecase for now, and given the complexity it implies, I agree that it's not probably worth it (hence my approval)

[mdjastrzebski]

Isn't TextInput (composite) always rendering TextInput (host) as its child? In such case we might only focus on one of these (preferably host one). Though TextInput (host) might have different props/prop naming than TextInput(composite).

Wdyt?

[mdjastrzebski]

BTW I am against having filterNodeByType to match against both type and type.name has it would get confusing what thing will you actually get back.

[pierrezimmermannbam]

Yes TextInput (composite) is always rendering a host TextInput, I was suggesting events could only be triggered on host component in the discussion on the other pr #1080 (comment) but as @AugustinLF noticed, it would be an issue for props with different naming. This could maybe be fixed by mapping native props name with composite components props name but it seems rather complicated and I'm not sure such a mapping is entirely possible. Also props from custom components that do not match any native prop couldn't be triggered through fake events anymore

[mdjastrzebski]

re query returning host components: iirc we had this assumption with @thymikee that all of the queries should always return host components (maybe except the UNSAFE_getBy... queries), just like in RTL where you are given actual DOM components (like div, etc), and not composite components as only host components result directly with "visible" UI.

re TextInput predicate: we have have at least two options for now

1. Find out which host TextInput prop is set by composite TextInput component when it has editable=false.
2. Assuming that component structure is fixed we might reach from host TextInput to parent composite TextInput using node.parent to verify its props.

Both of the methods rely in some way on "internal" component structure from React Native, the question is which is less likely to change. I suspect that 1st option might be more stable as it probably refers to touch event handling, but we would need to research which prop is actually being set when editable=false.

@pierrezimmermannbam, @AugustinLF, @thymikee which one would you pick, do you see other alternatives?

Additionally we should create a test case where we test whether our simulation works fine, so that we are able to detect when things break. That probably should be something like:

test(`fireEvent handles implicitly editable TextInput', () => {
  const onChange = jest.fn();
  render(<TextInput testID="subject" onChange={jest.fn} />);
  const subject = screen.getByTest("subject");
  expect(subject.type).toBe("TextInput");
  fireEvent.change(subject, "new value");
  expect(onChange).toHaveBeenCalledWith("newValue");
}

test(`fireEvent handles explicitly editable TextInput', () => {
  const onChange = jest.fn();
  render(<TextInput testID="subject" onChange={jest.fn} editable />);
  const subject = screen.getByTest("subject");
  expect(subject.type).toBe("TextInput");
  fireEvent.change(subject, "new value");
  expect(onChange).toHaveBeenCalledWith("newValue");
}

test(`fireEvent handles non-editable TextInput', () => {
  const onChange = jest.fn();
  render(<TextInput testID="subject" onChange={jest.fn} editable={false} />);
  const subject = screen.getByTest("subject");
  expect(subject.type).toBe("TextInput");
  fireEvent.change(subject, "new value");
  expect(onChange).not.toHaveBeenCalled();
}

[mdjastrzebski]

I've proposed a slightly different solution in #1080 where instead of checking if element.type === 'TextInput' I use element.parent.type === TextInput for detecting host TextInput. It seems we have two very similar PRs now :-)

Both are a bit fragile, as they depend on RN implementation details, this PR depends on host component naming, and #1080 PR depends on composite/host component structure. As written in previous comments I'm not sure which one is more stable, but either way we should add tests like the ones I've proposed in a comment above in order to detect when our simulation breaks due to RN changes.

@pierrezimmermannbam @AugustinLF wdyt?

[pierrezimmermannbam]

I've proposed a slightly different solution in #1080 where instead of checking if element.type === 'TextInput' I use element.parent.type === TextInput for detecting host TextInput. It seems we have two very similar PRs now :-)

Both are a bit fragile, as they depend on RN implementation details, this PR depends on host component naming, and #1080 PR depends on composite/host component structure. As written in previous comments I'm not sure which one is more stable, but either way we should add tests like the ones I've proposed in a comment above in order to detect when our simulation breaks due to RN changes.

@pierrezimmermannbam @AugustinLF wdyt?

I think the tests are indeed a good addition. Regarding the implementation I can't see any decisive argument in favor of either one, I think both are about equivalent. They are indeed a bit fragile but I can't think of a more robust implementation that wouldn't require a lot of changes so I'd say it's a good place to start

[mdjastrzebski]

@thymikee pls advise which approach you seem as more robust:

1. Matching host TextInput using element.parent.type === TextInput (parent being RN TextInput)
2. Matching host TextInput using element.type === 'TextInput' (host component string name)

Or perhaps you have some other alternative for handling passing host TextInput to fireEvent.

[AugustinLF]

I'm with @pierrezimmermannbam here, I don't really care, either implementation will work. I think the most important going forward is to either fix our queries to return only one type, or figure a way for our types to make sure we don't make this mistake again.

[mdjastrzebski]

Looks good! Let's get this fix merged.