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

feat(core): add aria-current to active links (close #2116) #3073

Merged
merged 5 commits into from
Mar 20, 2020

Conversation

smhigley
Copy link
Contributor

@smhigley smhigley commented Dec 21, 2019

Hi! First contributor + pretty new to Vue 👋 😄

This PR adds aria-current="page" to active links, and resolves #2116. I made the following two choices:

  1. Use aria-current="page" over other token values
    The original issue mentions allowing custom token values, or future <router-link> extension possibilities. That still seems useful, but aria-current="page" seems like the right choice at least 90% of the time. The other tokens are either much less common (e.g. step) or unlikely to be used with a router at all (e.g. date)

  2. Use the activeClass logic over only applying it to the exact active route
    This is more hand-wavy, but seems like the better choice -- URL params shouldn't necessarily affect aria-current, and it's not the worst thing to have parent routes also receive aria-current if exact is not applied.

I'm hoping this is a small enough change that it can go in before the harder work of figuring out how to extend <router-link> is done, and it seems like a sensible default even with customization. I've run into some issues using vue-router b/c of this, so I'd personally love to see aria-current added :). I'm happy to answer any questions on the accessibility side, and let me know if I missed anything PR-wise. Thanks!

@posva
Copy link
Member

posva commented Mar 13, 2020

hey @smhigley thanks for the well detailed PR!

I think this makes sense and is safe to add as a default for vue router but I was wondering about point 2: applying it to active instead of exact-active. I think the latter makes more sense as a default because it is more likely to end up in one single element marked as aria-current="page" which is what is recommended on the spec (https://www.w3.org/TR/wai-aria-1.1/#aria-current). Do you know if this may not be true anymore or if there is something else to consider that isn't present on the spec?

It's not directly related but I also wanted to note that active and exact behavior is very likely to change on v4 (vuejs/rfcs#136) and I was going to use exact active to set it. If you have any input for that, please go ahead 🙂 !

@smhigley
Copy link
Contributor Author

smhigley commented Mar 13, 2020

@posva I updated the PR to add aria-current to only exact active links :).

I originally did non-exact active because the tutorials and examples I happened to see seemed to be adding active styles based on the active class, and I was trying to choose whichever would add most parity between visual active styles and aria-current. I don't have a very good feel of which is more helpful in practice, though, since I'm fairly new to Vue. I'm happy to defer to the one you think fits better :)

test/e2e/specs/active-links.js Show resolved Hide resolved
src/components/link.js Outdated Show resolved Hide resolved
src/components/link.js Outdated Show resolved Hide resolved
src/components/link.js Outdated Show resolved Hide resolved
test/e2e/specs/active-links.js Outdated Show resolved Hide resolved
Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@posva posva merged commit 6ec0ee5 into vuejs:dev Mar 20, 2020
@smhigley
Copy link
Contributor Author

Thank you! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend <router-link> to set an aria-current attribute for active links, instead of an active class
3 participants