-
Notifications
You must be signed in to change notification settings - Fork 211
Make pure menu dropdowns keyboard accessible #568
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
Conversation
This is an awful lot of JavaScript... Is there a way to do this with CSS instead? |
I'm fine adding JavaScript to improve accessibility (if it can't be done with HTML itself), but it should be in a separate JavaScript style: we'll soon want to add a CSP, and if we do that we won't enable inline JS. |
Also, it's not working really well... |
Parts of this — binding to the escape key and arrow keys, and focus trapping — are impossible with just CSS. There are some nasty hacks that you can do with checkboxes to at least make opening the menus possible, but it would require us to modify the markup and basically fork Pure.css to do it. So, to get something truly good, you need some JavaScript anyway.
... like WHAT?! |
I see. I took a look at the purecss docs and it looks like they have a recommended javascript solution: https://purecss.io/js/menus.js. Could we use that instead instead of writing something from scratch? |
Doesn't work right with the crate mega-dropdown. That's why I wrote a custom one. |
@pietroalbini I pushed a commit to move the menu script to a separate file. It's also minified. |
It's still not working for me? |
@GuillaumeGomez Okay, after running through a few more tests, I think I've found the nature of your problem. Probably. I hadn't been using the Docker scripts (actually, I've just been using the Firefox Developer Tools to inject my script into the actual docs.rs site). Now that I tried using them, I found that I had failed to update the Dockerfile. I've fixed that. I've literally run it on my local machine and it works. If it still isn't working for you, could you please check your Developer Console in your browser? |
No, I meant the JS code. I ran it into my browser directly. |
Apprently, the regular footer is not used by the rustdoc pages.
So when you do that, is anything written to the console? What browser are you using? Are you just mousing over, clicking, or pressing Enter on the menu button? I would like you to write detailed instructions for reproducing your problem. |
No, the code is fine apparently. But when clicking on the dropdown menu, I randomly get a grey background all over the page and I have to click again to me it disappear. That's definitely not very nice. :-/ |
If other people also dislike it, I'm fine with removing the backdrop thing. Does anybody else have an opinion on it? |
I'm not a fan of focus-stealing or modals in general. |
Based on feedback from multiple people.
Okay, the grey background is now gone. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few comments, @GuillaumeGomez may have more (he knows JavaScript better).
It's not worth adding a dependency for it.
This fixes a regression added by the keyboard-accessible JavaScript menus, where clicking the link opened the menu instead of the linked page. It checks if the link points somewhere, and lets it behave normally if it does. Also adds keyboard handling for the space key, so you can still get to the menu.
Did you try with |
If they click the backdrop, give focus to the rustdoc container, and otherwise give the focus to the menu button itself.
2a47d4b
to
d195ea6
Compare
What part of this functionality would I use |
By the way, just tested this locally, and it works great! Thanks for working on this! |
@notriddle The point was to make it work with the keyboard, no? With |
It already is selectable using TAB. The problem is making the menu open when you press DOWN on it. |
Looks great to me! @GuillaumeGomez What's this about tabindex? |
Just deployed, this looks great :) |
Fixes #67