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
12.99k stars 1.13k forks source link

Sticky `data-hovered` on `Row` if `Popover` can be opened from inside it #7004

Open levrik opened 2 months ago

levrik commented 2 months ago

Provide a general summary of the issue here

When a Popover can be opened from inside a Row with href, data-hovered can get stuck after closing the Popover again.

๐Ÿค” Expected Behavior?

data-hovered gets removed after the Popover has been closed. I think it's kinda expected to stay while the Popover is open even if the cursor has been moved away from the Row.

๐Ÿ˜ฏ Current Behavior

When a Popover is opened from inside a Row with href, data-hovered is stuck after closing the Popover again if the cursor has been moved above another row.

๐Ÿ’ Possible Solution

I understand that this might be tricky to solve as the Row is missing mouse events while the popover is open. Maybe a check on the mouse position could be done after the Popover has been closed?

๐Ÿ”ฆ Context

No response

๐Ÿ–ฅ๏ธ Steps to Reproduce

https://github.com/user-attachments/assets/c92ed335-f69f-45eb-8a5a-57637059e6f3

https://codesandbox.io/s/purple-pond-qssgxg

Version

1.3.3

What browsers are you seeing the problem on?

Chrome, Microsoft Edge

If other, please specify.

No response

What operating system are you using?

macOS

๐Ÿงข Your Company/Team

No response

๐Ÿ•ท Tracking Issue

No response

snowystinger commented 2 months ago

Thanks for the issue! Probably an issues with React and the way they bubble events through portals. Probably best to start by seeing if we are already ignoring hover events if they come through a portal.

levrik commented 2 months ago

@snowystinger I don't think this has anything to do with React but simply that the mouseout/mouseleave event never fires on the Row as the Popover puts a full screen overlay over everything except the Popover itself. When the Popover then closes the mouse cursor is in a different position and the Row never noticed that it's not being hovered anymore.

snowystinger commented 2 months ago

Also very possible. We'll want to make sure that is the root of the issue in order to start thinking about a fix.

levrik commented 2 months ago

@snowystinger Just to ensure I'm not misunderstanding you: Do you expect me to verify this or is it something the React Aria team wants to do?

Just to be clear: I'm open to also investigate a bit more from my side in case I find time for it. Just want to ensure that expections are set correctly ๐Ÿ˜…

snowystinger commented 2 months ago

Anyone, just trying to make it easier for anyone to look at by providing some pointers based on past issues.

If you want it prioritized, then it's in your best interest to have a go at it. Otherwise it'll have to fit into our priorities.

emily-curry commented 1 month ago

This does appear to be because of how react events propagate through portals. mouseout and pointerout do appear to fire, but mouseleave and pointerleave will not if the pointer remains within an element of the same react component tree. The difference in behavior between react event listeners and direct usage of event listeners can be observed in this codesandbox.

Since I'm using useHover directly, I'm working around this at the moment by patching the hoverProps which that hook returns, and adding out/over handlers:

 const hover = useHover({ isDisabled: false });
  const hoverProps = useMemo(() => {
    const { onPointerLeave, onPointerEnter, onMouseLeave, onMouseEnter } = hover.hoverProps;

    function wrapHandler<E extends React.PointerEvent<FocusableElement> | React.MouseEvent<FocusableElement>>(
      handler?: (e: E) => void
    ): ((e: E) => void) | undefined {
      if (handler == null) {
        return undefined;
      }
      return (e) => {
        // ref is the element which will also receive these props
        if (e.relatedTarget instanceof Element && ref.current?.contains(e.relatedTarget) === false) {
          handler(e);
        }
      };
    }

    return mergeProps(hover.hoverProps, {
      onPointerOut: wrapHandler(onPointerLeave),
      onPointerOver: wrapHandler(onPointerEnter),
      onMouseOut: wrapHandler(onMouseLeave),
      onMouseOver: wrapHandler(onMouseEnter),
    } satisfies DOMAttributes<FocusableElement>);
  }, [hover.hoverProps]);
levrik commented 1 month ago

@emily-curry But this probably causes the row hover to disappear while the Popover is open, right? This is actually an expected behavior for me but I would like the hover to disappear after the Popover was closed.

emily-curry commented 4 weeks ago

Yes, it would cause the hover state to be removed when the user hovers the popover instead of the row, though the hover state should still show on the row when that is hovered directly. I looked into this a little more, I think the cause of this behavior is in the useHover hook of react-aria here: https://github.com/adobe/react-spectrum/blob/a45e2a5ecc553bd461dcfbe0a6a00722fbd624cc/packages/%40react-aria/interactions/src/useHover.ts#L167

Basically, the library is only handling the leave/enter pointer events (or mouse if PointerEvent is not available, but that's not relevant). The "pointerleave" event never gets fired when the pointer moves from the row to the popover, because the popover is a member of the row's react component tree, even though the row does not contain that DOM node. This part is a quirk of react. Then, when the pointer finally does leave the popover, the "pointerleave" event does fire. However, the code I linked above will ignore this event, e.currentTarget.contains(e.target) will be false because the row does not contain the popover, resulting in the hover state being stuck.