Improve accessibility (Section 508, WCAG) #9137

Closed
wants to merge 32 commits into
from

Projects

None yet

8 participants

@ajb
Contributor
ajb commented Aug 6, 2013

This PR significantly improves Bootstrap's accessibility for users of assistive technology, such as screen readers. Some of the these changes add additional markup to the source examples, but we believe that the sacrifice in readability is worth achieving more widespread usage of accessibility best-practices.

What was done

  • Added lots of WAI-ARIA attributes
  • Added .sr-only helper class, that is only readable by screen readers (and invisible for all other users). This lets us - make progress bars and paginations accessible to screen reading users.
  • Advised users to always use label elements. For inline forms, they can hide them with .sr-only
  • Added 'Skip navigation' link
  • Added "Accessibility" section to getting-started.html.

What wasn't done

  • Contrast issues (twbs#3572)
  • Tooltips (twbs#8469)
  • Documentation re: usage of icons, since they now live in a separate repo

Major props to all that contributed: @bensheldon, @jasonlally, @criscristina, and @louh. Feel free to chime in, guys, if I've left anything out.

@mdo mdo commented on an outdated diff Aug 6, 2013
components.html
<div class="container">
<!-- .navbar-toggle is used as the toggle for collapsed navbar content -->
<button type="button" class="navbar-toggle" data-toggle="collapse" data-target=".navbar-responsive-collapse">
+ <span class="sr-only">Expand Navigation</span>
@mdo
mdo Aug 6, 2013 Member

What about changing these to Toggle navigation? This is the trigger to expand and collapse, and I push for sentence case just about everywhere :).

@mdo mdo commented on an outdated diff Aug 6, 2013
components.html
@@ -1498,7 +1504,7 @@ <h2 id="pagination-default">Standard pagination</h2>
<div class="bs-example">
<ul class="pagination">
<li class="disabled"><a href="#">&laquo;</a></li>
- <li class="active"><a href="#">1</a></li>
+ <li class="active"><a href="#">1<span class='sr-only'>(current)</span></a></li>
@mdo
mdo Aug 6, 2013 Member

Double quotes or bust on all these.

Do you need a space between the number and span?

@mdo mdo commented on an outdated diff Aug 6, 2013
components.html
@@ -2055,6 +2079,7 @@ <h3 id="progress-stacked">Stacked</h3>
<div class="progress-bar progress-bar-success" style="width: 35%"></div>
<div class="progress-bar progress-bar-warning" style="width: 20%"></div>
<div class="progress-bar progress-bar-danger" style="width: 10%"></div>
+ <span class='sr-only'>Some kind of explanatory text</span>
@mdo
mdo Aug 6, 2013 Member

What?

@mdo mdo commented on an outdated diff Aug 6, 2013
javascript.html
@@ -192,6 +192,29 @@ <h4 class="modal-title" id="myModalLabel">Modal Heading</h4>
<a data-toggle="modal" href="#myModal" class="btn btn-primary btn-lg">Launch demo modal</a>
</div><!-- /example -->
{% highlight html %}
+<<<<<<< HEAD
@mdo
mdo Aug 6, 2013 Member

You've committed a merge conflict here.

@mdo mdo commented on an outdated diff Aug 6, 2013
less/scaffolding.less
@@ -93,3 +93,24 @@ hr {
border-top: 1px solid @hr-border;
}
+// Only display content to screen readers
+// -------------------------
+
+.sr-only {
@mdo
mdo Aug 6, 2013 Member

Some comments in the CSS explaining why these properties are necessary would be boss. Why the !important on some, but not all?

You can also drop the IE6-7 stuff—we're only supporting IE8+ in v3.

@mdo mdo commented on an outdated diff Aug 6, 2013
less/scaffolding.less
@@ -93,3 +93,24 @@ hr {
border-top: 1px solid @hr-border;
}
+// Only display content to screen readers
+// -------------------------
+
+.sr-only {
+ position: absolute !important;
+ clip: rect(1px 1px 1px 1px); /* IE6, IE7 */
+ clip: rect(1px, 1px, 1px, 1px);
+ padding:0 !important;
+ border:0 !important;
@mdo
mdo Aug 6, 2013 Member

Add spaces after the : following padding and border.

@mdo mdo and 1 other commented on an outdated diff Aug 6, 2013
less/scaffolding.less
@@ -93,3 +93,24 @@ hr {
border-top: 1px solid @hr-border;
}
+// Only display content to screen readers
+// -------------------------
+
+.sr-only {
+ position: absolute !important;
+ clip: rect(1px 1px 1px 1px); /* IE6, IE7 */
+ clip: rect(1px, 1px, 1px, 1px);
+ padding:0 !important;
+ border:0 !important;
+ height: 1px !important;
+ width: 1px !important;
+ overflow: hidden;
+}
+
+body:hover {
@mdo
mdo Aug 6, 2013 Member

Again here, let's get some comments—why is this stuff necessary?

@ajb
ajb Aug 6, 2013 Contributor

agreed -- this is fairly weird stuff, and there's conflicting documents all over the web.

Started here: http://a11yproject.com/posts/how-to-hide-content/

Ended up copying the class from h5bp, which you can see in the latest diff: https://github.com/h5bp/html5-boilerplate/blob/master/css/main.css#L152

@cvrebert cvrebert commented on an outdated diff Aug 6, 2013
@@ -1263,9 +1263,15 @@ <h3 id="forms-inline">Inline form</h2>
<h4>Requires custom widths</h4>
<p>Inputs, selects, and textareas are 100% wide by default in Bootstrap. To use the inline form, you'll have to set a width on the form controls used within.</p>
</div>
+ <div class="bs-callout bs-callout-danger">
+ <h4>Always add labels</h4>
+ <p>Screen readers will have trouble with your forms if you don't include a label for every input. For this inline forms, you can hide the labels using the <code>.sr-only</code> class.</p>
@cvrebert
cvrebert Aug 6, 2013 Member

English grammatical error in the latter sentence here.

@cvrebert cvrebert commented on an outdated diff Aug 6, 2013
getting-started.html
+ <p class="lead">Bootstrap follows common web standards, and with minimal extra effort, can be used to create sites that are accessibile to users using assistive technology (AT). However, it's useful to take the following into consideration:</p>
+
+ <p>If your navigation contains many links and comes before your main content in the DOM, add a <code>Skip to content</code> link immediately after your opening <code>body</code> tag. <a href="http://a11yproject.com/posts/skip-nav-links/">(read why)</a></p>
+
+{% highlight html %}
+<body>
+ <a href="#content" class="sr-only">Skip to content</a>
+ ...
+ <div class="container" id="content">
+ The main page content.
+ </div>
+ ...
+</body>
+{% endhighlight %}
+
+ <p>Another "gotcha" has to do with how you nest your <code>header</code> elements. <a href="http://squizlabs.github.io/HTML_CodeSniffer/Standards/Section508/">Section 508</a> states that your largest header must be an <code>h1</code>, and the next header must be an <code>h2</code>, etc. This is hard to achieve in practice, but it's if the largest header on your site is smaller than Bootstrap's default 38px, you should consider modifying your stylesheets before using a smaller header element.</p>
@cvrebert
cvrebert Aug 6, 2013 Member

"but it's if" ?

@ajb
Contributor
ajb commented Aug 6, 2013

Thanks so much for the quick review. I think I've touched on all your comments with these added commits.

@mdo
Member
mdo commented Aug 7, 2013

We've done a lot to the docs today—any chance you could peep this tonight or tomorrow and get it up to date so we can do an easy merge without having to deal with conflicts?

@ajb
Contributor
ajb commented Aug 7, 2013

Definitely, will do so tonight. Should just a matter of merging the upstream repo in and fixing conflicts, right?

@mdo
Member
mdo commented Aug 7, 2013

Thanks, and yup, that should be it.

@ajb
Contributor
ajb commented Aug 7, 2013

Done, but not really sure what's up with the BrowserStack key and why tests are failing.

@cvrebert
Member
cvrebert commented Aug 7, 2013

That's expected; ignore it.

@ajb
Contributor
ajb commented Aug 7, 2013

Awesome. Let me know if there's anything else!

@mdo
Member
mdo commented Aug 7, 2013

I tried my hand at it, but failed and can't devote more time to it right now. Can you by chance rebase this @adamjacobbecker so we get it a single commit?

@ajb
Contributor
ajb commented Aug 7, 2013

I think you might have put too much faith in a guy who committed a merge conflict earlier. Having some trouble with this... let me try again, and then I'll call in the cavalry...

@mdo
Member
mdo commented Aug 7, 2013

@adamjacobbecker Hah, I know that feel :). I've never really tried before and but I'm so swamped I can't spend more time giving it another go. @cvrebert might have some guidance—he did one earlier. I was working from one of several guides I found whilst Googling.

@cvrebert
Member
cvrebert commented Aug 7, 2013

I'm currently giving it a go myself.

ajb and others added some commits Aug 4, 2013
@ajb ajb add screenreader-only label tags to inline-form
related to #7
5a8d585
@ajb ajb add callout re: labels in inline-form 28b8e28
@jasonlally @ajb jasonlally change modal example and add callouts about accessibility 5598714
@ajb ajb Pagination component should have something to direct user which item …
…is current (besides class name)

Edit

closes #13, major props @dannychapman!
a2c9061
@ajb ajb Progress bars should have text (hidden or otherwise) to display text …
…version of percent complete.

closes #14.

this is pretty ugly syntax, but necessary for a11y. we could use a
title attribute, but apparently that's not quite as good of a solution.
4b16762
@ajb ajb globally replace 'visually-hidden' with 'sr-only'
it's 1/2 as many characters and just as semantic*

*although i guess you could argue that we shouldn't be abbreviating
"screen reader" to "sr". oh well.
2487fe4
@ajb ajb add accessibility section to docs d943dd5
@ajb ajb document sr-only a0d3a4c
@ajb ajb s/screenreaders/screen readers 9b0b9a5
@ajb ajb clean up modal changes 6edbfa5
@dannychapman @ajb dannychapman Added ARIA for progress bars, changed markup on sr-only classes to use " 3b9a843
@ajb ajb fix merge conflict 8877417
@ajb ajb s/Expand Navigation/Toggle navigation f639b78
@ajb ajb single quotes -> double quotes, add space 896c42f
@ajb ajb adjust .sr-only helper to match h5bp d8af939
@ajb ajb fix grammatical errors 9b21656
@ajb ajb fix progressbar examples a73bb13
@criscristina @ajb criscristina added callout for carousel accessibility d40f08f
@ajb
Contributor
ajb commented Aug 7, 2013

Might have it done. Just gotta verify that I didn't lose anything.

@ajb
Contributor
ajb commented Aug 7, 2013

Let me know how that looks.

@cvrebert
Member
cvrebert commented Aug 7, 2013

@adamjacobbecker Er, nope, still a kajillion commits.

@cvrebert
Member
cvrebert commented Aug 7, 2013

Result of my attempt: #9186
Please double-check it.

@mdo mdo closed this Aug 7, 2013
@mgifford

So are these changes in 3.0.2?

@cvrebert
Member

@mgifford They're in v3.0.0+. Read the successor pull request's merge commit.

@mgifford

Thanks!

@cvrebert cvrebert locked and limited conversation to collaborators Sep 17, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.