Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Dialog component in combination with http-fetch in a watcher breaks autofocus #680

Open
heladrion opened this issue Feb 13, 2024 · 5 comments · May be fixed by #685
Open

[Bug]: Dialog component in combination with http-fetch in a watcher breaks autofocus #680

heladrion opened this issue Feb 13, 2024 · 5 comments · May be fixed by #685
Assignees
Labels
bug Something isn't working

Comments

@heladrion
Copy link

Environment

Development OS: Windows 11 22631.3007
Node version: 20.9.0
Package manager: npm@10.1.0
Radix Vue version: 1.4.3
Vue version: 3.4.18
Client OS: Windows 11 22631.3007
Browser: Chrome 121.0.6167.161

Link to minimal reproduction

https://stackblitz.com/edit/vitejs-vite-upttnb?file=src%2FApp.vue

Steps to reproduce

Using Chrome on Windows - Firefox worked! Don't know about Mac/Linux

  • Click on "Edit Profile"
    => The focus is on the first field
  • Close and open "Edit Profile"
    => The focus is still on the first field
  • Try it as often as you like. It always works, as it should obviously

  • Now type something into the edit field and click again on "Edit profile"
    => The autofocus of the first field is gone and will forever be gone. Doesn't matter how often you close and open it.

Describe the bug

After typing something into the edit field the autofocus in die Dialog component doesn't work anymore. The edit field itself triggers a watcher which fires an http-fetch.
To encounter the bug the fetch has to succeed. If the fetch fails the bug won't happen.

Expected behavior

The autofocus should focus the first element.

Context & Screenshots (if applicable)

Some things I noticed:

  • It works after removing the Close-Icon from the Dialog.vue
  • It works when the fetch is called via a HTML-Button Click.
  • The error is somehow related to the MutationObserver and the await nextTick(). But the relation to fetch is completely unknown.

The await nextTick() fires the MutationObserver because the icon gets loaded not on mount, but delayed. During the MutationObserver handling the init isn't really done yet, so the focus gets reset to the dialog itself. After the nextTick the document.activeElement gets used, which is already changed to the dialog, due to the MutationObserver.

I mean, this could be circumvented with some flags, or saving the activeElement before nextTick. But why does it only happen after a fetch in a watcher. This is baffling. Maybe someone can figure it out. I'm on day 3 meanwhile.

Initially the error was in a bigger project.

There I had a teleport on the dialog <!-- teleport start --><!-- teleport end --> which triggered the MutationObserver while opening. And vue-query which fired the fetch in a watcher.

@heladrion heladrion added the bug Something isn't working label Feb 13, 2024
@zernonia zernonia self-assigned this Feb 14, 2024
@heladrion
Copy link
Author

Hello!

Thank you for looking into it!
I've seen the fix.

I've spend so much time today again on it and also saw some strange behavior when removing nextTick() completely. I often - not always - got an endless update loop.

I tried three other solutions today

  1. Moving
    const previouslyFocusedElement = document.activeElement as HTMLElement | null
    between
  const container = currentElement.value
  //--> here
  await nextTick()

That way we can remember the activeElemement, which gets changed by the MutationObserver. But this might break something else, when during nextTick the focus gets changed.

  1. Checking, if the container equals document.activeElement
    const hasFocusedCandidate = container.contains(previouslyFocusedElement)
    If yes, set hasFocusedCanditate to false
    const hasFocusedCandidate = container.contains(previouslyFocusedElement) && document.activeElement !== container

That way we would rely on the implementation of the handleMutation, that it changes the focus to container on failure. Don't like it.

  1. Using a flag to disable the handleMutations.
initEffectsActive += 1
await nextTick().finally(() => initEffectsActive -= 1);
  function handleMutations(mutations: MutationRecord[]) {
    if(initEffectsAtive > 0)
      return;
    const isLastFocusedElementExist = container.contains(lastFocusedElementRef.value)
    if (!isLastFocusedElementExist)
      focus(container)
  }

This would be actually my preferred way, as it is clear, that we don't want the MutationObserver to fix the focus, while we haven't init ourself during the watchEffect. As the watchEffect actually has the task to set the focus initially.

And if some user-code sets the focus during nextTick somewhere else the init routine wouldn't mess with it, as hasFocusedCandidate would be true as a result.

It is a counter, as the async function gets called twice before the first is actually finished.

@heladrion
Copy link
Author

Sorry for spamming, I've optimized the 3rd option.
I've never done a pull request before. But I could try tomorrow. It is already late here.

I wrote this code here blind without IDE, might have still typos.

function useAsyncExecuting<T>(promise: () => Promise<T>)
{
  let counter = 0;

  async function execute()
  {
    try
    {
	counter += 1;
        return await promise();
    }
    finally
    {
        counter -= 1;
    }
  }

  function isExecuting()
  {
     return counter > 0;
  }

  return {
    execute,
    isExecuting
  }
}


const { execute: executeNextTick, isExecuting: isExecutingNextTick }  = useAsyncExecuting(nextTick);
  function handleMutations(mutations: MutationRecord[]) {
    // Do not handle mutations for the duration of nextTick during the initialization
    // The initialization will take care of finding and focusing the first element
    if(isExecutingNextTick())
      return;
    const isLastFocusedElementExist = container.contains(lastFocusedElementRef.value)
    if (!isLastFocusedElementExist)
      focus(container)
  }

During the async watchEffect:

await executeNextTick();

@zernonia
Copy link
Collaborator

Thanks for digging into this @heladrion ! Instead of using a workaround for this.. I might need to might the root cause of why the fetch is causing the nextTick to behave differently, causing the previouslyFocusedElement to be different.

FIY: I've tried your suggest solution and it's breaking tests. So it might not be feasible.

@zernonia
Copy link
Collaborator

zernonia commented Feb 15, 2024

After testing for quite a bit, I believe your proposed solution #1 previouslyFocusedElement should fix the issue. As it make sense to capture the previously focused element before we have the nextTick.

@heladrion
Copy link
Author

I digged today into Vue itself with the SFC Playground, but couldn't find any differences between the calls. But it is super hard to debug.

Yes #1 previouslyFocusedElement should do the trick. But the handleMutation is still happening and the handleMutation temporarily moves the focus to the body. Before the actual autofocus-onMount call. I'm scared, that this might just hide the problem, but I guess every solution has the problem in the end :/

I fixed version #3
https://github.com/heladrion/radix-vue/commits/fix(FocusScope)-autoFocus-not-working%2C-when-fetch-called-during-a-watcher/
All tests are ok, but I also had to change AlertDialogContent.vue

    @open-auto-focus="
      () => {
        nextTick(() => {
          cancelElement?.focus({
            preventScroll: true,
          });
        });
      }
    "

to the following

    @open-auto-focus.prevent="(event) => {
      cancelElement?.focus({
        preventScroll: true,
      })
    }"

which is actually more in line with radix-ui
https://github.com/radix-ui/primitives/blob/c31c97274ff357aea99afe6c01c1c8c58b6356e0/packages/react/alert-dialog/src/AlertDialog.tsx#L131

If someone wants to debug it deeper on Chrome, here is the Vue SFC Playground URL for localhost on port 5173. The files are encoded in the URL, so it should work.
localhost:5173 Playground

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants