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

Improve .form-group accessibility #1028

Merged
merged 2 commits into from
Mar 24, 2020
Merged

Conversation

simurai
Copy link
Contributor

@simurai simurai commented Feb 19, 2020

This is a first attempt to make our .form-group component more accessible.

Problem

In our docs we suggest using a "definition list" (<dl>) for the .form-group component. We even "bake it in" and make it a requirement by using selectors like dl.form-group > dd. The problem is that it can be "too noisy" for screen readers when there is a <dt> with just one <dd>.

From https://github.com/github/github/issues/132195 by @jscholes
When a screen reader user encounters repeated list containers with only a single item, it significantly increases the speech verbosity of page navigation. This is because the software will announce the fact that the cursor is entering and leaving each list.

Fix

This PR replaces...

  • <dl class="form-group"> 👉 <div class="form-group">
  • <dt> 👉 <div class="form-group-header">
  • <dd> 👉 <div class="form-group-body">

in the docs.

Before

<dl class="form-group">
  <dt>
    <label for="example-text">Example Text</label>
  </dt>
  <dd>
    <input class="form-control" type="text" value="Example Value" id="example-text" />
  </dd>
</div>

After

<div class="form-group">
  <div class="form-group-header">
    <label for="example-text">Example Text</label>
  </div>
  <div class="form-group-body">
    <input class="form-control" type="text" value="Example Value" id="example-text" />
  </div>
</div>

For now, the CSS selectors e.g dl.form-group > dd are unchanged. We'll have to wait to deprecate them until the next major release. Also, first we'd have to replace all the markup everywhere on dotcom.

Alternative

We might not even need those "wrapper" elements and could reduce the markup down to:

<div class="form-group">
  <label  class="form-group-label" for="example-text">Example Text</label>
  <input class="form-group-input form-control" type="text" value="Example Value" id="example-text" />
</div>

It would probably require some more testing and maybe refactoring on dotcom? 🤔

/cc @primer/ds-core

@vercel
Copy link

vercel bot commented Feb 19, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/primer/primer-css/q3v2t0610
✅ Preview: https://primer-css-git-accessible-form-groups.primer.now.sh

docs/content/components/box.md Outdated Show resolved Hide resolved
@simurai simurai changed the base branch from master to release-14.3.0 March 24, 2020 01:53
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.

4 participants