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 #1172 #1167 #1171 #1252

Merged
merged 29 commits into from
Jul 19, 2021
Merged

Conversation

atarix83
Copy link
Contributor

@atarix83 atarix83 commented Jun 24, 2021

References

Description

This issue resolve accessibility problems reported in the issue.

Several issues were moved to new tickets, as they could not be resolved quickly:

Instructions for Reviewers

Check if the issues reported are resolved

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 TSLint validation using yarn run lint
  • My PR doesn't introduce circular dependencies
  • 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, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

@atarix83 atarix83 requested a review from tdonohue June 24, 2021 16:49
@tdonohue tdonohue added this to the 7.0 milestone Jun 24, 2021
@tdonohue tdonohue added the 1 APPROVAL pull request only requires a single approval to merge label Jun 24, 2021
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.

@atarix83 : Gave this a test today...While there are some improvements here, there seems to be several issues which were overlooked. Please make sure you are using Deque's free Chrome plugin to check your work on each page...as I found a lot of overlooked "serious" level issues using that tool. Here's what I see that is outstanding:

  1. In [Deque Analysis] Header's "critical" accessibility issues #1167, when I scan the header using Deque's Axe plugin, I'm still seeing this error:
    • IDs/Labels must be unique: Two elements exist having each ID=inputEmail and ID=inputPassword (It is now fixed for ID=dropdownLang and ID=dropdownUser, but still appears for inputEmail and inputPassword.)
  2. Also part of [Deque Analysis] Header's "critical" accessibility issues #1167... I'm noticing the two keyboard navigation issues were not fixed.
    • The "All of DSpace" dropdown is inaccessible to keyboard navigation (tabbing to it and pressing Enter). You must click on it with a mouse.
    • The Language dropdown can be opened via keyboard navigation (tabbing to it and pressing Enter), but you cannot select a different language via your keyboard once it is opened. Additionally, as noted in [Deque Analysis] Header's "critical" accessibility issues #1167, it was suggested that we may want to change this dropdown into a <select> to fix this issue...currently it's a <ul>.
  3. Part of [Deque Analysis] Submission / MyDSpace "critical" accessibility issues #1171, on the MyDSpace page, the "Show" selectbox (on left side) is still not connected to a label/name. So, the Axe Tools still show a "Select element must have an accessible name" error. This is part of issue 3 in that ticket.
  4. Part of [Deque Analysis] Submission / MyDSpace "critical" accessibility issues #1171 on the Submission page, the "Delete" icon (next to multi-valued values) is showing up as having an invalid ARIA attribute. The Deque Axe tools say this cannot have an aria-label as it's a <span>. This might be fixed by turning this into a <button>
  5. Part of [Deque Analysis] Submission / MyDSpace "critical" accessibility issues #1171 on the Submission page, the "Subject Keywords" field has an invalid ARIA attribute of aria-expanded. This isn't valid on an <input> as that's not a collapsible field.
  6. Part of [Deque Analysis] Submission / MyDSpace "critical" accessibility issues #1171 on the Submission page...still seeing an error that the dc.type field is unlabelled. It looks like it has a aria-labelledby="label_dc_type", but there's no label that matches that value.
  7. Part of [Deque Analysis] Submission / MyDSpace "critical" accessibility issues #1171 on the Submission page. the Identifiers selectbox (ISSN, ISBN, etc) also has no label.
  8. Part of [Deque Analysis] Edit Item/Collection "critical" accessibility issues #1172 (number 5 in that ticket) on Edit Collection page, when logo is uploaded, the red trash icon has no label or text.
  9. Part of [Deque Analysis] Edit Item/Collection "critical" accessibility issues #1172 (number 3 in that ticket) the button to reorder bitstreams in Item Edit is not keyboard accessible. I can tab to it, but clicking "Enter" does nothing. If this is hard to achieve we could move it to a later ticket.

There's one inline comment as well (see below)

@tdonohue tdonohue requested a review from artlowel July 1, 2021 14:43
# Conflicts:
#	src/app/submission/sections/upload/file/section-upload-file.component.html
@atarix83
Copy link
Contributor Author

atarix83 commented Jul 8, 2021

@tdonohue

I fixed all except for the following :

Part of [Deque Analysis] Submission / MyDSpace "critical" accessibility issues #1171 on the Submission page, the "Subject Keywords" field has an invalid ARIA attribute of aria-expanded. This isn't valid on an as that's not a collapsible field.

the aria is added by the ngbTypeahead component, I can't avoid it

Part of [Deque Analysis] Submission / MyDSpace "critical" accessibility issues #1171 on the Submission page. the Identifiers selectbox (ISSN, ISBN, etc) also has no label.

I can't find a solution because that element is added by the form library

Part of [Deque Analysis] Edit Item/Collection "critical" accessibility issues #1172 (number 3 in that ticket) the button to reorder bitstreams in Item Edit is not keyboard accessible. I can tab to it, but clicking "Enter" does nothing. If this is hard to achieve we could move it to a later ticket.

this is hard to achieve, it should be done in a specific ticket

@atarix83 atarix83 requested a review from tdonohue July 8, 2021 18:37
@tdonohue
Copy link
Member

@atarix83 : It looks like recent updates have resulted in a test failure in this PR. So, while I can go ahead and review it, we'll need to get the test fixed as well obviously. Thanks!

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.

@atarix83 : It's looking better now, but after testing, I still see a few issues remaining:

  1. [Deque Analysis] Header's "critical" accessibility issues #1167 appears to be fixed EXCEPT FOR ONE ISSUE
    • (ID 469256) Language Dropdown needs to be accessible via keyboard (current you can tab to it, but cannot arrow down to select a different language)....this might be fixed by turning this into a <select> as well.
    • The behavior is improved. I can now tab to a different language, but if I click "Enter" nothing happens. There is no way to select a different language without using your mouse. If somehow this is too hard to achieve, we can split this out.
  2. [Deque Analysis] Edit Item/Collection "critical" accessibility issues #1172 appears to be fixed EXCEPT FOR ONE Issue
    • (ID: 471077) Edit Collection/Community, Logo/file delete button (red trash icon) does not have a name/label. (To reproduce, you have to first upload a logo...then scan the page when you see the red trash can icon)
  3. [Deque Analysis] Submission / MyDSpace "critical" accessibility issues #1171 appears to be fixed (except where you noted)

So, this is very close now. In the meantime, I'll work on moving the "known issues" (ones you've said you cannot fix quickly) into separate tickets so that they can be rescheduled for 7.1.

@atarix83
Copy link
Contributor Author

@tdonohue

[Deque Analysis] Header's "critical" accessibility issues #1167 appears to be fixed EXCEPT FOR ONE ISSUE
(ID 469256) Language Dropdown needs to be accessible via keyboard (current you can tab to it, but cannot arrow down to select a different language)....this might be fixed by turning this into a <select> as well.
The behavior is improved. I can now tab to a different language, but if I click "Enter" nothing happens. There is no way to select a different language without using your mouse. If somehow this is too hard to achieve, we can split this out.

I've fixed it, now it's possible to select a language entry by pressing "enter"

[Deque Analysis] Edit Item/Collection "critical" accessibility issues #1172 appears to be fixed EXCEPT FOR ONE Issue
(ID: 471077) Edit Collection/Community, Logo/file delete button (red trash icon) does not have a name/label. (To reproduce, you have to first upload a logo...then scan the page when you see the red trash can icon)

Both Edit Item/Collection and Logo/file delete button already have an aria-label attribute, I don't kown how to fix otherwise

@tdonohue tdonohue self-requested a review July 15, 2021 14:13
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.

👍 @atarix83 : This looks "good enough". However, I will note that this issue still is definitely not fixed:

[Deque Analysis] Edit Item/Collection "critical" accessibility issues #1172 appears to be fixed EXCEPT FOR ONE Issue
(ID: 471077) Edit Collection/Community, Logo/file delete button (red trash icon) does not have a name/label. (To reproduce, you have to first upload a logo...then scan the page when you see the red trash can icon)

It looks like you attempted to fix the issue, but the "aria-label" attribute you added never appears on the <button> tag when you inspect the page in the browser. I also noticed that same <button> doesn't have a title attribute even though you attempted to add that as well. So, I think there's a bug in the code you added there (see inline comments below as well)

If you have a chance to glance at that again, it might be very quick to fix. Otherwise, I'll merge this on/by Monday, and we'll just have to solve that bug at a later time. Overall, this PR looks good though and looks (to me) like it fixes everything else that it is supposed to. Thanks!

@tdonohue
Copy link
Member

👍 Looks good to me now. All unresolved issues have been moved to separate tickets (see links in updated description above)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge accessibility bug high priority
Projects
None yet
2 participants