IQSS/8321 Accessibility #8322
Merged
IQSS/8321 Accessibility #8322
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
per https://web.archive.org/web/20190116170516/https://www.marcozehe.de/2018/09/22/wai-aria-menus-and-why-you-should-generally-avoid-using-them/ - it doesn't seem like we're using this role as intended in most cases, so I'm doing a blanket removal to see if there are adverse UI impacts or new accessibility issues being reported. With this role, menus like the Edit menu in the files table report an issue (from AXE) with the menu not containing child elements with the right role (i.e. menuitem), and a seemingly spurious 'li elements must be contained in a ul or ol' error. Conflicts: src/main/webapp/file.xhtml
id="versionLink" appears once per row in the versionTable - not sure why that is only an accessibility issue (i.e. not a problem for jsp generation). The id wasn't used anywhere that I can see.
When editing the metadata allowed in a dataset, if you select 'Use browse/search facets from <root dataverse>, all of the disabled entries have insufficient contrast. One option to fix this would be increasing the contrast, however, setting aria-disabled=true also works - disabled entries don't have to have high contrast.
works via PrimeFaces to assign a label to the row selection to avoid a ARIA toggle fields must have an accessible name
Links with the same name should have similar purpose This change uses the i18n names of individual previewers for the preview button, which reduces the ambiguity, but doesn't give the preview in each row a unique title. (I.e. two images still have 'View Image' as the title, but 'Read Text' for a text preview would now be different.) We could add the filename to the title to make this fully unique, either with/without using the previewer names. Note: Deque says this is an issue unless the links are 'intentionally ambiguous' - not sure if that includes assuming the row context is enough.
the existing code had labeling, but the ids labels used to refence inputs were incorrect. This commit also adds a label for CVV fields - (not multival in compound fields which may or may not exist in the wild).
tabindex>0 generated a warning. In this case, the tabindex is on a <span> within an <a> which already causes a tab stop. So I've removed the tabindex altogether, which I think just removes a redundant stop on this element.
Since the support form in in the menu's <ul>, it needs to be wrapped in a <li> to avoid a warning
same as for other role=menu
used an aria-labelledby to associate the a element with the text in the a child element (#userDisplayIfoTitle).
Colors are a complex issue - this commit has a few changes that remove Axe warnings related to links, muted text, and some earning/danger labels. Several of these, e.g. labels come from bootstrap and so will change with bootstrap version. In many cases, Axe can't determine whether there is an issue due to gradient backgrounds and/or overlapping elements.
added css to keep font-size 30px for h1 Login element (which it was when it was an h2 element)
mismatched id/aria-labelledby values
this applies the File Count label to the individual row tds rather than to the header which shows a file count.
the row labels avoid the select check boxes having a warning about no text the labels/message boxes for the file name and directory label inputs were broken the facet for the empty header stops a warning about no screen reader accessible text for that metadata column header
Conflicts: src/main/webapp/filesFragment.xhtml
fixes to label for= values h2 -> h1 for lvel-one heading
qqmyers
added a commit
to QualitativeDataRepository/dataverse
that referenced
this pull request
Dec 22, 2021
scolapasta
approved these changes
Jan 12, 2022
|
Issues/questions:
|
|
For 1), I think I missed adding the css change required to reduce the size of h1 elements (the main headings were h2 but needed to be h1 for accessibility, and the css just resets the sizing back to what the h2 elements had) |
Merged
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.
What this PR does / why we need it: As noted in the issue, this PR includes improvements to accessibility developed at/by QDR.
Which issue(s) this PR closes:
Closes #8321
Special notes for your reviewer: As noted in the issue, the fixes primarily address warnings from Deque Axe. For those, it is often not completely clear what the best fix is, especially given our use of PrimeFaces and Bootstrap 3. Rather than make many PRs, I've tried to make the individual commit messages informative enough that one could find the page involved and the warning(s) fixed in the PR and decide if the fix makes sense.
For the remaining issues, especially those related to PrimeFaces, it does look like some of them have been raised with PrimeFaces already, but it could be worth adding notes/updating existing issues or adding new issues.
There are a few color changes at the bottom of structure.css that override the Bootstrap defaults. I kept these separate as a bootstrap update (as in #6045) would probably mean these could be dropped.
Also note that QDR's issue list at https://github.com/QualitativeDataRepository/dataverse/issues from ~80 to 104 may include additional info including screenshots (from QDR's fork) and descriptions of the problems/solutions. (Issues primarily before 80 involve QDR/s Drupal/SSO components, but were added to the dataverse repo issue list to keep them together and since there are common css/header/footer links - basically one should ignore issues that don't refer to Dataverse specifically and realize that specific color contrast issues may be only for QDR's altered styling.)
Suggestions on how to test this: The simplest option would be to use the Deque Axe plugin and simply verify that warnings that exist on the dev branch go away with the PR. (Note that not all warnings go away, but the PR should not create new ones.) In general, there should not be any significant changes to the UI, i.e. the fixes are intended to help screen readers, mouse-less users, etc. There are a couple - the user page did not have any main title, so I added a title element. Beyond that, I think there are some places where I added a popup title as the way to provide a label for accessibility. Overall, nothing should be obviously different or broken across any area of functionality.
Community members who have other/better tools and/or testers with accessibility training may want to look at this PR as well. (I mentioned this in the latest community meetings - have not yet gotten any feedback though.)
Does this PR introduce a user interface change? If mockups are available, please link/include them here: Two minor changes noted in the testing section. Otherwise, the changes should be only related to accessibility. That does include providing screen-reader labels, etc. where the wording might be of interest - scanning the Bundle.properties file would be a good way to see new strings.
Is there a release notes update needed for this change?: might be nice to mention the overall accessibility improvement, but there is nothing admins have to do/no new config options,etc.
Additional documentation: