-
Notifications
You must be signed in to change notification settings - Fork 913
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
Add aria-label to aside landmarks on home page (Fixes #15000) #15016
Add aria-label to aside landmarks on home page (Fixes #15000) #15016
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15016 +/- ##
=======================================
Coverage 77.64% 77.64%
=======================================
Files 162 162
Lines 8341 8341
=======================================
Hits 6476 6476
Misses 1865 1865 ☔ View full report in Codecov by Sentry. |
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.
Can more banner instances appear on the same page? You may run into the same labelling ambiguity in such case.
55983a2
to
4fa5283
Compare
bedrock/base/templates/includes/banners/mobile/firefox-app-store.html
Outdated
Show resolved
Hide resolved
bedrock/base/templates/includes/banners/mobile/focus-app-store.html
Outdated
Show resolved
Hide resolved
4fa5283
to
56b939b
Compare
Thanks for the suggestions, updated! |
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.
R+ 🏷️
<aside class="c-newsletter" aria-label="{{ ftl('newsletter-form-label') }}"> | ||
<div class="c-newsletter-wrapper mzp-l-content mzp-t-content-lg"> | ||
<div class="c-newsletter-info"> |
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.
Would it make sense to also use this newly added string as a legend
for fieldset.mzp-c-newsletter-content
there, that's missing one?
bedrock/bedrock/newsletter/templates/newsletter/includes/form.html
Lines 35 to 38 in 3e8f11b
<fieldset class="mzp-c-newsletter-content"> | |
<div class="mzp-c-form-errors {% if not form.errors %}hidden{% endif %}" id="newsletter-errors" data-testid="newsletter-error-message"> | |
{{ form.non_field_errors()|safe }} | |
<ul class="mzp-u-list-styled"> |
One-line summary
Gives each
<aside>
landmark on the home page a unique label for screen readers.Issue / Bugzilla link
#15000
Testing
http://localhost:8000/en-US/