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

Minor Accessibility Fixes & Enable accessibility scan on more pages #2468

Merged
merged 9 commits into from
Aug 30, 2023

Conversation

tdonohue
Copy link
Member

Description

Minor dependency updates to support accessibility fixes:

  • Upgrade to latest Cypress (only used for e2e tests) and axe-core (used for accessibility scans in e2e tests)
  • Upgrade to latest ng2-nouislider and nouislider (both used for date filter search slider) to fix accessibility issues

Accessibility issues fixed:

  • Fixed ARIA tags on date filter search slider (allows for selecting date ranges in filters on search page & mydspace). Added missing aria-label values to slider handlebars. Fix is verified by changes to my-dspace.cy.ts and search-page.cy.ts as we no longer have to exclude the nouislider from accessibility tests.
  • Fixed "heading order" accessibility issue in two places:
    • In Search filters - verified by changes to my-dspace.cy.ts and search-page.cy.ts
    • On /community-list page - verified by changes to community-list.cy.ts
    • In both situations, I had to apply minor CSS changes to ensure the pages still look OK. For example <h5> tags were being used based on their style even though an h2 or h4 provided better heading ordering.
  • Removed exclusions from e2e accessibility tests where the accessibility issue is already fixed.
  • Added e2e accessibility tests for several pages:
    • Full item page
    • Login module
    • Page not found page

Instructions for Reviewers

  • Verify all e2e tests pass (they should). All these changes have corresponding accessibility tests in our e2e tests. So this proves these changes are accessible.
    • You can also review the changes to the e2e tests, but all is just ensuring additional accessibility tests run properly.
  • Verify no major differences in style or behavior of following pages:
    • Community List (/community-list)
    • Item pages (/items/[uuid])
    • Search filters (especially Date filter / slider) on /search or /mydspace

@tdonohue tdonohue added bug accessibility 1 APPROVAL pull request only requires a single approval to merge testing framework Related specifically to Unit or Integration (e2e) Tests port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release labels Aug 29, 2023
@tdonohue tdonohue added this to the 7.6.1 milestone Aug 29, 2023
@tdonohue tdonohue self-assigned this Aug 29, 2023
@alanorth
Copy link
Contributor

alanorth commented Aug 30, 2023

The changes from <h5> to <h4> in the search filters cause the filter labels to be almost comically large (and in our case one starts wrapping).

Before and after:

Search filters before Search filters after

More seriously, the date slider doesn't work. It seems to be fatally bugged. Here's a recording:

date-slider.mp4

Alternate link for video since it's not showing for me here: https://imgur.com/a/wwzuJqk

Before testing I removed my npm_modules directory and did a yarn install to make sure everything was up to date.

@tdonohue
Copy link
Member Author

@alanorth : Thanks for your testing! Both those issues sound like browser caching issues. I forgot to mention that this PR changes CSS, which means you will need to do a hard refresh/reload in your browser (Ctrl+Reload button) after installing the PR. Our UI / Angular will cache CSS files for a period of time... so if you attempt to run the PR with old CSS, you will get odd behavior.

The <h4> issue you can see is fixed in the updates to _global-styles.css in this PR... I swapped the old h5 facet style over to h4 here: https://github.com/DSpace/dspace-angular/pull/2468/files#diff-6383630facc57c68afedd870483806bd21bbf33e27c6bbfa6755ed43d203e87fL20

The slider issues are also CSS. I saw these briefly myself as our _vendor.css now loads an updated nouislider.min.css from a new location...if the old version is cached, the slider won't work. A hard refresh again fixes this issue.

If you have a chance to re-test, I think a hard refresh will solve both issues. (I should also note this sort of caching can occur with any PR which changes CSS. It's not unique to this PR, so if you are testing other PRs that modify CSS, it's good to get in the habit of doing a hard refresh anytime you notice something odd... it could be just that your browser has cached the old CSS.)

@tdonohue tdonohue requested a review from alanorth August 30, 2023 11:47
@alanorth
Copy link
Contributor

alanorth commented Aug 30, 2023

@tdonohue ah right! A shift-refresh fixed the broken date slider, and the <h4> style change had to be mirrored in my theme's _global-styles.scss. Now looking and working well!

The community list, item view, and search pages are all working as expected for me now.

@alanorth alanorth merged commit 02f81f1 into DSpace:main Aug 30, 2023
10 checks passed
@dspace-bot
Copy link
Contributor

Successfully created backport PR for dspace-7_x:

@tdonohue tdonohue deleted the cypress_accessibility_testing branch August 30, 2023 14:50
@tdonohue tdonohue modified the milestones: 7.6.1, 8.0 Aug 30, 2023
@tdonohue tdonohue removed the port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release label Aug 30, 2023
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 testing framework Related specifically to Unit or Integration (e2e) Tests
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants