-
Notifications
You must be signed in to change notification settings - Fork 1k
Refactor dropdowns to follow WAI recommendations #3287
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
@nlhkabu The indentation is kinda wonky in the HTML here, maybe you're using tabs and not spaces? Could you fix it? |
Fixed @di - thanks for catching it. I think it was because I copy pasted from the WAI page :( |
Hi @nlhkabu ! I took a look at this one in Chrome (Version 65.0.3325.124 (Official Build) beta (64-bit)) and it seems like the bottom of one of the menus is getting chopped off when it interacts with the bottom of the page. (The dropdown menu on the top menu item looks fine.) The same cutoff of the bottom of the menu happens in Firefox also. |
@nlhkabu I'm not sure if I really understand what needs done here. The tutorial is suggesting that we make the target content appear on a click rather than a hover, which means that we'd have to a) remove the current hover CSS and b) add some JS to handle the click event instead. However this would not degrade well if folks don't have JS enabled, as they would have no way to open the menus. Instead we can bind to a I'll push a commit which does this, and you can tell me if that is what you're looking for. |
ad99b59
to
630136e
Compare
Hi @di. Reading over the tutorial - I don't think we need to remove the hover CSS, rather, we can have both the CSS and JS (with click) working in parallel. A couple of problems I found (I'm not sure if they're on my side or yours)
@lgh2 I just pushed a fix for the cut-off problem. It would be great if you could help us to continue to test this PR :) The criteria is:
|
677e153
to
7777eeb
Compare
Ah yeah, I think my selector might not be catching this.
OK, finally figured this out. Reading https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_button_role was a tremendous help. We need to refactor how we structure these dropdown menus a bit. Only I made the necessary refactoring for one such dropdown in 7777eeb, the main things are:
This actually looks pretty good to me (aside from the buttons in the dropdown having default styling): @nlhkabu Does that all make sense? I think if you take the same approach with the logged-in user dropdown, it should just work. Can you do the same refactoring elsewhere, style the dropdown buttons, and ensure I didn't totally break anything? 🙂 |
Heads up @nlhkabu I figured out how to do this myself so I think I can wrap this up on my own! |
7777eeb
to
61ca968
Compare
@nlhkabu I think this is pretty much done! The only thing I couldn't figure out was how to properly set the (You'll need to tab to it to get it to have |
ah great @di! You beat me to it!!! Let me pull down and tweak and review 🕺 |
@nlhkabu I think https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_button_role indicates that we can just give FWIW, the reason that links are wrapping buttons and vice versa is because Firefox does support tabbing to links. Chrome supports tabbing to links and buttons, hence all the |
I don't think adding
It appears that the 'not tabbing to links' issue is actually a mac system setting: https://stackoverflow.com/questions/11704828/how-to-allow-keyboard-focus-of-links-in-firefox I think it is safe to say that any user who wants to use their keyboard extensively would have updated this setting. I therefore think we should revert this PR to use links where appropriate :) |
@di - I've pushed a commit (with styling) as per my previous comment. The top dropdown is almost working perfectly for me. The only thing is that when I tab all the way through the list (and then out of the dropdown) the dropdown remains open. For the 'options' buttons, I am getting a JS error: |
Hi @nlhkabu ! I tested the following things from your comment up thread: Use only your keyboard to navigate around the page Can you open each dropdown only using your keyboard? Can you access each of the sub menu items in the dropdown using only your keyboard? Now using your only mouse - does each dropdown still work as expected? |
Thanks @lgh2 this is consistent with what I am seeing on my side :) |
@nlhkabu I'm seeing merge conflicts here in case you want to resolve those :) |
@brainwane I think this is on my plate right now, I'll be working on this post-launch. |
4e3effd
to
acd7157
Compare
@nlhkabu I squashed all the previous commits here to make it easier for me to resolve the merge conflicts, there was a lot. Sorry if that causes any trouble!
I think I've resolved this issue (at least I can't reproduce it now).
I think this should be the expected behavior? E.g. on Github, you can tab out of the menu and it stays open, and the only way to close it is to go back to the parent and "click" it again with the keyboard. The only thing remaining I think is that sometime since we last worked on this the focus styles became transparent (but only when using the keyboard): |
acd7157
to
cdd895b
Compare
bedaf5f
to
6cc2f72
Compare
Works for me!
Updated :) |
🎉 thanks for your work on this @di - we got there in the end 🕺 💃 |
Closes #3194, closes #2127.
@di, we now just need to add the JS, as per "approach 1" here: https://www.w3.org/WAI/tutorials/menus/flyout/
Note that our class is not called
has-submenu
as in the example, it's calleddropdown__trigger