-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[Header drawer] fix header locales closing dropdown too soon and ESC behaviour #2472
Changes from all commits
1b8c30f
636e99f
f439347
7c5f04e
df6fa0c
bad3342
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,7 @@ | |
{% render 'icon-close' %} | ||
</span> | ||
</summary> | ||
<div id="menu-drawer" class="gradient menu-drawer motion-reduce" tabindex="-1"> | ||
<div id="menu-drawer" class="gradient menu-drawer motion-reduce"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here I couldn't see where this could be used. I tried to look at where we apply focus in our JS but it didn't seem like we are actually focusing on that element at any moment 🤔 Basically here, when clicking on the scrollbar from the locale dropdown, it would register the click as if it was on this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's an interesting one, I tried to remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm I don't like removing stuff I can't vouch for, but I also couldn't find the purpose for it being on this element. If anything I would expect the tabindex to be on the role="list". |
||
<div class="menu-drawer__inner-container"> | ||
<div class="menu-drawer__navigation-container"> | ||
<nav class="menu-drawer__navigation"> | ||
|
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.
This is to fix the issue where when using the
ESC
key to close the menu drawer, it doesn't remove the overlay.The
aria-expanded
attribute is being toggle based on a click event on line17-19
. But there is nothing taking care of it for theESC
event.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.
Naturally I went to see why the more generic handlers and onKeyUpEscape weren't also managing the esc event, and it appears to be because of the nested details/summary elements specifically involved in the header drawer using
if (summary.closest('header-drawer')) return;
I don't like that this needs to be an exception for a specific element, but since header drawer is already handling other managing other stuff on open/close, it's probably ok.Though I notice the filters drawer always closes the whole drawer on esc, even from a nested details. I'd somewhat expect if we solved the header issue more generically, it would carry over to this (and maybe future use cases) https://screenshot.click/05-40-k2cj6-im39u.mp4
That said, it's probably more important that we fix the header issues so I won't block this fix.
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.
Ok so I found the source of the problem. I pushed a fix for it. I don't think it's ideal in a way as it might not be very future proof and we could maybe have a better logic. But for now it does the trick. Let me know what you think 👍
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.
Yeah I'm imagining targeting those elements without using section-specific classes by checking if they don't have
details
parents or something, so it's completely generic, but I also don't know how feasible that is in practice, so what you have covers us for now.