spectator.blur(..) broken in jest tests since v8.0.3

[mvindahl]

I'm submitting a...

[x] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[ ] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request
[ ] Other... Please describe:

Current behavior

spectator.blur(..) does not work in jest tests when using spectator 8.0.3.

We believe this to be due to the recent fix for #373.
Specifically, we believe the regression to be caused by the following change which alters the behavior when running jest: johncrim@9ff8bd4#diff-fdb99b18d9100abb482776b5c9d72409c00c0d64e919c825e993de6f643865a0R16

Expected behavior

It should work like it did in 8.0.2, and like it continues to do for jasmine tests.

Minimal reproduction of the problem with instructions

The regression is easily reproduced by copying the events component spec into the spectator/jest/test folder, "jestifying" the spectator import, and executing jest tests:

Using MacOS bash:

cp -r projects/spectator/test/events projects/spectator/jest/test
sed -i '' s#'@ngneat/spectator'#'@ngneat/spectator/jest'# projects/spectator/jest/test/events/events.component.spec.ts
yarn test:jest

The output will now show the blur method to not work:

Summary of all failing tests
 FAIL  projects/spectator/jest/test/events/events.component.spec.ts (12.443 s)
  ● EventsComponent › should handle blur

    Expected element to have  text 'blur', but had ''

      17 |   it('should handle blur', () => {
      18 |     spectator.blur('input');
    > 19 |     expect(spectator.query('h1')).toHaveText('blur');
         |                                   ^
[...]
Test Suites: 1 failed, 1 skipped, 28 passed, 29 of 30 total

On a side note, it would probably be a good idea to keep this jest specific test as part of the fix since focus and blur can no longer be assumed to be handled in the same way across jasmine and jest.

Environment

Checked out the most recent master from ngneat/spectator. The issue is specific to jest tests so browsers are not relevant.

Angular version: X.Y.Z


Browser:
- [ ] Chrome (desktop) version XX
- [ ] Chrome (Android) version XX
- [ ] Chrome (iOS) version XX
- [ ] Firefox version XX
- [ ] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [ ] IE version XX
- [ ] Edge version XX
 
For Tooling issues:
- Node version: v14.16.1  
- Platform: Mac 

Others:

[NetanelBasal]

@johncrim could you please check it?

[mvindahl]

I researched it a bit further.

This test, as it currently is, will fail when run as a jest test:

  it('should handle blur', () => {
    spectator.blur('input');
    expect(spectator.query('h1')).toHaveText('blur');
  });

However, if I focus the element before blurring it, it will work.

  it('should handle blur', () => {
    spectator.focus('input'); // <-- inserting this line makes the test pass
    spectator.blur('input');
    expect(spectator.query('h1')).toHaveText('blur');
  });

I guess this mostly boils down to a difference between how the jasmine spectator patch works vs. how JSDom works. The spectator patch would just mindlessly forward the blur event where as JSDom goes "hey, this element has no focus, blur makes no sense, I'll just ignore it".

So although this does break tests between 8.0.2 and 8.0.3 I'm not sure that the JSDom approach is wrong, it's just a different implementation choice. But it is confusing that blur works differently between jasmine and jest and at the very least I think that this should be documented somewhere.

[mvindahl]

Also, if it is decided to align jasmine blur() with jest blur() then it's probably far easier to change the spectator patching code than to convince JSDom to do things differently. In this case I would suggest a deprecation period where jasmine blur continues to work as it currently does but it logs a warning if blurring a non-focused element.

Anyway, now that we know what's going on it's no longer a problem for us so just offering my five cents.

[NetanelBasal]

I appreciate your investigation. This might not be a good decision to change JSDom's behavior. Adding a note to the documentation will be helpful.

Do you want to add it?

[mvindahl]

Absolutely, I'll be pleased to do so.

[mvindahl]

I chose to describe the limitation using the vague wording of "may be ignored" rather than describing the exact conditions (Spectator 8.0.3+ combined with jest) as that would probably be too much detail.

[johncrim]

Hi @mvindahl - I made the change to Spectator + jest behavior that went into 8.0.3. In general it is more correct now in Jest in that it is consistent with browser behavior (and relies on JSDOM handling focus and blur instead of skipping jsdom), but it can break tests that rely on incorrect behavior. I should have updated docs with this info.

I did comment about this in the issue:

One thing I should point out now that Jest tests use the native (jsdom) focus() and blur() within Spectator.focus() and Spectator.blur():

Only legitimate focusable elements can be focus()ed, and only an element with focus can be blur()ed.

For example, elements that aren't normally focusable (eg a <div>) must have tabindex="0" or similar.

This is proper behavior, but this could conceivably break some Jest tests if the tests were written with the previous behavior, where any element can be focus()ed or blur()ed.

Karma/browser tests can still focus() or blur() any element.

I'm not sure how much of this should go in the docs - it seems appropriate in release notes as a potentially breaking change, but it's a one time change, and documenting correct browser focus/blur behavior is probably outside the scope of Spectator docs. I apologize for not including that, this case was not on my mind when I completed the PR.

The focus/blur patch is needed in karma tests/browsers b/c browsers don't properly focus/blur when the component is not visible. JSDOM doesn't have this limitation - my thinking is that since JSDOM is a highly functional test fake we should use it.

I think making focus/blur work 100% correctly in browsers is outside the scope of Spectator - for example, we can't update document.activeElement in a browser, and faking that would cause more problems than it would solve.

[johncrim]

Perhaps this info should be in the docs:

In Jest tests, only legitimate focusable elements can be focus()ed, and only an element with focus can be blur()ed. For example, elements that aren't normally focusable (like a <div>) must have tabindex="0" or similar to make the element focusable.

In Karma/browser tests Spectator.focus() and Spectator.blur() can be called on any element, and not all browser side effects from focusing and blurring may occur (eg document.activeElement may not be updated, depending on the browser).

This inconsistency between Jest and Karma behavior occurs because some browsers don't properly focus/blur when the component is not visible in the browser, such as when running karma; Spectator.focus() and Spectator.blur() detect when the browser did not send the expected focus/blur events, and fake events are dispatched.

The reason I was hesitant to include something like this in the docs is that it may add more confusion than it does clarity.

[mvindahl]

Hello @johncrim , thanks for your feedback. I had initially reached the conclusion that blur behaviour had been broken -- seeing some of our existing tests that would previously pass but would now fail. On closer reflection, and having investigated the details, my understanding now aligns with yours: JSDom does a better job of mimicking the browser than did the spectator focus/blur patch.

Your comment on the 373 issue explains it well; I had somehow managed to overlook that, for which I apologise.

As for the docs, I agree that we should not describe the subtle differences in painstaking detail in the docs; this would just leave people confused. How do you like my PR suggestion which adds a somewhat broadly phrased one liner to the docs?

[johncrim]

@mvindahl: I think your comment is good - it at least gives the reader something to try if they're encountering a problem with blur(), without bogging them down with possibly unnecessary details. How about adding a link to this issue if the reader needs more info on .focus() and .blur() methods?

[NetanelBasal]

We should also mention the environment.

[mvindahl]

@NetanelBasal , which level of detail would you prefer?

1. Current suggestion, arguably too broad

Note that if the element does not have focus, blur() may be ignored.

2. Explicit mention that this only applies to jest tests:

Note that if using the jest framework, blur() only works if the element is focused.

3. .. and a link to John's fine description for the curious reader:

Note that if using the jest framework, blur() only works if the element is focused. Details.

4. Even more details?

[NetanelBasal]

Option 3, thanks.

[mvindahl]

👍

I have updated the PR accordingly.