Make sure hamburger menu is accessible by keyboard navigation

[ktbee]

Hello! Thank you for making such a lovely, clean looking theme.

I am currently using Ed for my blog and noticed that I could make some improvements to the accessibility of its hamburger menu. With the current version, you can't navigate to the menu using the keyboard. If you're not familiar with this idea, I'm using tab navigation and based my changes on these sorts of guidelines.

Proposed changes in this PR:

• Replace the menu label element with a button
• Add ARIA attributes to the menu button to support screen readers (ie aria-label and aria-expanded)
• Move the focus to the first link in the sidebar menu once it has finished sliding out (on transitionend).

I also simplified the sidebar toggle JavaScript by a fair amount, so please let me know if I missed something in those changes. My goal was to make sure the theme still looked and behaved the same while allowing for tabbing to the menu. You can see these changes in effect on my blog:

https://blog.katiebroida.com/

Chrome has the most obvious focus highlighting out of the box, so you may want to tab through it in Chrome.

Thanks again for this theme! Looking forward to your thoughts on this.

[karlstolley]

Thank you so much for your pull request, Katie. I'll have a look at this in the next day or two and get back to you. The keyboard navigation issue is a big problem, and I really appreciate your concern about it.

[karlstolley]

Hi again, Katie.

This looks like a good start. I’ve found a few issues that need to be cleared up, and a few while-we’re-at-it enhancements.

Let me know if you’re interested in running with any of these; otherwise I will fork your branch and incorporate your work before merging it fully into Ed’s master branch.

1. One advantage (let's say trade-off) of the original <label> element is that, while not keyboard-accessible, it does continue to function in the absence of JavaScript. The <button> element of course has no functionality without JavaScript behind it. So one option here would be to inject the <button> element with JavaScript, which ensures the scripting the button requires is available. (More on that below)
2. You’re right that <button> belongs semantically up in <header>, but for the 1.x version of Ed that I’ve been working on, I’m trying to leave all of the source order in place, just to accommodate any custom modifications users have made to the header, etc.
3. Your use of aria attributes is fantastic. It would be good to also have the aria-hidden attribute inserted and toggled via JavaScript on the navigation bar, along with an aria-controls attribute on the toggler itself that references the sidebar ID on the nav bar.
4. Regarding your JavaScript enhancements: I’m a huge fan of the arrow-function syntax, but Ed is for the time being trying to accommodate IE 9, so a vanilla function expression would be preferable.
5. One breaking change introduced by your JavaScript, by setting addEventListener on toggle, is that only the toggle button can close the navigation menu. While somewhat convoluted, the existing JavaScript permits clicking anywhere in the sidebar to close it, which is a somewhat more user-friendly option for users with pointing devices.
6. A complete JavaScript enhancement (that is, injecting <button> rather than hard-coding it into the HTML) could mean getting rid of the checkbox input entirely, rather than extending its hacky use of the :checked pseudo-class all over the CSS. But that would require some additional style rewrites, and possibly some duplication if the JavaScript-less CSS-only toggle remains.

For my own part, I’m going to do some research and see if anyone has come up with a way to make a <label> element both focusable (which adding tabindex="0" seems to achieve) and keyboard accessible in the absence of JavaScript. The closest I’ve come to the latter is to set an accesskey attribute on the checkbox input itself, but accesskey introduces a bunch of problems on its own: WebAIM: Keyboard Accessibility - Accesskey

[ktbee]

Thanks for taking a look @karlstolley! I'd be happy to keep going with these changes, if you don't mind it taking me a few days. Some thoughts on your points:

One advantage (let's say trade-off) of the original <label> element is that, while not keyboard-accessible, it does continue to function in the absence of JavaScript. The <button> element of course has no functionality without JavaScript behind it. So one option here would be to inject the <button> element with JavaScript, which ensures the scripting the button requires is available. (More on that below)

Oh good point, I hadn't thought about how the menu could work without a button. That's clever using a label and checkbox instead.

You’re right that <button> belongs semantically up in <header>, but for the 1.x version of Ed that I’ve been working on, I’m trying to leave all of the source order in place, just to accommodate any custom modifications users have made to the header, etc.

Good to know, I'll experiment and see if I can get the styling and tab navigation to work outside of the header. The reason I moved the button was because I was finding that some of it's positioning styling (I believe absolute) was preventing tab navigation. Though now that I'm looking at my changes again, I didn't remove that style from the button. Perhaps this change isn't necessary!

Your use of aria attributes is fantastic. It would be good to also have the aria-hidden attribute inserted and toggled via JavaScript on the navigation bar, along with an aria-controls attribute on the toggler itself that references the sidebar ID on the nav bar.

Good point! I'll add those

Regarding your JavaScript enhancements: I’m a huge fan of the arrow-function syntax, but Ed is for the time being trying to accommodate IE 9, so a vanilla function expression would be preferable.

Whoops, I've been spoiled by Babel. I can make that a regular function.

One breaking change introduced by your JavaScript, by setting addEventListener on toggle, is that only the toggle button can close the navigation menu. While somewhat convoluted, the existing JavaScript permits clicking anywhere in the sidebar to close it, which is a somewhat more user-friendly option for users with pointing devices.

Ah gotcha, that makes a lot of sense I had a feeling I was missing why the event was on the document and checking the target. I can re-add that functionality.

A complete JavaScript enhancement (that is, injecting rather than hard-coding it into the HTML) could mean getting rid of the checkbox input entirely, rather than extending its hacky use of the :checked pseudo-class all over the CSS. But that would require some additional style rewrites, and possibly some duplication if the JavaScript-less CSS-only toggle remains.

I had thought about that as well, but didn't want to wade too far into CSS changes since I didn't feel like I had a good idea of Ed's CSS system in my head. If you don't mind, I think I will skip this enhancement :) Also, I might be good to keep the checkbox. I like the idea of there still being some interactivity even without JavaScript.

[ktbee]

@karlstolley I made most of your requested changes, if you don't mind taking another look. I also added a small amount of focus state styling to the menu toggle since I got tired of straining to see if it was focused in Firefox. I can take it out though, if it's something that might interfere with the Ed theme. A couple of follow things/questions I wanted to point out:

• I found I was able to move the toggle button out of the header and place it before the header in the HTML. I had forgotten that the actual reason I had moved it was because previously you couldn't tab to it until you got to the very end of the page. Now it's the first thing you tab to, but it is awkward to close the menu again because you can't tab back to the toggle button easily. One thought I had was to add an event listener for the escape key and close the sidebar when it's emitted. I think that's a common convention and discoverable for keyboard users, let me know if you would like me to add this!

• Did you have any luck looking into how to make the label element focusable by keyboard navigation? I saw another possible technique we can try, which is using an anchor link instead and style the sidebar with the :target pseudoclass. I'm curious what you think of that option, it would be nice to have the sidebar work without JS and not have to duplicate it's functionality with JavaScript and a button.

[karlstolley]

@ktbee awesome. Give me a few days to have a look. Have you pushed all of your commits for this work? I just see the one, 2b6aaa -- I just want to make sure I'm not missing anything

[ktbee]

@karlstolley whoops sorry to not clarify earlier, I have indeed pushed all of my changes up! There are two commits, cfe87db and 2b86aaa. They are just buried in between my comment essays :)

[karlstolley]

@ktbee I got sidetracked this week. Just a ping to let you know I've not forgotten about your contribution.

[ethanwhite]

@ktbee - just wanted to say thanks for this PR. I'm using it to solve the same issue on a site using Poole which uses a similar sidebar design.