Skip to content
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

Addresses admonition accessibility issue #2928

Merged
merged 13 commits into from
May 11, 2023

Conversation

jonathan343
Copy link
Contributor

@jonathan343 jonathan343 commented May 1, 2023

Overview

This PR addresses an accessibility issue related to the titles of what sphinx calls paragraph-level directives (also know as notes/warnings/admonitions). This currently generates both the title and message content using the HTML <p> tag. This causes issues in the following categories:

  1. Visual Hierarchy - "Making texts larger helps guide the eye around the page." Using headings and making them visually apparent is especially helpful for users with cognitive disabilities.". By not providing an apparent visual difference between the title and message content, we're providing certain users with a poor user experience.
  2. Screen Reader - " screen reader users can also benefit from headings. Screen reader users can navigate a page according to its headings, listen to a list of all headings, and skip to a desired heading to begin reading at that point. Screen reader users can use headings to skip the repeated blocks of content like headers, menus, and sidebars, for example." By not having the titles as a heading, we're providing screen reader users with a bad user experience.

Solution

New Approach inspired by similar changes here.

By inheriting from the sphinx HTML5Translator class, we can define a custom translator that takes uses <h3> tags where we want.
Cases:

  1. For every admonition we'll use <h3> for the title instead of the default <p>.
  2. For a specific list of sub-headings we currently generate as strong text (bold).

Without customization, a .. note:: generates something like:

<div class="admonition note">
<p class="admonition-title">Note</p>
...
</div>

After the customization we get something like

<div class="admonition note">
<h2 class="admonition-title">Note</h2>
...
</div>

Results

Before:
Screen Shot 2023-05-04 at 11 06 35 AM

After:
Screen Shot 2023-05-04 at 11 05 59 AM
Corresponding HTML:

Additional Note

Custom CSS was applied to make the change function correctly.

References

https://usability.yale.edu/web-accessibility/articles/headings

@codecov-commenter
Copy link

codecov-commenter commented May 1, 2023

Codecov Report

Patch coverage: 5.55% and project coverage change: -0.12 ⚠️

Comparison is base (8d9f1cf) 93.41% compared to head (9e012cb) 93.30%.

❗ 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    #2928      +/-   ##
===========================================
- Coverage    93.41%   93.30%   -0.12%     
===========================================
  Files           63       64       +1     
  Lines        13561    13589      +28     
===========================================
+ Hits         12668    12679      +11     
- Misses         893      910      +17     
Impacted Files Coverage Δ
botocore/docs/translator.py 0.00% <0.00%> (ø)
botocore/docs/client.py 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@jonemo jonemo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming that we aren't missing a clever way to configure the HTML tag for admonition headings in docutils, this does seem like the best way to do this. Two small comments/questions from me, but otherwise this looks good.

tests/unit/docs/test_client.py Outdated Show resolved Hide resolved
tests/unit/docs/bcdoc/test_style.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jonemo jonemo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me, four days ago:

Assuming that we aren't missing a clever way to configure the HTML tag for admonition headings in docutils

Thanks for finding this alternative approach and proving my assumption wrong! This is much better.

One small comment regarding an unlikely but possible failure mode.

docs/source/conf.py Outdated Show resolved Hide resolved
Copy link
Contributor

@dlm6693 dlm6693 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some notes on doc strings. Not blockers though.

docs/source/conf.py Outdated Show resolved Hide resolved
docs/source/conf.py Outdated Show resolved Hide resolved
docs/source/conf.py Outdated Show resolved Hide resolved
docs/source/conf.py Outdated Show resolved Hide resolved
docs/source/conf.py Outdated Show resolved Hide resolved
docs/source/conf.py Outdated Show resolved Hide resolved
@jonathan343
Copy link
Contributor Author

Just some notes on doc strings. Not blockers though.

Thanks for the feedback! I addressed all issues/suggestions in the most recent commit. Lmk if there is anything I missed or something else you'd like to see in this PR.

Copy link
Contributor

@dlm6693 dlm6693 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@jonathan343 jonathan343 merged commit 3879f9a into boto:develop May 11, 2023
aws-sdk-python-automation added a commit that referenced this pull request May 11, 2023
* release-1.29.133:
  Bumping version to 1.29.133
  Update to latest partitions and endpoints
  Update to latest models
  Update black formatting
  Include params set in provide-client-param event handlers in dynamic context params for endpoint resolution (#2920)
  Addresses admonition accessibility issue (#2928)
  Update pre-commit hooks versions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants