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

usePress / useLink is not compatible with NextJS's next/link #2525

Open
sdnts opened this issue Nov 6, 2021 · 15 comments
Open

usePress / useLink is not compatible with NextJS's next/link #2525

sdnts opened this issue Nov 6, 2021 · 15 comments

Comments

@sdnts
Copy link

sdnts commented Nov 6, 2021

🐛 Bug Report

Because of NextJS's next/link component's special behaviour, custom components using the usePress (or useLink) do not work the way I'd expect

🤔 Expected Behavior

When I use a component built with react-aria hooks as a child of next/link, I expect successful client-side navigation to the destination

😯 Current Behavior

Depending on the usage, either I see an error being thrown, or I see successful server-side navigation (a full page reload)

This is the error I see (you'll be able to see it too in the CodeSandbox example I've linked)

Unhandled Runtime Error

TypeError: can't access property "nodeName", e.currentTarget is undefined
linkClicked
node_modules/next/dist/client/link.js (39:27)
onClick
node_modules/next/dist/client/link.js (201:28)
triggerPressEnd
node_modules/@react-aria/interactions/dist/module.js (213:0)
onPointerUp
node_modules/@react-aria/interactions/dist/module.js (427:0)

💁 Possible Solution

React-aria's PressEvent could expose properties available on the native MouseEvent. Howver, I'm not sure about the feasibility / consequences of this

🔦 Context

We use React-aria in our design system for building a Button component. In our app (which uses NextJS), some buttons only take you to a different page on the app, so I treat them as links (that just happen to look like buttons)

In Next, the recommended way to use links is via their next/link component. That component injects an onClick prop to its first child, and this onClick calls event.preventDefault to be able to do client-side navigation (AKA navigation without a full page refresh)

I expect to be able to use my Button this way:

import Link from 'next/link'

function TakeMeSomewhereNice() {
  return (
    <Link href="/somewhere" passHref>
      <Button />
    </Link>
  )
}

However, this throws an error (you should be able to see this in the CodeSandbox I've linked below).

I understand this is quite a niche use-case, and I'm not even sure if react-aria can / should handle this. I can't imagine where else this problem would pop up, so as far as I know, this issue is limited to next/link.

I have investigated the issue and do have ways to get around this but the solution defeats the purpose of using react-aria in the first place, so it is not ideal. I'm happy to explain in detail exactly what's going on if you'd like (in Next and react-aria), but I'm not sure if that's relevant / required right now.
For some more context, here is Next's next/link component: https://github.com/vercel/next.js/blob/5e185fc5da227801d3f12724be3577f4a719aa69/packages/next/client/link.tsx

💻 Code Sample

Here is a minimal repro:
https://codesandbox.io/s/next-link-react-aria-incompatibility-l0k0j?file=/pages/index.jsx

The idea is that navigating between the / & /about pages should not reload the page.
You'll see that I have two components on each page. The Button uses the usePress hook, and the Link uses the useLink hook.
The Button crashes, while the Link navigates correctly, but the entire page is reloaded.

🌍 Your Environment

Software Version(s)
@react-aria/interactions 3.6.0
@react-aria/link 3.1.4
Browser Irrelevant
Operating System Irrelevant

🧢 Your Company/Team

🕷 Tracking Issue (optional)

@snowystinger
Copy link
Member

Hey! thanks for opening this, it can be a little bit of work to get some other libraries to work with react spectrum, but you should be able to address this by changing your Button implementation to

export const Button = ({ children, onClick }) => {
  const { pressProps } = usePress({
    onPress: (e) =>
      onClick({ ...e, currentTarget: e.target, preventDefault: () => {} })
  });

  return <button {...pressProps}>{children}</button>;
};

It should be safe to pass a dummy preventDefault because we already call it in usePress in most cases.
You can find more info on it here #963

@sdnts
Copy link
Author

sdnts commented Nov 17, 2021

Hey Robert, sorry for getting back to this so late, but your suggestion does work! I think it's a pretty nice "workaround" to fake the event for consumers that need it, so I'll close this. Thanks so much!

@sdnts sdnts closed this as completed Nov 17, 2021
@alirezamirian
Copy link
Contributor

alirezamirian commented Dec 16, 2021

@snowystinger
I'm facing the same issue trying to use react-router's useLinkClickHandler for a Button implemented with useButton (very similar to React Spectrum Button). Your suggestion works to call the click handler in onPress, but the event is not defaultPrevented and therefore a full reload happens. Are you sure preventDefault is already called in most cases? From what I see, preventDefault for click events is called only when the element is disabled

@snowystinger
Copy link
Member

If you're using a button, then how is a full reload happening? Buttons don't have a default behavior to navigate. If you used an anchor element, I could see that happening.
Can you provide a codesandbox showing the issue? Maybe we'll need to figure out a way to expose this behavior depending on the use case.

@alirezamirian
Copy link
Contributor

alirezamirian commented Dec 17, 2021

@snowystinger
I should have been more clear. I meant button appearance but as an anchor element. Like how you can render a React Spectrum button as an anchor element via elementType.
Here is a codesandbox trying to create a LinkButton component inspired by the suggestion from react router docs. preventDefault being a noop function, just prevents the runtime error, but doesn't prevent the default reload behavior of course.
We could even create a useLinkPressHandler hook, similar to useLinkClickHandler, but that would still require ability to call preventDefault on the original event.

@snowystinger
Copy link
Member

Bringing it up with the team, I'll reopen for now for more visibility.

@snowystinger snowystinger reopened this Dec 21, 2021
@csantos-nydig
Copy link

It should be safe to pass a dummy preventDefault because we already call it in usePress in most cases.

I've read this in many many places by now, but it's really not clear when usePress preventDefault the interaction.

I'm trying to understand usePress.ts code and it seems all events do shouldPreventDefault check before calling preventDefault()
but the only one that does not do it is the actual onClick: https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/interactions/src/usePress.ts#L263-L265

I wonder if it should be:

if (isDisabled || shouldPreventDefault(e.currentTarget as HTMLElement)) {
  e.preventDefault();
}

cc: @devongovett

@monikakinder
Copy link

@snowystinger is there any news for this issue? I really like react-aria lib but to be able to use it in our projects we would need a support for nextjs link to work without a full reload. Thank you!

@snowystinger
Copy link
Member

snowystinger commented May 24, 2022

Sorry, I lost this thread, thanks for the reminder and patience. I'll remember to bring it up this week.

So unfortunately it won't quite be as simple as adding that OR statement. Doing that causes checkboxes, switches, and radios to fail.

Test Suites: 7 failed, 2 skipped, 156 passed, 163 of 165 total
Tests:       38 failed, 51 skipped, 3931 passed, 4020 total

@tgelu
Copy link

tgelu commented Jan 4, 2023

Hi. Is there any chance there's an update on this? 😄

@snowystinger
Copy link
Member

snowystinger commented Jan 4, 2023

There is a solution if you're using react-aria, you can preventDefault yourself. Here's a codesandbox based on the one in the description
https://codesandbox.io/s/next-link-react-aria-incompatibility-forked-seom13?file=/pages/about.jsx

@frankieali
Copy link

I'm facing the same issue with react-router-dom and react-aria. Here's a simple demo showing the difference between plain navigation links that work as expected vs buttons in a component https://codesandbox.io/s/react-aria-issue-77070k?file=/src/Pages/Layout.js:167-177

@devongovett
Copy link
Member

Buttons that navigate to a new page should not be buttons, they should be links. That way they are announced by assistive technology as links. They can be styled to look like buttons if you want, but they should use the <a> element under the hood.

In your example, you have a <Link> (i.e. <a>) wrapping a button. This is invalid HTML. Links cannot contain other interactive elements inside them. See MDN.

I would suggest moving the <Link> inside the <Button> component for rendering button-looking links. You should also probably use useLink rather than useButton, but that is debatable. If you use useButton, set elementType: 'a' to get correct accessibility semantics (role="button") applied. In either case, React Aria detects that the element is a link and will not prevent default on it.

Here's a forked version of your sandbox with the changes applied: https://codesandbox.io/s/react-aria-issue-forked-u7llrm?file=/src/Components/Button/index.js

@frankieali
Copy link

Thanks @devongovett . I discovered the same as well in #963. I've restructured my navigation in the following sandbox demo to avoid buttons in anchor links. https://codesandbox.io/s/react-aria-issue-forked-1nqk27?file=/src/Pages/Layout.js.
My team is just getting started with react-aria. Thanks for the tip on useLink.

@jkhaui
Copy link

jkhaui commented Nov 9, 2023

Good OP, thanks for the detailed description of what's going on. I would be less apologetic, however, in calling out this use-case.

Seems evident from the discussion here that implementing a bespoke Link component (which augments a base HTML anchor element) is essential for any modern-day JS framework. It's the only way to integrate client-side navigation with core platform attributes (e.g. accessibility, prefetching). And the only way to enable client-side navigation using an <a> tag is to directly call the event's preventDefault method.

Could we do something like extend usePress with a prop like preventHrefDefault? In this way, it should be possible for a user to directly control href behaviour by setting the flag. And it shouldn't break other react-aria components.

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

No branches or pull requests

9 participants