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

Assorted accessibility (and some consistency) fixes for documentation #14951

Closed
wants to merge 29 commits into from
Closed

Assorted accessibility (and some consistency) fixes for documentation #14951

wants to merge 29 commits into from

Conversation

patrickhlauke
Copy link
Member

No description provided.

the tabindex prevents the segmented dropdown from being
keyboard-accessible
combination of aria-hidden=true and sr-only spans
Also includes a call-out explaining why these are necessary.

removed two unnecessary style attributes; removed use of unnecessary
toolbar markup for the group sizing examples (and instead used <br> to
separate them, as was done elsewhere in the document)
role="tablist" is only a valid role when it is, in fact, a tab panel
widget. In cases where navs are used purely as visual navigations, it's
incorrect. added note about needing the role(s) when doing a proper tab
panel to the existing JS requirement callout.
assume this is still in the document by mistake? otherwise, why have two
email fields?
if this *was* intended to be there for some reason (to show the addon
stuff?), then this input will also need a <label class="sr-only" ...>
like the other two text/password inputs.
particularly calling out alternatives to just using visually hidden
<labels>, and clarification on use placeholder in line with
http://www.w3.org/TR/html5/forms.html#the-placeholder-attribute
@cvrebert cvrebert added the docs label Oct 31, 2014
It seems strange that the original example has *two* "enter email"
fields - one with an sr-only label, and one with an input-group-addon.
If nothing else, this blows up the inline form as it's too wide.
Logically, it makes no sense as an example form (compared to all other
form examples, that have a single email field). If the intent was to
also show the use of input-group with the extra addon, then this change
makes the most sense to me...
Admittedly wordy advice callouts and slightly lengthy markup.
@karlgroves
Copy link

Just a quick comment on the above discussion re: tabs
(this is a duplicate of my comment over at paypal/bootstrap-accessibility-plugin#59)

"tabs" != "navigation"

This is (and menu) a very common misunderstanding, but it is important to keep in mind that navigation is a very different thing within a tabpanel context vs. a document (or even application) context.

In the WAI-ARIA authoring practices document this is explained a bit
http://www.w3.org/TR/2010/WD-wai-aria-practices-20100916/

3.2.5. Managing the Perception of a Dual Focus

Often applications give the perception of having a dual focus. Two examples of this are multi-selection list boxes and selected tabs, within a tablist, when focus is in a tabpanel. ... In the case of a tabpanel the user selects a tab but then navigates to a focused item in the corresponding tabpanel the tab appears to also have focus. In reality, only one element may have focus in an application. In these scenarios authors should ensure keyboard focus is set on the current element that visibly receives keyboard user input while ensuring other "highlighted" elements have an aria-selected state of "true" until de-selected. When the de-selection occurs, such as when ... a user moves to a new tab and de-select the old tab, the author should ensure that the visible selection of the de-selected item is removed.

In other words, the use of tab-related roles is only relevant in the context of a tabpanel. Navigation has an entirely different context - the entire document. No matter whether something looks like tabs or not, if they do not behave as tabs and operate in the context of a tabpanel (or, accordion) interface then they must not be given tab-related roles.

also added .sr-only text to the links with class .active
not changing the examples, as this is very dependent on the actual
content being highlighted/colored
and, in the case of navbar, make the "Link", "Link", "Link" navigation
links consistent with the other two navbar examples
including TPG's CCA (and Chrome's A11y DevTools extension) in the
resources too
@@ -23,6 +23,10 @@ <h3 id="helper-classes-colors">Contextual colors</h3>
<h4>Dealing with specificity</h4>
<p>Sometimes emphasis classes cannot be applied due to the specificity of another selector. In most cases, a sufficient workaround is to wrap your text in a <code>&lt;span&gt;</code> with the class.</p>
</div>
<div class="bs-callout bs-callout-warning">
<h4>Conveying meaning to assistive technologies</h4>
<p>Using color to add meaning only provides a visual indication, which will not be conveyed to users of assistive technologies – such as screen readers. Ensure that information denoted by the color is either obvious from the content itself (the contextual colors are only used to reinforce meaning that is already present in the text/markup), or is included through alternative means, such as additional text hidden with the <code>sr-only</code> class.</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a dot to the class reference: .sr-only

Copy link
Member Author

Choose a reason for hiding this comment

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

done 2eb04f0

@hnrch02
Copy link
Collaborator

hnrch02 commented Nov 4, 2014

(@cvrebert twbs/rorschach#15 would pose a problem with for instance this PR.)

@patrickhlauke
Copy link
Member Author

@hnrch02 i'm probably the overly fussy exception to the rule... ;)

@hnrch02 hnrch02 added this to the v3.3.1 milestone Nov 5, 2014
@mdo
Copy link
Member

mdo commented Nov 9, 2014

How we looking on this? Still hoping we can get it in v3.3.1? Need anything from me?

@cvrebert
Copy link
Collaborator

cvrebert commented Nov 9, 2014

@mdo Just your approval, since I think everyone else's comments have been addressed.
(And then we just need to do some squashing when we merge.)

@@ -62,7 +62,7 @@
</li>
</ul>
<ul class="nav navbar-nav navbar-right">
<li class="active"><a href="./">Default</a></li>
<li class="active"><a href="./">Default<span class="sr-only">(current)</span></a></li>
Copy link
Member

Choose a reason for hiding this comment

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

Add a space before the <span> or break this into new lines.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had left them out for fear of potential layout disruption (e.g. that it would mess up centering of the text), but if it's unaffected, yay :)

@hnrch02 hnrch02 closed this in bb89657 Nov 11, 2014
@hnrch02 hnrch02 mentioned this pull request Nov 11, 2014
@hnrch02
Copy link
Collaborator

hnrch02 commented Nov 11, 2014

Thanks once again for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants