Fix accessibility issues (part 2) #2717
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexandrevryghem
added
bug
accessibility
medium priority
claimed: Atmire
tdonohue
added
the
port to dspace-7_x
|
Hi @alexandrevryghem, |
alexandrevryghem
force-pushed
the
fix-accessibility-issues_contribute-main
branch
from
fcbf485 to
1bdeabb
Compare
alexandrevryghem
force-pushed
the
fix-accessibility-issues_contribute-main
branch
3 times, most recently
from
9af6aa5 to
c4f1540
Compare
|
Hi @alexandrevryghem, |
- Fixed header ordering - Fixed input field not having a description what it does (because the header isn't always shown I decided to use aria-labels instead of regular labels)
…nt displacement when expanding com/col
- Fixed input not describing itself - Put the items from the dropdown in an ul - Fixed broken aria labels - Removed titles who where identical to the displayed value
alexandrevryghem
force-pushed
the
fix-accessibility-issues_contribute-main
branch
from
c4f1540 to
3b0748a
Compare
alexandrevryghem
force-pushed
the
fix-accessibility-issues_contribute-main
branch
from
3b0748a to
bac2137
Compare
This was referenced Jan 19, 2024
Closed
tdonohue
requested changes
Jan 29, 2024
There was a problem hiding this comment.
@alexandrevryghem : Overall, this looks great! I've verified each commit individually & verified the fixes listed in the description,..and I've only noticed a few minor issues:
- The feedback form no longer works. See inline comment below
- The fix for [Deque Analysis] Browse by Author "serious" issues #1176 seems to be partial? I see the "Results Per Page" in the menu now has an
aria-selected=truenext to the checked value. However, the "Sort Options" section of that same menu does NOT have that next to the checked value. So, I think we just need to update the "Sort Options" in the same way.
Everything else looks great. So once these minor issues can be fixed, I think this can be merged & backported to 7.6.x.
src/app/info/feedback/feedback-form/feedback-form.component.html
Outdated
Show resolved
Hide resolved
- Replaced the h6 tags with the role heading - Gave the gear button the roles in order to be detected as an expandable menu - Replaced the dropdown structure to render a menu of listboxes - Added the aria-selected attribute
- Removed empty label of the select box and replaced it with an aria-label - Fixed empty table header
- Removed empty label of the select box and replaced it with an aria-label - Fixed empty table header
- Removed empty label of the select box and replaced it with an aria-label - Fixed empty table header
- Replaced invisible label in AccessControlArrayFormComponent that was only used for styling with span - Fixed radio buttons not being part of a fieldset - Fixed empty table header
- Hide fontawesome icons for screen readers - Replaced dangling labels with spans and added more descriptive aria label to browse link - Fixed empty label/missing header in edit resource policy table - Added missing comcol logo al text - Refactored dso-edit-menu-section to not display empty link buttons and duplicate titles
- The form used a fieldset tag instead of a form tag
- Hide fontawesome icons for screen readers - Added missing aria-label to metadata field input
- Added the correct autocomplete value - Removed dangling labels, because aria labels already describe those input fields & we can't use ids in this component because otherwise there are duplicate ids on the /login page
- Removed non-existing dropdownMenuButton references - Added appropriate roles to dropdown menus
alexandrevryghem
force-pushed
the
fix-accessibility-issues_contribute-main
branch
from
bac2137 to
ca24713
Compare
|
@tdonohue: Thnx for the feedback! I fixed both issues and amended the fixes for a cleaner history (I only updated the |
tdonohue
approved these changes
Jan 31, 2024
There was a problem hiding this comment.
👍 Thank you @alexandrevryghem ! This looks good now and everything is working as expected. Merging & auto-porting back to 7.6.x
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin dspace-7_x
git worktree add -d .worktree/backport-2717-to-dspace-7_x origin/dspace-7_x
cd .worktree/backport-2717-to-dspace-7_x
git switch --create backport-2717-to-dspace-7_x
git cherry-pick -x 2f5370a08569045239fc73076552d70dfcffe6d8 086c5463a8c1e84145139dd2ad084749d275a62a 3c1243f6f183d12fa39a15984077086f41c264fc d876fadd2a79de5f4919eb10bf92d9e695708a0d b8e040138874757581f178bcdee57f75982bf027 1989a6c04237c39b69de3dc53f02a02b4cbafadb 8ab4f1c074e927838a4e8d0b02b6ffb52fdb9ded 54ef97d6070fab289ee02cd47535d58b0f64047c c30a2eea45c9ad2e6bc773a44c720c7490ea4de6 07e89acab5b2ce718dc7e997047c2a53cf827c6d 7f0264ed1ced6120ad50c1a97313b6f28fd33135 8ad1b4b0d4a88b437feb43b20d3ca0c608d77715 e36bf645f4848171d3301e61155a30dac58f9770 9e29cfb68d617f6316d4882608a50f4e6039bea9 ca24713d6d36afa275bd9d85a486c47931ca2ce5 |
This was referenced Jan 31, 2024
|
Because of a minor code conflict with |
tdonohue
removed
the
port to dspace-7_x
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
/community-list#2697Description
This is a follow up PR on #2683, last time I only used Axe DevTools & Ligthouse. This time I used Wave to find additional errors. I didn't fix the header errors and warnings and also didn't fix the contrast issues because seperate PRs lready exists for those.
Instructions for Reviewers
Instead of making a big list of changes here I seperated the fixes by page/type of issue and used the commit message to describe the fixes to facilitate the review process.
Checklist
yarn lintyarn check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.