react-spectrum icon indicating copy to clipboard operation
react-spectrum copied to clipboard

When selecting a Menu item, element behind popup receives pointer event

Open stefanprobst opened this issue 3 years ago • 22 comments

🐛 Bug Report

When a Menu item is selected on a touch device, interactive elements behind the menu popup receive the pointer event as well.

🤔 Expected Behavior

Interactive elements behind a menu popup should do nothing when selecting a menu item.

😯 Current Behavior

See this codesandbox for an example with buttons behind a menu popup. When selecting a menu item, the button also gets clicked (and displays the alert).

https://user-images.githubusercontent.com/20753323/106186087-0e74d280-61a4-11eb-8df9-76d419d3e039.mp4

Note that this only happens on mobile devices, and when the pointer device simulation is turned on in browser devtools.

💁 Possible Solution

🔦 Context

💻 Code Sample

https://codesandbox.io/s/react-spectrum-menu-pointer-events-wze00?file=/src/App.js

🌍 Your Environment

Software Version(s)
react-spectrum 3.7.0
Browser Firefox, Chrome
Operating System

stefanprobst avatar Jan 28 '21 19:01 stefanprobst

Double checked, doesn't happen in Safari. Also still happens with using our Buttons https://codesandbox.io/s/react-spectrum-menu-pointer-events-forked-8u83p?file=/src/App.js The next step would be to reproduce this with just React or even better, just javascript, so we can figure out what's actually going on. I have a hunch it's related to https://bugs.chromium.org/p/chromium/issues/detail?id=1150073 but we should make sure because our Buttons don't rely on 'click' in most cases

snowystinger avatar Jan 28 '21 21:01 snowystinger

  • interestingly, with rsp buttons (your codesandbox example) i can not reproduce in firefox anymore, but still in chrome
  • however, when putting links with useLink behind the popup, i can still reproduce with both firefox and chrome (see here)

stefanprobst avatar Jan 29 '21 08:01 stefanprobst

Looked into it more, it does appear that this is the chromium bug I referenced earlier.

You are right about FF, honestly don't know what I was doing there, I forgot to turn on touch simulation. The reason that it's not causing an issue with our buttons is that we actually have a line in our code that will ignore the click if it's simulated https://github.com/adobe/react-spectrum/blob/main/packages/@react-aria/interactions/src/utils.ts#L24, but it looks like Firefox is actually plagued by the same bug. I've opened a ticket with them here: https://bugzilla.mozilla.org/show_bug.cgi?id=1692033

As for useLink, I believe that to be that links have their own click event default handler, which we do not prevent https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/interactions/src/usePress.ts#L226 and is still the same bugs as above.

For reference, here's the minimal reproduction I could think of https://codesandbox.io/s/inspiring-lehmann-i1hjf And here's the same think with links https://codesandbox.io/s/trusting-poitras-u89ub?file=/index.html which is a more compelling case I think, so I've updated both bugs with a comment about it in hopes that gets it more attention.

snowystinger avatar Feb 10 '21 20:02 snowystinger

@manelsanz Yes, the bug notes that it happens in FF as well. I would disagree that it's a library issue. You can see the issue happen without our library. I do understand what you're saying though, that this should be something we can help mitigate or work around. Do you have any ideas for solving it?

@equinusocio I'm not really sure what you're saying. Are you suggesting a way of fixing it? Are you using pointer events? or what do you use? We've found that there are issues if you try to mix pointer events and mouse events. So we're stuck with using pointer events for this.

Some ideas I'd had but haven't had a chance to implement.

  • delay closing of the overlay by a tick, might be unexpected by tests so potentially a lot of things would need updating not just in our library, but in users tests as well
  • apply user events none to everything that is currently aria-hidden (seems costly)
  • global listener for the start of the event chain so we know if it's actually a virtual event or something that leaked through (I'm fairly sure we treat a click event alone as virtual) but this doesn't help when the thing behind that's getting the click event isn't from our library

snowystinger avatar Dec 15 '21 20:12 snowystinger

@equinusocio I'm not sure react-focus-on solves it, see https://codesandbox.io/s/react-focus-on-react-spring-forked-rfzt4?file=/src/index.js

snowystinger avatar Dec 15 '21 20:12 snowystinger

@snowystinger Dunno why by using react-focus-on we don't have that issue. Anyway as you can see from the sandbox, the issue is not related to the responsive mode, since it occurs even with the normal view.

equinusocio avatar Dec 16 '21 11:12 equinusocio

Possible workaround here.

Rember to:

  1. Enter dev tools.
  2. Enable touch emulation.

For the record, this is how browsers should behave when users interact using a touch device.

Quoting W3C:

To avoid processing the same interaction twice for touch (once for the touch event, and once for the compatibility mouse events), developers should make sure to cancel the touch event, suppressing the generation of any further mouse or click events. Alternatively, see the InputDeviceCapabilities API for a way to detect mouse events that were generated as a result of touch events

I think that in this case, React is failing to preventDefault touch events. One possible workaround is to use the Web API to add an event listener to the interactive element, capture the touch event and prevent the default behavior. This way the compatibility mouse events won´t happen.

Here an example of React failing to prevent compatibility events.

hernanponcetta avatar Dec 16 '21 16:12 hernanponcetta

@hernanponcetta thanks for that extra info. Looks like React is both aware of it and not going to do anything about it https://github.com/facebook/react/issues/9809

Interesting, it does appear that you must cancel the touchstart, doing it on pointerdown isn't enough.

The issue logged against the browsers has more to do with click being fired on the wrong element because the original target has been removed. So there are two things happening here. I think cancelling the touchstart though is a good direction to try first, it'd render the other issue a moot point if it works for us. I'm not sure how we'd put it on everything though since not everything may use usePress. But maybe it's enough for us to handle it there and in useInteractOutside. That should cover a lot of it.

snowystinger avatar Dec 16 '21 22:12 snowystinger

Have there been an updates on fixing this? I see there's a PR that is WIP that hasn't been updated since early April. For the record, i see this happening on real phone and tablet devices, not just Chrome's dev tools.

chrisspadanuta avatar Jun 30 '22 02:06 chrisspadanuta

Hey, we've been trying things, as you can see in the history of the ticket. We've been unable to find a satisfactory workaround to this browser bug thus far and other priorities took us away from looking at it. It could help to chime in on the tickets that were opened against FF and Chrome. We're aware that this happens on real devices as well, unfortunately. We welcome any fresh ideas on how to work around it while we wait for browsers to fix this issue.

snowystinger avatar Jun 30 '22 20:06 snowystinger

@chrisspadanuta here is an example of the workaround we are using.

// Workaround for react/react-aria #1513
useEffect(() => {
  ref.current?.addEventListener('touchstart', (event: TouchEvent) => {
    event.preventDefault();
  });
}, []);

Hope that it helps.

hernanponcetta avatar Jul 01 '22 11:07 hernanponcetta

Thanks @hernanponcetta, that workaround is working for us too.

peteragarclover avatar Sep 22 '22 01:09 peteragarclover

@hernanponcetta's version works fine but breaks scrolling on the elements.

Using this keeps the scroll, but has a performance impact on scrolling (from what I understand, though seemed negligible on faster devices)

    useEffect(() => {
        ref.current?.addEventListener('touchend', (e) => {
                e.preventDefault();
        }, { passive: false, once: true });
    }, []);

Slooowpoke avatar Sep 26 '22 12:09 Slooowpoke

Another workaround comes from https://github.com/adobe/react-spectrum/issues/4027 where you can control the open state of your menu and call close after a slight delay: https://codesandbox.io/s/react-spectrum-menu-pointer-events-forked-w9wxog?file=/src/App.js

LFDanLu avatar Feb 08 '23 19:02 LFDanLu

@hernanponcetta's version works fine but breaks scrolling on the elements.

Using this keeps the scroll, but has a performance impact on scrolling (from what I understand, though seemed negligible on faster devices)

    useEffect(() => {
        ref.current?.addEventListener('touchend', (e) => {
                e.preventDefault();
        }, { passive: false, once: true });
    }, []);

Just want to highlight two edge cases where this breaks normal behavior:

  • using it on buttons with type="submit" will block form submission on touch devices.
  • using this on links will prevent opening the link on touch devices (this can happen if you have buttons that are link underneath)

I use something like this:

  useEffect(() => {
    ref.current?.addEventListener(
      'touchend',
      (e) => {
        /**
         * The first condition is necessary to avoid the issue that a button with
         * type="submit" inside a <form> would not trigger the form submit
         * event on a device with touch input.
         *
         * The second condition is needed for thse buttons that are links
         * underneath and will have a href. If "touchend" is prevented, links
         * won't open by touch.
         *
         * Admittedly, this is an ugly patch, but given the react-spectrum
         * issue it seems necessary. If this hook grows any larger after
         * further bug reports and edge cases, we may want to reconsider
         * this approach.
         */
        if (
          (e.currentTarget as HTMLElement)?.getAttribute('type') !== 'submit' &&
          typeof (e.currentTarget as HTMLElement)?.getAttribute('href') !== 'string'
        ) {
          e.preventDefault();
        }
      },
      { passive: false }
    );
  }, []);

tgelu avatar Jul 19 '23 10:07 tgelu

Anyone had any issues with android browsers with this workaround? We got multiple reports from users on some android devices that our buttons stoped working... We can quite consistently reproduce the issue on browserstack, but cannot find a suitable fix for it.

Been pulling my hair on it and considered deferring the execution of button click with setTimeout or requestAnimationFrame, but this completely breaks our jest tests (all clicks need runAllTimers to work).

Was there any development to find a solution for this issue? It's been open for a long time and it surely is a problem for a lot of users or react-aria.

dnaploszek avatar Nov 06 '23 22:11 dnaploszek

Was there any development to find a solution for this issue? It's been open for a long time and it surely is a problem for a lot of users or react-aria.

Ran into this issue with a Dialog and a Select (using hooks) and am curious as well.

mryechkin avatar Nov 07 '23 01:11 mryechkin

I would like to also bring this issue to attention as all usePress interactions are affected by this problem which can be proved by the Demo present in a similar issue.

dnaploszek avatar Nov 07 '23 07:11 dnaploszek

Tried both workarounds - the one with useEffect doesn't work for me at all (for some reason the touchend/touchstart event is never triggered), second workaround using setTimeout in onChange event handler seem to fix it only for chrome touch simulator mode. Using real devices (tested on ipad and iphone) in safari causes the pointer events on components beneath it.

Further testing with different libraries shows that this must be problem in the library itself because for example radixui can handle popovers just fine. Have you considered using similar approach as them?

EDIT: I looked more closely at their select component example and they have similar issue. There are mentioned few approaches that could potentially solve it.

  1. transparent overlay right beneath the popover - might have to do some z-index calculation
  2. add touchend listener directly on popover reference
<Popover ref={(ref) => ref?.addEventListener('touchend', (e) => e.preventDefault())} >
      <ReactAriaListBox className={theme.pickerList} items={props.items} data-testid={"picker-list"}>
        {props.children}
      </ReactAriaListBox>
</Popover>

Im not sure if no. 2 cancels something that shouldn't be cancelled

Inkvii avatar Jan 19 '24 14:01 Inkvii

I can reproduce on iOS 17.2.1 Safari -- having trouble with the dev environment, seems like it's only happening in prod 🤔

Edit: after messing around in chrome, replacing the close button in the modal with a <button instead of a <Button from react aria (aka not using usePress) fixes the issue

sbdchd avatar Feb 06 '24 02:02 sbdchd

I agree with @snowystinger that my issue #5802 is a dupe of this one.

:warning: EDIT: right after posting this comment I ran into the issue again while using the workaround :see_no_evil:

My current workaround is the below. Feel free to make me aware of any issues with doing it like this—it seems to avoid the issue for my cases:

const { isPressed, buttonProps } = useButton(
    {
      ...useButtonProps,
      onPress(e) {
        if (e.pointerType === "mouse" || e.pointerType === "keyboard") {
          useButtonProps.onPress?.(e);
          return;
        }

        setTimeout(() => {
          useButtonProps.onPress?.(e);
        }, 1);
      },
    },
    ref,
  );

I decided to preemptively only allow mouse and keyboard events to be handled "normally", as I'm not sure what other event types have the issue.

jeyj0 avatar Feb 06 '24 07:02 jeyj0

Ran into this issue also, using <Modal> and <Dialog>. Solved it by calling the modal's onOpenChange handler during the useEffect cycle. Don't know if there are any downsides to that yet (I'd like to know if there are), but just thought I'd share my solution using a <CustomModal> implementation:

import { forwardRef, useEffect, useState } from 'react';
import { Modal, ModalOverlayProps } from 'react-aria-components';

export default forwardRef<HTMLDivElement, ModalOverlayProps>(
  function CustomModal({ onOpenChange, ...props }, ref) {
    const [openSignal, setOpenSignal] = useState<boolean | undefined>();

    useEffect(() => {
      if (openSignal !== undefined) {
        onOpenChange?.(openSignal);
        setOpenSignal(undefined);
      }
    }, [openSignal, onOpenChange]);

    return <Modal ref={ref} onOpenChange={setOpenSignal} {...props} />;
  }
);

emamoah avatar Feb 12 '24 01:02 emamoah

@devongovett Hi. Why is React Spectrum not using native dialog? The API is weird, but can be fixed:

https://github.com/evoluhq/evolu.me/blob/main/components/Modal.tsx#L83

Btw, I'm considering moving from React Native for Web to React Aria (since I don't need React Native), but mobile issues hold me back. My question is, do you know RNfW internals? It could be a good source of knowledge. Anyway, thank you for your work.

steida avatar Mar 08 '24 12:03 steida