Accessibility engineering review for Link component. (#3317)

* Remove as prop from Link.

* Underline Link when inside of a paragraph tag, hover behavior should match focus behavior.

* Create curly-otters-repeat.md

* Soft deprecate as prop for Link so we don't break compatibility.

* Fix issues with Link-dependent components when as prop is not accepted.

* Remove ReactRouterLikeLink function from NavList tests.

* Remove references to as prop from Link docs.

* Update generated/components.json

* Remove reference to as prop in ActionList.LinkItem docs.

* Update generated/components.json

* Add link variant to button to match PVC implementation.

* Update generated/components.json

* Allow up to 4 levels of nesting for the NavList (#3343)

* allows up to 4 levels of nesting for the NavList

* adds changeset

* Update src/NavList/NavList.tsx

Co-authored-by: Cole Bemis <colebemis@github.com>

* updates snapshots

---------

Co-authored-by: Cole Bemis <colebemis@github.com>

* Refactor(ActionList): ActionList.Item should render content as a button if parent is not interactive. (#3284)

* Using button as the underlying tag for ActionList.Item content.

* Fix subnav layout.

* Changing ActionList content to button if the top-level is not a button. ActionMenu is busted.

* Fix issue where ActionList.Item content would render as a button inside ActionMenu, breaking keyboard navigation.

* Create purple-crabs-matter.md

* test(vrt): update snapshots

* Fix bug with MenuContext not being exported properly.

* Fix color violation in ActionList.Item.

* Formatting, update snapshots.

* test(vrt): update snapshots

* Fix a11y violation in ActionList story.

* Formatting.

* Fix ts-ignore comment.

* Adjust line-height when button is rendered.

* Revert "test(vrt): update snapshots"

This reverts commit ba6d863.

* Revert "test(vrt): update snapshots"

This reverts commit 31d5358.

* Update snapshots.

* Remove flexGrow from ActionList.Item to remove extra 1px vertical height.

* Set padding to 0 and put flexGrow back to fix padding issue for ActionList buttons.

* Fix ActionList text wrap story.

* Fix underlinenav story by checking ActionList.Item if as prop is a.

* Update snapshots and formatting.

* Update snapshots.

* If ActionList.Item content is rendered as a button, remove tabIndex from li and fix padding.

* Fix various layout edge cases.

* ts-ignore event handlers on ItemWrapper.

* Update snapshots.

* Revert fontWeight config that differed from production docs.

* Pass selectionVariant prop illegally in UnderlineNav story to fix issue with Selection spans showing up.

* fix: prevent closing menu when `event.preventDefault()` is called on `ActionList.Item`'s `onSelect` handler by @gr2m (#3346)

* fix: prevent closing menu when `event.preventDefault()` is called on `ActionList.Item`'s `onSelect` handler (#3163)

* test: prevent closing menu when `event.preventDefault()` is called on `ActionList.Item`'s `onSelect` handler

Failing test for #3162

* fix: prevent closing menu when `event.preventDefault()` is called on `ActionList.Item`ߴs `onSelect` handler

* add storybook example: Delayed Menu Close

* update docs

* docs: changeset

* Update changelog

---------

Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com>

* Update generated/components.json

---------

Co-authored-by: Gregor Martynus <39992+gr2m@users.noreply.github.com>
Co-authored-by: siddharthkp <siddharthkp@users.noreply.github.com>

* Check for tabIndex value in isTopLevelInteractive.

* Reference styles in updated menuProps.

* Updated snapshots.

* Fix padding setting instead of using values from styles, which are inverted.

* Update snapshots.

---------

Co-authored-by: radglob <radglob@users.noreply.github.com>
Co-authored-by: Gregor Martynus <39992+gr2m@users.noreply.github.com>
Co-authored-by: siddharthkp <siddharthkp@users.noreply.github.com>

* Rollback attempt at underlying links inside other bits of text.

* Update src/Button/styles.ts

Co-authored-by: Katie Langerman <18661030+langermank@users.noreply.github.com>

---------

Co-authored-by: radglob <radglob@users.noreply.github.com>
Co-authored-by: Mike Perrotti <mperrotti@github.com>
Co-authored-by: Cole Bemis <colebemis@github.com>
Co-authored-by: Gregor Martynus <39992+gr2m@users.noreply.github.com>
Co-authored-by: siddharthkp <siddharthkp@users.noreply.github.com>
Co-authored-by: Katie Langerman <18661030+langermank@users.noreply.github.com>

.changeset/curly-otters-repeat.md

@@ -0,0 +1,6 @@
+---
+"@primer/react": minor
+---
+
+Link components no longer accept an `as` prop. If you need link-like styling, you should use a different component and style accordingly.
+Button has a `link` variant which can be used for this purpose.

docs/content/Link.mdx

@@ -10,10 +10,6 @@ import data from '../../src/Link/Link.docs.json'
 
 The Link component styles anchor tags with default hyperlink color cues and hover text decoration. `Link` is used for destinations, or moving from one page to another.
 
-In special cases where you'd like a `<button>` styled like a `Link`, use `<Link as='button'>`. Make sure to provide a click handler with `onClick`.
-
-**Important:** When using the `as` prop, be sure to always render an accessible element type, like `a`, `button`, `input`, or `summary`.
-
 ## Examples
 
 ```jsx live

generated/components.json

@@ -215,11 +215,6 @@
               "name": "ref",
               "type": "React.RefObject<HTMLAnchorElement>"
             },
-            {
-              "name": "as",
-              "type": "React.ElementType",
-              "defaultValue": "\"a\""
-            },
             {
               "name": "sx",
               "type": "SystemStyleObject"
@@ -900,7 +895,7 @@
         },
         {
           "name": "variant",
-          "type": "| 'default'\n| 'primary'\n| 'danger'\n| 'outline'\n| 'invisible'",
+          "type": "| 'default'\n| 'primary'\n| 'danger'\n| 'outline'\n| 'invisible'\n| 'link'",
           "defaultValue": "'default'",
           "description": "Change the visual style of the button."
         },
@@ -2062,7 +2057,7 @@
           "name": "href",
           "type": "string",
           "defaultValue": "",
-          "description": "URL to be used for the Link. (The `href` is passed to the underlying `<a>` element. If `as` is specified, the link behavior may need different props)."
+          "description": "URL to be used for the Link. (The `href` is passed to the underlying `<a>` element)."
         },
         {
           "name": "muted",
@@ -2086,11 +2081,6 @@
           "name": "ref",
           "type": "React.RefObject<HTMLAnchorElement>"
         },
-        {
-          "name": "as",
-          "type": "React.ElementType",
-          "defaultValue": "\"a\""
-        },
         {
           "name": "sx",
           "type": "SystemStyleObject"

src/ActionList/ActionList.docs.json

@@ -114,11 +114,6 @@
           "name": "ref",
           "type": "React.RefObject<HTMLAnchorElement>"
         },
-        {
-          "name": "as",
-          "type": "React.ElementType",
-          "defaultValue": "\"a\""
-        },
         {
           "name": "sx",
           "type": "SystemStyleObject"

src/ActionList/LinkItem.tsx

@@ -9,6 +9,7 @@ import {ActionListItemProps} from './shared'
 type LinkProps = {
   download?: string
   href?: string
+  to?: string
   hrefLang?: string
   media?: string
   ping?: string
@@ -21,7 +22,7 @@ type LinkProps = {
 // LinkItem does not support selected, variants, etc.
 export type ActionListLinkItemProps = Pick<ActionListItemProps, 'active' | 'children' | 'sx'> & LinkProps
 
-export const LinkItem = React.forwardRef(({sx = {}, active, as: Component, ...props}, forwardedRef) => {
+export const LinkItem = React.forwardRef(({sx = {}, active, ...props}, forwardedRef) => {
   const styles = {
     // occupy full size of Item
     paddingX: 2,
@@ -35,6 +36,8 @@ export const LinkItem = React.forwardRef(({sx = {}, active, as: Component, ...pr
     '&:hover': {color: 'inherit', textDecoration: 'none'},
   }
 
+  if (props.href === undefined && props.to !== undefined) props.href = props.to
+
   return (
     <Item
       active={active}
@@ -45,14 +48,7 @@ export const LinkItem = React.forwardRef(({sx = {}, active, as: Component, ...pr
           props.onClick && props.onClick(event as React.MouseEvent<HTMLAnchorElement>)
         }
         return (
-          <Link
-            as={Component}
-            sx={merge(styles, sx as SxProp)}
-            {...rest}
-            {...props}
-            onClick={clickHandler}
-            ref={forwardedRef}
-          >
+          <Link sx={merge(styles, sx as SxProp)} {...rest} {...props} onClick={clickHandler} ref={forwardedRef}>
             {children}
           </Link>
         )

src/Button/Button.docs.json

@@ -13,7 +13,7 @@
     },
     {
       "name": "variant",
-      "type": "| 'default'\n| 'primary'\n| 'danger'\n| 'outline'\n| 'invisible'",
+      "type": "| 'default'\n| 'primary'\n| 'danger'\n| 'outline'\n| 'invisible'\n| 'link'",
       "defaultValue": "'default'",
       "description": "Change the visual style of the button."
     },
@@ -67,4 +67,4 @@
       ]
     }
   ]
-}
+}

src/Button/Button.stories.tsx

@@ -22,7 +22,7 @@ export default {
       control: {
         type: 'radio',
       },
-      options: ['default', 'primary', 'danger', 'invisible', 'outline'],
+      options: ['default', 'primary', 'danger', 'invisible', 'outline', 'link'],
     },
     alignContent: {
       control: {

src/Button/styles.ts

@@ -186,6 +186,25 @@ export const getVariantStyles = (variant: VariantType = 'default', theme?: Theme
         borderColor: 'btn.outline.selectedBorder',
       },
     },
+    link: {
+      color: 'accent.fg',
+      backgroundColor: 'transparent',
+      display: 'inline-flex',
+      border: 'none',
+      height: 'unset',
+      padding: 0,
+      '&:hover:not(:disabled)': {
+        textDecoration: 'underline',
+      },
+      '&:focus-visible, &:focus': {
+        outlineOffset: '2px',
+      },
+      '&:disabled, &[aria-disabled=true]': {
+        color: 'primer.fg.disabled',
+        backgroundColor: 'transparent',
+        borderColor: 'transparent',
+      },
+    },
   }
   return style[variant]
 }

src/Button/types.ts

@@ -8,7 +8,7 @@ export const StyledButton = styled.button<SxProp>`
   ${sx};
 `
 
-export type VariantType = 'default' | 'primary' | 'invisible' | 'danger' | 'outline'
+export type VariantType = 'default' | 'primary' | 'invisible' | 'danger' | 'outline' | 'link'
 
 export type Size = 'small' | 'medium' | 'large'
 

src/Link/Link.docs.json

@@ -9,7 +9,7 @@
       "name": "href",
       "type": "string",
       "defaultValue": "",
-      "description": "URL to be used for the Link. (The `href` is passed to the underlying `<a>` element. If `as` is specified, the link behavior may need different props)."
+      "description": "URL to be used for the Link. (The `href` is passed to the underlying `<a>` element)."
     },
     {
       "name": "muted",
@@ -33,15 +33,10 @@
       "name": "ref",
       "type": "React.RefObject<HTMLAnchorElement>"
     },
-    {
-      "name": "as",
-      "type": "React.ElementType",
-      "defaultValue": "\"a\""
-    },
     {
       "name": "sx",
       "type": "SystemStyleObject"
     }
   ],
   "subcomponents": []
-}
+}

src/Link/Link.features.stories.tsx

@@ -1,5 +1,6 @@
 import Link from '../Link'
-import {Meta} from '@storybook/react'
+import Box from '../Box'
+import {Meta, StoryFn} from '@storybook/react'
 import React from 'react'
 import {ComponentProps} from '../utils/types'
 
@@ -19,3 +20,20 @@ export const Underline = () => (
     Link
   </Link>
 )
+
+export const WithinText: StoryFn<typeof Link> = args => (
+  <Box as="p">
+    This{' '}
+    <Link href="#" {...args}>
+      link
+    </Link>{' '}
+    is inside of other text.
+  </Box>
+)
+
+WithinText.args = {
+  muted: false,
+  underline: false,
+}
+
+WithinText.argTypes = {}

src/Link/Link.tsx

@@ -1,4 +1,4 @@
-import React, {forwardRef, useEffect} from 'react'
+import React, {forwardRef} from 'react'
 import styled from 'styled-components'
 import {system} from 'styled-system'
 import {get} from '../constants'
@@ -23,55 +23,27 @@ const hoverColor = system({
 const StyledLink = styled.a<StyledLinkProps>`
   color: ${props => (props.muted ? get('colors.fg.muted')(props) : get('colors.accent.fg')(props))};
   text-decoration: ${props => (props.underline ? 'underline' : 'none')};
-  &:hover {
+  &:hover,
+  &:focus {
     text-decoration: ${props => (props.muted ? 'none' : 'underline')};
     ${props => (props.hoverColor ? hoverColor : props.muted ? `color: ${get('colors.accent.fg')(props)}` : '')};
   }
-  &:is(button) {
-    display: inline-block;
-    padding: 0;
-    font-size: inherit;
-    white-space: nowrap;
-    cursor: pointer;
-    user-select: none;
-    background-color: transparent;
-    border: 0;
-    appearance: none;
-  }
   ${sx};
 `
 
-const Link = forwardRef(({as: Component = 'a', ...props}, forwardedRef) => {
+const Link = forwardRef(({as, ...props}, forwardedRef) => {
   const innerRef = React.useRef<HTMLAnchorElement>(null)
   useRefObjectAsForwardedRef(forwardedRef, innerRef)
 
-  if (__DEV__) {
-    /**
-     * The Linter yells because it thinks this conditionally calls an effect,
-     * but since this is a compile-time flag and not a runtime conditional
-     * this is safe, and ensures the entire effect is kept out of prod builds
-     * shaving precious bytes from the output, and avoiding mounting a noop effect
-     */
-    // eslint-disable-next-line react-hooks/rules-of-hooks
-    useEffect(() => {
-      if (
-        innerRef.current &&
-        !(innerRef.current instanceof HTMLButtonElement) &&
-        !(innerRef.current instanceof HTMLAnchorElement)
-      ) {
-        // eslint-disable-next-line no-console
-        console.error(
-          'Error: Found `Link` component that renders an inaccessible element',
-          innerRef.current,
-          'Please ensure `Link` always renders as <a> or <button>',
-        )
-      }
-    }, [innerRef])
+  if (as !== undefined) {
+    // eslint-disable-next-line no-console
+    console.warn(
+      'Links no longer accept an as prop. If you need to style another tag as a link, you should use a different component and apply appropriate styling.',
+    )
   }
 
   return (
     <StyledLink
-      as={Component}
       {...props}
       // @ts-ignore shh
       ref={innerRef}

src/Link/__tests__/Link.test.tsx

@@ -5,7 +5,7 @@ import {render as HTMLRender} from '@testing-library/react'
 import {axe} from 'jest-axe'
 
 describe('Link', () => {
-  behavesAsComponent({Component: Link})
+  behavesAsComponent({Component: Link, options: {skipAs: true}})
 
   checkExports('Link', {
     default: Link,
@@ -29,24 +29,11 @@ describe('Link', () => {
     expect(render(<Link sx={{fontStyle: 'italic'}} />)).toHaveStyleRule('font-style', 'italic')
   })
 
-  it('applies button styles when rendering a button element', () => {
-    expect(render(<Link as="button" />)).toMatchSnapshot()
-  })
-
   it('respects the "muted" prop', () => {
     expect(render(<Link muted />)).toMatchSnapshot()
   })
 
   it('respects the  "sx" prop when "muted" prop is also passed', () => {
     expect(render(<Link muted sx={{color: 'fg.onEmphasis'}} />)).toMatchSnapshot()
   })
-
-  it('logs a warning when trying to render invalid "as" prop', () => {
-    const consoleSpy = jest.spyOn(global.console, 'error').mockImplementation()
-
-    HTMLRender(<Link as="i" />)
-    expect(consoleSpy).toHaveBeenCalled()
-
-    consoleSpy.mockRestore()
-  })
 })