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

Accessibility improvements #1501

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

mullermp
Copy link
Contributor

@mullermp mullermp commented Jul 5, 2023

Description

For AWS SDK for Ruby, we had to make some accessibility fixes to the docs, involving some styling and color changing. I think some of these may be helpful to contribute back.

I have not written tests yet because I want to know if these are changes that we can (and should) make before I go and invest that time.

Completed Tasks

  • I have read the Contributing Guide.
  • The pull request is complete (implemented / written).
  • Git commits have been cleaned up (squash WIP / revert commits).
  • I wrote tests and ran bundle exec rake locally (if code is attached to PR).

Copy link
Contributor Author

@mullermp mullermp left a comment

Choose a reason for hiding this comment

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

Just adding some comments to explain code changes.

Comment on lines +23 to +24
li.collapsed a.toggle { cursor: default; background-position: top left; }
li { color: #666; cursor: pointer; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opacity was removed and color changed from 888 to 666 to allow for at least a 4.5 contrast ratio (accessibility requirement). It looks roughly the same, just a little darker.

@@ -82,6 +82,12 @@ body {
#search { display: none; }
}

@media (max-width: 320px) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had a requirement to support devices with < 320 px width (like smart phone). I'm not a CSS expert, but the way this is coded in style.css for 920px min-width interferes with 320px unless I copy over the same properties. I effectively am only adding break-word here. I am probably not the best person to untangle / cascade this properly. It removes all horizontal scrolling except for tables and code snippets which is acceptable for the requirement.

@@ -106,6 +112,7 @@ h2 small a {
position: relative;
padding: 2px 7px;
}
a { font-weight: 550; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bold links are 700, normal is 400. This is a weight in between. The requirement was to make links more obvious for people with low vision.

Comment on lines +29 to +32
<div id="search">
<label for="search-class">Search:</label>
<input id="search-class" type="text" />
</div>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just adding a label for search for screen readers to know what it's for.

@@ -1,5 +1,5 @@
<!DOCTYPE html>
<html>
<html <%= "lang=\"#{html_lang}\"" unless html_lang.nil? %>>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We hardcoded lang="en" in our docs, but I think it makes sense to have this as a setup method.

@@ -1,9 +1,9 @@
<% if object.has_tag?(:example) %>
<div class="examples">
<p class="tag_title">Examples:</p>
<h4 class="tag_title">Examples:</h4>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One requirement we had was to not have paragraph sections for examples. We set these to h4 and h5 headers so I think this is reasonable as they are supposed to be sections.

@@ -225,15 +231,17 @@ def class_list(root = Registry.root, tree = TreeContext.new)
has_children = run_verifier(child.children).any? {|o| o.is_a?(CodeObjects::NamespaceObject) }
out << "<li id='object_#{child.path}' class='#{tree.classes.join(' ')}'>"
out << "<div class='item' style='padding-left:#{tree.indent}'>"
out << "<a class='toggle'></a> " if has_children
accessible_props = "aria-label='#{name} child nodes' aria-expanded='false' aria-controls='object_#{child.path}'"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting standard aria properties. The JS code will flip these.

$('#noresults').text('No results were found.').hide().fadeIn();
} else {
$('#noresults').text('').hide();
// This is read out to screen readers
$('#noresults').text('There are ' + found + ' results.').hide().fadeIn();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to change noresults to searchresults or something, but I figure I cannot change this for existing templates out there. I've just updated it to output the results at the bottom of the list, and it's read out to screen readers.

@@ -91,7 +108,7 @@ function enableSearch() {
}
});

$('#full_list').after("<div id='noresults' style='display:none'></div>");
$('#full_list').after("<div id='noresults' role='status' style='display:none'></div>");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Role is added for screen reader.

highlight();
});

// navigation of nested classes using keyboard
$('#full_list a.toggle').on('keypress',function(evt) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Part of this change allows you to tab navigate to the arrow/carrot in the full list menu, then you can press enter to expand the section without visiting a new page.

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.

None yet

1 participant