Bug/373 jest focus #473
Bug/373 jest focus #473
Conversation
In Jest, don't patch HTML Element focus() or blur() methods, b/c they break correct jsdom behavior. In browsers, improve HTML Element focus() patch so it triggers a blur on the activeElement, and updates the activeElement.
In browsers, HTMLElement.focus() always sets `document.activeElement`, but focus + blur events may or may not be sent depending on focus. This ensures that focus + blur events are always sent if appropriate, and `document.activeElement` is set.
|
Let me know if the docs need to be updated - I'm not aware of a need. |
| this._active?.addEventListener('blur', this); | ||
| } | ||
| public handleEvent(evt: Event): void { | ||
| if (evt.type === 'focus') { |
There was a problem hiding this comment.
this._focused = type === 'focus';
this._blurred = type === 'blur';There was a problem hiding this comment.
That's incorrect - we are listening for both a blur event and a focus event on different elements, that would overwrite the first event when the second event occurs.
| private _focused = false; | ||
|
|
||
| constructor(private readonly _e: HTMLElement) { | ||
| this._active = _e.ownerDocument.activeElement; |
There was a problem hiding this comment.
| this._active = _e.ownerDocument.activeElement; | |
| this.activeElement = _e.ownerDocument.activeElement; |
There was a problem hiding this comment.
I went with priorActiveElement.
| this._active?.removeEventListener('blur', this); | ||
|
|
||
| // Ensure activeElement is blurred | ||
| if (this._active && !this._blurred && this._active === this._e.ownerDocument.activeElement) { |
There was a problem hiding this comment.
Isn't the following condition more accurate?
if(!this._blurred && this.activeElement === this.element) {}There was a problem hiding this comment.
activeElement should be the previous document.activeElement when the watcher was created - I'm renaming the field to clarify that.
But I think you're right that the ownerDocument.activeElement check is wrong and potentially problematic (interesting that it wasn't caught by tests). We do expect the document.activeElement to change when the original HTMLElement.focus() method is called. Since the focus() method isn't cancellable in event handlers, I think the blur event should be sent, if it wasn't sent by the browser, regardless of how the document.activeElement has changed from calling HTMLElement.focus().
| * This is necessary, because some browsers, like IE11, will call the focus handlers asynchronously, | ||
| * while others won't fire them at all if the browser window is not focused. | ||
| * | ||
| * patchElementFocus(triggerEl); | ||
| */ | ||
| export function patchElementFocus(element: HTMLElement): void { | ||
| element.focus = () => dispatchFakeEvent(element, 'focus'); |
There was a problem hiding this comment.
Why deleting the Jaminse functionality?
There was a problem hiding this comment.
It's replaced with a better patched focus().
Also, fix potentially problematic blur check.
|
I've addressed all your feedback. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #373
What is the new behavior?
HTMLElement.focus()andHTMLElement.blur()is not applied.HTMLElement.focus()is patched the patched implementation is more robust. It calls the browser'sHTMLElement.focus()method first, and if the focus and/or blur events are not detected whenfocus()is called, it sends the fake events. This ensures thatdocument.activeElementis set (so thetoBeFocused()matcher works, and app behavior that depends ondocument.activeElementworks), and it ensures that a single focus event, and 0-1 blur events, are emitted.Does this PR introduce a breaking change?
There is no API change, but there is a change in behavior, that could conceivably break tests that depend on the old behavior (most likely to work around it). However this should be unlikely.
focus()andblur()were functionally broken in Jest, so this shouldn't break jest tests. The change for jest is that the jsdom focus() and blur() methods are no longer patched, but they didn't need to be.With the previous implementation,
focus()andblur()worked partially in karma, butdocument.activeElementwasn't set by the patched methods. The old behavior ensured that focus and blur events were always sent when their respective methods were called, but it didn't handle blurring the previous activeElement. It also didn't set the activeElement. The new implementation calls the browser'sfocus()method, which seems to always set thedocument.activeElement, but doesn't always send focus/blur events depending on the browser state. The new patch listens for browser-emitted focus/blur events, and sends fake ones (same as before) if they weren't emitted by the browser.It might be a good idea to call the browser's
blur()method first, and send a blur event only if one doesn't occur. However I haven't had to deal with this possible inconsistency between test environments and browsers, so I didn't make this fix.