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

Popover enabling :focus-visible state on mouse interaction #1694

Closed
sibbng opened this issue Jul 18, 2022 · 17 comments · Fixed by #2347
Closed

Popover enabling :focus-visible state on mouse interaction #1694

sibbng opened this issue Jul 18, 2022 · 17 comments · Fixed by #2347
Assignees

Comments

@sibbng
Copy link

sibbng commented Jul 18, 2022

What package within Headless UI are you using?

@headlessui/react
@headlessui/vue

What version of that package are you using?

v1.6.6

What browser are you using?

Chrome

Reproduction URL

https://headlessui.com/react/popover
https://stackblitz.com/edit/vitejs-vite-93uk6f?file=src/App.jsx

Describe your issue

When I click Popover.Button it enables :focus-visible state. AFAIK it should do that only with keyboard interactions. At least that's how regular HTML buttons work.

@sibbng sibbng changed the title Popover enabling :focus-hover state on mouse interaction Popover enabling :focus-visible state on mouse interaction Jul 18, 2022
@leonwright
Copy link

I see this behaviors but I am not sure if this is on purpose or not. One thing I do know is that once the popover disappears the focus should be disabled. Can someone confirm whether it is supposed to be enabled or not at all? I would love to help with this issue.

@sibbng
Copy link
Author

sibbng commented Jul 19, 2022

This can't be on purpose. This is not how :focus-visible is supposed to work. Also if you clicked an <a> or a <button> before interacting with Popover.Button, this doesn't happen.

@sibbng
Copy link
Author

sibbng commented Jul 21, 2022

Removing the following lines fixes the issue. In my opinion these manual focus calls are unnecessary. If I click somewhere with my mouse, I know where my focus is going to.

if (!isFocusableElement(target, FocusableMode.Loose)) {
event.preventDefault()
button?.focus()
}

Also there are similar focus related issues with other components too.

Modal component:

  1. Click "Open Dialog"
  2. Press Tab on your keyboard, "Got it, thanks!" button will get its focus-visible styles as expected
  3. Click outside
  4. Unexpectedly "Open Dialog" button has focus-visible styles despite closing the modal with mouse.

Tabs component:

  • Clicking on the second tab will cause focus flash around the tab.

@gcavanunez
Copy link

Yup just encountered this on the vue tab component, odd enough when I click another button on the screen the behavior becomes the expected one. Not sure if some off ergonomic around focus-visible or the proposed fix covers it.

Reproduction URL
https://stackblitz.com/github/gcavanunez/unknown-tab/tree/focus-visible-issue

@rookbreezy
Copy link

rookbreezy commented Jul 26, 2022

I get :focus-visible on links when a modal is opened with a mouse. For some reason I could only reproduce it with Next.js

Minimal reproduction: https://stackblitz.com/edit/github-vhafgm?file=pages/index.tsx

@adamwathan
Copy link
Member

Hoping we can improve this but it might be out of our hands — we do need to programmatically focus things a lot in Headless UI for accessibility reasons and often the browser treats that programmatic focusing as if it were keyboard driven and triggers a focus indicator even though I totally agree that we don't want it to.

On our own sites we use the PostCSS focus-visible polyfill still and not the native :focus-visible pseudo class because it behaves better in this way. Fingers crossed there's some trick we can come up with that works though — hate seeing focus styles when using the mouse.

@Archetipo95
Copy link

I have to click 2 times on the popover here: https://headlessui.com/vue/popover and the only browser that works at first click is firefox. Any update on this?

@kripod
Copy link
Contributor

kripod commented Oct 10, 2022

As a quick, rudimental fix, the following helped me out for the Tab component in React:

<Tab
  onClick={() => {
    requestAnimationFrame(() => {
      if (document.activeElement instanceof HTMLElement) {
        document.activeElement.blur();
      }
    });
  }}
></Tab>

@victor-ponamariov
Copy link

victor-ponamariov commented Jan 2, 2023

Any update on this?

In VueJS I was able to fix the first focus (when we click on the PopoverButton) by using this:

const onPopoverClick = async () => {
  await nextTick()
  document.activeElement.blur()
}

But how to catch the event when Popover closes? Popover doesn't fire any events. It's possible, I guess, to put a global event handler that blurs Popover, but looks hacky.

Also, if I set PopoverButton to be a link (by using as="a"), the issue disappears, but the tab focus stops working. Maybe it's easier to fix in this case, since Popovers are often used in navigation which is typically a set of links. So I would prefer to use a link, but how to make the tab focus work in this case?

@paulwongx
Copy link

Not sure if it's just me, but even after adding the postcss-focus-visible polyfill in Tailwind 3.0+ using JIT, it still doesn't work on the popover as a default button. Instructions here on adding postcss-focus-visible in postcss.config.js
tailwindlabs/tailwindcss#3325 (comment)

@RobinMalfait
Copy link
Member

Hey, thank you for this bug report! 🙏

This should be fixed by #2347, and will be available in the next release.

However, it will require a bit of setup to make it behave how you want exactly because right now there is no way to control this behavior via a browser API.

I wrote more about the solution in the PR itself and how you can make it work: #2347 (comment)

You can use the insiders builds to play with the fix:

  • npm install @headlessui/react@insiders.
  • npm install @headlessui/vue@insiders.
  • npm install @headlessui/tailwindcss@insiders.

I also updated the original reproduction URL shared by @sibbng using the above versions, I also made sure to:

  • Add the @headlessui/tailwindcss plugin to the tailwind.config.js
  • Updated the focus-visible: variants in the template with ui-focus-visible:

Link: https://stackblitz.com/edit/vitejs-vite-mggdgh?file=src/App.jsx

@rczobor
Copy link

rczobor commented Mar 12, 2023

Hey, thank you for the fix!
I played with the @insiders version. While it works great for resolving this exact issue, it doesn't seem to be working for child elements using group.

I added group-ui-focus-visible:bg-black to the chevron: https://stackblitz.com/edit/vitejs-vite-8hakuu?file=src%2FApp.jsx

@agusterodin
Copy link

It appears as if there is no group-focus-visible equivalent for the Headless UI Tailwind plugin. Am I missing something?

@ala-garbaa-pro
Copy link

You can remove focus-visible by adding this classname focus-visible:outline-none:

 <Popover className="relative">
          <Popover.Button className="... focus-visible:outline-none">
            <VscInfo className="ml-4 h-7 w-7 cursor-pointer text-gray-200" />
          </Popover.Button>
...

@logemann
Copy link

You can remove focus-visible by adding this classname focus-visible:outline-none:

you just saved me from more hours searching for the right flag. Thanks.!

@ariaspabloi
Copy link

Wow, is 2024 and this has not yet been fixed (at least in the Popover component).

@sibbng
Copy link
Author

sibbng commented Aug 26, 2024

There is a standard that disables the visible focus state on manual focus() calls, but it is currently only implemented by Firefox: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/focus#focusvisible

I have come up with this monkey patch that specifically targets the Popover behavior. It also lazily restores focus to the trigger if the next input is a keyboard event.

Note that I don't use this function myself. Since it overrides some globals, it may break your application code. Please test thoroughly if you plan to use it.

if (typeof window !== "undefined" && !globalThis.popover_patched) {
  globalThis.popover_patched = true
  
  let pointerDown = false
  addEventListener("click", () => { pointerDown = true }, true)
  
  const _focus = HTMLElement.prototype.focus;
  HTMLElement.prototype.focus = function () {
    if (pointerDown && this.dataset?.headlessuiState?.includes("open")) {
      const handler = (e) => {
        _focus.bind(this)();
        if(event.code === "Space") this.click()
      }
      addEventListener("keydown", handler, {once:true})
      addEventListener("pointerdown", () => {removeEventListener("keydown", handler)},{once:true})
      return
    };
    _focus.bind(this)();
    pointerDown = false
  };
  
  const _preventDefault = Event.prototype.preventDefault;
  Event.prototype.preventDefault = function () {
    console.log(this.target.dataset?.headlessuiState);
    if (this.target.dataset?.headlessuiState?.includes("hover")) return;
    _preventDefault.bind(this)();
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.