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.76k stars 1.09k forks source link

onClick cannot be triggered using continuePropagation #6267

Closed Ciavarella closed 2 months ago

Ciavarella commented 5 months ago

Provide a general summary of the issue here

onClick should be able to be triggered on parent but it is not, calling the e.continuePropagation does not change the parent onClick to be able to be triggered.

๐Ÿค” Expected Behavior?

onClick should be able to be triggered on parent

๐Ÿ˜ฏ Current Behavior

onClick is not triggered on parent when clicking a child onPress or onPressStart with continuePropagation.

๐Ÿ’ Possible Solution

No response

๐Ÿ”ฆ Context

No response

๐Ÿ–ฅ๏ธ Steps to Reproduce

https://codesandbox.io/p/sandbox/pensive-monad-vp8yp2?file=%2Fsrc%2FApp.js%3A25%2C17

Version

-

What browsers are you seeing the problem on?

Chrome

If other, please specify.

No response

What operating system are you using?

Mac

๐Ÿงข Your Company/Team

No response

๐Ÿ•ท Tracking Issue

No response

sookmax commented 5 months ago

onClick is not triggered on parent when clicking a child onPress or onPressStart with continuePropagation.

As far as I can tell, only PressEvent handlers of the parent (a 'pressable' component, using usePress()) see the propagated event when PressEvent.continuePropagation() is called in the child (again, a 'pressable' component, using usePress()):

https://github.com/adobe/react-spectrum/blob/b2c9961586795542603b899a1e1925cbeee9f34f/packages/%40react-aria/interactions/test/usePress.test.js#L2896-L2921

The description of continuePropagation() also seems to say a similar thing: https://github.com/adobe/react-spectrum/blob/b2c9961586795542603b899a1e1925cbeee9f34f/packages/%40react-types/shared/src/events.d.ts#L48-L53

Crustan commented 5 months ago

onClick is not triggered on parent when clicking a child onPress or onPressStart with continuePropagation.

As far as I can tell, only PressEvent handlers of the parent (a 'pressable' component, using usePress()) see the propagated event when PressEvent.continuePropagation() is called in the child (again, a 'pressable' component, using usePress()):

This is a major issue since you might have third-party libraries that apply event listeners on wrapping components, that ain't a PressEvent (like MouseEvent), expecting the event to propagate.

Right now it's impossible to prevent the propagation from being stopped in those cases which totally breaks the other library, in our case, hotjar (https://help.hotjar.com/hc/en-us/articles/7053232973975-Heatmap-Data-Troubleshooting).

staticshock commented 5 months ago

Related: I'm seeing this in the context of a Checkbox, where a press (combined with continuePropagation) sometimes makes it through to the parent component when using tap-to-click on a trackpad, but not consistently. Tap-to-click the "Test" button a bunch of times here and eventually you'll see a state change:

https://codesandbox.io/p/sandbox/trusting-night-9q9824

https://github.com/adobe/react-spectrum/assets/13100/c8c459c3-a061-42fb-b097-4f3cadb2487d

torgeadelin commented 3 months ago

Also, If I'm not mistaken, there doesn't seem to be a way to continue propagation for the onClick? https://github.com/adobe/react-spectrum/blob/b2c9961586795542603b899a1e1925cbeee9f34f/packages/%40react-aria/interactions/src/usePress.ts#L303-L335

UNLESS the event is virtual (Keyboards, Assistive Technologies, and element.click()) I want to be able to continuePropagation for normal mouse click as well.

In my example I have a 3rd party component that is inside a list item that is implemented via usePress. I can't interact with the 3rd party element via mouse click, because the propagation is stopped by the list item.

snowystinger commented 3 months ago

If your component is inside the one using usePress, then propagation shouldn't matter? We're not using a capturing listener, so your 3rd party component would get the event first wouldn't it?

IonelLupu commented 2 months ago

@snowystinger, in my case, my component that has the onClick handler IS the parent of a Button (the react-aria-components Button). I am building a platform similar to Webflow that allows people to change the styles of the selected elements on the page.

I have an onClick handler on the main parent element (just a div) and I use the event.target property to get the element the user clicked on/selected. The react-aria-components Button cannot be selected because the main parent onClick handler doesn't get triggered even if I use the .continuePropagation() method. I said: "OK, maybe I need to use the usePress hook on the parent element too".

What's worse is that I can't even use the usePress hook to attach the onPress event handler to my main parent div because the event.target property is always that main div. It's not the actual clicked element on the page. The normal browser onClick event.target returns the actual clicked element.

Here is a code example. Go an click on each paragraph and buttons on the page and watch the logs:

  1. when clicking on a react-aria-components Button, there isn't an event triggered event if I use .continuePropagation() and even if the parent is using the usePress hook
  2. when clicking any other element in the parent container, the onPress event.target doesn't return the clicked element just like the normal onClick event does. It always returns the element the onPress was attached to

https://codesandbox.io/p/devbox/toggle-button-react-aria-forked-ckh7fd?file=%2Fsrc%2FApp.jsx%3A34%2C1&welcome=true

image

snowystinger commented 2 months ago

The codesandbox is private, you'll need to make it public.

However, from looking at the screenshot you provided, continuePropagation just isn't being called in enough places. This is a pretty advanced use case and is not generally recommended as it may have unintended consequences. For instance, if you place a Button inside a Table, the press should be handled by the button, not the cell or row containing it.

In order to continuePropagation, you need to continue it on ALL press events, including Start and End.

IonelLupu commented 2 months ago

@snowystinger, I just made the codesandbox public and also added the onPressStart and onPressEnd handlers: https://codesandbox.io/p/devbox/toggle-button-react-aria-forked-ckh7fd?file=%2Fsrc%2FApp.jsx%3A23%2C28&welcome=true

It seems it now correctly propagates the event up. But I still have an issue: The parent onPress handler doesn't correctly return the clicked target. Check the logs in the 1st and 3rd example in the codesandbox. I get the Div element instead of getting the clicked paragraph or the clicked button

The browser's onClick handler returns the clicked element, not the element that has the onClick handler attached.

I have this platform I am building where users can click anywhere on the page to "select" an element and then change its styles? The only way I could do this is with an onClick handler added to the main container of the page and then using event.target to get the clicked element.

PS. What is the reasoning for "In order to continuePropagation, you need to continue it on ALL press events, including Start and End."? Why not just onPress?

snowystinger commented 2 months ago

PS. What is the reasoning for "In order to continuePropagation, you need to continue it on ALL press events, including Start and End."? Why not just onPress?

It has to do with how browsers work; if you stop events earlier in the chain, the actual browser onClick event will not fire. So you have to continue them all.

This shouldn't be a common thing to need to do. Attaching a click to the entire page will miss potentially many events from keyboard/assistive technology/touch and even mouse users. Many libraries will stop events, so it's not good to rely on an event that can be stopped so easily.

Instead, if you need to guarantee that you handle an event, I'd recommend looking into capture phase listeners for pointerdown/keydown. There are some other discussions in the repo about this with regard to 3rd party analytics libraries. I suggest having a search for those, it may help your use case as well.

Out of curiosity, why do you need the other target? can you usePress on that element? seems more React like and it'll prevent potential double/multiple handling. Or it sounds like you shouldn't be using usePress in this scenario?

IonelLupu commented 2 months ago

Out of curiosity, why do you need the other target? can you usePress on that element? seems more React like and it'll prevent potential double/multiple handling. Or it sounds like you shouldn't be using usePress in this scenario?

@snowystinger, imagine you are using my platform to edit some React components or pages that you have in your project. You open your page (by selecting the YourPage.tsx file) and it will be rendered on the platform (imagine Wix website builder or Webflow). Then, you can click on any element on your page that you want to change the styles for. You can have anything on your page: headings, paragraphs, buttons, divs etc.

To have this "click on anything to change styles" feature I can have one big event handler added to the container that has you page's HTML OR I can iterate over every element on your page and manually adding the onClick event handler on them (or maybe onPress events!?). The 2nd option sounds tedious and that's why I want one main click handler.

Now, you may say: "looks like your platform is pretty advanced and you are better off not using react aria for this. You should only use simple React and Javascript events because you have more control".

Well, I tried. I added the onClick event on the main parent and it works perfectly the way I described above, but only for normal HTML elements. The problem is this: if you have react-aria buttons on your page (or drag&drop a new button from the platform's built-in components which are react-aria buttons) and try to select them by clicking on them, they don't trigger to the native onClick handlers on the parent because react aria doesn't propagate the event up. (and they also don't react on the onClick events. I have to have the onPress on the parent)

One solution is to not have react aria components in the platform's built-in components and use something else all together. ๐Ÿ˜ž But this is not ideal because you, as a user, might want to use anything that is available: radix, tailwind UI, material UI, react-aria etc.

I really don't know how to solve this for react-aria components. I really want users to have anything on the page that they can select. Seems like the tedious solution described above might be the way to go... ๐Ÿ˜ž

snowystinger commented 2 months ago

I think I understand your usecase better now. I would say you're trying to force React Aria to be something that it isn't. If you click on anything to change styles, then by definition, everything is a button. This directly conflicts with the roles and behaviors components may have.

I imagine that you'll want to be able to drag the components around after initially dropping them as well.

This falls into some categories of discussions that have come up recently. Given that everything is just buttons so you can drag them or open a popover from them, the component is really just a visual representation, not the actual functionality of the component. If you have a toggle between the edit mode and a preview mode, an option you have is you can swap out for the real ones at that time.

Another possibility would be to wrap each component in inert or something like this https://jsfiddle.net/bekuqamj/3/

Or you could add an overlay on top of your entire application to intercept events, or you can use capture phase on pointer events/keyboard events.

Keep in mind that our components are meant to be interacted with only in the way their roles dictate.

I'm going to close this issue as continue propagation can be achieved.

Also, responding to another comment above https://github.com/adobe/react-spectrum/issues/6267#issuecomment-2089083707 This is an invalid configuration, you cannot have a button inside a checkbox or label. Please position the button outside and next to the checkbox.