fix(trigger('focus')) added natural to jsdom behavior #1777

Th3Un1q3 · 2021-01-29T09:04:49Z

When I have a component that manipulates the focus of multiple inputs, I expect that wrapper.trigger('focus') would really focus an input, but actually, it does not.

This PR introduces a way of triggering some kind of events more closely to their natural behavior.

What kind of change does this PR introduce? (check at least one)

Does this PR introduce a breaking change? (check one)

Yes
No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

It's submitted to the dev branch.
When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
All tests are passing: https://github.com/vuejs/vue-test-utils/blob/dev/.github/CONTRIBUTING.md#development-setup
New/updated tests are included

If adding a new feature, the PR's description includes:

A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:
Added a test case and a fix for wrapper.trigger.

Added test case and the fix for wrapper trigger

…r-focus-js-dom

lmiller1990

Sure, I guess! I am not sure exactly what happens in a browser when you do focus - like you are supposed to trigger some other events manually. But this seems purely additive.

I wonder if we can do something similar in vue-test-utils-next?

Th3Un1q3 · 2021-02-11T08:36:29Z

Sure, I guess! I am not sure exactly what happens in a browser when you do focus - like you are supposed to trigger some other events manually. But this seems purely additive.

I wonder if we can do something similar in vue-test-utils-next?

I have not worked with -next yet. I believe when you work on the edge between vue and DOM something similar can occur. So For me simulation of natural behavior is what makes the developer experience more enjoyable.

lmiller1990 · 2021-02-11T10:46:23Z

Yep, agreed on a good DX. What I mean is we should try keep both this and -next the same; so we should port this at some point.

bel3atar · 2021-05-06T08:18:40Z

I think this PR broke the wrapper.emitted().
Emitting 'focus' doesn't put an entry there.

Supposing we have a button with @focus="$emit('focus')"
This
await button.trigger('focus'); expect(wrapper.emitted().focus).toHaveLength(1) // fails now

In a previous version this worked correctly.
@Th3Un1q3

Th3Un1q3 · 2021-05-06T09:19:03Z

I think this PR broke the wrapper.emitted().
Emitting 'focus' doesn't put an entry there.

Supposing we have a button with @focus="$emit('focus')"
This
await button.trigger('focus'); expect(wrapper.emitted().focus).toHaveLength(1) // fails now

In a previous version this worked correctly.
@Th3Un1q3

@bel3atar Hi!
I tried to reproduce what you reported, but no luck. Do you see anything I've made differently from your example?

TestNativeEvent.vue

<template>
  <div>
    <button type="button" data-test="button" @focus="$emit('focus')">test</button>
  </div>
</template>

<script>
  export default {};
</script>

TestEventChild.spec.js

import TestEventChild from '../TestNativeEvent.vue';
import { shallowMount } from '@vue/test-utils';

describe('Button focus', () => {
  it('should correctly pass emits', async () => {
    const wrapper = shallowMount(TestEventChild);

    expect(wrapper.emitted('focus')).toBeFalsy();

    await wrapper.get('[data-test="button"]').trigger('focus');

    expect(wrapper.emitted('focus')).toBeTruthy();
    expect(wrapper.emitted('focus').length).toEqual(1);
  });
});

package.json

Test output

bel3atar · 2021-05-06T10:25:43Z

@Th3Un1q3 No idea what is going on, but I can reproduce with this simple example

This test passes on the previous version.

Th3Un1q3 · 2021-05-06T10:44:59Z

@Th3Un1q3 No idea what is going on, but I can reproduce with this simple example

This test passes on the previous version.

Ok, I see the difference is that in your example button is a root element. I'll try to dig this way.

bel3atar · 2021-05-06T10:46:45Z

wrapping the button in a div doesn't change anything, the test still fails

Th3Un1q3 · 2021-05-06T12:59:12Z

wrapping the button in a div doesn't change anything, the test still fails
Tried your code and... it works for me:

import { shallowMount } from '@vue/test-utils';

const Component = {
  template: `
  <button type="button" data-test="button" @focus="$emit('focus')"/>
  `,
};

describe('Button focus', () => {
  it('should correctly pass emits', async () => {
    const wrapper = shallowMount(Component);

    expect(wrapper.emitted('focus')).toBeFalsy();

    await wrapper.find('button').trigger('focus');

    expect(wrapper.emitted('focus')).toBeTruthy();
    expect(wrapper.emitted('focus').length).toEqual(1);
    console.log(wrapper.html());
  });
});

Check your .lock file. And @vue/cli-plugin-unit-jest version. It can be related to some dependencies. At this point I can't find exact reason of it not working on one setup and working on another.

bel3atar · 2021-05-06T13:06:24Z

My project doesn't use @vue/cli-plugin-unit-jest

phegman · 2021-05-07T23:52:20Z

I think I am running into the same issue as @bel3atar. I am trying to upgrade to v1.2.0 in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/61293 but have a few failing tests related to trigger('focus'). They are:

So far I haven't been able to identify what is causing the issue, but I'll keep playing around with it and see if I can narrow it down.

lmiller1990 · 2021-05-08T08:05:57Z

1.2 (minor version bump) touches a few particularly complex and gnarly parts of the code base. That's probably what led to this regression.

If you (or someone) can find out the problem and submit a fix (or even a good reproduction and explanation, possibly isolate the problem) that would be really useful. I think we can do another really release soon (1.2.1) to fix these problems once we have a fix.

The way I usually do this is

clone this repo
link locally somehow
check out commit by commit from the previous known working state (1.1.4 I guess?), build, run the known failing spec (so discussin_reply_placeholder_spec.js)
repeat until offending commit is found

This is a brute force, kind of slow but reliable way to isolate the offending commit.

Th3Un1q3 · 2021-05-08T20:07:34Z

@phegman hi, what jsdom do you use in your tests? I suspect it can behave differently.

phegman · 2021-05-10T18:49:40Z

@Th3Un1q3 It does look like it might have something to do with the jsdom version. We are using v16.4.0 of jsdom. I downgraded to v16.3.0 of jsdom and the test passed.

I took a look at a fresh install with Vue CLI and it looks like it uses v15.2.1 of jsdom which is quite old.

So long story short my conclusion is it may have something to do with the jsdom changes in v16.4.0 🤔

phegman · 2021-05-10T18:51:59Z

@Th3Un1q3 maybe it has something to do with this: https://github.com/jsdom/jsdom/releases/tag/16.4.0

Fixed el.focus() to do nothing on disconnected elements.

Not sure what a "disconnected element" is though

phegman · 2021-05-10T19:02:12Z

Ahhh okay I think the "Fixed el.focus() to do nothing on disconnected elements." change in jsdom is the issue.

If I change my tests to use the attachTo option when mounting then the tests pass. I think it is because the element is not actually appended to the document so the focus event is not fired.

lmiller1990 · 2021-05-11T01:15:59Z

I guess that makes sense in some kind of weird DOM-like manner.

Good to hear it's fixed - seems like there isn't any work to be done here.

phegman · 2021-05-11T15:37:29Z

@lmiller1990 yeah I would say it is more of a workaround than a fix. I could see this becoming more of an issue if Vue CLI upgrades jsdom in the future. I may try and see if there is a change we can make so attachTo is not required for trigger('focus'). Might not be possible though, haven't dug in too deep yet.

Anyways thanks all for the help in debugging this 🙂

lmiller1990 · 2021-05-12T00:41:52Z

Hmm, I'm not sure it makes sense to work around what appears to be an intentional part of jsdom. It's likely that somewhere in the spec it says "you cannot focus an input not on the DOM". I'd say we should try running the same test in a real browser, without using attachTo and see what happens - I have a feeling jsdom is doing exactly what it's supposed to be.

An alternative in the meantime might just be documenting attachTo and some of the known situations you need to use it (like with focus). Happy to explore other solutions if you have any ideas though 👍

phegman · 2021-05-12T16:29:59Z

@lmiller1990 fair point, maybe it makes more sense to document this caveat with trigger('focus')

lmiller1990 · 2021-05-14T01:44:15Z

Sure, feel free to make a PR if you have a chance, or I can do it when I get some time.

landerson-gc · 2021-08-30T22:17:08Z

FYI:
While simulating interactions with https://github.com/mengxiong10/vue2-datepicker that is being portalled into a bootstrap-vue modal component and mounted using mount with attachTo: document.body.
This change was causing wrapper.find('input').trigger('focus') to only work once. To "reset" the input to be able to trigger focus on it a second time, it was necessary to call wrapper.find('input').element.blur() after the picker popup was closed.

environment:
@vue/cli-plugin-unit-jest@4.5.13
jest@24.9.0
jsdom@11.12.0

mst101 · 2022-10-11T13:28:43Z

For me, the changes in this PR are causing wrapper.find("input").trigger("focus") to stop working, so I'm currently using the workaround described in #1932 i.e.wrapper.find("input").element.dispatchEvent(new Event("focus")).

environment:
"@vue/vue2-jest": "29.1.1",
"jest": "29.1.2",
"jest-environment-jsdom": "29.1.2",

lmiller1990 · 2022-10-31T01:43:06Z

One thing you can try is ensuring you mount with attachTo: document.body. Docs. You cannot focus an element not in the DOM (browser thing).

Not sure if that is the actual issue here, but worth a try.

mst101 · 2022-11-01T09:19:53Z

Thanks. I already tried attachTo: document.body, but to no avail.

wrapper.find("input").trigger("focus") works fine in my vue-3 branch with vue-test-utils@2.1.0, but not in my vue-2 branch with vue-test-utils@1.3.0.

lmiller1990 · 2022-11-01T11:27:51Z

Hm okay that's interesting, we should just compare the two and figure out the difference. I think the trigger function is quite small (but almost identical between the two). Are you interested in taking a look?

mst101 · 2022-11-01T13:03:07Z

I just had a look, but I fear your code base is so unfamiliar to me that it would take me a long while to be of much use.

In case it helps, you can see that in the vue-datepicker project I've been contributing to, the tests in commit b013baa of the vue-2 version require wrapper.find("input").element.dispatchEvent(new Event("focus")) to pass. However, the tests in commit 4b7af0d of the vue-3 version pass fine with just wrapper.find("input").trigger("focus").

lmiller1990 · 2022-11-02T00:23:08Z

Hm okay, not sure I can fix it right now but will leave some info here for the future if someone or myself has time

VTU v1 trigger

vue-test-utils/packages/test-utils/src/wrapper.js

Line 824 in 7f2855f

const event = createDOMEvent(type, options)
VTU v2 trigger https://github.com/vuejs/vue-test-utils/blob/dev/packages/test-utils/src/wrapper.js#L849

Both use createDOMEvent.

v1:

vue-test-utils/packages/test-utils/src/create-dom-event.js

Line 98 in 7f2855f

export default function createDOMEvent(type, options) {
v2: https://github.com/vuejs/test-utils/blob/f61b8aeb84352b29e29666326939531cd81c3bd6/src/createDomEvent.ts

I guess there is some difference. I think the first step would be reproducing the bug in this code base in a test.l

Th3Un1q3 added 3 commits January 29, 2021 10:52

Fix for trigger('focus')

d9d30d1

Added test case and the fix for wrapper trigger

Merge remote-tracking branch 'origin-before-fork/dev' into fix/trigge…

6efd195

…r-focus-js-dom

Extended types notation and jsdoc

9cd50d8

Th3Un1q3 changed the title ~~Fix for trigger('focus') behavior in js dom.~~ fix(trigger('focus')) added natural to jsdom behavior Jan 29, 2021

Added type check and fallback for focus processing

0edc835

lmiller1990 approved these changes Feb 10, 2021

View reviewed changes

lmiller1990 merged commit 6e33636 into vuejs:dev Feb 10, 2021

phegman mentioned this pull request May 19, 2021

docs(test-utils): add tip about using trigger('focus') #1848

Merged

12 tasks

lmiller1990 mentioned this pull request May 20, 2021

wrapper.vm.hasFocus returns opposite value starting in v1.1.3 #1846

Closed

landerson-gc mentioned this pull request Aug 30, 2021

[Bug] Unable to trigger focus on input multiple times. mengxiong10/vue2-datepicker#639

Open

ElVisPL mentioned this pull request Nov 24, 2021

.trigger("focus") doesn't work anymore for contenteditable element #1932

Open

Super-Chama mentioned this pull request Jan 4, 2023

OHRM5X-1888: Upgrade oxd dependencies orangehrm/oxd#658

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(trigger('focus')) added natural to jsdom behavior #1777

fix(trigger('focus')) added natural to jsdom behavior #1777

Th3Un1q3 commented Jan 29, 2021 •

edited

Loading

lmiller1990 left a comment

Th3Un1q3 commented Feb 11, 2021

lmiller1990 commented Feb 11, 2021

bel3atar commented May 6, 2021 •

edited

Loading

Th3Un1q3 commented May 6, 2021

bel3atar commented May 6, 2021 •

edited

Loading

Th3Un1q3 commented May 6, 2021

bel3atar commented May 6, 2021 •

edited

Loading

Th3Un1q3 commented May 6, 2021

bel3atar commented May 6, 2021

phegman commented May 7, 2021

lmiller1990 commented May 8, 2021

Th3Un1q3 commented May 8, 2021

phegman commented May 10, 2021

phegman commented May 10, 2021

phegman commented May 10, 2021

lmiller1990 commented May 11, 2021

phegman commented May 11, 2021

lmiller1990 commented May 12, 2021 •

edited

Loading

phegman commented May 12, 2021

lmiller1990 commented May 14, 2021

landerson-gc commented Aug 30, 2021

mst101 commented Oct 11, 2022

lmiller1990 commented Oct 31, 2022

mst101 commented Nov 1, 2022

lmiller1990 commented Nov 1, 2022

mst101 commented Nov 1, 2022 •

edited

Loading

lmiller1990 commented Nov 2, 2022

fix(trigger('focus')) added natural to jsdom behavior #1777

fix(trigger('focus')) added natural to jsdom behavior #1777

Conversation

Th3Un1q3 commented Jan 29, 2021 • edited Loading

lmiller1990 left a comment

Choose a reason for hiding this comment

Th3Un1q3 commented Feb 11, 2021

lmiller1990 commented Feb 11, 2021

bel3atar commented May 6, 2021 • edited Loading

Th3Un1q3 commented May 6, 2021

bel3atar commented May 6, 2021 • edited Loading

Th3Un1q3 commented May 6, 2021

bel3atar commented May 6, 2021 • edited Loading

Th3Un1q3 commented May 6, 2021

bel3atar commented May 6, 2021

phegman commented May 7, 2021

lmiller1990 commented May 8, 2021

Th3Un1q3 commented May 8, 2021

phegman commented May 10, 2021

phegman commented May 10, 2021

phegman commented May 10, 2021

lmiller1990 commented May 11, 2021

phegman commented May 11, 2021

lmiller1990 commented May 12, 2021 • edited Loading

phegman commented May 12, 2021

lmiller1990 commented May 14, 2021

landerson-gc commented Aug 30, 2021

mst101 commented Oct 11, 2022

lmiller1990 commented Oct 31, 2022

mst101 commented Nov 1, 2022

lmiller1990 commented Nov 1, 2022

mst101 commented Nov 1, 2022 • edited Loading

lmiller1990 commented Nov 2, 2022

Th3Un1q3 commented Jan 29, 2021 •

edited

Loading

bel3atar commented May 6, 2021 •

edited

Loading

bel3atar commented May 6, 2021 •

edited

Loading

bel3atar commented May 6, 2021 •

edited

Loading

lmiller1990 commented May 12, 2021 •

edited

Loading

mst101 commented Nov 1, 2022 •

edited

Loading