-
Notifications
You must be signed in to change notification settings - Fork 56
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
Fix a11y audit: Missing aria-hidden on some decorative SVGs #2691
base: main
Are you sure you want to change the base?
Conversation
…ves? - removing visually-hidden text redundant with svg's title
✅ Deploy Preview for boosted ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…ves? - Fix missing aria-hidden="true" in docs version and mode submenus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing some in:
- Orange Navbars with mode selector
- The first Tag example
- Download app page for the mode selector
- Form example page for the mode selector
- Icons > External SVG file
- Checks & radios > With icon
- Position > Examples
docs-subnav.html
example.html
shortcode replacementorange-global-headers.html
for mode selectororange-logo.svg
About > BrandOBS-logo.svg
About > Brand
I don't know what should be done on the `site/static/docs//assets//.svg
What do we decide for nested svgs ? There might be issues with download app page and placeholder shortcode.
Maybe some of the examples above need to be backported to Bootstrap first
Do we need any migration guide for this ? Maybe enumerating the different possibilities could so the trick.
<svg class="bi theme-icon-active" aria-hidden="true"><use href="#ui-auto-mode"></use></svg> | ||
<span class="{{ if $isExamples }}visually-hidden{{ else }}d-lg-none ms-2{{ end }}" id="bd-theme-text">Toggle mode</span> | ||
</button> | ||
<ul class="dropdown-menu dropdown-menu-end{{ if $isExamples }} shadow{{ else }} mb-2{{ end }}" aria-labelledby="bd-theme-text"> | ||
<li> | ||
<button type="button" class="dropdown-item d-flex align-items-center" data-bs-theme-value="light" aria-pressed="false"> | ||
<svg class="bi me-2"><use href="#ui-light-mode"></use></svg> | ||
<svg class="bi me-2" aria-hidden="true"><use href="#ui-light-mode"></use></svg> | ||
Light | ||
<svg class="bi ms-auto d-none"><use href="#check2"></use></svg> | ||
<svg class="bi ms-auto d-none" aria-hidden="true"><use href="#check2"></use></svg> | ||
</button> | ||
</li> | ||
<li> | ||
<button type="button" class="dropdown-item d-flex align-items-center" data-bs-theme-value="dark" aria-pressed="false"> | ||
<svg class="bi me-2"><use href="#ui-dark-mode"></use></svg> | ||
<svg class="bi me-2" aria-hidden="true"><use href="#ui-dark-mode"></use></svg> | ||
Dark | ||
<svg class="bi ms-auto d-none"><use href="#check2"></use></svg> | ||
<svg class="bi ms-auto d-none" aria-hidden="true"><use href="#check2"></use></svg> | ||
</button> | ||
</li> | ||
<li> | ||
<button type="button" class="dropdown-item d-flex align-items-center active" data-bs-theme-value="auto" aria-pressed="true"> | ||
<svg class="bi me-2"><use href="#ui-auto-mode"></use></svg> | ||
<svg class="bi me-2" aria-hidden="true"><use href="#ui-auto-mode"></use></svg> | ||
Auto | ||
<svg class="bi ms-auto d-none"><use href="#check2"></use></svg> | ||
<svg class="bi ms-auto d-none" aria-hidden="true"><use href="#check2"></use></svg> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be done in Bootstrap PR too
I did a full check of code and of your remarks. I have some remarks myself:
I don't think we need
I guess you're talking about the line 44 but I'm not sure what I should do...
For the logos in
You mean the SVG files? Nothing has to be done, we have to set
I tried with NVDA, it only says "Graphique" and the title provided, not mentioning the inside SVG... We should ask to EASE when we will talk about this audit...
Yes, see PR twbs/bootstrap#40756 that is complete at this time.
Well, yes, we could attract users' attention on the fact that we added aria-hidden in Orange navbar mode selector, tags with icon, and toggle buttons? |
coming over here from the bootstrap PR, I just want to ask directly: are you currently seeing a problem with decorative i mean, as a belt-and-braces "just in case at some point we do use an |
Well, you're right, I can't see the issue in my tests (with NVDA/Chrome, the SVGs without aria-hidden are correctly ignored) but this has been asked by our accessibility experts... So I trust their advice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think dark-mode.md:8356
SVG would need an aria-hidden="true"
and all the SVGs coming from it (30 SVGs until line 8821) (btn-group with icon for Day Week and Month).
Missing some in:
- Orange Navbars with mode selector
- The first Tag example
- Download app page for the mode selector
- Form example page for the mode selector
- Icons > External SVG file
I don't think we need
aria-hidden
here. It is a SVG code to be saved in an external file and then used in HTML in a<svg>
tag witharia-hidden
. What do you think?
Fine with this.
- Checks & radios > With icon
- Position > Examples
docs-subnav.html
example.html
shortcode replacementI guess you're talking about the line 44 but I'm not sure what I should do...
Ah nevermind, I thought something broke the shortcode, but it seems to be a previous commit (Difference between the generated code in the v5.2 and v5.3). I'll create an issue on our side.
orange-global-headers.html
for mode selectororange-logo.svg
About > BrandOBS-logo.svg
About > BrandFor the logos in
<figure>
withfigcaption
, I think we should keep thealt
s (maybe enhanced: Orange full logo, Orange reduced logo and OBS logo) withoutaria-hidden
because all examples I'v seen withfigcaption
do so... What do you think?
Already seen but yeah definitely.
I don't know what should be done on the `site/static/docs//assets//.svg
You mean the SVG files? Nothing has to be done, we have to set
aria-hidden
or not when we use these files in code.
True, sorry about that.
What do we decide for nested svgs ? There might be issues with download app page and placeholder shortcode.
I tried with NVDA, it only says "Graphique" and the title provided, not mentioning the inside SVG... We should ask to EASE when we will talk about this audit...
I think we should have a consistent approach, I've seen the nested SVG having an aria-hidden
and some having nothing.
Do we need any migration guide for this ? Maybe enumerating the different possibilities could so the trick.
Well, yes, we could attract users' attention on the fact that we added aria-hidden in Orange navbar mode selector, tags with icon, and toggle buttons?
Maybe just a mention to say that people should check their SVG to have either aria-hidden="true"
, or aria-label
, or aria-labelledby|describedby
, or a <title>
, or an alternative text.
Done
Maybe we can ask tomorrow to EASE about this...?
OK, we'll ask about that to EASE tomorrow as we planned
Done |
Putting it "on hold" until twbs/bootstrap#40756 is reviewed and merged to have a consistent approach. |
Note: Please transform
- [ ]
into- (NA)
in the description when things are not applicableRelated issues
#2667
Description
Add missing aria-hidden on SVGs when needed:
NB:
To be noted:
role="img"
+aria-label
+ eventually<title>
inside, on child SVGaria-hidden="true"
Motivation & Context
A11y audit fixes: these SVGs are decorative, they must have an attribute aria-hidden="true" so that assistive technologies ignore them.
Types of change
Live previews