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.11k stars 1.14k forks source link

PressUp event bubbles to parent node #3292

Closed sanyabas closed 2 years ago

sanyabas commented 2 years ago

๐Ÿ› Bug Report

If I render two nested nodes with usePress hooks, clicking child node also triggers pressUp for parent node.

๐Ÿค” Expected Behavior

I'd expect that clicking child node only triggers press events on child node.

๐Ÿ˜ฏ Current Behavior

PressUp event is triggered on parent node, while PressStart, PressEnd are not.

๐Ÿ’ Possible Solution

๐Ÿ”ฆ Context

๐Ÿ’ป Code Sample

https://codesandbox.io/s/ecstatic-nova-c5khl9?file=/src/App.js

Clicking blue square triggers PressStart, Press, PressEnd, PressUp for parent. Clicking red square triggers all needed events for child node and PressUp for parent. BTW tapping using macbook touchpad sometimes works as expected.

๐ŸŒ Your Environment

Software Version(s)
react-aria 3.17.0
Browser Chrome 100, Safari 15.3,Firefox 102
Operating System macOS 12.2.1

๐Ÿงข Your Company/Team

๐Ÿ•ท Tracking Issue (optional)

snowystinger commented 2 years ago

Unlike the other listeners, it looks like we don't preventDefault/stopPropagation https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/interactions/src/usePress.ts#L379 I'll check with the team if it's something we did on purpose

snowystinger commented 2 years ago

PressUp is different than PressEnd, PressUp can fire on whatever element the cursor or finger is over when it's lifted regardless of status as parent https://codesandbox.io/s/inspiring-hofstadter-1tofg5?file=/src/App.js

This is used in our selectable items for example https://github.com/adobe/react-spectrum/blob/f0c9a597aa8d9eadeb47e81aacd9af65dca77700/packages/%40react-aria/selection/src/useSelectableItem.ts#L202-L224 (thanks @LFDanLu for finding this)

I would say this is working as intended.

sanyabas commented 2 years ago

Oh, I see. But shouldn't is still be stopPropagatoned so that only one element receives event like other press events? I suppose selectables shouldn't break as user still clicks on selectable node itself.

snowystinger commented 2 years ago

Unfortunately, we can't do that right now, we rely on onPointerUp at the document level in order to do hit detection because there are some differences between browsers that we're compensating for. https://github.com/adobe/react-spectrum/blob/f0c9a597aa8d9eadeb47e81aacd9af65dca77700/packages/%40react-aria/interactions/src/usePress.ts#L348 That was Safari and a couple of versions ago, so at some point, yes, we should be able to. Until then, you could listen for PressStart before PressUp, or you can just use Press/PressEnd, which already has that logic built in.

LFDanLu commented 2 years ago

Closing, feel free to reopen if the above work around doesn't work for you.