Fix sidebar a11y issues

[ybnd]

References

• Fixes [Deque Analysis] Admin/Action Sidebar's "serious" accessibility issues #1185

Description

Address accessibility issues with the admin sidebar

Instructions for Reviewers

List of changes in this PR

• Made HTML valid: only <li> elements in sidebar's <ul>
• Combined menu item icons & text into a single <a> element
• Added ARIA attributes
  • aria-expanded
  • aria-disabled
• Improved keyboard UX
  • Expand the sidebar once you "tab into it"
  • Adjusted item spacing so focus outlines are more clear
  • Double checked that items in collapsed submenus can't receive focus

Reviewing

• Run deque a11y check

• The sidebar should look & act the same as on main

  • (Except keyboard navigation, which should be improved as described above)

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

• 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.

[lgtm-com]

This pull request introduces 1 alert when merging 2a10f6b into 5a12f34 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[tdonohue]

@ybnd : Thanks for this PR! Overall, it's much improved behavior, and running an accessibility scan (using Axe DevTools in my browser) shows the main issues are fixed.

However, I noticed a few small issues in testing this PR

1. First, I no longer can open admin menu sections using the keyboard only. This works fine on the demo7.dspace.org site. Here's how to reproduce:
   • Login
   • Pin the sidebar (just to make it easier to see)
   • Using the "Tab" key on your keyboard, highlight the "New" section header in the sidebar.
   • Click "Enter" on keyboard. Nothing happens (on demo site, this section will open)
   • NOTE however that if you click on a non-section link (like "Admin Search"), clicking "Enter" works correctly and brings you to the page. So, it's just the expandable sections that don't work correctly via keyboard only.
2. A tiny issue. If you open one of the sections, like "New" and hover over a sub-link (e.g. "Community" or "Collection"), the title text all says "New menu section". Ideally, all links should have their own title, so that they don't inherit the title of the section. This is more an annoyance though, so if it's hard to fix, we can leave the current behavior as-is.

Beyond that it's looking good. Thanks again!

[ybnd]

@tdonohue thanks, I'll take a look at both issues soon!

[ybnd]

Not sure how useful it is to have it be the same as the title of the link though.
Maybe it would make sense to add an optional tooltip field to the MenuItemModels and use that if it exists, fall back to text otherwise?

[ybnd]

Due to the [routerLink] above keyup.enter events could still propagate to parent expandable sections and toggle them.
This fixes that issue (@artlowel 👍)

[tdonohue]

@ybnd : I gave this another test today. The keyboard issue is fixed with the sidebar!

I did notice though that the other issue still isn't a perfect fix.

1. Open the New menu
2. Hover over "Community" with your mouse, and you'll see the title is now menu.section.new_community.
3. Hover over "Collection" with your mouse, and you'll see the title is now menu.section.new_collection
4. This is now the case for all sub-menu items. They all have missing titles.

The strange thing is that these keys all seem to exist in our src/assets/i18n/en.json5. So, I suspect you are just missing a lookup of the key value?

Beyond that, this is ready to go. Hopefully that's a quick (possibly even one line) fix.

[ybnd]

@tdonohue Oh right, looks like I missed a | translate somewhere, thanks for pointing this out 👍

[tdonohue]

👍 Looks great now, @ybnd ! Thanks for the small updates.