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

aria-current has incorrect value "true" #6109

Closed
rmccue opened this issue Apr 26, 2018 · 7 comments
Closed

aria-current has incorrect value "true" #6109

rmccue opened this issue Apr 26, 2018 · 7 comments

Comments

@rmccue
Copy link

rmccue commented Apr 26, 2018

Version

4.2.2

Test Case

https://codesandbox.io/s/wol5pjk8m5

(I couldn't actually get the original CodeSandbox link to work, but the code should be right.)

Steps to reproduce

  1. Add NavLink to page
  2. Navigate to the page linked by that link
  3. Note value of aria-current is "true"

Expected Behavior

aria-current should have a valid value; namely "page"

Actual Behavior

aria-current has an invalid value; namely "true"


Hi! aria-current is an attribute which takes one of a few valid string values. Currently in react-router-dom, this is set to the value "true" instead. See comment; the error comes from the default being set to "true".

This was caught during an accessibility audit of our site. :)

@rmccue
Copy link
Author

rmccue commented Apr 26, 2018

Looking at this again, the propTypes are already set to the valid set of aria-current values; the issue is that the default is incorrectly set to "true" instead of "page".

@timdorr
Copy link
Member

timdorr commented Apr 26, 2018

Yep, I brought this up the first time: https://github.com/ReactTraining/react-router/pull/4708/files#r120510832

A PR to change it would be appreciated!

@brandontruggles
Copy link
Contributor

Hello, I'd like to take this! I'll send over a PR shortly.

@brandontruggles
Copy link
Contributor

@timdorr @rmccue Correct me if I'm wrong, but based on the article linked above (https://tink.uk/using-the-aria-current-attribute/), it looks to me that false is also a valid option for aria-current. However, this is not the case within NavLink. In addition to setting "page" as the default, should I also add "false" as an option? Thanks!

@pshrmn
Copy link
Contributor

pshrmn commented Apr 28, 2018

@brandonrninefive I believe so. The spec includes false.

@rianrietveld
Copy link

Taking the liberty to cc @LJWatson in this conversation.

@LJWatson
Copy link

LJWatson commented May 1, 2018

Per the aria-current definition, true is a permitted value. It's the generic fallback value, for when none of the other token values is appropriate.

Technically speaking, it's ok to usetrue in this context. It's better to use page though, because it makes more information available to screen reader users:

  • aria-current="true" is announced as "Current"
  • aria-current="page" is announced as "Current page"

timdorr pushed a commit that referenced this issue May 8, 2018
* Added 'false' as a valid value for aria-current in NavLink. Also made the default value 'page' instead of 'true'.

* Remove "false" string as a permitted aria-current value.
@timdorr timdorr closed this as completed May 9, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants