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

Fix accessibility issues #2683

Merged

Conversation

alexandrevryghem
Copy link
Member

@alexandrevryghem alexandrevryghem commented Dec 3, 2023

References

Description

This PR fixes many accessibility bugs, such as missing ARIA tags, absent roles, and incorrect header ordering. I have seperated the fixes by page/type of issue to facilitate the review process.

Instructions for Reviewers

I used the Axe DevTools plugin and the Lighthouse plugin on most of the pages to find and address numerous issues. I did not fix any of the color contrast issues, as these are already being resolved by #2587, except for the ones on the processes, which were not covered in that particular PR.

Checklist

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes ESLint validation using yarn lint
  • My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • If my PR fixes an issue ticket, I've linked them together.

- Added aria-label to the new submission buttons (for repositories with more than 1 entity type)
- Fixed aria-controls pointing to non-existing ids
- Added aria-labels to trash & select bitstream icon
- Added aria-labels ui-switch components (had to upgrade ngx-ui-switch to 14.1.0)
- Fixed aria-controls pointing to non-existing ids
- Fixed bulk-access-browse not having the tab role on it's tabs
- Fixed role="tablist" not having direct role="tab" by adding role="presentation" on the li elements
- Fixed aria-expanded being set to true when collapsed and backwards for BulkAccessBrowseComponent & BulkAccessSettingsComponent
- Fixed aria-controls on HealthComponentComponent, HealthInfoComponent, HealthInfoComponentComponent, HealthPanelComponent pointing to non-existing id
- Fixed the tabs not having the role tab on its tabs
- Fixed aria-expanded being set to true when collapsed and backwards for HealthPanelComponent & HealthInfoComponent
- Fixed role="tablist" not having direct role="tab" by adding role="presentation" on the li elements
- Fixed minor alignment issues
- Added missing aria-label to buttons
- Added missing aria-label to delete button
- Added missing aria-label to delete buttons
- Moved hardcoded translation to translation files
- Fix color contrast issues on buttons
- Fix minor alignment issues
- Added missing aria labels to input and select elements
- Made Process Output keyboard accessible
- Added missing aria-labels to input checkboxes
- Fixed minor css alignment
- Added missing aria-labels to input checkboxes
- Fixed role="tablist" not having direct role="tab" by adding role="presentation" on the li elements
- Added missing aria-labels to input checkboxes
- Fixed multiple tab related accessibility issues
…g for OrgUnits without organization.legalName field
@alexandrevryghem alexandrevryghem self-assigned this Dec 3, 2023
@alexandrevryghem alexandrevryghem changed the title Fix accessibility issues contribute main Fix accessibility issues Dec 3, 2023
@tdonohue tdonohue added the port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release label Dec 4, 2023
@tdonohue tdonohue self-requested a review December 4, 2023 22:19
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Thank you so much @alexandrevryghem ! I went through all 19 commits one by one and verified that each page's accessibility warnings (from the Deque Axe DevTools plugin) are fixed as described. Also checked the code and everything looks good as well.

I've also double checked the referenced tickets, and the issues you referenced all appear to be fixed. In some situations, that essentially "closes" the old ticket. In others, there's still one or two issues on those pages, but I'll make that clearer in the tickets themselves.

Overall, this is a significant amount of accessibility fixes, so it's much appreciated! I'll make sure it is automatically backported to 7.x (via the port to dspace-7_x label), so that these fixes can all be included in 7.6.2.

@tdonohue
Copy link
Member

tdonohue commented Dec 5, 2023

@alexandrevryghem : After finishing my review, I was double checking the referenced tickets. I wanted to note that I **don't believe this PR fixes 470941 in #1191:

"Non-decorative content is inserted using CSS pseudo-elements.". Every button on this page (Upload, download, edit, delete, undo) is represented by an icon, but those icons are inserted using CSS :before and :after pseudo-element.

I believe the bug in that case is that these buttons disappear completely if you turn off CSS. I can still reproduce that using the WAVE plugin for Chrome (if I turn off styles on that Item Edit -> Bitstreams tab. The action buttons on bitstreams still appear empty & it's difficult to understand them.

I think the problem is likely that we aren't adding extra accessibility for these FontAwesome icons like is described here: https://fontawesome.com/docs/web/dig-deeper/accessibility#icons-used-as-semantic-elements-2

In other words, I think we'd need to have an invisible <span> to describe each button (for when CSS is turned off). We can move this to a different PR though, if you'd rather. I just wanted to point out that I think this bug still needs resolving.

…f & replaced authentication dropdown menu link with button

- Also fixed box-shadows not working the same way for all footer buttons/links
@alexandrevryghem
Copy link
Member Author

@tdonohue: I fixed the issue. At first, I addressed it using the fa-sr-only class that you mentioned. However, Wave flagged additional warnings because the display text and the title attribute had the same value. Therefore, I opted to use the aria-label attributes instead (which is also more commonly used in the codebase). This fix also solves the problem when you disable CSS, so I believe this might be a better solution.

@tdonohue
Copy link
Member

tdonohue commented Dec 6, 2023

Thanks @alexandrevryghem ! I've verified today that the latest commits do fix 470941 in #1191. I'll get this merged & ported to 7.x today (and update all the tickets where portions were fixed).

Thanks again so much for all the work to clean up accessibility. This PR cleans up a ton of small issues to get us to zero (non-contrast issue) errors (in Axe and WAVE) on many pages. So, this combined with #2587 is a wonderful improvement!

@tdonohue tdonohue added this to the 8.0 milestone Dec 6, 2023
@tdonohue tdonohue merged commit 8ba14aa into DSpace:main Dec 6, 2023
12 checks passed
@dspace-bot
Copy link
Contributor

Successfully created backport PR for dspace-7_x:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants