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

feat: User Event type() #1396

Merged
merged 8 commits into from
Jul 24, 2023
Merged

feat: User Event type() #1396

merged 8 commits into from
Jul 24, 2023

Conversation

mdjastrzebski
Copy link
Member

@mdjastrzebski mdjastrzebski commented Apr 28, 2023

Summary

Minimal User Event type() implementation including initial User Event setup() API mimicking RTL's one.

Scope:

  • Basic User Event API:
    • userEvent.setup
    • wait utility
  • type() API:
    • Perform experiments on single-line and multi-line TextInputs
    • Basic single-line implementation and tests
    • Basic multi-line implementation and test
    • Works correctly with both managed and un-managed TextInput
    • Works correctly when managed TextInputs performs input transformation
    • Support for Backspace button
    • Support for fake timers (modern & legacy)
    • Edge cases:
      • Restrict API to host TextInput elements
      • Works correctly with not all event handlers attached
    • Docs:
      • API Docs
      • TSDoc comments (types and functions)

Exclusions:

  • Simulating target and eventCount event props (just pass 0 there)

Test plan

Added tests covering 100% specs

@mdjastrzebski mdjastrzebski force-pushed the feat/user-event-type branch 2 times, most recently from 65b98db to 803d8ed Compare May 2, 2023 22:08
@mdjastrzebski mdjastrzebski changed the base branch from main to refactor/fire-event-cleanup May 2, 2023 22:08
Base automatically changed from refactor/fire-event-cleanup to main May 3, 2023 10:30
@mdjastrzebski mdjastrzebski force-pushed the feat/user-event-type branch 2 times, most recently from 8a40e8e to acf58dc Compare May 4, 2023 08:20
@mdjastrzebski mdjastrzebski marked this pull request as ready for review May 5, 2023 11:11
@mdjastrzebski mdjastrzebski changed the title feat: User Event type() [WIP] feat: User Event type() May 5, 2023
@mdjastrzebski mdjastrzebski force-pushed the feat/user-event-type branch 2 times, most recently from e0ce685 to 4cd2d35 Compare May 10, 2023 09:25
</Text>
);

dispatchOwnHostEvent(screen.getByTestId('text'), 'press', TOUCH_EVENT);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't find this naming very clear, I understood what it meant but it needed some reflexion. Not sure I could come up with a better one though this one is tricky. How about dispatchHostEventWithoutBubbling?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah naming here is difficult.

As far as I understand the React event processing, capture and bubbling phases, the looking up just for the first ancestor is not actually bubbling, as bubbling would be invoking given event for all ancestors listening for it. So using bubbling might be confusing in this case.

function getEnabledEventHandler(
element: ReactTestInstance,
eventName: string
): EventHandler | null {
const touchResponder = isTouchResponder(element) ? element : undefined;

const handler = getEventHandler(element, eventName);
if (handler && isEventEnabled(element, eventName, touchResponder)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we shoud use the function isEventEnabled for userEvent. It is used for any kind of events and supports every eventName, whereas here we could have a pretty straightforward check on the editable prop. We know we want to type something and not do something else and that allows better handling for specific events, which fireEvent lacks because it's generic and I think we should use that for userEvent

Copy link
Member Author

Choose a reason for hiding this comment

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

I have to check experimentally if parent pointerEvents could block TextInput focus, if not then we could just inspect the editable prop.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I hadn't thought about that, I'll probably need to check for pointer events as well for Text and TextInput for user.press

Copy link
Member Author

Choose a reason for hiding this comment

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

So on iOS: pointerEvents="none" on either View wrapping TextInput or TextInput itself prevents TextInput from gaining focus
On Android: pointerEvents="none" on either View wrapping TextInput or prevents TextInput from gaining focus, but on TextInput itself is not preventing focus 🤦‍♂️

@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.51 🎉

Comparison is base (5bb5d2d) 97.02% compared to head (8284dbb) 97.53%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1396      +/-   ##
==========================================
+ Coverage   97.02%   97.53%   +0.51%     
==========================================
  Files          68       72       +4     
  Lines        3863     4225     +362     
  Branches      568      624      +56     
==========================================
+ Hits         3748     4121     +373     
+ Misses        115      104      -11     
Impacted Files Coverage Δ
src/fireEvent.ts 100.00% <100.00%> (ø)
src/helpers/accessiblity.ts 100.00% <100.00%> (ø)
src/helpers/deprecation.ts 77.35% <100.00%> (ø)
src/helpers/host-component-names.tsx 100.00% <100.00%> (ø)
src/user-event/event-builder/common.ts 100.00% <100.00%> (ø)
src/user-event/event-builder/index.ts 100.00% <100.00%> (ø)
src/user-event/event-builder/text-input.ts 100.00% <100.00%> (ø)
src/user-event/index.ts 100.00% <100.00%> (ø)
src/user-event/press/index.ts 100.00% <100.00%> (ø)
src/user-event/press/press.ts 100.00% <100.00%> (ø)
... and 9 more

... and 3 files with indirect coverage changes

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

@pierrezimmermannbam
Copy link
Collaborator

This looks really good! I have one question left though: a lot of code that's in dispatch-event seems to be quite similar to the code of fireEvent (e.g. getEventHandler). Is it intentional to have code duplication or could those be reused in fireEvent? Also I'm still not convinced that the check on wether the event is enabled or not should be done at dispatchOwnHostEvent level. What bothers me is that this is very generic so the check has to work for all kind of events but that can lead to the same issues that we had with fireEvent previously (lot of if conditions, hard to maintain, lot of bugs). Another thing is that if we call userEvent.type on a non editable TextInput, we'll call dispatchOwnHostEvent with the same events and wait between each events even though dispatching the events will do nothing, and I think it would be more natural to skip for instance the whole type events emitting, wdyt?

@mdjastrzebski
Copy link
Member Author

Good point, in the specific case of TextInput we can do single check on TextInput if it's enabled.

I wonder if we could do something similar for more general case, e.g. When doing press on Text child of 'Pressable'. In this case not only enabled check should be lifted out of dispatch but also locating the actual element to press (going from child Text to parent Pressable).

chore: restore tests

chore: more tests

chore: improve coverage

chore: moar tests

docs: improve the docs

chore: fix lint

refactor: code review changes

chore: fix lint

chore: tweak return type

refactor: flush microtasks after event

refactor: re-organize waits

refactor: improve dispatch event functions

refactor: remove flushMicroTasks calls
@mdjastrzebski
Copy link
Member Author

mdjastrzebski commented Jul 17, 2023

Rebased after merging userEvent.press (I didn't address code comments yet)

@mdjastrzebski
Copy link
Member Author

@pierrezimmermannbam could you take a look at this PR.

I've address the remaining comment about making TextInput enable check separately from dispatching the events. I've also greatly simplified the dispatchEvent method to make it really simple yet useful.

Finally, I've tweaked some of the press & type code to have matching code style (e.g. by using dispatchEvent).

Copy link
Collaborator

@pierrezimmermannbam pierrezimmermannbam left a comment

Choose a reason for hiding this comment

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

This looks very good! I just caught some typos in the docs but other than that this is more than what I had expected for a v1 of userEvent, i look forward to trying it!

@mdjastrzebski mdjastrzebski merged commit 574205a into main Jul 24, 2023
9 checks passed
@mdjastrzebski mdjastrzebski deleted the feat/user-event-type branch July 24, 2023 09:53
@mdjastrzebski
Copy link
Member Author

🎉 This PR has been released in v12.2.0 🚀

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

Successfully merging this pull request may close these issues.

None yet

2 participants