Closed ibenjelloun closed 2 years ago
Don't know if using the @HostBinding
decorator would be a good way of doing this :
@HostBinding("attr.aria-current") get ariaCurrent() {
return this.isActive ? "Page" : undefined;
}
Running version of the RouterLinkActive : https://stackblitz.com/edit/angular-n7sh9u?file=src%2Fapp%2Frouter-link-active2.directive.ts
Hello @atscott,
I'd gladly submit a PR to solve this issue !
As said by @ibenjelloun, is it better to use:
host
property (HostBinding
is rarely used within angular source code)Renderer2
, which is already used within routerLinkActive to add the active classes, as seen hereOr are we considering a completely different solution, such as another directive?
FYI, such behavior is already implemented in angular/components (PR)
@QuentinFchx - Awesome, thanks for volunteering. Let's follow the pattern used in angular/components, using host
to assign the attribute. And let's do it on routerLinkActive
rather than another directive. Just a note here: please add the aria-current
in addition to rather than "instead of" like the title on this issue mentions.
Hmm, after reading a bit more on aria-current
, I'm a bit concerned that this won't always be the correct thing to do. I think generally speaking, routerLinkActive
is used when links are in a logical group. However, there's no requirement that the directive be used in that way and there's no requirement that no two links in a group be active at the same time, especially given that you can provide different classes for them to make two active links still distinguishable.
Baking this in to the existing directive seems like it could be problematic, but I'm open to discussion here.
Edit: I'm also worried that this type of change will cause similar issues as with the tabindex (where it's not always the right thing to do) as is discussed here: https://github.com/angular/angular/issues/28345
I'm thinking that this might be better suited for a directive in user code. It would be relatively simple to add this in that way:
import { Directive, HostBinding, Host } from "@angular/core";
import { RouterLinkActive } from "@angular/router";
@Directive({
selector: "[routerLinkActive]"
})
export class RlaPage {
constructor(@Host() private rla: RouterLinkActive) {}
get isActive() {
return this.rla.isActive;
}
@HostBinding("attr.aria-current") get ariaCurrent() {
return this.isActive ? "Page" : undefined;
}
}
This might be the better option rather than providing functionality we "think" is right but ends up being wrong in cases where people know exactly what they're doing and need configurability. If we added this, we could easily get ourselves into the situation that we now have with the tabindex
where usually it's correct but for the cases where it isn't, developers have no option for disabling the built-in behavior.
I'm too thinking, it's better to let a developer decide when to set aria-current. E.g. for breadcrumbs usage, it's more recommended to use aria-current="location" than aria-current="page" (https://www.aditus.io/patterns/breadcrumbs/). As another idea, we could add possibilities to configure the attribute in routerLinkActiveOptions. Also I think the label should only be set if the exact match option is set to true. Otherwise, again for example in breadcrumbs usage, you would have more than one link set with aria-current.
Just a heads up that we kicked off a community voting process for your feature request. There are 20 days until the voting process ends.
Find more details about Angular's feature request process in our documentation.
@jelbourn, @twerske, and I discussed this change and agreed that based on discussions above about how setting aria-current="page"
is not the correct thing for all cases, we don't want to bake this behavior in to the RouterLinkActive
. I've updated the issue to indicate that this should be addressed through a11y best practices documentation.
I really like and agree with @jeripeierSBB's idea of adding an option in the routerLinkActiveOptions
with such implementation the user could just provide the aria-current value to use and the binding of that would just be taken care of by the routerLinkActive
directive.
Is there any downside in such implementation?
@dario-piotrowicz This would be reasonable as an opt-in feature on an input. I don't think routerLinkActiveOptions
is the best place for it. Instead, this could be a new input (name TBD). Would you like to take this on?
ah I see, sure, I would love to give it a shot :smiley::+1:
This issue has been automatically locked due to inactivity. Please file a new issue if you are encountering a similar or related problem.
Read more about our automatic conversation locking policy.
This action has been performed automatically by a bot.
🚀 feature request
Relevant Package
This feature request is for @angular/router
Description
RouterLinkActive
adds a class to the element, it should add thearia-current
attribute which must be set to'Page'
whenisActive
is true in theRouterLinkActive
.Describe alternatives you've considered