-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Accessibility Enhancements #2949
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## develop #2949 +/- ##
===========================================
- Coverage 93.29% 93.28% -0.01%
===========================================
Files 64 64
Lines 13588 13589 +1
===========================================
Hits 12677 12677
- Misses 911 912 +1
☔ 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.
The GitHub diff is a bit messy and confusing. Here is screenshot provides a diff when compared to the original page.html from furo.
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.
Nit: Seems strange that we have one modification surrounded by "[start/end] of AWS modifications of Furo template" but not the other modifications.
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.
Updated the image with a new diff. I think i solved the issue you're talking about? It was extra whitespace and newlines.
<div class="sidebar-div show-div-sm justify-content-right"> | ||
<label class="nav-close-icon" id="nav-menu-close" for="__navigation" tabindex="0"> | ||
<div class="visually-hidden">Toggle site navigation sidebar</div> | ||
<i class="icon"><svg><use href="#svg-close"></use></svg></i> |
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.
What black magic is this? I've seen <use>
for re-using SVG content within the same <svg>
element, but this is re-using it from somewhere else?
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 was mainly trying to stay as consistent as possible with what Furo does. They have an icons.html page where they define different symbols and later include this in page.html. There is no easy way to add symbols to this list so I added the new symbol directly here.
This seems like valid behavior according to svgwg.org
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.
Nit: Seems strange that we have one modification surrounded by "[start/end] of AWS modifications of Furo template" but not the other modifications.
</div> | ||
</a> | ||
{%- endif %} | ||
{% block body -%} |
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.
What is the reason for the changes to template blocks? I think body
is completely new and others have moved around.
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.
So in furo's original page.html the footer
block is nested in the body
block. Previously, we only needed to modify the footer
so utilizing jinja2's template inheritance we could just do:
{%- extends "!page.html" %}
{% block footer %}
# Custom HTML that overrides furo's footer block
{%- endblock %}
This uses all the original body
block content but replaces the footer
block with what we defined. Ideally, as many things would be wrapped in its own block to make things more easily customizable without having to rewrite an entire template. In this case we need to modify elements in the body (outside of the footer), so we need to now do something like:
{%- extends "!page.html" %}
{% block body %}
# Custom HTML before footer
{% block footer %}
# Custom HTML that overrides furo's footer block
{%- endblock %}
# Custom HTML after footer
{%- endblock %}
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.
Where (and how) does this file get used? I only see the content of it duplicated in page.html
, but no reference to this file.
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.
This file gets referenced in conf.py. More information on customizing the left sidebar can be read on furo's doc site.
docs/source/_static/css/custom.css
Outdated
/* Hides the right sidebar on a medium screen by default. */ | ||
.toc-drawer { | ||
display: none; | ||
} |
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.
Is this PR actually changing when the toc drawer and sidebar drawer get shown?
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.
No, but the way furo currently does this is by animating the left/right side bar outside the visible screen area by moving its position. The issue with this is that keyboard users can continue to tab/focus on elements they cant see. Applying the display: none;
style prevents this behavior. The downside of this is that we loose the cool animation. Im looking into a way to achieve both.
botocore/docs/translator.py
Outdated
if ( | ||
len(node) == 1 | ||
and isinstance(node[0], nodes.strong) | ||
and len(node[0]) == 1 | ||
and isinstance(node[0][0], nodes.Text) | ||
and node[0][0].astext() not in self.IGNORE_IMPLICIT_HEADINGS | ||
): |
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.
Does this logic also need to handle situations like <p> <strong>Text</strong> </p>
where there's a whitespace-only text node between the tags? Or does this logic get applied after the whitespace stripping from #2855 happens?
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.
This logic happens after #2855 applies changes in the transition from upstream HTML --> RST
. This translator extends the one from sphinx that is used in the transition from RST --> Boto Docs HTML
.
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.
Cool, this all looks great. Thanks for the thorough deep dive on this @jonathan343 and it's exciting we may be able to port some of this back upstream.
My only question is how does that transition look if it is released in a future version of furo. Do we get that for free and can revert this or will we need to keep specific parts in the Translator/CSS/Page Template?
@nateprewitt If pradyunsg/furo#660 is ever merged and released in furo, we'll be able to revert the parts related to keyboard navigation (everything except changes to |
Improve keyboard navigation and remove extra implicit headings.
Improve keyboard navigation and remove extra implicit headings.
Overview
This PR provides two enhancements related to accessibility for the botocore/boto3 documentation websites.
STRONG_TO_H3_HEADINGS
texts value we wanted to address. Since then, we've learned that we're responsible for converting more unpredictable values. This PR replaces the old method with a new one that uses the common pattern of<p><strong>Implicit Headings</strong><p>
to detect and replace with<h3>Implicit Heading</h3>
.Solutions
Keyboard Navigation
tabindex="0"
icon should be focused on. Same should happen for the left side bar. This feature is demonstrated in the section below.
Implicit Headings
<p><strong>Implicit Headings</strong><p>
docutil.nodes.strong
. This child node must also only have 1 child node of the typedocutil.nodes.Text
. The text value of this node should not exist in ourIGNORE_IMPLICIT_HEADINGS
list.docutil.nodes.Text
node and write this as anh3
tag instead.raise nodes.SkipNode
to skip visiting the current nodes children and its call of the depart method.Demo