Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Link API breaks a11y intuition #5533

Closed
danny-andrews-snap opened this issue Oct 26, 2018 · 22 comments
Closed

Link API breaks a11y intuition #5533

danny-andrews-snap opened this issue Oct 26, 2018 · 22 comments
Assignees
Labels
Navigation Related to Next.js linking (e.g., <Link>) and navigation.

Comments

@danny-andrews-snap
Copy link

danny-andrews-snap commented Oct 26, 2018

Feature request

I want a more intuitive Link API which respects expectations we have around native a tags.

Is your feature request related to a problem? Please describe.

The current link API looks like the following:

<Link href="/about">
  <a>here</a>
</Link>

This is problematic for a few reasons:

  1. It breaks intuition around a11y. When I see an a tag without an href, alarm bells go off, because that is a UX anti-pattern. In fact, this API makes it impossible to appease the anchor-is-valid eslint-plugin-jsx-a11y rule (see https://github.com/evcohen/eslint-plugin-jsx-a11y/issues/402).
  2. It requires a call to cloneElement which should be avoided as its behavior is confusing. (How would I know my a tag will be given an href? Nothing about this API suggests that.)

Describe the solution you'd like

I would like a solution which doesn't require you to type an a tag without an href and also doesn't require you to duplicate the href information.

Describe alternatives you've considered

There are a few good alternatives:

  1. Default to a tag if no children are given:
<Link href="/about" />

This is the approach react-router uses and it works quite well.


  1. Use a render prop:
<Link href="/about">
  {href => <a href={href}>here</a>}
</Link>

This method appeases the anchor-is-valid rule and doesn't require the user to duplicate href information since the build href is passed down to the component they are rendering.


  1. Component injection:
<Link href="/about" component={a} />

A combination of 1 and 2 would be powerful enough to cover all reasonable use cases.

Additional context

6431f5f

marcqualie added a commit to moltin/taxjar-example that referenced this issue Dec 7, 2018
Had to override one of the link accesibility lints due to nextjs bug

vercel/next.js#5533
marcqualie added a commit to moltin/taxjar-example that referenced this issue Dec 8, 2018
Had to override one of the link accesibility lints due to nextjs bug

vercel/next.js#5533
@alexandernanberg
Copy link
Contributor

@timneutkens Would you accept a PR that would uses a similar approach like this? It would be a pretty big breaking change but imo it's worth it. I think the current next/link API i one of next's weak points atm.

@StefKors
Copy link

Any progress on this? running into the same issue trying to setup tests for a11y things.

@gromchen
Copy link

We had to use the following solution https://github.com/evcohen/eslint-plugin-jsx-a11y/issues/402#issuecomment-368305051, but it would be nice to have other options.

@Timer Timer added this to the 9.0.x milestone Jul 25, 2019
@Timer Timer modified the milestones: 9.0.x, backlog Aug 30, 2019
@WestonThayer
Copy link
Contributor

Worth pointing out that the above solution (https://github.com/evcohen/eslint-plugin-jsx-a11y/issues/402#issuecomment-368305051) disables the rule's check for an href attribute altogether. That's 90% of the linting rule's value for me.

@eunicekokor
Copy link

Yes, I would agree with/echo @WestonThayer's point! The point of having the accessibility plugin is to audit for cases where we aren't supporting important rules. If the fundamental Linking structure of Next doesn't allow for us to check if we're being compliant, we'll have to continue manually do this checking in our process or handle it later on in our build process (adding more unnecessary dev time).

@hasparus
Copy link

Isn't this a limitation of styled-jsx? Anchor rendered by Links wouldn't have styles applied.

@renet
Copy link

renet commented Jan 10, 2020

Any news on this issue, or a good workaround that doesn't disable the href-rule?

@hasparus
Copy link

@renet build the Link we want from the Link we get
The limitation is -- it breaks styled-jsx for a, so you'd need an alternative CSS-in-JS solution.

"jsx-a11y/anchor-is-valid": [
  "error",
  {
    "components": ["Link"]
  }
],
"jsx-a11y/anchor-has-content": [
  "error",
  {
    "components": ["Link"]
  }
]
import NextLink, { LinkProps as NextLinkProps } from "next/link";
import { ComponentProps } from "react";

export interface LinkProps
  extends NextLinkProps,
    Omit<ComponentProps<"a">, keyof NextLinkProps> {}

export const Link = ({
  href,
  as,
  replace,
  scroll,
  shallow,
  passHref,
  prefetch,
  ...rest
}: LinkProps) => (
  <NextLink
    href={href}
    as={as}
    replace={replace}
    scroll={scroll}
    shallow={shallow}
    passHref={passHref}
    prefetch={prefetch}
  >
    {/* href is passed by NextLink */}
    {/* eslint-disable-next-line jsx-a11y/anchor-is-valid, jsx-a11y/anchor-has-content */}
    <a {...rest} />
  </NextLink>
);

@mosesoak
Copy link

This issue from 2018 is still relevant. All three solutions proposed in the original post are great. They're called out as breaking changes but I would point out that the current api could continue to be supported, making it a non-breaking change. Simply adding any of those 3 options would make Next.js play nicely with jsx-a11y.

@schwigri
Copy link

schwigri commented Sep 30, 2020

I think this is really important. Right now, Next.js is competing with Gatsby and I've realized very quickly that I cannot use Next.js due to the lack of accessibility support. Accessibility is highly important and should be a priority (and goes further than just complying with jsx-a11y).

@cyrus-za
Copy link

cyrus-za commented Dec 9, 2020

Also running into this. I am using nx.dev with nextjs which sets up jsx-a11y for me. I'd hate to have to disable the eslint rule globally as there will be anchor link without Link parent for example external links

@Timer Timer self-assigned this Dec 31, 2020
@Timer Timer modified the milestones: iteration 15, iteration 16 Jan 4, 2021
@415DomSmith
Copy link

Hitting this with an old school bump.

Still an issue in 2021, and taking off the a11y guardrails linting is not an option for big teams and projects.

@MartinDevillers
Copy link

An alternative (but hacky) solution is to enable the passHref property and set the inner href to a dummy value. Next.js will override the dummy value with the correct value from the outer component, so everything will work correctly and it'll pass the jsx-a11y/anchor-is-valid rule.

  <Link href="/my-amazing-page" passHref>
    <a href="dummy">Go to my amazing page</a>
  </Link>

@timneutkens
Copy link
Member

Still an issue in 2021

We're still working on solving this. It's more complicated than you're making it out to be as you're not considering the hundreds of thousands existing Next.js applications that would not be able to upgrade to a version that has the proposed solution in this issue.

In Next.js 10 we introduced https://nextjs.org/blog/next-10#automatic-resolving-of-href as an incremental step towards this. And there's an open RFC to build on top of that: #8207 to enable <Link to=""> to automatically handle the <a> always being injected. Using href for this behavior would cause issues when people upgrade, potentially a codemod could be written though.

https://twitter.com/timneutkens/status/1295363065056301056

@cyrus-za
Copy link

A codemod would be great. Alternatively a eslint plugin that extends from the a11y one but allows anchors without href if they are wrapped in a Link tag

@zackdotcomputer
Copy link
Contributor

Excited to see progress on this. While waiting for a longer term solution, I've been using a utility component that handles the creation of the Link and a components together. This way you can add the utility component to the components-to-validate list in eslint's config and get both the safety of the linting and the features of <Link> until the api can be shifted.

@Timer
Copy link
Member

Timer commented Jan 21, 2021

RFC: #8207

@Jak-Ch-ll
Copy link

Jak-Ch-ll commented Mar 10, 2021

I have a question regarding this issue:

Omitting the a-tag and only having text inside the Link Component seems to work, because of the following part of the code for next/link:

    // Deprecated. Warning shown by propType check. If the childen provided is a string (<Link>example</Link>) we wrap it in an <a> tag
    if (typeof children === 'string') {
      children = <a>{children}</a>
    }

What are the downsides to using this as a workaround?
Styled-jsx was already mentioned above and it's also not possible to give the Link-Component a class.

P.S.: The comment says, Warning shown by propType check., but there is no warning shown for me (with typescript).

@Timer Timer modified the milestones: iteration 17, 10.x.x Mar 15, 2021
@GiancarlosIO

This comment has been minimized.

@wtakayama-chwy
Copy link

I was taking a look on this issue and I thought if this would be a good workaround:

import React, { forwardRef, ReactNode } from 'react';
import Link, { LinkProps } from 'next/link';

interface NextLinkProps extends LinkProps {
  children?: ReactNode;
  className?: string;
  style?: CSSProperties;
  prefetch?: boolean;
  target?: '_blank';
  shallow?: boolean;
  scroll?: boolean;
}
export default forwardRef(
  (
    {
      href,
      children,
      shallow = false,
      className,
      style,
      target,
      scroll = true,
      ...props
    }: NextLinkProps,
    ref: any,
  ) => {
    return (
      <Link href={href} passHref shallow={shallow} scroll={scroll}>
        <a
          {...props}
          ref={ref}
          href="dummy"
          style={{
            textDecoration: 'none',
            color: 'var(--color-text)',
            ...style,
          }}
          className={className}
          target={target}
          rel={target ? 'noreferrer' : undefined}
        >
          {children}
        </a>
      </Link>
    );
  },
);

@benediktdertinger
Copy link

Hi guys, any news on how to handle this properly?

@ACPK
Copy link

ACPK commented Sep 17, 2021

An alternative (but hacky) solution is to enable the passHref property and set the inner href to a dummy value. Next.js will override the dummy value with the correct value from the outer component, so everything will work correctly and it'll pass the jsx-a11y/anchor-is-valid rule.

  <Link href="/my-amazing-page" passHref>
    <a href="dummy">Go to my amazing page</a>
  </Link>

@timneutkens - It would be nice if this temporary solution was included in Next.js's Link documentation.

@styfle styfle modified the milestones: 11.x.x, 12.0.4 Nov 5, 2021
@timneutkens timneutkens added the Navigation Related to Next.js linking (e.g., <Link>) and navigation. label Nov 17, 2021
@timneutkens timneutkens removed this from the 12.0.5 milestone Nov 17, 2021
@vercel vercel locked and limited conversation to collaborators Dec 7, 2021
@balazsorban44 balazsorban44 converted this issue into discussion #32233 Dec 7, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
Navigation Related to Next.js linking (e.g., <Link>) and navigation.
Projects
None yet
Development

No branches or pull requests