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

Why is preventDefault not made available on press events? #963

Closed
justinanastos opened this issue Aug 14, 2020 · 18 comments Β· Fixed by #904
Closed

Why is preventDefault not made available on press events? #963

justinanastos opened this issue Aug 14, 2020 · 18 comments Β· Fixed by #904

Comments

@justinanastos
Copy link

❔ Question

Why is preventDefault not available in PressEvent?

πŸ”¦ Context

I'm replacing the guts our of style system with react-spectrum and ran into an incompatibility with our codebase because preventDefault is not in onPress from useButton (or usePress).

There's a few use cases. First, we have clickable rows in a list that have a delete button inside of them. The delete button needs to prevent the default so the row itself won't navigate away.

πŸ’» Code Sample

🌍 Your Environment

Software Version(s)
react-spectrum v3.1
Browser Chrome Mac
Operating System

🧒 Your Company/Team

Apollo GraphQL πŸš€

@snowystinger
Copy link
Member

We preventDefault in many place in usePress https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/interactions/src/usePress.ts#L302
Are we just missing one?
As for why we don't expose preventDefault itself, I'm not 100%, I'll try to get a better answer. In the meantime, there are parts of it here https://react-spectrum.adobe.com/blog/building-a-button-part-1.html

@justinanastos
Copy link
Author

We preventDefault in many place in usePress https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/interactions/src/usePress.ts#L302
Are we just missing one?
As for why we don't expose preventDefault itself, I'm not 100%, I'll try to get a better answer. In the meantime, there are parts of it here react-spectrum.adobe.com/blog/building-a-button-part-1.html

Thanks for getting back to me @snowystinger . I'm looking forward to hearing more!

@snowystinger
Copy link
Member

snowystinger commented Aug 14, 2020

Can you tell us which event it is that you need to prevent default on?

@justinanastos
Copy link
Author

justinanastos commented Aug 14, 2020

Can you tell us which event it is that you need to prevent default on?

@snowystinger I need to call preventDefault in an onPress event. I have a list of graphs that are rendered as <a> so they will navigate somewhere else in the application. Some of them have a delete button, a button created implementing useButton. When I click that delete button, I need to call event.preventDefault to prevent the navigation resulting from clicking <a>.

@snowystinger
Copy link
Member

Sorry, can you tell if it's the default of the mouse/pointer down, up? there are several events present in the construction of onPress

@snowystinger
Copy link
Member

Also, what is the dom structure for this? I'm envisioning something along these lines and want to double check

<a href="somewhere">This will take you elsewhere <button>Delete</button></a>

@justinanastos
Copy link
Author

@snowystinger That is exactly the DOM structure we're looking at; a <button> nested under a <a>. As for the events, the call stack is my onPress function called by triggerPressEnd called by onPointerUp.

@snowystinger
Copy link
Member

Ah, that is not valid HTML, so you probably shouldn't be doing that.
https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-a-element

Content model:
Transparent, but there must be no interactive content descendant, a element descendant, or descendant with the tabindex attribute specified.

@snowystinger
Copy link
Member

o, and this is probably why we aren't exposing preventDefault on the press events

there are several events present in the construction of onPress

@justinanastos
Copy link
Author

Ah, that is not valid HTML, so you probably shouldn't be doing that.
html.spec.whatwg.org/multipage/text-level-semantics.html#the-a-element

Content model:
Transparent, but there must be no interactive content descendant, a element descendant, or descendant with the tabindex attribute specified.

@snowystinger While I know we shouldn't be doing this; the unfortunate fact is we are πŸ˜ƒ I think exposing preventDefault, even if its use is strongly advised against, would make incremental adoption of useButton much easier.

@snowystinger
Copy link
Member

Well you may be in luck, it looks like we're doing something odd here https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/interactions/src/usePress.ts#L228
We should probably be consistent with the rest of our events, but this is going to require some serious testing, so not something that will get in this release most likely.

in the meantime, you could use the deprecated onClick prop to handle it yourself until you can get the button out of the link :) onClick={e => e.preventDefault()} it will result in a console warning, but won't actually break anything
I'll post a link to the branch we're testing on once i have that up

@justinanastos
Copy link
Author

Thank you @snowystinger ! I'll do that for the time being πŸ˜ƒ

@deebov
Copy link
Contributor

deebov commented Aug 15, 2020

@snowystinger I think usePress does not support keyboard events for anchor elements. by default, only the enter key triggers the press event, the spacebar is ignored. But if you do onClick={e => e.preventDefault()} the enter key will stop working too. here the sandbox

@snowystinger
Copy link
Member

I'm not sure I follow your concern @deebov this is the temporary workaround I was suggesting https://codesandbox.io/s/busy-hooks-dhx4u
Is there something in that not working as expected? (minus the invalid html structure)

If you have a concern about something we might break by us default calling preventDefault onClick would you mind commenting on this PR #904
We're definitely apprehensive about a change like this, so more eyes on it is definitely helpful.

We do have some anchor tag handling in usePress, though we'd optimally like to remove it from there to a more appropriate aria hook(s) https://github.com/adobe/react-spectrum/blob/main/packages/@react-aria/interactions/src/usePress.ts#L599

@jrmyio
Copy link

jrmyio commented Mar 4, 2021

I have a link button with an onPress, I want the link to be just there but customize what happends when it is clicked.

Is there any way to prevent the default link behavior?

@snowystinger
Copy link
Member

Sure, if you're just using the hooks libraries, add an onClick to the anchor element and prevent default in it.
If you're using the React Spectrum components, you can still do that, but you'll get a warning about deprecated onClick, as mentioned above.

@GriffinSauce
Copy link

It would be helpful to at least add this to the documentation. preventDefault is very commonly used - being directed to use a deprecated function to solve it is not great but it would be nice to at least have this limitation front and center so it's not a surprise that requires a roundtrip to this issue.

I have a similar use case, an <a> link with a click handler. The click handler facilitates better in-context UX and the link facilitates opening the feature in a tab. That pattern may not be super common but in the past years I have had to reach for it several times to reach the desired outcome.

Instead of providing the preventDefault to the event handler it could potentially also be an option on the hook, that would allow you to handle it consistently internally, whatever the mix of events is that is required to build onPress.

@devongovett
Copy link
Member

That's a reasonable suggestion. Should it go on useLink since it's fairly link specific though?

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 a pull request may close this issue.

6 participants