Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support onClick as an alias for onPress #7891

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

devongovett
Copy link
Member

@devongovett devongovett commented Mar 7, 2025

Proposal for discussion. See this discussion: https://x.com/deebovv/status/1897355956188537016

In particular:

For us (@UntitledUI) it was the unfamiliarity of the community with the “onPress” prop and the “onClick” prop being deprecated in React Aria buttons. Therefore we had to map the onClick to onPress internally for buttons. I wish it “just worked” without people having to relearn what they have been doing for years (I think bun is a great example).

The problem is that onPress makes it harder to integrate with other libraries.

For example, a 3rd party popover library might pass down onClick to the trigger. But if the trigger is React Aria button we have to remap onClick to onPress.

Or the opposite, cannot use a native button as a trigger with React Aria popover because it uses onPress and we have to remap it again.

We solved the second case with <Pressable> added in the last release. The first case of using a RAC Button as a trigger for a third-party component that passes in onClick remains.

This PR basically treats onClick as an alias for onPress. It gets fired at the same times. If we have a native MouseEvent event available, it gets passed to the handler. If not, e.g. in a keyboard event, we synthesize a fake click event and pass that. This matches what browsers do for certain elements: https://html.spec.whatwg.org/#fire-a-synthetic-pointer-event.

Questions:

  • We do some event filtering in usePress to avoid edge cases. For example, we only fire onPress for left clicks, not right clicks, and we don't fire onPress for repeating keyboard events. Seems like matching this in onClick would be good to take advantage of this behavior, but could be unexpected that it doesn't match the browser behavior for onClick. But if people will be using onClick just because they're used to that name, they'd lose out on some of this improved behavior, so I lean towards having it match onPress. Thoughts?
  • Also, is it unexpected to fire onClick in cases where the browser doesn't do so?
  • I marked onClick as @deprecated in the prop definition, which means it'll be omitted from the docs. This way we nudge people toward onPress but it works regardless, e.g. when used with third party libraries. Also means it'll be rendered with a strikethrough in people's IDEs. But, there's no more console warning. Thoughts?

@rspbot
Copy link

rspbot commented Mar 7, 2025

@rspbot
Copy link

rspbot commented Mar 7, 2025

## API Changes

@react-aria/dnd

/@react-aria/dnd:DropResult

 DropResult {
   dropButtonProps?: AriaButtonProps
-  dropProps: HTMLAttributes<HTMLElement>
+  dropProps: DOMAttributes
   isDropTarget: boolean
 }

@deebov
Copy link
Contributor

deebov commented Mar 7, 2025

Thanks for opening up this discussion @devongovett. I would like to first inform myself better with the issues usePress tackles before making a thoughtful suggestion.

How much of the issues you described in (Building a Button Part 1: Press Events) is still relevant? It's been 5 years since the article was published — I assume browsers had at least fixed some of the inconsistencies mentioned in the article. Or are there as well other sources to check which issues usePress solves?

@devongovett
Copy link
Member Author

Some of it is still relevant, but there have been quite a few improvements. I'm planning on writing an update to that blog post series soon.

The major thing that changed recently is that buttons now receive focus consistently in all browsers, so this part is no longer necessary. Therefore, as of #7542 we no longer use preventDefault in usePress. That PR lists a whole bunch of issues that were fixed.

Also in that PR, we now trigger onPress during the native onClick event instead of onPointerUp, which fixed some additional issues. The old browser fallbacks for mouse/touch events are also no longer necessary since all browsers support pointer events (unfortunately JSDOM does not, which prevents us from removing it entirely). The custom hit testing described there is also no longer needed. At this point, onPress is only fired during onClick and onKeyUp. onPressStart and onPressEnd are fired during pointer events.

However, other behaviors described in the original blog post, such as text selection behavior, active state, hover state, cancelation, virtual clicks, repeating keyboard events, etc. still apply. I believe it's still important to handle those for a good experience, especially on mobile/touch devices, but luckily we can now rely on native events more.

@MilllerTime
Copy link

MilllerTime commented Mar 8, 2025

Looking good @devongovett!

We do some event filtering in usePress to avoid edge cases. For example, we only fire onPress for left clicks, not right clicks, and we don't fire onPress for repeating keyboard events. Seems like matching this in onClick would be good to take advantage of this behavior, but could be unexpected that it doesn't match the browser behavior for onClick. But if people will be using onClick just because they're used to that name, they'd lose out on some of this improved behavior, so I lean towards having it match onPress. Thoughts?

I think your onPress implementation makes a lot of sense for common UI components – onClick would benefit from that work too, and it may not be that much different from native behavior. In your example of filtering only left clicks, browsers do this too, right? I don't trust my memory, so quickly tested this with a mouse on both a <button> and <div> in Chrome and FF (Safari isn't handy atm). "click" events only fired on a left click for each.

Also, is it unexpected to fire onClick in cases where the browser doesn't do so?

What are the cases? Anything beyond keyboard events?

I marked onClick as @deprecated in the prop definition, which means it'll be omitted from the docs. This way we nudge people toward onPress but it works regardless, e.g. when used with third party libraries. Also means it'll be rendered with a strikethrough in people's IDEs. But, there's no more console warning. Thoughts?

If I saw a strikethrough, but it worked, though I couldn't find info in the docs, I'd be a little confused. Would it be okay to treat onClick as a sort of "compatibility alias", not a deprecated feature, and the docs explain this while recommending onPress when possible?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants