Skip to content

fix: prevent non editable textinputs from being updated through getByTestID #1092

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Sep 13, 2022
Merged

fix: prevent non editable textinputs from being updated through getByTestID #1092

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e582ddd
fix: prevent non editable textinputs from being updated through getBy…
Sep 4, 2022
a23787e
refactor: add comment to explain double check in isTextInput function
Sep 5, 2022
9d4aaf1
Update src/__tests__/fireEvent.test.tsx
mdjastrzebski Sep 13, 2022
26d484b
Update src/__tests__/fireEvent.test.tsx
mdjastrzebski Sep 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/__tests__/fireEvent.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,24 @@ test('should not fire on non-editable TextInput', () => {
expect(onChangeTextMock).not.toHaveBeenCalled();
});

test('should not fire on non-editable host TextInput', () => {
const testID = 'my-text-input';
const onChangeTextMock = jest.fn();
const NEW_TEXT = 'New text';

const { getByTestId } = render(
<TextInput
editable={false}
testID={testID}
onChangeText={onChangeTextMock}
placeholder="placeholder"
/>
);

fireEvent.changeText(getByTestId(testID), NEW_TEXT);
expect(onChangeTextMock).not.toHaveBeenCalled();
});

test('should not fire on non-editable TextInput with nested Text', () => {
const placeholder = 'Test placeholder';
const onChangeTextMock = jest.fn();
Expand Down
16 changes: 15 additions & 1 deletion src/fireEvent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { ReactTestInstance } from 'react-test-renderer';
import act from './act';
import { filterNodeByType } from './helpers/filterNodeByType';

type EventHandler = (...args: any) => unknown;

Expand All @@ -8,8 +9,21 @@ const isHostElement = (element?: ReactTestInstance) => {
};

const isTextInput = (element?: ReactTestInstance) => {
if (!element) {
return false;
}

const { TextInput } = require('react-native');
return element?.type === TextInput;
// We have to test if the element type is either the TextInput component
// (which would if it is a composite component) or the string
// TextInput (which would be true if it is a host component)
// All queries but the one by testID return composite component and event
// if all queries returned host components, since fireEvent bubbles up
// it would trigger the parent prop without the composite component check
return (
filterNodeByType(element, TextInput) ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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();
}

filterNodeByType(element, 'TextInput')
);
};

const isTouchResponder = (element?: ReactTestInstance) => {
Expand Down
2 changes: 1 addition & 1 deletion src/helpers/filterNodeByType.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ import * as React from 'react';

export const filterNodeByType = (
node: ReactTestInstance | React.ReactElement,
type: React.ElementType
type: React.ElementType | string
) => node.type === type;