adobe / react-spectrum

A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences.
https://react-spectrum.adobe.com
Apache License 2.0
13.05k stars 1.13k forks source link

Menu/Select pick immediately when Popover overlaps its Trigger #3864

Open sebald opened 1 year ago

sebald commented 1 year ago

🐛 Bug Report

When there is not enough space for the popover to be placed above/below its trigger, it will overlap the trigger. This causes the popover to be immediately closed and whatever item is underneath the pointer will be selected.

🤔 Expected Behavior

Items should not automatically be selected.

😯 Current Behavior

Kapture 2022-12-20 at 09 46 39

💁 Possible Solution

The issue is caused by conflicting event handlers. The popover opens on onPointerDown and the selection event for both, the menu and select (listbox), happens on onPointerUp. One possible solution might be to move the selection to another event, e.g. onPointerDown.

A temporary fix is the following:

  let { menuItemProps } = useMenuItem(
    { key: item.key },
    state,
    ref
  );

  let { onPointerUp, ...rest } = menuItemProps;

  return (
    <li
      {...mergeProps(rest, { onPointerDown: onPointerUp })}
      ref={ref}
    >
      {item.rendered}
      {isSelected && <span aria-hidden="true">✅</span>}
    </li>
  );

💻 Code Sample

Codesandbox Playground: https://codesandbox.io/s/shy-paper-fi7w0f?file=/src/App.js

To see the behavior, reduce the size of the browser.

🌍 Your Environment

Software Version(s)
react-aria 3.22
react-stately 3.20
Browser Chrome 108.0.5359.124
Operating System MacOS Ventura 13.0.1
snowystinger commented 1 year ago

Thanks for reporting this.

We don't want to move to pointerDown selecting the item and closing the menu. That wouldn't match native controls. I think we should instead ignore the pointerUp that follows opening the menu. But to match native, there's a timeout where if you hold the mouse down, then lifting will immediately select and close. https://jsfiddle.net/snowystinger/otk36jux/

LFDanLu commented 1 year ago

Just some more investigation notes for posterity: I had thought that we could leverage some of the allowsDifferentPressOrigin logic here to prevent pressUp from selecting the option but this is a case where we DO want this behavior, just not if the pointerDown + pointerUp happens in quick succession. A timeout to match native seems like appropriate solution

Flammae commented 1 year ago

I believe I'm experiencing the same bug in my popover components, but there might be something more going on than what meets the eye.

  1. When overlapping, using pointer or touch to trigger a picker, quickly closes the popover and selects whatever item you happen to be hovering
  2. On Firefox, using touch to trigger a popover also quickly closes the popover even if not overlapping (underlay is still overlapping though). I've not yet been able to reproduce it on chrome.
  3. On Chrome using touch to trigger a picker also closes the popover without selecting a value even if not overlapping. Although it happens more randomly.

The Firefox bug is present in your tailwind example of select component: https://codesandbox.io/s/hardcore-moon-xzc4r?file=/src/Popover.tsx

To reproduce, open Codesandbox project in a new window, then press ctrl + shift + m and enable touch simulation, then click on second select component.

I was not able to reproduce the Chrome bug in this example. My implementation of select (where I found the bug), might be a little different from the example though. For example, mine renders overlay inside underlay where in this example underlay is a sibling of overlay.

I'll try to make reproducible example in the following days

snowystinger commented 1 year ago

On Firefox, using touch to trigger a popover also quickly closes the popover even if not overlapping (underlay is still overlapping though). I've not yet been able to reproduce it on chrome.

I followed your steps to reproduce in FF with that codesandbox, I'm noticing that the options take a moment to show up, but I do not notice the menu closing on its own. I'm on FF 109 in MacOS 13.1

Flammae commented 1 year ago

ezgif com-gif-maker (2)

@snowystinger I'm on FF 109 in windows 10

LFDanLu commented 1 year ago

Reproduced in Windows Firefox 109 w/ touch emulation and on Android Firefox 108

Flammae commented 1 year ago

I really want to emphasize the bug is present in regular popovers and not just pickers.

That said, I think I found the source of the bug. When you render an underlay in a popover and the underlay is drawn on top of the application something causes the popover to close. Note, if underlay is hidden thru display: none; or z-index: -1 the popover wont close.

The reason why the bug happens in the select example, is the missing prop isNonModal on popover which conditionally renders the underlay. That's why it only happens on the select component and not on other pickers in the example.

If this is a feature would you please provide the reasoning behind the isNonModal prop, when should it be used in a popover?

Although there's use cases for underlays always rendering inside the popover, like being able to change popover to bottom sheet on mobile with css

Flammae commented 1 year ago

Just checked and @sebald's original bug is still there even with underlay gone, so I was wrong in believing that these things were related

LFDanLu commented 1 year ago

The reason for the isNonModal is for components like ComboBox where a popover is rendered but we do not want to cover the input field with an underlay nor hide it via ariaHideOutside which usually applies aria-hidden to everything outside of an overlay. ComboBox maintains focus inside its input field even when its dropdown is open and as such needs to still be interactable to users and visible to accessibility tools.