fix: remove aria-current from navLink when inactive

[AlmeroSteyn]

Partially closes #5507.

Refer to the following two aria-current pattern docs from Léonie Watson:
https://tink.uk/using-the-aria-current-attribute/
http://design-patterns.tink.uk/aria-current/

Both indicates the aria-current ARIA state only being rendered on the current element, instead of setting it to false.

I have changed the NavLink component to not render this attribute unless it should be set to true.

This change fixes one of the issues from #5507 , namely that only the active link is then read out by NVDA to be active, instead of all the NavLinks.

[AlmeroSteyn]

@timdorr Nice change to pass all the allowed values through! Actually because it is a camelCase prop I did not even think it was part of the component interface, sorry!

Can I change it to accept the aria-current as per React standard? All aria-* props should be kebab-cased.

The following change would allow the prop to be passed as aria-current and should then get standard UI and tooling support.

What do you think?

/**
 * A <Link> wrapper that knows if it's "active" or not.
 */
const NavLink = ({
  to,
  exact,
  strict,
  location,
  activeClassName,
  className,
  activeStyle,
  style,
  isActive: getIsActive,
  'aria-current': ariaCurrent,
  ...rest
}) => (
  <Route
    path={typeof to === 'object' ? to.pathname : to}
    exact={exact}
    strict={strict}
    location={location}
    children={({ location, match }) => {
      const isActive = !!(getIsActive ? getIsActive(match, location) : match)

      return (
        <Link
          to={to}
          className={isActive ? [ className, activeClassName ].filter(i => i).join(' ') : className}
          style={isActive ? { ...style, ...activeStyle } : style}
          aria-current={isActive && ariaCurrent || null}
          {...rest}
        />
      )
    }}
  />
)

NavLink.propTypes = {
  to: Link.propTypes.to,
  exact: PropTypes.bool,
  strict: PropTypes.bool,
  location: PropTypes.object,
  activeClassName: PropTypes.string,
  className: PropTypes.string,
  activeStyle: PropTypes.object,
  style: PropTypes.object,
  isActive: PropTypes.func,
  'aria-current': PropTypes.oneOf(['page', 'step', 'location', 'true'])
}

NavLink.defaultProps = {
  activeClassName: 'active',
  'aria-current': 'true'
}

export default NavLink

[pshrmn]

@AlmeroSteyn It's your PR, you can make any changes that you'd like 😉.

I would recommend adding a couple of tests just to verify the behavior. One where the <NavLink> is active to verify that the rendered anchor's aria-current attribute is correct and another to verify that that attribute does not exist when the <NavLink> is not active. The <NavLink> tests are in this file. You should be able to just copy one of the other test cases and change the props/attribute checked in an expect call.

[AlmeroSteyn]

@pshrmn Thank so much for the info! I saw @timdorr had pushed changes so was not sure if I should overwrite it :-)

Will get to this as soon as I have a spare moment.

[AlmeroSteyn]

@pshrmn I changed the prop to kebab-case and added values date and time as well as they are also catered for according to the spec.

And added the tests.

[pshrmn]

This seems good to me.

For anyone interested, using aria-* in JSX is documented in the accessibility docs. While a <NavLink> isn't a native component, I think that it makes sense to make its ARIA prop consistent with <input>, et al.

[timdorr]

I wonder if we should be worrying about all the possible values and just doing a string type here.

[AlmeroSteyn]

The current standard confix for create-react-app includes the eslint-plugin-jsx-a11y rule jsx-a11y/aria-proptypes activated by default. It already validates all aria-* values which, in this case, shows the following warning message for this rule at compile time:

The value for aria-current must be a single token from the following: page,step,location,date,time,true,false

So I agree that this is redundant. Better to have such a dedicated library worry about these details.

Shall I remove it?

[timdorr]

LGTM

[timdorr]

Weird, had to do it twice... Something's up with Github.

Anyways, this should be good to go. Will merge when it goes green.

[AlmeroSteyn]

Thank you for merging this!