SpectatorHost.focus() is broken in a few ways when using Jest

[johncrim]

I'm submitting a...

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

Current behavior

When using jest, calls to SpectatorHost.focus() do dispatch a (fake) focus event, but the current implementation prevents automatic blur events from occurring, and also causes the toBeFocused() matcher to not work. This is due to the call to patchElementFocus() in DomSpectator.focus(), which overwrites valid and more useful jsdom focus() methods with versions that don't work as well.

The following test shows the issue, and a workaround:

import { ChangeDetectionStrategy, Component } from '@angular/core';
import { createHostFactory, SpectatorHost } from '@ngneat/spectator/jest';

@Component({
  selector: 'ui-test-focus',
  template: `<button id="button1" (focus)="countFocus('button1')" (blur)="countBlur('button1')">Button1</button>
             <button id="button2" (focus)="countFocus('button2')" (blur)="countBlur('button2')">Button2</button>`,
  changeDetection: ChangeDetectionStrategy.OnPush,
  host: {
    '[attr.tabindex]': '0',
    '(focus)': 'countFocus("ui-test-focus")',
    '(blur)': 'countBlur("ui-test-focus")'
  }
})
export class TestFocusComponent {

  private readonly focusCounts = new Map<string, number>();
  private readonly blurCounts = new Map<string, number>();

  public countFocus(id: string) {
    this.focusCounts.set(id, this.focusCount(id) + 1);
  }

  public countBlur(id: string) {
    this.blurCounts.set(id, this.blurCount(id) + 1);
  }

  public focusCount(id: string): number {
    return this.focusCounts.get(id) ?? 0;
  }

  public blurCount(id: string): number {
    return this.blurCounts.get(id) ?? 0;
  }

}

describe('Spectator focus() in jest', () => {

  const createHost = createHostFactory(TestFocusComponent);
  let host: SpectatorHost<TestFocusComponent>;

  beforeEach(() => {
    host = createHost('<ui-test-focus></ui-test-focus>');
  })

  it('SpectatorHost.focus() in jest does not track active element', () => {
    host.focus('#button1');

    // FAILS
    expect(host.query('#button1')).toBeFocused();
  });

  it('HTMLElement.focus() in jest tracks the active element', () => {
    (host.query('#button1') as HTMLElement).focus();

    // passes
    expect(host.query('#button1')).toBeFocused();
  });

  it('SpectatorHost.focus() in jest does not cause blur events', () => {
    host.focus();
    host.focus('#button1');
    host.focus('#button2');

    // FAILS: blur counts are not present
    expect(host.component.focusCount('ui-test-focus')).toBe(1);
    expect(host.component.blurCount('ui-test-focus')).toBe(1);
    expect(host.component.focusCount('button1')).toBe(1);
    expect(host.component.blurCount('button1')).toBe(1);
    expect(host.component.focusCount('button2')).toBe(1);
    expect(host.component.blurCount('button2')).toBe(0);
  });


  it('HTMLElement.focus() in jest does cause blur events', () => {
    host.element.focus();
    (host.query('#button1') as HTMLElement).focus();
    (host.query('#button2') as HTMLElement).focus();

    // passes
    expect(host.component.focusCount('ui-test-focus')).toBe(1);
    expect(host.component.blurCount('ui-test-focus')).toBe(1);
    expect(host.component.focusCount('button1')).toBe(1);
    expect(host.component.blurCount('button1')).toBe(1);
    expect(host.component.focusCount('button2')).toBe(1);
    expect(host.component.blurCount('button2')).toBe(0);
  });

});

Expected behavior

I think the best fix would be to remove patchElementFocus() from all platforms, but I'm not 100% sure why it's there (the comment says something about IE 11). My thinking in recommending this is that we want to test browser or platform capabilities as much as possible (eg if running karma on IE11, I would want the browser implementation to be used, b/c I'm testing browser compatibility).

Alternatively, patchElementFocus() could be left as is for non-jest, and overridden for jest only, and only patch the focus() and blur() methods when they're not present.

Minimal reproduction of the problem with instructions

git clone git@github.com:johncrim/jest-spectator-bugs.git
yarn
yarn test

What is the motivation / use case for changing the behavior?

Make host.focus() work as expected.

Environment

jest: 26.6.3
spectator: 6.1.2

$ ng version

Angular CLI: 11.0.3
Node: 15.3.0
OS: win32 x64

Angular: 11.0.3
... animations, cli, common, compiler, compiler-cli, core, forms
... platform-browser, platform-browser-dynamic, router
Ivy Workspace: 

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1100.3
@angular-devkit/build-angular   0.1100.3
@angular-devkit/core            11.0.3
@angular-devkit/schematics      11.0.3
@schematics/angular             11.0.3
@schematics/update              0.1100.3
ng-packagr                      11.0.3
rxjs                            6.6.3
typescript                      4.0.5

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: XX  
- Platform: Windows

Others:

[johncrim]

I'm happy to provide a PR for this and add some tests, but I'd like guidance on whether you're ok with removing patchElementFocus(), or if you'd prefer a jest-only fix.

[NetanelBasal]

We use the same code from Angular CDK. You can have a look and see what's the issue - https://github.com/angular/components/blob/master/src/cdk/testing/testbed/fake-events/element-focus.ts

[johncrim]

Damn you're fast! :) I'll take a look.

[johncrim]

Given what I can tell from the CDK comments, and given that no one else has complained about it outside of jest, it's probably safest to leave it as-is for browsers. But given that I've spent a bunch of time digging into where this is going wrong, I'm quite confident in saying it should be avoided for jest.

I can pretty easily put the same tests in karma and test in a handful of browsers to see which versions of the test work in which browsers. I'll report back.

[NetanelBasal]

Any news here?

[johncrim]

Yes: I apologize for the delay wrapping this up. I've tested this informally in karma, and the conclusion is:

• karma definitely needs the patch to reliable focus events in real browsers
• jest works properly without the patch, and works incorrectly with the patch

I've had this on my TODO list to figure out how to fix; lmk if you want a PR or if you'd prefer to fix.

[NetanelBasal]

You can create a PR.

[johncrim]

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.