An input or textarea element becomes the activeElement of the document (gets the input focus) when a user types into the field.
Currently, after the type function executes on an active element, document.activeElement still points to document.body and the onFocus handler is not called.
Agreed, mid-type focus should be testable, or at least configurable so that it doesn't blur on finish
This looks like an easy fix, does anyone want to take it?
An input or textarea element becomes the activeElement of the document (gets the input focus) when a user types into the field.
It's the other way around. The active element is the target of keyboard interactions which is why I don't think type() should focus but rather it should check that the target is document.activeElement || document.body || document.documentElement
The event target of a key event is the currently focused element which is processing the keyboard activity. [...] If no suitable element is in focus, the event target will be the HTML body element if available, otherwise the root element.
-- https://w3c.github.io/uievents/#events-keyboard-event-order
Otherwise this tool might hide bugs in your implementation if you expect that a previous event shifted focus.
Note: implementing this will be a breaking change.
Spoke to @eps1lon a bit about this on Twitter and I'm a little bit conflicted myself. On the one hand, currently user events like click and tab do focus the target element for you, and seems like Cypress' type dispatches a click for you and additionally supports body for things like keyboard shortcuts.
On the other hand, typically a user has no way to focus an input element apart from clicking or tabbing into it first and only then typing (unless something else programmatically brought it into focus beforehand).
That being said, maybe there is value in mimicking Cypress here and performing a user click (or just focus)? As well as throwing if the target is not focusable, unless it's body as suggested by @kentcdodds on the PR.
That being said, maybe there is value in mimicking Cypress here and performing a user click (or just focus)? As well as throwing if the target is not focusable, unless it's body as suggested by @kentcdodds on the PR.
I'm a fan of this approach. @doniyor2109, could you update your PR to handle this? I think it's the best of all trade-offs.
Specifically I think we should trigger a user click on the target and throw if the target is not focusable (unless it's body).
Would you like a comprehensive isFocusable check such as https://github.com/cypress-io/cypress/blob/ee494d04ee532edec41f227ced0c639b3db78f5c/packages/driver/src/dom/elements.ts#L499-L504 ?
Yeah, I think copy/pasting/modifying (and attributing) that logic would be great 馃憤
@kentcdodds @NMinhNguyen I have triggered click on target however tests are failing. onFocus callback is not called and activeElement points to body.
Here are tests:
https://github.com/testing-library/user-event/pull/231/files#diff-b5b36d5698a23d7f6cff3f2af7795257R220-R222
Would you like a comprehensive isFocusable check such as cypress-io/cypress:packages/driver/src/dom/elements.ts@ee494d0#L499-L504 ?
What do we need this for? The DOM already has this built-in. If an element isn't focusable you can't focus it. The cypress logic was a source of very frustrating bugs and they were not able to reconcile it.
I have triggered click on target however tests are failing. onFocus callback is not called and activeElement points to body.
A click does not focus an element. Only certain platforms focus the element under the cursor on mousedown. I think macOS does not move focus on mousedown if I recall correctly.
Are you suggesting that we attempt to focus and if activeElement doesn't change to that element we throw?
@eps1lon were you thinking something like https://twitter.com/brian_d_vaughn/status/1237495862508408832 ?
The cypress logic was a source of very frustrating bugs and they were not able to reconcile it.
Thanks for the heads-up, I wasn't aware of this. Will look through their issues.
A click does not focus an element. Only certain platforms focus the element under the cursor on
mousedown. I think macOS does not move focus onmousedownif I recall correctly.
Are you referring to a user click or domNode.click()? Because I think the former should focus it, or am I being too naive here?
Are you referring to a user click or domNode.click()? Because I think the former should focus it, or am I being too naive here?
No I mean an actual mouse click that a real user does. These are split up into mousedown, mouseup, click. Only (after? before?) mousedown the element is focused on some platforms. Some don't. But that is not important for this issue. We're talking about key presses.
Are you suggesting that we attempt to focus and if activeElement doesn't change to that element we throw?
Maybe. Personally I'd very much like a comprehensive keypress user event but if that also moves focus I wouldn't use it since these are two separate concerns. Just like I wouldn't want a click() user event to move the pointer cursor I don't want a keypress to move the focus cursor.
Currently, after the type function executes on an active element, document.activeElement still points to document.body and the onFocus handler is not called.
This is contradictory. If the element is the active element then document.activeElement can't point the document.body
Personally I'd very much like a comprehensive keypress user event but if that also moves focus I wouldn't use it since these are two separate concerns.
Are you suggesting that what Cypress does with auto focusing/clicking isn't a good idea? And that the recommendation is for users to call the imperative domNode.focus() before calling userEvent.type(), and inside type throwing an error if the target is not document.activeElement || document.body || document.documentElement, where the error would guide users towards the recommended approach?
Just like I wouldn't want a click() user event to move the pointer cursor I don't want a keypress to move the focus cursor.
I think the click() user event is currently "moving the focus cursor" - do you have a problem with that too? Or only with the pointer cursor (if it were to be implemented, hypothetically speaking)?
I guess what I still don't feel good about is the inconsistency between click() and tab() performing focus for you, and type() not.
click() performing focus is quite dangerous. Hopefully they're handling platform inconsistencies. Otherwise your test results might be misleading.
There should be no inconsistency between type('Tab') and tab(). Otherwise there should be because type('a') and tab() are different actions. You wouldn't expect those to perform the same action, no?
Are you suggesting that what Cypress does with auto focusing/clicking isn't a good idea?
I'm not familiar with what cypress does. I find their user event handling too opaque for my taste and have been burnt by buggy implementations in the past. If you want to reduce your tests to a single statement then go for it. I don't think user-event wants to be on the same level as cypress, no? Otherwise why not just recommend using cypress or any other e2e test runner?
And that the recommendation is for users to call the imperative domNode.focus() before calling userEvent.type(), and inside type throwing an error if the target is not document.activeElement || document.body || document.documentElement, where the error would guide users towards the recommended approach?
Yes. The overhead in writing pays of very quickly in readability from my experience. Seeing explicitly which element should be focused in your test is very helpful. But that's just my opinion.
click() performing focus is quite dangerous. Hopefully they're handling platform inconsistencies. Otherwise your test results might be misleading.
In case I wasn't clear before, this is what I was referring to by the way: https://github.com/testing-library/user-event/blob/1af67066f57377c5ab758a1215711dddabad2d83/__tests__/react/click.js#L178-L199
If you want to reduce your tests to a single statement then go for it.
Personally for me, I would say that the motivation isn't really the reduced LOC in test files but a more user-friendly testing API where the user doesn't have to be aware of the intricacies of focus management, and perhaps I was being too naive but I was hoping that userEvent.type() could
domNode.focus()domNode === document.activeElement (I could be wrong as to the exact condition here, but basically check that it received focus somehow - there's further suggestions in https://twitter.com/brian_d_vaughn/status/1237495862508408832 as to how it could be done)Kind of in keeping with the spirit of RTL calling act on your behalf and treating act as an implementation detail.
At the same time, I am also onboard with your proposal to require users to call focus() - basically as long as the final implementation contains a check about the currently focused element I'm happy - whether it calls .focus() under the hood or requires you to call it yourself isn't too important at the end of the day.
I totally get the urge to hide focus away. There's a fine balance between abstracting certain aspects and requiring knowledge. I'm currently siding with "you should definitely know what focus() is" because it (hopefully) leads you to think more about accessibility. So I'd rather gatekeep devs that don't know what focus is than gatekeep users because our testing utilities helped building inaccessible components (not that this is the case here directly).
Following https://github.com/testing-library/user-event/issues/178#issuecomment-610811605 might very well work for 99% of the cases. I'm just being cautious because almost all feature requests for testing utilities are concerned with writing. Reading code is way more important though.
For consistency, would you also suggest .focus() be taken away from userEvent.click(), userEvent.tab()? Or are they fine to have auto focus?
userEvent.click()
That one is tricky. It should consider platform inconsistencies but it's mostly used in JSDOM, no? If it's run in an actual browser it should definitely consider platform inconsistencies. If it focuses on macOS + firefox (might be another combination) then I'd definitely consider this a bug.
userEvent.tab()
I mean the whole point of this is to move focus, no? Should definitely stay.
I think we're doing the right defaults at this point.
If anyone comes up with new issues, please open a new issue :)