jsx-eslint / eslint-plugin-jsx-a11y

Static AST checker for a11y rules on JSX elements.
MIT License
3.4k stars 637 forks source link

Add option for passed hrefs (as in Next.js) (anchor-is-valid) #402

Closed sk22 closed 2 years ago

sk22 commented 6 years ago

I'd suggest adding an option for a tags that don't have an href because its containing component passes the href to it.

For example, Next.js' Link component takes the href but requires an a tag as a child:

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

Creating such structure triggers the anchor-is-valid rule:

[eslint] The href attribute is required on an anchor. Provide a valid, navigable address as the href value. (jsx-a11y/anchor-is-valid)

At the moment, I don't see any practical usage of the anchor-is-valid rule that allows passing the href to the child.

A possible fix could be to check if the parent component (i.e. Link) has the passHref prop set (which is optional in Next.js when the child is an a tag without an own href). Other than that, the rule could just check for the <Link href><a></Link> structure.

sk22 commented 6 years ago

Here's a workaround (i.e., disabling the rule's noHref aspect):

"jsx-a11y/anchor-is-valid": [ "error", {
  "components": [ "Link" ],
  "specialLink": [ "hrefLeft", "hrefRight" ],
  "aspects": [ "invalidHref", "preferButton" ]
}]
ljharb commented 6 years ago

That sounds like a really strange API; if it’s a Link why would it need an a tag to be passed in? All that does is force a cloneElement, needlessly

sk22 commented 6 years ago

It's indeed a strange API. The reason they did it like this is probably so the Link element can be rendered as any, not only an anchor. But there's an as prop on Link, I'll just check out if this can be used instead. Edit: Nevermind as; seems like it does something completely else.

sk22 commented 6 years ago

Btw; yep, what you said is literally what the Next.js Link does... https://github.com/zeit/next.js/blob/canary/lib/link.js#L149 Also see this commit: https://github.com/zeit/next.js/commit/6431f5fce2593d8cadb81841eb0e717facbe4aa6

ljharb commented 6 years ago

I’m not sure why they wouldn’t accept a component (or string “a”) so they can render the element directly rather than cloning :-/

Either way I’m not sure that supporting this generically is something either possible or reasonable to do.

sk22 commented 6 years ago

I see... Thanks!

ljharb commented 6 years ago

I’ll leave this open if somebody has an idea of how to handle it, but hardcoding an arbitrary library’s strange convention is a nonstarter.

trevordmiller commented 6 years ago

I'm not sure how to handle this but I'm also running into the same issue:

image

danny-andrews-snap commented 5 years ago

Yeah, this is really a fault of the next.js Link API. I submitted an issue. https://github.com/zeit/next.js/issues/5533

T04435 commented 4 years ago

Here's a workaround (i.e., disabling the rule's noHref aspect):

"jsx-a11y/anchor-is-valid": [ "error", {
  "components": [ "Link" ],
  "specialLink": [ "hrefLeft", "hrefRight" ],
  "aspects": [ "invalidHref", "preferButton" ]
}]

This will disable all linting a tags without href, is there a way to:

<Link href='/'><a>Disable lint error</a></Link>
<a>Enable lint error</a>
takebo commented 4 years ago

@T04435 looking for the same solution. Disabling eslint for the nested anchor inside the Link, and not globally.

trigunam commented 4 years ago

In response to @sk22 adding the following to .eslintrc.json still causes errors.

"invalidHref", "preferButton"

Does this also mean, eslint is suggesting to use <button> instead of <a> tag right? For Next.js will replace a button work within <Link>?

I would rather turn off the rule to make it work with Next.js.

ljharb commented 4 years ago

If it has a URL to navigate to, it should be an a, which in next.js and react-router means a Link. A button should only be for non-navigation behavior.

FDiskas commented 4 years ago

If it has a URL to navigate to, it should be an a, which in next.js and react-router means a Link. A button should only be for non-navigation behavior.

But why not? Button can be used to redirect user to some page. For example the form usually has a "submit" button and "cancel" - and end user by pressing cancel button is waiting to be redirected out of the form and "submit" button also should submit and redirect somewhere. It's a natural behavior.

ljharb commented 3 years ago

@FDiskas because that’s not how semantic html works. Buttons are for submitting forms, and that’s not navigation. Cancel buttons shouldn’t exist; that should either be a link, or the normal browsers’ reload button.

burntcustard commented 3 years ago

The suggested linting workaround to disable the noHref check works, but there are additional checks done on <a>s that can give more errors. For example, adding an onClick={} to an <a> triggers multiple rules incorrectly, including "Static HTML elements with event handlers require a role" (no-static-element-interactions), because anchors "without" hrefs are static elements.

While this is obviously a weirdness with Next.js' Link component inserting the href into the <a> after jsx-a11y is run, a potential solution could be some sort of config to disable all rules for children of particular components that jsx-a11y doesn't understand?

MartinDevillers commented 3 years ago

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>
zackdotcomputer commented 3 years ago

I just put my hacky solution up as a gist - I've been using a wrapper component that internally creates the <Link> and <a> components together. This way you can add the LinkTo component to this rule's components list and it will validate properly.

wheelmaker24 commented 3 years ago

As usually you won't use the Link component on many pages (mostly only within a Navigation or Layout component), you could also easily disable the next line:

return (
    <Link href={yourUrl}>
        {/* eslint-disable-next-line jsx-a11y/anchor-is-valid */}
        <a {...yourProps}>{yourLinkName}</a>
     </Link>
);
zackdotcomputer commented 3 years ago

@wheelmaker24 Agreed, a one-by-one disable or the passHref solution above both work fine if your site only needs a few Links and/or they're gathered into a couple central components.

I don't think those are going to be efficient if you're building a site that uses links in more contexts, though, like an ecommerce or a webapp. If you have Links all over the place you're basically stuck either turning off the rule in your config or pulling in a wrapper component to work around the issue until Next or ESLint can fix this. :-\

ljharb commented 3 years ago

There's nothing we can really do except hardcoding the next.js pattern, which is all kinds of brittle. The proper solution is for next.js to follow the react best practice that long predates their existence: avoid patterns that necessitate cloning elements.

zackdotcomputer commented 3 years ago

No disagreement on what the ideal solution is @ljharb, but I disagree that hardcoding an exception for the Next.js pattern is the only non-brittle solution. If we could not only add certain components to the scope of this rule (as the "components" argument allows us to) but also to exclude <a> components (which we are specifically not allowed to do currently), then we could configure this rule to work with the Next.js pattern on our side as the end-user without needing to make any hardcoding to this project.

ljharb commented 3 years ago

@zackdotcomputer true. but then users would be able to exclude <a> components when not using this specific Next.js pattern, and I don't think that downside is worth it.

zackdotcomputer commented 3 years ago

If you're worried about missing un-nested <a> tags in a Next.js project, I know we could find a way to make the configuration sufficiently targeted (e.g. make it specifically "ignore <a> tags that are direct children of these tags...").

If you're just worried about developers being able to disable <a> validation in a more general way, I think it'd still be a net upside if a flag like that could rescue developers who would otherwise disable <a> validation by just disabling this rule entirely. Even as a developer who cares enough about a11y and linting to be here in this thread, if a rule is fighting my tech stack and I can't configure them to play nicely, I'm going to turn off the rule before I swap out the tech stack.

ljharb commented 3 years ago

Hmm. I suppose if:

then perhaps the risk is minimal, and the only downside would be that we'd have to maintain a rule option forever because a single framework made a poor design choice and hasn't fixed it after a number of years :-/

zackdotcomputer commented 3 years ago

¯\_(ツ)_/¯ yup, that's about the speed of things. Just eslint's bad luck as a build-tool that the framework that made that bad choice also happened to become super popular.

Since I'm having trouble reading the tone of voice on your post as to whether you will or won't consider adding this option, I'm going to open a PR to add a "Case: I use Next.js Link Components" to the description of this rule so that folks can more easily find this thread of workarounds. If this option winds up getting added, that case can be updated to point users to it.

T04435 commented 3 years ago

This is my work around

// MyLink.tsx
import React, { PropsWithChildren } from 'react';
import Link, { LinkProps } from 'next/link';

// This will allow you to pass any other props that the next Link supports
export type MyLinkProps = LinkProps;

const MyLink = (props: PropsWithChildren<MyLinkProps>) => {
  const { children, ...linkProps } = props;
  return <Link {...linkProps} passHref ><a href="passRef">{children}</a></Link>;
};

export default MyLink;

# usage

<MyLink href="/about" scroll >About</MyLink>
ljharb commented 3 years ago

@zackdotcomputer your tone read is accurate, as I'm not convinced either way myself, as every outcome seems differently terrible. I'll take a look at the PR.

chiptus commented 3 years ago

I know this problem from nextjs, but just started using @ui-router/react for an hybrid angularjs application (I know, ugh), and it has the same api.

zackdotcomputer commented 3 years ago

@chiptus I noticed that the new NextJS eslint env turns off this rule since it can't be configured to work with their API. I think the unfortunate end state here for at least the near future is that this rule will have to be disabled if one is using a framework that follows the "wrapper component takes the href" pattern.

That pattern doesn't seem to be going away and there doesn't appear to be any movement to alter this rule to support it.

Gamote commented 2 years ago

I'm also looking 👀 for a way to deal with this. My current work around looks like this:

import Link, { LinkProps } from "next/link";
import React, { FC } from "react";

type NextLinkProps = LinkProps & {
  href: string;
  className?: string;
};

/**
 * Standard way of using the Next's `Link` tag together with the `a` tag
 */
const NextLink: FC<NextLinkProps> = ({ href, className, children, ...rest }) => (
  <Link href={href} {...rest}>
    <a href={href} className={className}>
      {children}
    </a>
  </Link>
);

export default NextLink;

Usage:

<NextLink href={"/register"} className="font-medium">
  Register
</NextLink>
ljharb commented 2 years ago

This seems resolved, both on the next side in a future major and on this side via config. I’ll reopen if that’s not the case.

visnup commented 2 years ago

This seems resolved, both on the next side in a future major and on this side via config. I’ll reopen if that’s not the case.

Could you link to the respective changes in Next and config you're referring to if you have them handy?

ljharb commented 2 years ago

https://github.com/vercel/next.js/discussions/8207#discussioncomment-2803431

The needed config is mentioned multiple times upthread.