[Select] Selecting an option triggers a touch event on elements positioned behind

chrisarnold commented 2 years ago

Bug report

Current Behavior

Using the Select primitive, when selecting an option, the touch event is interacting with any elements positioned underneath the list. This isn't happening on desktop on mouse click but can be replicated in Chrome when using the device simulation

https://user-images.githubusercontent.com/1044655/189703615-f4d8d640-81f4-4263-bc98-a157afce9e0a.mov

Expected behavior

When an option is selected, the menu should close and no events should be triggered on other inputs

Reproducible example

https://codesandbox.io/s/late-snowflake-39o7x7?file=/App.js

Additional context

Your environment

Software	Name(s)	Version
Radix Package(s)	@radix-ui/react-select	1.0.0
React	n/a	17
Browser	Chrome	104

benoitgrelard commented 1 year ago

Hi @chrisarnold,

Whilst I can reproduce it in emulation mode on Chrome, I cannot replicate it on an actual phone (iPhone iOS 16). Are you seeing this issue natively anywhere?

chrisarnold commented 1 year ago

Hey @benoitgrelard, hmmm that's strange... it's happening natively for me as well. On both Safari and Chrome on an iPhone running iOS 15.6.1

https://user-images.githubusercontent.com/1044655/191274645-f914c5c1-1476-495e-a85e-305c66ccc8eb.MOV

andy-hook commented 1 year ago

I was also able to repro on 15.4.1

benoitgrelard commented 1 year ago

Interesting, different behaviour in iOS 16 then?

https://user-images.githubusercontent.com/1539897/191276693-da30a99d-5417-4ca6-89ec-4ac3cf076041.mov

andy-hook commented 1 year ago

Interesting, different behaviour in iOS 16 then?

Seems so 🤔 the behaviour I'm seeing matches those posted by @chrisarnold .

benoitgrelard commented 1 year ago

That's going to make testing trickier haha, at least we can reliably reproduce in emulation mode, although not sure what to make of that or the change between iOS15 and 16 😅

cpt-westphalen commented 1 year ago

I have the same problem on desktop/Firefox right now. The click on any option starts a text selection on the elements behind. I've managed to work around this by using "user-select: none" on the CSS, but this shouldn't be the solution. :sweat_smile:

https://user-images.githubusercontent.com/97403327/191869917-c40cf54a-45cc-4647-ad9b-840d296cc5d6.mp4

andy-hook commented 1 year ago

@cpt-westphalen There's a workaround for that issue in https://github.com/radix-ui/primitives/issues/1488#issuecomment-1237705742 . I do wonder if the Firefox selection bug is symptomatic of the same underlying issue though 🤔

shyamlohar commented 1 year ago

I am also able to reproduce on select 1.0.0

benoitgrelard commented 1 year ago

@shyamlohar, just to confirm the theory, are you on iOS 15?

shyamlohar commented 1 year ago

@benoitgrelard i tried on two android phones one is a Google pixel 5 and one is redmi k20 pro (physical device) on chrome 105. let me know if i can help in any way.

vitalikda commented 1 year ago

The same issue on v1.0.0, can not reproduce on v0.1.1 Mobile chrome v105 and chrome mobile emulator

veiico commented 1 year ago

Same issue, not only on desktop chrome, but also on physical android mobile (Samsung Note 20 Ultra, chrome browser, nextjs development env). This issue makes it, sadly, not fully unusable. (Video from desktop chrome, but same behaviour on phone too)

My environment

Desktop: macOS - 12.6 chrome - 106.0.5249.103

Mobile: samsung note 20 ultra - android 12 chrome - 106.0.5249.118

Main dependencies: react - 18.2.0 @radix-ui/react-select - 1.0.0

Code snippet (tested on):

<SelectPrimitive.Root
    defaultValue={activeLink?.href}
    value={activeLink?.href}
    onValueChange={onChange}
>
    <SelectPrimitive.Trigger asChild aria-label="Food">
        <button>
            <SelectPrimitive.Value>{activeLink?.label}</SelectPrimitive.Value>

            <SelectPrimitive.Icon>
                <ChevronDownIcon />
            </SelectPrimitive.Icon>
        </button>
    </SelectPrimitive.Trigger>

    <SelectPrimitive.Portal>
        <SelectPrimitive.Content className="z-[100]">
            <SelectPrimitive.ScrollUpButton>
                <ChevronUpIcon />
            </SelectPrimitive.ScrollUpButton>

            <SelectPrimitive.Viewport>
                <SelectPrimitive.Group>
                    {links.map(({ label, id, href }) => (
                        <SelectPrimitive.Item
                            key={id}
                            value={href}
                            onPointerUp={(event) => event.stopPropagation()}
                            onClick={(event) => event.stopPropagation()}
                        >
                            <SelectPrimitive.ItemText>{label}</SelectPrimitive.ItemText>

                            <SelectPrimitive.ItemIndicator>
                                <CheckIcon />
                            </SelectPrimitive.ItemIndicator>
                        </SelectPrimitive.Item>
                    ))}
                </SelectPrimitive.Group>
            </SelectPrimitive.Viewport>

            <SelectPrimitive.ScrollDownButton>
                <ChevronDownIcon />
            </SelectPrimitive.ScrollDownButton>
        </SelectPrimitive.Content>
    </SelectPrimitive.Portal>
</SelectPrimitive.Root>

Video (button has click event to change bg color to random)

https://user-images.githubusercontent.com/10235669/196123154-1a93e67c-d626-4120-a039-8f6bd9600c79.mov

Temporary "fix"

I've made a simple mitigation (not a fix) for the time being. It has it's own flaws and changes the behaviour a little bit, but until this issue is addressed, it might help others to temporarily patch and use this select.

Added fixed full page overlay (invisible). Rendered only when select is opened
Made overlay z-index 100
Made select content and trigger z-index 101 (only when select is opened)
Added setTimeout of 10ms (just-in-case) for overlay removal upon select closing

myrjola commented 1 year ago

I ran into same issue in Chrome and Firefox mobile emulation. Also in Native Android Chrome and Firefox. Fortunately, the workaround proposed by @veiico works. Here's the relevant code using styled components:

Introduce an overlay:


const StyledOverlay = styled.div`
  position: fixed;
  top: 0;
  right: 0;
  bottom: 0;
  left: 0;

  /* Choose whatever z-index makes most sense to you */
  z-index: 1050;
  isolation: isolate;
`
/* Workaround for touch events propagating to underlying elements https://github.com/radix-ui/primitives/issues/1658 */
const Overlay = ({ open }: { open: boolean }) => {
  const [visible, setVisible] = useState(open)
  useEffect(() => {
    if (!open) {
      const timer = setTimeout(() => {
        setVisible(false)
      }, 200)
      return () => {
        clearTimeout(timer)
      }
    }
    setVisible(true)
    return () => {}
  }, [open])

  return enabled ? <StyledOverlay onClick={(e) => e.stopPropagation()} /> : null
}

Add it inside the portal:

        <SelectPrimitive.Portal>
          <>
            <Overlay open={open} />
            <StyledContent>
              ...
            </StyledContent>
          </>
        </SelectPrimitive.Portal>

Edit: Added onClick={(e) => e.stopPropagation()} to the overlay Edit2: Increased the timeout to make the workaround work on Firefox Android

laozhu commented 1 year ago

The same issue here, can we just add a prop stopPropagation for SelectPrimitive.Content to prevent background click?

KoenRijpstra commented 1 year ago

Non of the workarounds worked for me. So I "fixed" it by adding a eventlistener to the underlying items and prevent the default event when the select is open:

  const handleOnOpenChange = (open) => {
    // Change `menu__link` to a class that selects the underlying items
    const items = document.querySelectorAll(".menu__link");

    items.forEach((item) => {
      if (open) {
        item.addEventListener("click", function(e){
          e.preventDefault();
          e.stopPropagation();
        });
      } else {
        item.removeEventListener("click", function(e){
          e.preventDefault();
          e.stopPropagation();
        });
      }
    });
  };

And add handleOnOpenChange to the select root:

<Select.Root onOpenChange={handleOnOpenChange} >

Hacky but it works for me!

demondobf commented 1 year ago

I came up with this hacky solution, it's kind of similar to Koen's.

const useSelectInteractionFix = (selectors: string) => {
  const timeoutRef = useRef<number | undefined>();
  const root = document.querySelector<HTMLElement>(selectors);

  if (!root) {
    throw new Error("Root was not found");
  }

  const disableClicks = () => {
    root.style.setProperty("pointer-events", "none");
  };

  const enableClicks = () => {
    root.style.removeProperty("pointer-events");
    // or root.removeAttribute("style") to remove empty attribute.
  };

  const openChangeHandler = (open: boolean) => {
    if (open) {
      clearTimeout(timeoutRef.current);
      disableClicks();
    } else {
      // Casting it here because Node is returning `Timeout` and this handler will run in the browser.
      timeoutRef.current = setTimeout(enableClicks, 50) as unknown as number;
    }
  };

  return openChangeHandler;
};

const App = () => {
  // I am using Next.js but it should work with any other root element.
  const handleOpenChange = useSelectInteractionFix("#__next");

  return <Select.Root onOpenChange={handleOpenChange} />
}

artekr commented 1 year ago

still experience the same issue in Chrome mobile emulator, and in mobile Safari as well. Any plan to provide a fix? 🙏🏻

omer-os commented 1 year ago

Bug report

Current Behavior

Using the Select primitive, when selecting an option, the touch event is interacting with any elements positioned underneath the list. This isn't happening on desktop on mouse click but can be replicated in Chrome when using the device simulation

Radix_Select_touch_bug.mov

Expected behavior

When an option is selected, the menu should close and no events should be triggered on other inputs

Reproducible example

https://codesandbox.io/s/late-snowflake-39o7x7?file=/App.js

Suggested solution

Additional context

Your environment

Software Name(s) Version Radix Package(s) @radix-ui/react-select 1.0.0 React n/a 17 Browser Chrome 104

hi

After conducting several tests, I believe that I have identified the root of the problem. Essentially, when clicking on an item, the select value changes so quickly that the popper disappears and the touch event simultaneously clicks the element behind it. To address this issue, I implemented a solution that resolved the problem. By using the following code:

<Select.Root open={Open} onOpenChange={() => { setTimeout(() => { setOpen(!Open); }, 100); }}>

The select menu now waits for 100ms to finish the touch event trigger before closing, preventing any accidental clicks on elements behind the select component.

omer-os commented 1 year ago

i hope radix team will solve this issue as soon as possible

drianoaz commented 1 year ago

@omer-os Hi there! I tried out your solution using setTimeout and noticed that it worked intermittently and caused a delay in the select. As a result, it may not be the best solution for the problem 😕

https://user-images.githubusercontent.com/15058771/234055067-ccc028ae-5fe6-49c6-9b88-21c896e564b9.mov

omer-os commented 1 year ago

@omer-os Hi there! I tried out your solution using setTimeout and noticed that it worked intermittently and caused a delay in the select. As a result, it may not be the best solution for the problem 😕

https://user-images.githubusercontent.com/15058771/234055067-ccc028ae-5fe6-49c6-9b88-21c896e564b9.mov

Hi sir,

I think my solution is not the best but it's simplest, if 200ms effects the UX of your app you need to implement something better.

I'm sure you'll find a better way as we know what's the cause of this problem as i said in my original comment, or dont even use radix UIs select component if you didn't find a good solution.

drianoaz commented 1 year ago

this opened PR definitively solved the problem for me 🎉

https://user-images.githubusercontent.com/15058771/234159292-53278d83-127b-40e3-94f5-fc3fcd2e45ab.mov

myo commented 1 year ago

This 8+ month-old bug renders the whole package useless, why not merge the proposed fix?

fvanharreveld commented 1 year ago

Any progress update on the issue?

myo commented 1 year ago

Any progress update on the issue?

yeah just use this fork (on this branch specifically), works perfectly https://github.com/joaom00/primitives/tree/select-touch

Aarbel commented 1 year ago

I sometimes think radix is not developped by pure front end developers, as most complex use case are not covered:

mobile touch interractions
elements above each other thru different components (dialogs, etc..)

benoitgrelard commented 1 year ago

@Aarbel, this isn't a helpful comment. If you have specific difficulties, open issues to discuss things. Otherwise take this behaviour somewhere else, this isn't the kind of community we want to foster here.

Aarbel commented 1 year ago

@benoitgrelard

Not sweet to hear i agree, but surely had to be said. I work with production teams that lost weeks fixing / contributing to radix since 2 years, and we are still facing basic bugs which where not existing in the past versions. Thanks again for your work, i know that accessibility on all devices is a big challenge, but please consider that radix contributions / maintenance doesn't look serious to really solve this big problem, example:

About the PR linked to this issue https://github.com/radix-ui/primitives/pull/2085
We can see that it has been approved by @bryanltobing, but absolutely no "domain" comments were written and no test example is added to avoid the problem in the future. So the fix is fully opaque for future developers, and there's a high probability it can cause other bugs.

Suggestions:

enforce radix repo reviewing process for new PRs (domain inline comments, video screncaptures, potential side effects overview)
add storybook "corner" interfaces for tests (like this example) to be tested by every contributor, otherwise we have no warranty this kind of bug will not re-appear in the next updates.
require manual webview touch mobile tests (android and ios simulators) by every contributors

Happy to help if needed.

codingwithchris commented 1 year ago

yeah just use this fork (on this branch specifically), works perfectly

How can this be used as an installed package and consumed in a typescript project? I am trying to use it in a nextjs project. I installed it and I'm importing it successfully, but next is choking on it... it's weirdly having issues with the typescript.

williamtobing commented 1 year ago

yeah just use this fork (on this branch specifically), works perfectly

How can this be used as an installed package and consumed in a typescript project? I am trying to use it in a nextjs project. I installed it and I'm importing it successfully, but next is choking on it... it's weirdly having issues with the typescript.

I've faced the same problem, it works on react, but not in next.js, just use headless ui instead, seeing this issue is ignored by radix since last year

eposha commented 1 year ago

The solution that worked for me

58bits commented 1 year ago

Hi All - we've just run into this as well - it's definitely happening on 'touch/tap' events for Chrome - both when emulating mobile/responsive mode on Chrome Desktop, as well as Chrome Mobile (Android and iOS), and Firefox Mobile - although not 100% sure about Safari Mobile.

I posted a discussion here - https://github.com/radix-ui/primitives/discussions/2324 , and found another very similar issue reported in another project here - https://github.com/JedWatson/react-select/issues/532

~I tried @eposha 's attempt above but isn't working for me. Will try some of the other suggested workarounds above.~

Hope some of this helps.

58bits commented 1 year ago

Oops I spoke too soon - @eposha 's solution above IS working! Thank you!

victorgaard commented 1 year ago

What worked for me was to pass user-select: none; as CSS to Select.Viewport component. If you're using Tailwind just add select-none to it. Hope that helps. :)

JohnGemstone commented 1 year ago

I tried the select-none for the viewport but didn't seem to work for me unfortunately as it would be an elegant solution. Not sure why and don't have the time to debug.

The ref solution from @eposha worked great though!

<SelectContent
  ref={(ref) => {
    if (!ref) return;
    ref.ontouchstart = (e) => {
      e.preventDefault();
    };
  }}
>

EDIT: looks like this prevents any touch scrolling when using ScrollArea.

EDIT2: I ended up using the overlay technique which works for me using shadcn.

piotrkulpinski commented 1 year ago

I've modified @eposha's solution slightly and it seems to be doing its job, while also not preventing any touch scrolling on mobile devices.

<SelectPrimitive.Content
  ref={(ref) => ref?.addEventListener('touchend', (e) => e.preventDefault())}
  {...props}
>

Could anyone confirm if it's a solid workaround?

JohnGemstone commented 1 year ago

Hey @piotrkulpinski yep that works for me thanks for sharing!

eposha commented 1 year ago

I've modified @eposha's solution slightly and it seems to be doing its job, while also not preventing any touch scrolling on mobile devices.
<SelectPrimitive.Content
  ref={(ref) => ref?.addEventListener('touchend', (e) => e.preventDefault())}
  {...props}
>
Could anyone confirm if it's a solid workaround?

I think you should remove listener before unmount

visualect commented 1 year ago

<SelectPrimitive.Content
  ref={(ref) => ref?.addEventListener('touchend', (e) => e.preventDefault())}
  {...props}
>

This solution is working for me

josephkruse commented 1 year ago

I've modified @eposha's solution slightly and it seems to be doing its job, while also not preventing any touch scrolling on mobile devices.
<SelectPrimitive.Content
  ref={(ref) => ref?.addEventListener('touchend', (e) => e.preventDefault())}
  {...props}
>
Could anyone confirm if it's a solid workaround?

This caused problems for me. Doesn't work if you need the ref prop I believe?

xmliszt commented 1 year ago

The solution that worked for me

Thanks for the solution! I was encountering this issue in shadcn ui Select as well.

xmliszt commented 11 months ago

Got it to work by making sure onClick is well received by the select item as well before we call the context.onOpenChange(false) to close the select content wrapper. #2403

mainseq commented 11 months ago

Adding this line:

 ref={(ref) => {
    if (!ref) return;
    ref.ontouchstart = (e) => e.preventDefault();
  }}

to Select.Content breaks my search functionality. (cannot focus on the input anymore) But when added to <Select.Group ref={(ref) => ref?.addEventListener('touchend', (e) => { e.preventDefault();})} >

It seems to work fine.

radix

markedby commented 11 months ago

Hello, radix team. Any progress update on the issue? I've checked this case on the latest @radix-ui/react-select version (v2.0.0) and it still occurs.

P.S. I'm big fan of your lib. Thank you for you work :)

Novo1999 commented 9 months ago

ref={(ref) => ref?.addEventListener('touchend', (e) => e.preventDefault())}

Thanks this worked for me

ParkerMJones commented 9 months ago

Using

<SelectContent
  ref={(ref) => {
    if (!ref) return;
    ref.ontouchstart = (e) => {
      e.preventDefault();
    };
  }}>

disabled scrolling within the <Select>, and I was still getting unwanted touch events with

<SelectPrimitive.Content
  ref={(ref) => ref?.addEventListener('touchend', (e) => e.preventDefault())}
>

I ended up just using state to control the pointer-events behavior of the underlying elements.

<Select onOpenChange={(open) => setSelectOpen(open)}>

elements underneath:

 <div className={clsx(selectOpen ? "pointer-events-none" : "pointer-events-auto")}>

priyesh-zero commented 7 months ago

I tried the ref solution, it didn't work for me.

<SelectContent
  ref={(ref) => {
    if (!ref) return;
    ref.ontouchstart = (e) => {
      e.preventDefault();
    };
  }}>

But after reading through proposed solution in #2403, I came up with a hacky one that works for the time being.

        <Select.Root
            onOpenChange={() => {
                /** Any other code you need to run **/
                setTimeout(() => {
                    const selection = document.getSelection()
                    if (selection) {
                        selection.removeAllRanges()
                    }
                }, 10)
            }}
        >

The timeout is so that the browser registers the select operation being started, if you remove it, the code will run first and the selection will start after that.

Hoping #2403 or a better solution comes along soon. :pray: Great library, absolutely love it, Thanks for all the hard work :heart:

rokaskasperavicius commented 5 months ago

Tried most above solutions but a combination of some of them seem to be the most stable:

const contentRef = React.useRef<HTMLDivElement>(null)

useEffect(() => {
  const node = contentRef.current
    if (!node) return

    node.addEventListener('touchend', (e) => e.preventDefault())

    return () => {
      node.removeEventListener('touchend', (e) => e.preventDefault())
    }
}, [contentRef.current, open])

contentRef is set on SelectPrimitive.Content

The important part here is the open in the useEffect dependencies. Otherwise, the useEffect only gets triggered after the dropdown had been opened the first time.

The fix seems to work on mobile with scrolling available.

maxkarkowski commented 5 months ago

is there any update on this issue? seems like this is bothering people for nearly two years. we have the case that it always triggers an -tag beneath the select and routes to another page

radix-ui / primitives