Fix repeater missing aria attributes #1758
Conversation
…ing-aria-attributes
Codecov Report
@@ Coverage Diff @@
## master #1758 +/- ##
==========================================
+ Coverage 99.94% 99.94% +<.01%
==========================================
Files 418 418
Lines 8917 8919 +2
Branches 1326 1326
==========================================
+ Hits 8912 8914 +2
Misses 5 5
Continue to review full report at Codecov.
|
…ing-aria-attributes
| <div | ||
| class="sky-repeater-item-content" | ||
| [id]="contentId" | ||
| role="region" |
There was a problem hiding this comment.
I saw this on the ARIA docs, "Avoid using the region role in circumstances that create landmark region proliferation, e.g., in an accordion that contains more than approximately 6 panels that can be expanded at the same time." https://www.w3.org/TR/wai-aria-practices-1.1/#accordion
@Blackbaud-MattGregg I'm curious if we should remove the role="region" since the repeater can have all items opened at once?
There was a problem hiding this comment.
@Blackbaud-SteveBrush I believe we should remove though I think the issue is more with these being <section> elements which are the same semantically as role="region" in ARIA. It also surfaces another thorny issue (heading hierarchy) to tackle. I'm not sure we want to open that can of worms at this point. I'd suggest we open a new issue and prioritize it separately.
I'm not sure why items in a list turn into sections when viewed as a repeater so maybe there is a back story there to understand. They don't seem like they should be sections from a semantic structure POV and this isn't a component to use as an accordion (i.e., separate sections of content with a heading). It's a list of something.
If we did remove the regions/sections, I think it would require us to decide what to do with the heading hierarchy. Currently the regions/sections use H1s for their headings which I assume was meant to work with the HTML5 outline algorithm where one lays out multiple sections each with their own hierarchy starting from H1. My understanding is this was reversed/deprecated in HTML 5.1 and browsers never implemented it well or consistently. Hence, we'd need to figure out what to do with heading strategy.
There was a problem hiding this comment.
@Blackbaud-MattGregg I believe the decision for <section> was made based on what you described, but has since been deprecated. I don't think it'll be a problem to replace the <section> tags with simple <div> tags.
For the headings, we could replace the <h1> and use <div role="heading">.
Based on reading this: https://www.w3.org/TR/WCAG20-TECHS/ARIA12.html (see "Example 1: Simple headings").
"...when retrofitting a legacy site where scripts depend on the existing element hierarchy or the level is unknown. For example, web content which is syndicated from various sources may be constructed without knowledge of what the final presentation will be."
Thoughts on that?
There was a problem hiding this comment.
@Blackbaud-SteveBrush @blackbaud-conorwright I think replacing the <section> with <div> would be the way to go. Given the sections don't then need headings, I don't think the system should provide headings at all.
If a using application actually has a page where the repeater has a list of heading for some reason they can put their headings in there in the content and use the appropriate heading level (h2, h3, etc.) given the structure of the page. I don't think we want to guess the heading level and I can't think of a good reason for enforcing headings here especially b/c I think in the majority of cases I've seen these wouldn't be headings.
…ing-aria-attributes
Blackbaud-SteveBrush
changed the title
Blackbaud-AlexKingman
changed the title
|
@Blackbaud-MattGregg and @Blackbaud-SteveBrush Alright, I've updated to remove the role, switch section to be a div, and I also changed the h1 tag to be a div so that users may supply their own inside it if they wish. |
| @@ -39,6 +39,8 @@ | |||
| .sky-repeater-item-title { | |||
| margin: 0; | |||
| flex-grow: 1; | |||
| line-height: 1.1; | |||
| color: #282b31; | |||
There was a problem hiding this comment.
Should be $sky-text-color-default, I believe.
There was a problem hiding this comment.
fixed.
blackbaud-conorwright
deleted the
fix-repeater-missing-aria-attributes
branch
Added missing aria labels for the repeater component
Resolves: #1377