[test] Use actual element over document.activeElement #20945
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Details of bundle changes.Comparing: 8f8294d...8042cac Details of page changes
|
|
I was wondering about it at some point, I liked how using document.activeElement fits closer to real-life usage. We get the guarantee that if an event moves the active element, we stay on the new element but don't like that we don't really know "where we are". No preference, it feels equality ok. Maybe there is an opportunity for a |
|
I think in almost all cases you shouldn't use |
oliviertassinari
approved these changes
May 8, 2020
| if (target !== element) { | ||
| // see https://www.w3.org/TR/uievents/#keyup | ||
| const error = new Error( | ||
| `\`keyup\` events can only be targeted at the active element which is ${prettyDOM( |
There was a problem hiding this comment.
Pretty cool. Should the logic move to testing-library later on?
There was a problem hiding this comment.
Was gonna suggest/ask the same actually 🙂
There was a problem hiding this comment.
I think most people wanted to do this in user-event but also with automatically focusing. I think this is fine for app-tests but for component-tests we want to have a bit more control. Basically be as explicit as possible (fire keyDown, keyUp rather than user-event.keypress) but catch non-sensical dispatches such as fireEvent.keyDown(<section />).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While it's definitely a good idea to make sure the keyboard event target is the active element, it hurts readability in many cases. It's oftentimes not immediately clear what element is currently focused.
We now lint against
fireEvent.key*(document.activeElement)while also ensuring that the target is focused. If it's not we throw and include the currently focused element.