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

[Select] Can't differentiate if select trigger was focused from mouse or keyboard #1803

Closed
dlacaille opened this issue Nov 25, 2022 · 18 comments

Comments

@dlacaille
Copy link

dlacaille commented Nov 25, 2022

Bug report

Current Behavior

There is currently no way to prevent highlighting a <Select.Trigger> when using the mouse.

image

Expected behavior

Using :focus-visible we should be able to only show a <Select.Trigger> element as focused when it is actually selected using the keyboard. Currently radix programmatically focuses the element, which prevents the browser from differentiating a keyboard focus from a mouse focus.

Reproducible example

Code Sandbox

This is a slightly modified version of the Radix UI Select example where I changed the :focus selector to .SelectTrigger:focus-visible to show that the mouse still triggers the browser's :focus-visible property.

Suggested solution

In other radix-ui components such as <Select.Item> we can use [data-highlighted], but it is not available for this component.

Additional context

An old issue resolved by the data-highlighted attribute:
#1164

Your environment

Software Name(s) Version
Radix Package(s) @radix-ui/react-select 1.1.2
React n/a @radix-ui/react-select
Browser Firefox 107.0
Assistive tech n/a n/a
Node n/a n/a
npm/yarn n/a n/a
Operating System MacOS Ventura 13.0.1
@dlacaille dlacaille changed the title [Select] Can't differentiate select trigger was focused from mouse or keyboard [Select] Can't differentiate if select trigger was focused from mouse or keyboard Nov 25, 2022
@shimbarks
Copy link

shimbarks commented Jan 23, 2023

I'm facing the same issue with <DropdownMenu.Trigger> of @radix-ui/react-dropdown-menu.

@benoitgrelard
Copy link
Collaborator

Hey @dlacaille,

Currently radix programmatically focuses the element, which prevents the browser from differentiating a keyboard focus from a mouse focus.

As you correctly put it, unfortunately the browser doesn't expose a primitive for us to tell it whether it's a keyboard or pointer focus. This is really the issue here. :focus-visible in browsers work based on a bunch of heuristics (not even necessarily aligned in each browsers) and AFAIK they all treat programmatic focus as :focus so there isn't anything we can do here.

In other radix-ui components such as <Select.Item> we can use [data-highlighted], but it is not available for this component.

This was introduced for a different reason that has nothing to do with focus-visible. The reason is because we didn't want users to rely on :focus to style the highlighted state of items as it's really an implementation detail. In the future we may for example move away from using focus and instead use aria-activedescendant.

I will close the issue as there isn't much we can do about this technically but feel free to discuss here if you have ideas you think we could explore!

@holdenmatt
Copy link

Just started using Radix and loving it so far (thank you!).

The one issue that's annoying me (relative to other frameworks I've used) is there seems to be no way to make focus rings appear only for keyboard (not mouse) input.

For example, React Aria does this by default: https://react-spectrum.adobe.com/react-aria/FocusRing.html

Mantine makes it a theme option, and keyboard-only by default: https://mantine.dev/theming/theme-object/#focusring

This flexibility is really nice from a design perspective, as you can have prominent focus rings to aid keyboard navigation without requiring them to appear for every mouse click. Many sites do this (such as this Github comment box :) ).

Adding an option for this would also make it easier to incrementally adopt Radix alongside other component libraries. For example, I'd like to use Radix for Dropdown/Select components but already have other Mantine components in my project, and want to make focus ring behavior consistent across them.

(thanks for considering!)

@mhuebert
Copy link

I also have been really enjoying radix, but find this issue is leading me to remove :focus-visible styles because they trigger during mouse interaction and are then rather confusing/unexpected (eg. using <DropdownMenu.Trigger>)

@rolanday
Copy link

This issue drove me nuts and caused me to abandon radix (and shadcn) for react-aria/rac, but I recently picked up another project that has radix and thankfully came across this post, which works well in conjunction w/ focus-visible.

<DropdownMenuContent onCloseAutoFocus={(e) => e.preventDefault()}>

credit: Thomas Stock
https://stackoverflow.com/questions/77227586/shadcn-focus-visible-after-click-outside-still-stands

@benface
Copy link

benface commented Dec 24, 2023

That's not a very accessible solution @rolanday. The trigger should be focused on close, but it shouldn't be "visibly focused".

@rolanday
Copy link

rolanday commented Dec 24, 2023

@benface , agreed, which is precisely why I've moved to react-aria for my main work (i.e., I'm unwilling to compromise on accessibility). That said, hiding the focus outline full stop, which is a common pattern from ppl trying to resolve this radix issue is also not accessible (I'm calling it a radix issue because despite above comment, this issue is not present in other UI frameworks so it's solvable despite browser limitations).

The focus ring not appearing on deselect via mouse interactions is table stakes. If you know of an accessible workaround , please please do share -- simply stating that the workarounds to address the issue are not accessibly friendly is not useful. Thanks.

@benface
Copy link

benface commented Dec 24, 2023

@rolanday – I actually do know and use a workaround, which I should have posted here before (thanks for asking me). I am using the focus-visible polyfill, which doesn't trigger on programmatic focus. So I style the focus ring with .focus-visible instead of :focus-visible.

@benface
Copy link

benface commented Dec 24, 2023

@benoitgrelard – This issue should be re-opened. It's a real problem, which I believe you'll be able to fix with the new focusVisible option of .focus().

@rolanday
Copy link

@benface , this is very helpful! Thanks you :-)

+1 for request to reopen and +100 that this is a real problem. To be frank, I'm surprised radix-ui and derivatives like shadcn-ui are so popular if devs need to always hide the focus ring in order to ensure proper mouse/touch behavior at the expense of keyboard interaction/accessibility.

@rolanday
Copy link

@benface , thanks again for the pointer -- works well. Super appreciate.

@benface
Copy link

benface commented Mar 6, 2024

I'm not sure how Headless UI does it, but it doesn't have this issue: https://headlessui.com/react/listbox

@hwride
Copy link

hwride commented Mar 12, 2024

Completely understand this is open source, but just to gauge interest, this is also somewhat of a blocker for our use of Radix Select.

@felipedeboni
Copy link

I was frustrated with this behavior to say the least. As @benface pointed out, @rolanday's solution was not very accessible. While using a polyfill is an option, it's not ideal for those of us who prefer to avoid adding more JS and CSS to the frontend.

I've found a solution that prevents autofocus only when the user uses a mouse. When using the keyboard, the focus-visible property will still work. This approach should save at least 1KB when gzipped, CSS will be a bit smaller, and it avoids adding several event listeners the polyfill would add.

Here's a snippet:

const Content = forwardRef<HTMLDivElement, DropdownMenuContentProps>(
  ({ onPointerDownOutside, onCloseAutoFocus, ...props }, ref) => {
    const isCloseFromMouse = useRef<boolean>(false);

    const handlePointerDownOutside = useCallback(
      (e: unknown) => {
        isCloseFromMouse.current = true;
        // @ts-expect-error - they don't export the PointerDownOutsideEvent
        onPointerDownOutside?.(e);
      },
      [onPointerDownOutside]
    );

    const handleCloseAutoFocus = useCallback(
      (e: Event) => {
        if (onCloseAutoFocus) {
          return onCloseAutoFocus(e);
        }

        if (!isCloseFromMouse.current) {
          return;
        }

        e.preventDefault();
        isCloseFromMouse.current = false;
      },
      [onCloseAutoFocus]
    );

    return (
      <Portal>
        <RadixContent
          ref={ref}
          {...props}
          onPointerDownOutside={handlePointerDownOutside}
          onCloseAutoFocus={handleCloseAutoFocus}
        />
      </Portal>
    );
  }
);

Content.displayName = 'DropdownMenu.Content';

@zommerberg
Copy link

zommerberg commented Jul 20, 2024

This should be reopened. @benoitgrelard

@shidoxo
Copy link

shidoxo commented Aug 8, 2024

Why this issue keeps getting closed without actually fixing the issue?
I just encountered it while using shadcn/ui, had to use the prevent default method.

@CarlosLleraMartin
Copy link

I was frustrated with this behavior to say the least. As @benface pointed out, @rolanday's solution was not very accessible. While using a polyfill is an option, it's not ideal for those of us who prefer to avoid adding more JS and CSS to the frontend.

I've found a solution that prevents autofocus only when the user uses a mouse. When using the keyboard, the focus-visible property will still work. This approach should save at least 1KB when gzipped, CSS will be a bit smaller, and it avoids adding several event listeners the polyfill would add.

Here's a snippet:

const Content = forwardRef<HTMLDivElement, DropdownMenuContentProps>(
  ({ onPointerDownOutside, onCloseAutoFocus, ...props }, ref) => {
    const isCloseFromMouse = useRef<boolean>(false);

    const handlePointerDownOutside = useCallback(
      (e: unknown) => {
        isCloseFromMouse.current = true;
        // @ts-expect-error - they don't export the PointerDownOutsideEvent
        onPointerDownOutside?.(e);
      },
      [onPointerDownOutside]
    );

    const handleCloseAutoFocus = useCallback(
      (e: Event) => {
        if (onCloseAutoFocus) {
          return onCloseAutoFocus(e);
        }

        if (!isCloseFromMouse.current) {
          return;
        }

        e.preventDefault();
        isCloseFromMouse.current = false;
      },
      [onCloseAutoFocus]
    );

    return (
      <Portal>
        <RadixContent
          ref={ref}
          {...props}
          onPointerDownOutside={handlePointerDownOutside}
          onCloseAutoFocus={handleCloseAutoFocus}
        />
      </Portal>
    );
  }
);

Content.displayName = 'DropdownMenu.Content';

@felipedeboni thanks for the snippet! It works well in our scenario and I think it's a fair workaround to achieve that behavior.

By the way JFYI, you can avoid disabling the TS check by getting the actual type of the event parameter of onPointerDownOutside with the following ugly construct:

type PointerDownOutsideEvent = Parameters<NonNullable<DropdownMenuContentProps['onPointerDownOutside']>>[0];

@zommerberg
Copy link

zommerberg commented Aug 30, 2024

@CarlosLleraMartin @felipedeboni

Sadly i think even that solution is not perfect. It only prevents the outline when clicked outside. Yet if an item is selected via a mouse it will still trigger the outline which is annoying, it should be there only when the items is selected via keyboard.

Unfortunately I wasn't able to find a good way to implement that.

Radix seems abandoned now so I switched to react-aria-components and I am using radix only for select few specific elements which I will probably replace in the future anyway.

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

No branches or pull requests