Header Accessibility Fix: Allow enter key or spacebar to expand or contract menu based on current active status

[tdonohue]

References

Small improvement to the fixes to #1167 in PR #1252

Description

Deque retested those header fixes, and found one issue was only partially fixed.

• If you use a Keyboard to select the "All of DSpace" menu in the Header, you can open it by pressing "Enter"
• However, once it is open, there is no way to close it using only the keyboard.

This PR fixes that issue by ensuring pressing "Enter" will open/close that menu based on the current status.

Instructions for Reviewers

• Review code changes
• Test keyboard interaction of header menu
  • Tab to the "All of DSpace" menu, press "Enter" (or spacebar). It should open
  • Click Tab again, reselecting "All of DSpace". press "Enter" (or spacebar) again. Now it should close.
  • Verify you can still tab to a submenu (e.g. "By Subject") and press "Enter" to go to that browse by page.

[artlowel]

Thanks @tdonohue!

This does what it says, and can be merged as is.

But in the admin sidebar menu (and most other places where we added these keyboard commands, e.g. opening and closing the search facets) we made both space and enter do the action, so for consistency it would be good if you could add space as well

[tdonohue]

@artlowel : Thanks for the initial review. Since it wasn't much work, I added support for spacebar as well. Just as a note though, at least in Chrome spacebar doesn't work as well as Enter key...as click "spacebar" on any menu item scrolls you down to the bottom of the page (while also now opening/closing the menu). This seems to be the same behavior as on demo7.dspace.org though & I didn't have time to try and debug it (as it seems unrelated to this larger accessibility issue)

[artlowel]

@tdonohue @ybnd had that same scrolling issue in a recent PR. IIRC the $event.stopPropagation() here fixes that

[tdonohue]

@artlowel : Unfortunately, that suggestion doesn't appear to work in this scenario. I tried it locally, and changed this line to:

(keyup.space)="$event.stopPropagation(); isActive ? deactivateSection($event) : activateSection($event)"

That seems to have no effect for me. I can still open/close the menu using the spacebar, but anytime I do so, it auto-scrolls me to the bottom of the page.

I think there may be a larger issue at play here, as anytime you hit the spacebar while highlighting any part of the header (even just with the header logo highlighted) you are scrolled to the bottom of the page.

So, I think we may want to treat this as a separate issue. It sounds like some sort of default behavior needs to be changed for the entire header, and it's not specific to this single menu option. Maybe we simply leave this PR as-is and open a separate ticket about the spacebar scrolling to the bottom of the page?

[ybnd]

@tdonohue AFAIK "spacebar to scroll down" is the default behaviour for most browsers though, so I think that's to be expected if the components in play here don't override defaults for keyup.space?

On any long page space / shift+space should just work the same as pagedown / pageup
I think the only way I know to disable this would be to explicitly handle space events for all controls we want to work ~ "space to navigate"...

I did solve a similar thing though, I'll go over that other PR tomorrow & see if there was anything more to it than that $event.stopPropagation() bit.

[ybnd]

Quick thought right now: could be that the callbacks in #1295 also included $event.preventDefault also, the current ones don't

[tdonohue]

@ybnd and @artlowel : I appreciate the help, but still no luck. I've gone ahead and pushed a commit with what I've tried -- adding both event.preventDefault(); to the methods and $event.stopPropagation(); to the (keyup.space). Still no luck. Menu still responds fine to spacebar, but I'm also always scrolled to the bottom of the page.

I'm willing to try a few more ideas if you think this should be solveable. But, at the same time, this wasn't reported as an accessibility bug by Deque (they only reported issues with using the Enter key to open/close menus) -- so, it's really a question of how much time we spend on trying to get spacebar to also work.

[tdonohue]

@ybnd and @artlowel : Found a bit of time to dig into this today. It turns out it was something simple... keyup vs keydown. I stumbled on some hints that most browsers implement the scrolling on spacebar keydown. So, I tried just switching all my code to use keydown instead, and it worked. Both Enter & Spacebar can control the menu & the scrolling no longer occurs. I did have to ensure the component methods call event.preventDefault();, but the call to event.stopPropagation(); was unnecessary (so I removed it).

If one of you could give this a test on your end, I'd appreciate it. I think it's probably ready to merge now.

[artlowel]

@tdonohue I tested it again, and while it works properly now to open and close dropdowns, I can no longer use enter or space to go to a link inside a dropdown. That does work on the demo site. Perhaps one of those prevent defaults interfering?

[tdonohue]

@artlowel : Good catch. It seems the event.preventDefault() MUST be on keydown (to stop the scrolling which takes place on spacebar keydown in browsers), BUT the rest of the action need to be on keyup (placing them on keydown disables links, and I cannot figure out a way around it). This is a bit strange to me still, but the latest commit seems to finally work. Please give it a try.

[artlowel]

That works!

Thanks @tdonohue!

[tdonohue]

Merging with one approval