add landmarks, use semantic html tags to admin templates #5290
Conversation
Manage this branch in SquashTest this branch here: https://hippogrifficadd-semantic-tags-19exs.squash.io |
There was a problem hiding this comment.
Great to see this happening! I've added a question about usage of aside, and this will need to add support for translations when using aria-label. I'm really happy to see the updated tabs markup, at the moment I think tabs are completely hidden from screen reader users.
There are some things that are not in the PR and I'd like to propose for inclusion:
role="banner"for current usage of theheaderelement, to maximise support by screen readersrole="contentinfo"for current usage of thefooterelement- Would there be a case for using the
footertag in other places, withrole="contentinfo"? - Looking at the list of landmark roles in https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques#Landmark_roles, are there places where we could be using
role="region"with a label, for regions that are very common in Wagtail?
Finally, I think we've discussed that already but @hippogriffic could you confirm that IE11 support isn't an issue for this?
Edit: I see this is making some of the tests fail in CI, presumably because they rely on the markup. Could you have a look at those failures?
wagtail/admin/templates/wagtailadmin/edit_handlers/tabbed_interface.html
Show resolved
Hide resolved
wagtail/admin/templates/wagtailadmin/shared/ajax_pagination_nav.html
Outdated
Show resolved
Hide resolved
wagtail/admin/templates/wagtailadmin/shared/pagination_nav.html
Outdated
Show resolved
Hide resolved
|
I'm not sure I see a lot of need for more IE11 support should be fine with these changes |
thibaudcolas
force-pushed
the
add-semantic-tags-landmarks
branch
from
241b3d2
to
47bb8ac
Compare
There was a problem hiding this comment.
Looks good! I have tested this in IE11 for browser support, Chrome with VoiceOver for the screen reader impact, as well as Axe.
I ended up adding some of the landmarks to more instances of how they had been added here – main for content-wrapper, the banner for header elements and contentinfo for footer, as well as navigation for the pagination UIs.
| {% for child in self.children %} | ||
| <li class="{{ child.classes|join:" " }} {% if forloop.first %}active{% endif %}"><a href="#tab-{{ child.heading|cautious_slugify }}" class="{% if forloop.first %}active{% endif %}">{{ child.heading }}</a></li> | ||
| <li id="tab-label-{{ child.heading|cautious_slugify }}" class="{{ child.classes|join:" " }} {% if forloop.first %}active{% endif %}"><a href="#tab-{{ child.heading|cautious_slugify }}" class="{% if forloop.first %}active{% endif %}">{{ child.heading }}</a></li> |
There was a problem hiding this comment.
This must have a role="tab", otherwise Axe raises https://dequeuniversity.com/rules/axe/3.2/aria-required-children?application=axeAPI.
| @@ -23,7 +23,7 @@ <h1 class="icon icon-doc-empty-inverse"> | |||
| <input type="hidden" name="next" value="{{ next }}"> | |||
| {{ edit_handler.render_form_content }} | |||
|
|
|||
| <footer> | |||
| <footer role="contentinfo"> | |||
There was a problem hiding this comment.
Axe complains about https://dequeuniversity.com/rules/axe/3.2/landmark-contentinfo-is-top-level?application=axeAPI. I still think this is valuable though, so will leave it for now.
thibaudcolas
force-pushed
the
add-semantic-tags-landmarks
branch
from
e632b34
to
6822e81
Compare
|
Merged! Thank you @hippogriffic, this will be released in Wagtail v2.6. |
Fixes #5275.