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

Feature/464 check accessibility #823

Merged
merged 7 commits into from
Mar 28, 2023
Merged

Conversation

maxdiebold-erg
Copy link
Collaborator

Related Issues:

Main Changes:

  • Updated several HTML element attributes in response to feedback from accessibility tools.
  • Switched to lazily loaded routes to cut initial bundle size from ~3,300 KiB to ~1,500 KiB.
  • Fixed issue of new surrounding features layers not appearing in legend.
  • Switched to single color for scatter plot dots on Monitoring Report page.
  • Added ability to focus and trigger map widgets with keyboard.
    • Updated layer labels in the surrounding widget so that they are read by screen reader when focused.
    • The visible state of the Add/Save Data Widget was not updating properly: I altered it so the trigger re-renders when the visibility state changes, so that it has the latest value of the state variable. This causes the trigger to re-render more often, but I believe it will also aid in the switch to React 18 in the future.
    • There is more that could be done to improve keyboard navigation, but these changes at least make it possible to operate the widgets with a keyboard.

Steps To Test:

  1. Navigate around various pages of the app and confirm that the lazily loaded routes still function as expected.
  2. Go to the Community page, open the Surrounding Features widget, and toggle on the "Dischargers" layer.
  3. Confirm that the "Permitted Discharger" item is visible in the legend.
  4. Toggle on the "Dischargers" layer in the layer list.
  5. Confirm that only one "Permitted Discharger" item is visible in the legend.
  6. Repeat steps 2 - 5 for the "USGS Streamgages" layer.

NOTE: The monitoringLocationsLayer and surroundingMonitoringLocationsLayer legend items are separate, because their cluster size varies between the layers.

  1. Test navigating around the map canvas with the keyboard and triggering widgets with the "Enter" or "Space" key.

TODO:

  • For the new items, there were additional accessibility issues, but they relate to other issues that were already present across the application: the "ARIA IDs are not unique" warnings all point to instances where the @reach/tabs library is used. This is because each ARIA ID is only unique within a particular Tabs group, but we use multiple such groups on a single page. This issue has been raised in Duplicate IDs when using multiple tabs instances reach/reach-ui#947, but it hasn't yet received a response, and, according to Reach UI is not currently maintained reach/reach-ui#972, the library currently isn't maintained. We should check back on this issue, but it might not be fixed anytime soon.
  • To improve keyboard navigation across map widgets, we should consider automatically focusing panels when they are opened, and returning focus to the trigger when they are closed. Additionally, to match the behavior of Esri widgets, we may want the Esc key to close focused panels.

Copy link
Collaborator

@cschwinderg cschwinderg left a comment

Choose a reason for hiding this comment

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

I'm still reviewing this PR, but I wanted to give you a heads up on some style issues I found.

  • The popups on the waterbody report, monitoring report, and plan summary pages are a different style than the rest of the app (see screenshots below)
    image
    image
    image

  • Tab styles are messed up on the about, state, tribe, and national pages
    image
    image
    image

@maxdiebold-erg
Copy link
Collaborator Author

@cschwinderg Ouch, I guess I forgot to check the other pages. I'm sure that has to do with the lazy loading. I only thought to add that because it was mentioned in the Lighthouse analysis, but it's definitely not necessary for this release and can be rolled back for now.

Copy link
Collaborator

@cschwinderg cschwinderg left a comment

Choose a reason for hiding this comment

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

Good catch on the keyboard navigation issues!

I left a couple more suggestions below related to the Add & Save Data Widget.

app/client/src/components/shared/MapWidgets.tsx Outdated Show resolved Hide resolved
@cschwinderg
Copy link
Collaborator

For the new items, there were additional accessibility issues, but they relate to other issues that were already present across the application: the "ARIA IDs are not unique" warnings all point to instances where the @reach/tabs library is used. This is because each ARIA ID is only unique within a particular Tabs group, but we use multiple such groups on a single page. This issue has been raised in reach/reach-ui#947, but it hasn't yet received a response, and, according to reach/reach-ui#972, the library currently isn't maintained. We should check back on this issue, but it might not be fixed anytime soon.

I don't think we should do anything about it this release, but we should switch to a different library (probably USWDS) or even make our own tab component. This will probably be a HMW V3 issue.

@sonarcloud
Copy link

sonarcloud bot commented Mar 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Collaborator

@cschwinderg cschwinderg left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes!

@cschwinderg cschwinderg merged commit 83e64e1 into develop Mar 28, 2023
@cschwinderg cschwinderg deleted the feature/464_check-accessibility branch March 28, 2023 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants