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 screen reader support, ARIA etc #104

Merged
merged 8 commits into from Dec 19, 2016

Conversation

Projects
None yet
4 participants
@colinrotherham

colinrotherham commented Nov 29, 2016

The UK Home office (and Department for Work and Pensions) have been maintaining a fork of the old—abandoned—Twitter branch for a little while.

We're using the typeahead in a number of our services for searching for countries. We'd be interested in you pulling in our additional ARIA support for this fork.

It includes these changes:

  1. Add an id attribute to each suggestion item
  2. Add role="option" to each suggestion item
  3. Add role="listbox" to the suggestions wrapper
  4. Add role="combobox" to the main input field
  5. Update aria-activedescendant on the input each time a suggestion is arrow-keyed etc
  6. Add a instructional status message (role="status", aria-live set to polite)
  7. Update dependencies (tests all pass, PhantomJS now runs on macOS Sierra)

What do you think?

easternbloc and others added some commits Apr 14, 2016

More accessible ARIA typeahead
Add new status class. This is used to assign aria attributes too and inform the user of dynamic element changes
Merge pull request #1 from UKHomeOffice/feature/add-aria-support
Add aria to typeahead to enhance accessibility
Use underscore bind instead of native.
This ensures we still work on older browsers.
Update status parameters
Fixes result count in ARIA status message
Update packages
Also fix broken promises tests
@colinrotherham

This comment has been minimized.

Show comment
Hide comment
@colinrotherham

colinrotherham Nov 29, 2016

One note to add, due to grunt-sed needing grunt@~0.4, I've replaced it with grunt-replace.

This allowed me to upgrade the other dependencies (e.g. newer and better performing grunt-contrib-uglify) without being held back.

colinrotherham commented Nov 29, 2016

One note to add, due to grunt-sed needing grunt@~0.4, I've replaced it with grunt-replace.

This allowed me to upgrade the other dependencies (e.g. newer and better performing grunt-contrib-uglify) without being held back.

@jlbooker

👍 Looks good to me, and thanks for the accessibility improvements!

@jlbooker jlbooker added this to the v1.1.0 milestone Nov 30, 2016

@jlbooker

This comment has been minimized.

Show comment
Hide comment
@jlbooker

jlbooker Nov 30, 2016

@corejavascript/collaborators Can we get another review before we merge this?

jlbooker commented Nov 30, 2016

@corejavascript/collaborators Can we get another review before we merge this?

@easternbloc

This comment has been minimized.

Show comment
Hide comment
@easternbloc

easternbloc commented Nov 30, 2016

@colinrotherham

This comment has been minimized.

Show comment
Hide comment
@colinrotherham

colinrotherham Nov 30, 2016

That's great, thanks.

Be wary about your other pull requests though, e.g. #54

An auto select feature may interrupt the "use up and down arrow keys to navigate" messaging we're now giving to screen readers. Pressing down may jump to the 2nd result, skipping the 1st one.

colinrotherham commented Nov 30, 2016

That's great, thanks.

Be wary about your other pull requests though, e.g. #54

An auto select feature may interrupt the "use up and down arrow keys to navigate" messaging we're now giving to screen readers. Pressing down may jump to the 2nd result, skipping the 1st one.

@jlbooker

This comment has been minimized.

Show comment
Hide comment
@jlbooker

jlbooker Nov 30, 2016

@colinrotherham Thanks for the heads up on that. Perhaps we should add the auto-select feature as an option (not enabled by default), and make a note of the potential issue in the docs.

jlbooker commented Nov 30, 2016

@colinrotherham Thanks for the heads up on that. Perhaps we should add the auto-select feature as an option (not enabled by default), and make a note of the potential issue in the docs.

@jlbooker

This comment has been minimized.

Show comment
Hide comment
@jlbooker

jlbooker Dec 19, 2016

Merging this, since we haven't heard from any other reviewers.

jlbooker commented Dec 19, 2016

Merging this, since we haven't heard from any other reviewers.

@jlbooker jlbooker merged commit 815ae42 into corejavascript:master Dec 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@colinrotherham

This comment has been minimized.

Show comment
Hide comment
@colinrotherham

colinrotherham commented Dec 20, 2016

Thanks @jlbooker

@A6Brgeuka

This comment has been minimized.

Show comment
Hide comment
@A6Brgeuka

A6Brgeuka Dec 21, 2016

Hey there, thanks for this feature
how can I hide this message, it breaks my design
I want to do it through config
image

@colinrotherham ^^

A6Brgeuka commented Dec 21, 2016

Hey there, thanks for this feature
how can I hide this message, it breaks my design
I want to do it through config
image

@colinrotherham ^^

@colinrotherham

This comment has been minimized.

Show comment
Hide comment
@colinrotherham

colinrotherham Dec 21, 2016

@A6Brgeuka That content is needed for screen readers. It announces that results are available, otherwise it's not obvious.

Use the .visuallyhidden class to hide it for sighted people?

Something like:

.visuallyhidden {
    position: absolute;
    overflow: hidden;
    clip: rect(0 0 0 0);
    height: 1px;
    width: 1px;
    margin: -1px;
    padding: 0;
    border: 0;
}

colinrotherham commented Dec 21, 2016

@A6Brgeuka That content is needed for screen readers. It announces that results are available, otherwise it's not obvious.

Use the .visuallyhidden class to hide it for sighted people?

Something like:

.visuallyhidden {
    position: absolute;
    overflow: hidden;
    clip: rect(0 0 0 0);
    height: 1px;
    width: 1px;
    margin: -1px;
    padding: 0;
    border: 0;
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment