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.

Sign up for GitHub

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

Jump to bottom

Improves accessibility of single and multi choice #156

Merged
merged 17 commits into from
Jul 29, 2020
Merged

Improves accessibility of single and multi choice #156

merged 17 commits into from
Jul 29, 2020

Conversation

bennapp
Copy link
Contributor

@bennapp bennapp commented Jul 20, 2020 •
edited
Loading

Motivation

Make things accessible woo! 🥳

https://www.pivotaltracker.com/story/show/173478278

Acceptance Criteria

  • Single choice bugs

    • (Only in Chrome) The field label is not read when focusing the input - DONE
    • (Only in Chrome) When selecting a new option, the voiceover reads the last option selected instead of the new one. - DONE
    • (Only in Chrome) When navigating between single choice fields, the labels are sometimes not read. - DONE
    • The clear icon is in page tab sequence (otherwise a keyboard only user cannot deselect a choice field). Voiceover should read "Remove selected choice" or something - DONE

  • Multi choice bugs

    • After selecting a choice, the autocomplete input should get focused - DONE
    • After selecting a choice, voiceover reads what was typed in autocomplete instead of the selected choice (or what should it read?) - DONE
    • The clear all icon is in page tab sequence? Voiceover should read "Remove all selected choices" or something - DONE
    • When invalid, voiceover should read the error message - DONE

PR upkeep checklist

@bennapp bennapp self-assigned this Jul 20, 2020
cincospenguinos
cincospenguinos reviewed Jul 24, 2020
View reviewed changes
src/components/form-control/form-control.test.jsx
});

it('uses a hint to alert users of the error message', () => {
render(<FormControl {...requiredProps} error="I am an error message" />);
Copy link

Choose a reason for hiding this comment

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

I think this prop switched over to errorText. Is it still error in FormControl? If so, we may want to update it as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can make a new story too to avoid having breaking changes in this particular PR (and thus keep the AC for the story focused)

cincospenguinos
cincospenguinos approved these changes Jul 24, 2020
View reviewed changes
Copy link

@cincospenguinos cincospenguinos left a comment

Choose a reason for hiding this comment

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

A couple suggestions, but otherwise LGTM

src/components/icon/icon.jsx Show resolved Hide resolved
juanca
juanca reviewed Jul 24, 2020
View reviewed changes
src/components/custom-field-input-multiple-choice/custom-field-input-multiple-choice.jsx
@@ -152,6 +158,8 @@ function CustomFieldInputMultipleChoice(props) {
fill="skip"
name={iconClear.id}
onClick={onChoicesClear}
onEnter={clearChoices}
tabable={true}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this isn't used anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not forget to clean this up

@juanca juanca requested review from juanca and removed request for Bloodyaugust July 28, 2020 18:01
@juanca
Copy link
Contributor

juanca commented Jul 28, 2020

I can give this another look over once its greened up.

Bloodyaugust
Bloodyaugust reviewed Jul 28, 2020
View reviewed changes
Copy link
Contributor

@Bloodyaugust Bloodyaugust left a comment

Choose a reason for hiding this comment

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

Looks great! I think there are a few things here that could be done for better semantics and clarity, but they're fairly minor (read: non-blocking).

src/components/custom-field-input-single-choice/custom-field-input-single-choice.jsx
};

const clearIcon = () => {
if (!props.readOnly && (value || searchValue)) {
return (<Icon
name={iconClear.id}
onClick={clear}
onEnter={clear}
ariaLabel={'Remove selected choice'}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this say

Suggested change
ariaLabel={'Remove selected choice'}
ariaLabel={'Clear selected choice'}

To keep the nomenclature consistent?

src/components/custom-field-input-single-choice/custom-field-input-single-choice.test.jsx
expect(screen.getByLabelText('Test label', { selector: 'input' })).toHaveValue('some test text');
userEvent.click(screen.getAllByRole('img')[0]);
expect(screen.getByLabelText('Test label', { selector: 'input' })).toHaveValue('');
it('clears a value when enter is pressed on the clear icon and the clear icon can be focused for accessibility', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also want an expectation about where the focus moves after clear is pressed?

src/components/icon/icon.jsx
role: 'img',
size: 'medium',
stroke: 'none',
title: undefined,
active: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be false, so multiple icons on the same page aren't all active.

src/components/icon/icon.jsx Outdated
role={props.role}
>
{ props.title && <title>{props.title}</title> }
<use onKeyDown={onKeyDown} xlinkHref={`#${props.name}`} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should only attach the onKeyDown handler if props.role === 'button' to keep it semantic. If we do make this change, the test should change along with it (and probably a new test should exist to cover the new behavior).

@bennapp bennapp merged commit 3ca2dd0 into master Jul 29, 2020
@bennapp bennapp deleted the webinf-accessible-fixes-single-multi-173478278 branch July 29, 2020 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bloodyaugust Bloodyaugust Bloodyaugust left review comments

@cincospenguinos cincospenguinos cincospenguinos approved these changes

@juanca juanca Awaiting requested review from juanca

Assignees

@bennapp bennapp

Labels
accessibility
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bennapp @juanca @Bloodyaugust @cincospenguinos