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.9k stars 1.12k forks source link

hoverEnd does not fire on button move/resize under cursor #3522

Closed xav-ie closed 2 years ago

xav-ie commented 2 years ago

🐛 Bug Report

If a user clicks a button and the button resizes or moves position on the page so that it is no longer under the cursor, the onHoverEnd event does not fire and isHovered remains true.

Steps to reproduce:

  1. Make button with react-aria's useButton and useHover
  2. Create effect that reacts to isHovered and isPressed and moves/resizes the button onPress so it is no longer under the cursor
  3. Notice that onHoverEnd did not fire and isHovered is still true.

Code sandbox link: https://codesandbox.io/s/hoverendnotfiring-25h7dh?file=/src/App.js

🤔 Expected Behavior

The isHovered variable should become false and onHoverEnd should be fired.

😯 Current Behavior

hoverEnd event does not fire and isHovered remains true!

Here is code sandbox link: https://codesandbox.io/s/hoverendnotfiring-25h7dh?file=/src/App.js

💁 Possible Solution

I think adding a delayed check to see if the button is still under cursor after onPressEnd will fix this problem. Or, we can work on making an all-in-one hook/solution for buttons that want hover effects. Right now, it feels a little weird to have useButton to also need useHover.

🔦 Context

Being inspired by Sam Selikoff's video, How to build an Animated Button with React Aria and Framer Motion, I set out to make my own "perfect" button with hover states.

Screen Shot 2022-09-15 at 6 40 27 PM

In the image above, the buttons circles are the problematic buttons that move/change shape on click, creating this issue of retaining their hover states.

💻 Code Sample

https://codesandbox.io/s/hoverendnotfiring-25h7dh?file=/src/App.js

🌍 Your Environment

Software Version(s)
react-aria 3.19.0
Browser Tested on Chromium, Firefox, and Safari and behavior is same
Operating System MacOS

This issue does not happen on mobile because there are no hovers on mobile (normally, lol).

I also use Framer motion to fix the active state not appearing, but that has no effect on this issue as I have a button in the code sandbox without framer-motion experiencing the same issue.

xav-ie commented 2 years ago

I have managed to boil it down to only useHover and checked that useButton is not affecting the useHover hook. Here is the code:

import { useRef, useState } from "react";
import { useHover } from "react-aria";
import { defaultState, hoverState } from "./Constants";

export default function ButtonEventQueue() {
  const ref = useRef(null);
  const [expanded, setExpanded] = useState(true);
  const { hoverProps, isHovered } = useHover({});

  return (
    <button
      {...hoverProps}
      ref={ref}
      onClick={() => setExpanded((e) => !e)}
      style={{
        background: isHovered ? hoverState.background : defaultState.background
      }}
    >
      Don't click this part (hovering: {isHovered ? "t" : "f"})
      {expanded && <span> | Tap this part</span>}
    </button>
  );
}

See https://codesandbox.io/s/hoverendnotfiring-25h7dh?file=/src/OnlyUseHover.jsx for live example.

snowystinger commented 2 years ago

Seems like a browser bug, but I can't find concrete support for that idea in the spec https://www.w3.org/TR/pointerevents/#the-pointerout-event and https://www.w3.org/TR/pointerevents/#the-pointerleave-event which both refer to what happens when the pointer is moved, not when the target changes dimensions

We could possibly add logic to track pointer move, but seems like it's a really heavy handed way to handle it. Same thing if we attached a ResizeObserver to the hovered element.

I'll bring it up with our team in our next grooming meeting.

xav-ie commented 2 years ago

Yes, I also came to this conclusion. The pointerout/leave events do not fire for these edge cases, but I think that these are important for when you are trying to style elements with useHover. I think the best way to approach this (or at least how I am trying to get around it is to set a timeout [100ms sounds good I guess??] check for onpointerup and compare the target boundingClientRect contains e.clientX and e.clientY). Here is my jerryrigged "perfect" button: https://codesandbox.io/s/perfectbutton-ve6j75?file=/src/PerfectButton.jsx

snowystinger commented 2 years ago

But that button isn't working, it still shows hovered as true. Also, if you hover it a second time, the state is stale and it won't trigger an on hover.

This is something we'd need to build into useHover. I'm just not sure the best way to do that without incurring performance hits like I mentioned.

I'll respond back here after I've had a chance to discuss it with the team.

devongovett commented 2 years ago

Tested a few browsers, seems pretty inconsistent: https://codepen.io/devongovett/pen/PoeKmwB

  1. Chrome - fires onpointerleave and removes :hover state immediately when the button moves (on click).
  2. Firefox - removes :hover immediately on click, fires onpointerleave after moving the mouse by at least 1px.
  3. Safari - does nothing on click, removes :hover and fires onpointerleave after moving the mouse.

But since onpointerleave does eventually get fired in all browsers, it should work.

snowystinger commented 2 years ago

Spent some more time looking at this after Devon's analysis

I've changed your default button in the codesandbox, and that now shows the correct behavior

https://codesandbox.io/s/hoverendnotfiring-forked-x83147?file=/src/RegularButton.jsx

The big thing to note is this change. When you spread them both independently, whichever one is spread last is going to overwrite any from the previous that have the same name. In this case, both button props and hover props define onPointerLeave functions. So they need to be merged.

{...hoverProps}
{...buttonProps}
// became
{...mergeProps(hoverProps, buttonProps)}

I'm closing for now, if you find another issue, please feel free to reopen

xav-ie commented 2 years ago

Thank you all for helping me so much with this! I am so happy everything is working now. One last thing to point out is that adding removing whole elements does not fire onpointerleave properly (but not a fault of react-aria!). The code:

{expanded && <span>Tap this part</span>}

inside a button will not fire onpointerleave when the button is not expanded and not showing the span.

However, the fix to this is to either do:

{expanded && "Tap this part"}

so the text inside the button fires onpointerleave properly. Or you can do:

<span>{expanded && "Tap this part"}</span>

If you need a span inside your button.

See https://codesandbox.io/s/weird-span-issue-lgxfxs?file=/src/App.js for examples. @snowystinger 's sandbox above still does not work for this reason. So, moving a button and changing text inside a button so it resizes fires onpointerleave eventually (thank you for finding this out, @devongovett !). However, adding/removing elements inside a button so it changes size does not fire onpointerleave.

I hope the sandbox above helps someone who runs into the same issue. I was confounding the mergeProps mistake, the weird <span> issue, and the weird way of when browsers decide to fire onpointerleave.

Same behavior found in Chromium, Firefox, and Safari.