fix(navigation-drawer): fix ARIA role issue warning. #14976
Conversation
…e the role="navigation" since we use <nav> which is semantic tag and there is no need to be wrapped in an element with a role.
desig9stein
added
❌ status: awaiting-test
simeonoff
changed the title
| @@ -14,7 +14,7 @@ | |||
| [class.igx-nav-drawer--disable-animation]="disableAnimation" | |||
| (click)="close()" #overlay> | |||
| </div> | |||
| <aside role="navigation" | |||
| <div | |||
There was a problem hiding this comment.
Shouldn't the correct semantic element be <nav>?
There was a problem hiding this comment.
In the app we already have the <nav> element inside, that's why I changed it to div, I removed the role since we are using the semantic <nav> element, and no role is needed for it.
There was a problem hiding this comment.
I think we should leave that for the user to decide, we don't know what will be inside, if we change it to nav this means that the drawer can only be used for navigation purposes, if that's what we want. then OK, I will change it.
There was a problem hiding this comment.
@simeonoff, @damyanpetev, what is your opinion?
There was a problem hiding this comment.
@desig9stein I'm saying this because so far, we'd decided by putting role="navigation".
There was a problem hiding this comment.
@kdinev you are correct, otherwise I will introduce a breaking change.
There was a problem hiding this comment.
Yup, from what I can see from the few sites I checked out (https://www.youtube.com, https://developer.mozilla.org/en-US/docs/Web, https://m3.material.io and https://w3c.github.io/aria/) just navigation seems to be quite enough.
Not sure where the aside came from, I probably took it from the simpler MDN description that mentions sidebar, but looking at https://w3c.github.io/aria/#complementary it seems it's intended for other purposes.
Anyway, will this involve an update on the docs too? Because those instruct users to add their own <nav> manually as well which shouldn't be needed even if it's not technically wrong. www.infragistics.com/products/ignite-ui-angular/angular/components/navdrawer
Yes @damyanpetev, thanks for pointing this out, I will update the doc samples as well |
kdinev
added
✅ status: verified
Change
<aside>to a simple<div>,and removerole="navigation"since we're using<nav>, which is a semantic tag that doesn’t need to be wrapped in an element with arole="navigation".Closes #14847
Additional information (check all that apply):
Checklist:
feature/README.MDupdates for the feature docsREADME.MDCHANGELOG.MDupdates for newly added functionalityng updatemigrations for the breaking changes (migrations guidelines)