fix(theme-classic): make nav dropdowns focusable

[robinmetral]

This PR adds href="#" to the classic theme's DropdownNavbarItem in order to make it focusable.

Here's the issue in action on pnpm.io (notice how tabbing skips the "Support us" item):

Screen.Recording.2021-11-24.at.12.07.55.mov

This solution is still not ideal in terms of a11y since dropdowns should close on ESC and should be navigable with arrow keys, but it makes them usable 🙂

This was already implemented in the locale dropdown, I simply moved the logic over to the DropdownNavbarItem instead.

[facebook-github-bot]

Hi @robinmetral!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[netlify]

✔️ [V2]

🔨 Explore the source changes: d427eb6

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61b0ecdfde88fc000795aab0

😎 Browse the preview: https://deploy-preview-6003--docusaurus-2.netlify.app

[github-actions]

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🟢 Performance	95
🟢 Accessibility	98
🟢 Best practices	100
🟢 SEO	100
🟢 PWA	95

Lighthouse ran on https://deploy-preview-6003--docusaurus-2.netlify.app/

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[Josh-Cena]

Thanks for the fix! It makes sense to me.

Actually, we'd like to do it all since you brought it up. Do you have the bandwidth to kindly address the a11y issue you mentioned, or would you like me to take care of the rest?

[robinmetral]

Do you have the bandwidth to kindly address the a11y issue you mentioned, or would you like me to take care of the rest?

Unfortunately not, this was more of a Boy Scout Rule-style pull request. I'd be happy to help review and test though if you need some a11y support 🙂

---

Update

dropdowns should close on ESC and should be navigable with arrow keys

I think I wrong here: actual menus should have this behavior (ex. it opens when you click it, or press ENTER/SPACE on keyboard), but these navigation submenus should just be tabbed through, like they currently are. Here's a good explainer

So if that's right (☝️), the issue with focusing the top-level menu item is the only issue. I can address your comment on this branch and it should be ready to go @Josh-Cena 🙂

[Josh-Cena]

Thanks, indeed we don't need to navigate with arrow keys / esc; however, focusing to a dropdown still doesn't expand its children and make them accessible with tabs. I do think that's worth fixing.

Edit. Actually, forgive me, they are accessible with Enter🤦‍♂️ Not sure if it's the most straightforward UX though

[robinmetral]

You're right, from what I've seen a best practice here is to make sure that the subnav opens on focus and allows tabbing through

[robinmetral]

We should probably do it since this is a subnav and not a menu, but the downside is that we'll "force" keyboard users to e.g. tab through all versions of the docs in a doc versions dropdown.

The alternative is to make this a menu, but the top-level item could not be a link anymore (it should just be a toggle in that case)

[Josh-Cena]

Well—I see the point. In that case using a separate key to expand the dropdown makes sense. I don't think there's any convention about visual cues that an element is Enter'able, esc'able, or allowing other interactions

[robinmetral]

Exactly, the main issue is what you're mentioning: there's no pattern to announce that there are subnav items, so screenreader users wouldn't find them (sighted keyboard users might, via the chevron icon 🔽).

I can think of several ways to address it:

• allow tabbing through the sublist without having to press enter (simplest)
• make the top-level item into a button that says something like "See versions" (so it's explicit that it can be entered)
• keep the top-level item link, but make the "🔽" icon focusable with an accessible label
• ensure the list of docs versions is accessible elsewhere (not a fan of this one because it creates a big usability gap between keyboard users and mouse users)

TBH I'd recommend going for the first option here, since there would be no change for mouse users and the PR is already an improvement for keyboard users.

So tl;dr:

• the PR in its current state already makes dropdowns focusable, which is already good (and thanks for catching this regression 🙌)
• however keyboard users would need to use enter to move focus into the nav sublist and be able to tab through it, which they might not find
• we could use focus-within to make sure that the nav sublist opens when the top-level item is focused

Thoughts?

[Josh-Cena]

1. I wouldn't like dropdowns to be expanded on hover for the reason you mentioned: it forces people to go all the way to get to the next dropdown. Using enter seems cool to me.
2. The docs version list is indeed "available elsewhere": https://docusaurus.io/versions

It's quite late here and I'm going 🤯 So I'll be revisiting this tomorrow. Thanks again for working on this!

[robinmetral]

Haha sorry for the late-night brain workout—thanks a lot for your quick response here and let me know if you need further inputs from me tomorrow

[slorber]

Hey

Agree that we may not do the right thing regarding a11y here

But this PR makes it consistent with other existing dropdowns, so that seems good enough to merge.
We can improve the a11y on all dropdowns at once in another PR

Thanks