vuejs / test-utils

Vue Test Utils for Vue 3
https://test-utils.vuejs.org
MIT License
1.04k stars 244 forks source link

Bug: focus active #2176

Closed hzhoup closed 1 year ago

hzhoup commented 1 year ago

Describe the bug

when i test the focs event, the document.activeElement is the full body, like this: image

To Reproduce

https://stackblitz.com/edit/github-3auzsz?file=src%2Fcomponents%2F__tests__%2FHelloWorld.spec.ts,src%2Fcomponents%2FHelloWorld.vue

Expected behavior

the document.activeElement is wrapper.element

Related information:

"@vue/test-utils": "^2.4.1" "jsdom": "^22.1.0" "vitest": "^0.34.3"

Additional context

cexbrayat commented 1 year ago

Hi @hzhoup

As you are triggering the focus event on the whole wrapper, that looks normal to me. What are you trying to test?

hzhoup commented 1 year ago

I want to test the wrapper focus state, in the MDN Doc, i know The activeElement read-only property of the Document interface returns the Element within the DOM that currently has focus.. I think when i trigger focus, the document.activeElement will be return button DOM instead of body, like try that on the browser.

The preview link: https://stackblitz.com/edit/github-3auzsz-nb39qq?file=src%2Fcomponents%2F__tests__%2FHelloWorld.spec.ts,src%2FApp.vue

On the stackblitz side preview is always iframe, i open port new tab, in the browser console document.activeElement will be show: image

freakzlike commented 1 year ago

Right now, trigger('focus') is only dispatching a DOM event and not calling element.focus(). This seems to have different behavior, as element.focus() is working with your repro: https://stackblitz.com/edit/github-3auzsz-1iuaj8?file=src%2Fcomponents%2F__tests__%2FHelloWorld.spec.ts

I'm not sure if we should call the methods on the DOM element (if exists) instead of dispatching a DOM event? I've created a PR with a test implementation #2180


Just an opinion on your test: You are using the wrapper as your work element (which is actually the button):

await wrapper.trigger('focus');
expect(document.activeElement).toBe(wrapper.element);

I would prefer to explicit use the button instead of wrapper. In that case it's clear the button should be the active element and will make the test still work when you need to add some <div> around or so.

const button = wrapper.find('button');
await button.trigger('focus');
expect(document.activeElement).toBe(button.element);
hzhoup commented 1 year ago

@freakzlike thaks, it helped me.

if I call button.trigger('focus') explicitly, then document.activeElement's expect must be correct. There's no doubt about it, and I don't think there's any need for testing.

In my component, there may be certain factors that prevent it from being in focus. And the root component is a dynamic component, not necessarily a button.

I don't think explicitly calling button.trigger('focus') meets my test need.

marina-mosti commented 1 year ago

Hey @hzhoup I was curious since I've had some pain testing focus events myself, so I took a hack at this.

If you open in a new tab and are very careful to not click anything, so using a keyboard shortcut for example to open the inspector you will notice that document.activeElement is actually the body as expected.

My guess is that either by mouse or keyboard you triggered the focus on the button before executing the document.activeElement call.

To be completely sure, I would add some thick border to the button on the :focus state so that you can be sure that you dont have it manually selected before executing the call. The default active element will usually be the body. In the stack page the active element is the iframe probably if you right click it to open the inspector.

What @cexbrayat suggested is correct though, and triggering the focus event on the wrapper should yield the whole wrapper as is happening in your test. The activeElement will not return the button unless you either:

  1. Mock the user focusing the button by trigger('focus')
  2. Programatically pre-focus the button for the user on the mounted hook for example
cexbrayat commented 1 year ago

Thanks @marina-mosti for taking a look. I agree with you 👍

Unless @freakzlike thinks #2180 is the way to go, I think this is expected behavior and we could close this.

freakzlike commented 1 year ago

I'm completely fine if we don't merge #2180. Adds too much complexity in my opinion

cexbrayat commented 1 year ago

👍 Let's close then