Skip to content

RFC: UserEvent to throw errors in case element is disabled or not focusable #1443

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

Closed
mdjastrzebski opened this issue Aug 8, 2023 · 8 comments
Labels
discussion enhancement New feature or request

Comments

@mdjastrzebski
Copy link
Member

Describe the Feature

User Event should throw errors in case of given element is disabled in any way that prevents the interaction to run in order to improve DX.

OG User Event does throw errors in similar cases for clear(), and click()/pointer() interactions.

Note: This would be a breaking change, but according to User Event beta status would only trigger a minor change in the version.

Note 2: In the past we fireEvent was already throwing errors in the past when no enable event handler was found but that has been removed. Here are refs to the issue: #469, and PR #470

Possible Implementations

  • type() & clear() should throw helpful error when element is not editable or not focusable due to pointerEvent prop on itself or any ancestor
  • press() should throw helpful error when element is not pressable due to pointerEvents prop on itself on ancestor.

These should not throw in case no event handler is found, as user interaction can invoke multiple events handler, hence any particular event handler is optional.

Related Issues

Initial discussion

CC: @pierrezimmermannbam @MattAgn @AugustinLF @thymikee

@mdjastrzebski mdjastrzebski added the enhancement New feature or request label Aug 8, 2023
@MattAgn
Copy link
Collaborator

MattAgn commented Aug 9, 2023

Sounds great to me!
I'm not sure i understand why we removed the error throwing in fireEvent, is it to test when touchable opacitys are disabled? if that's the case, we could promote more the helper toBeDisabled() instead

@pierrezimmermannbam
Copy link
Collaborator

If I recall correctly the reasoning behind removing the error in fireEvent was that it was a test specific behaviour and we try to match production as much as possible. I'm not sure why we would want to change this for userEvent. If the OG userEvent does throw then it's definitely an argument in favour of throwing though.

Personally I like the fact that fireEvent doesn't throw because it allows me to test that my button is disabled by trying to click on it and verifying nothing happened. I'd do the same things for inputs. I'd argue it's the best way to test that it's disabled because what you really want is that user cannot interact with it so how best to test it than to try to interact with it?

For press I don't really know how we could throw errors because we go up in the tree if the element is disabled so we could throw eventually if all elements are disabled but I'm not sure it would be very valuable

@mdjastrzebski I would be curious to have your thoughts on how throwing would improve DX. Thinking of it it could be useful for debugging tests but because I want to be able to fire events on disabled elements I would not want this feature always. Maybe it could be an optional behaviour that could be configured through configure API? It would allow more flexibility and like you could enable throw while debugging test. The downside is that it makes the API more complicated and I don't know if many users would use it

@pierrezimmermannbam
Copy link
Collaborator

Thinking more about it jest native marchers are a very good alternative to test disabled elements. The only downside is you need direct access to the TextInput but you also need it for type and clear anyway.

So now I think it's a good idea to throw. I'm just wondering now if the error message should recommend jest native marchers as a way to test that an element is disabled

@mdjastrzebski
Copy link
Member Author

I've done some experiments in RTL's User Event on how do they handle disabled elements:

  • click() - no error 🙈
  • type() - no error 🙈
  • clear() - throws error if not enable or not focusable

BTW: you can tinker yourself in #1447 RTL experiments app.

If we wanted to refer to OG User Event for consistent, well-thought approach, then it's not there 🤷‍♂️ While there might be some logic with click not throwing error, there is no reason why type does not throw and clear does throw on disabled element.

@mdjastrzebski
Copy link
Member Author

Few thoughts that I have about throwing/no throwing errors.

  1. We should throw errors if user does invalid action, e.g. calling type() on something other than TextInput, or invoking any operation on composite component, as UE does not support it, and could behave incorrectly.
  2. Invoking action on disabled element is kind of grey area, one might argue that this is valid, as users wants to check that disabled button does not cause event handler to fire, but another one may argue that this is invalid action as you shouldn't type in disabled text input. There is some merit in both sides of argument.
  3. From TTD perspective not throwing errors seems a slightly better option, but from diagnosing test issues throwing seems better.

@pierrezimmermannbam
Copy link
Collaborator

I think I would throw when user uses type on non editable TextInput but never throw with press because you can have an enabled pressable around, although one may argue this is a strange pattern

@mdjastrzebski
Copy link
Member Author

I am also leaning to the direction that we should throw on invalid actions: e.g. typing or clearing disabled TextInput. OG UserEvent seem so be confusing in that regard, and warning users (by throwing error) would improve the DX as they would not have to figure out why their interaction is not working.

@mdjastrzebski mdjastrzebski changed the title feature: UserEvent to throw errors in case element is disabled or not focusable RFC: UserEvent to throw errors in case element is disabled or not focusable Sep 13, 2023
@mdjastrzebski
Copy link
Member Author

Closing as stale.

@mdjastrzebski mdjastrzebski closed this as not planned Won't fix, can't repro, duplicate, stale Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants