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

Bug 1531243 - Fix some a11y issues with buttons and labels #4731

Merged
merged 3 commits into from
Mar 6, 2019

Conversation

camd
Copy link
Contributor

@camd camd commented Mar 1, 2019

This addresses Bug 1531243

Please see individual commits

@camd camd force-pushed the fix-a11y-buttons branch 2 times, most recently from baa7e5a to f90c61b Compare March 1, 2019 18:46
@camd camd requested a review from sarah-clements March 1, 2019 23:42
Copy link

@jcsteh jcsteh left a comment

Choose a reason for hiding this comment

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

I'm not very familiar with React, so I apologise if I'm misunderstanding anything.

<label>Action</label>
{/* This rule does not support the react-select component. */}
{/* eslint-disable-next-line jsx-a11y/label-has-associated-control */}
<label htmlFor="action-select">Action</label>
<Select
Copy link

Choose a reason for hiding this comment

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

AFAIK, react-select is not just a standard HTML <select>, but would instead use custom markup. <label forr=...> is only supported by HTML form elements, so I'm not sure if this will work. If you haven't already, you could double check this with the Accessibility inspector in the Firefox Dev Tools. For example, you could right click the control, select Inspect Accessibility Properties and check the name property for that element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made some modifications to this that may help. Though it looks like react-select just doesn't have good ARIA support. I may need to switch to another select type component to get this fixed.

JedWatson/react-select#2456

Copy link

Choose a reason for hiding this comment

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

Okay. I can't tell whether this is going to work without being able to see it rendered. I guess we can handle this in a follow up if this doesn't work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Though it looks like react-select just doesn't have good ARIA support. I may need to switch to another select type component to get this fixed.

Removing use of react-select would also fix a few other issues (see bug 1507903).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @edmorley ! :) Thanks for the context. Yeah, I think I will replace it in a follow-up PR.

ui/job-view/CustomJobActions.jsx Outdated Show resolved Hide resolved
ui/job-view/CustomJobActions.jsx Outdated Show resolved Hide resolved
ui/job-view/details/BugFiler.jsx Outdated Show resolved Hide resolved
ui/job-view/headerbars/ActiveFilters.jsx Outdated Show resolved Hide resolved
ui/job-view/headerbars/ActiveFilters.jsx Outdated Show resolved Hide resolved
ui/job-view/headerbars/ActiveFilters.jsx Outdated Show resolved Hide resolved
ui/job-view/headerbars/FiltersMenu.jsx Outdated Show resolved Hide resolved
ui/job-view/headerbars/FiltersMenu.jsx Outdated Show resolved Hide resolved
ui/job-view/headerbars/FiltersMenu.jsx Outdated Show resolved Hide resolved
Copy link
Contributor

@sarah-clements sarah-clements left a comment

Choose a reason for hiding this comment

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

Looks good to me (aside from James' comments). Although, it might be worth changing the buttons to reactstrap components rather than have to manually add the type. However, you've already done the work. :)

@camd camd force-pushed the fix-a11y-buttons branch 2 times, most recently from 5c429b5 to 93c4e90 Compare March 5, 2019 22:57
Copy link

@jcsteh jcsteh left a comment

Choose a reason for hiding this comment

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

My lack of familiarity with React notwithstanding, this looks pretty good to me. Thanks!

<label>Action</label>
{/* This rule does not support the react-select component. */}
{/* eslint-disable-next-line jsx-a11y/label-has-associated-control */}
<label htmlFor="action-select">Action</label>
<Select
Copy link

Choose a reason for hiding this comment

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

Okay. I can't tell whether this is going to work without being able to see it rendered. I guess we can handle this in a follow up if this doesn't work.

ui/job-view/details/tabs/TabsPanel.jsx Show resolved Hide resolved
@camd camd merged commit 0c7be61 into master Mar 6, 2019
@camd camd deleted the fix-a11y-buttons branch March 6, 2019 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants