Skip to content
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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Apr 6, 2023
Merged

[Header drawer] fix header locales closing dropdown too soon and ESC behaviour #2472

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1b8c30f
fix header locales when trying to close dropdown
ludoboludo Mar 27, 2023
636e99f
testing solutions
ludoboludo Mar 29, 2023
f439347
fix menu drawer not closing properly when ESC and changed approach to…
ludoboludo Mar 30, 2023
7c5f04e
Merge branch 'main' into fix-drawer-locales-closing-issues
ludoboludo Apr 5, 2023
df6fa0c
Fix ESC on filter drawer, closing the whole thing instead of just a s…
ludoboludo Apr 5, 2023
bad3342
address review comment
ludoboludo Apr 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion assets/global.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ document.querySelectorAll('[id^="Details-"] summary').forEach((summary) => {
event.currentTarget.setAttribute('aria-expanded', !event.currentTarget.closest('details').hasAttribute('open'));
});

if (summary.closest('header-drawer')) return;
if (summary.closest('header-drawer') || summary.closest('menu-drawer.mobile-facets__wrapper')) return;
summary.parentElement.addEventListener('keyup', onKeyUpEscape);
});

Expand Down Expand Up @@ -418,6 +418,8 @@ class MenuDrawer extends HTMLElement {
document.body.classList.remove(`overflow-hidden-${this.dataset.breakpoint}`);
removeTrapFocus(elementToFocus);
this.closeAnimation(this.mainDetailsToggle);

if (event instanceof KeyboardEvent) elementToFocus?.setAttribute('aria-expanded', false);
Copy link
Contributor Author

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 line 17-19. But there is nothing taking care of it for the ESC event.

Copy link
Contributor

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.

Copy link
Contributor Author

@ludoboludo ludoboludo Apr 5, 2023 •
edited
Loading

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 👍

Copy link
Contributor

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.

}

onFocusOut() {
Expand Down
2 changes: 2 additions & 0 deletions assets/localization-form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ if (!customElements.get('localization-form')) {
onContainerKeyUp(event) {
if (event.code.toUpperCase() !== 'ESCAPE') return;

if(this.elements.button.getAttribute('aria-expanded') == 'false') return;
this.hidePanel();
event.stopPropagation();
this.elements.button.focus();
}

Expand Down
2 changes: 1 addition & 1 deletion snippets/header-drawer.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 🤔
I might be missing something so let me know if you catch anything.

Basically here, when clicking on the scrollbar from the locale dropdown, it would register the click as if it was on this menu-drawer element and trigger the focusout event which closes the dropdown. Removing that tab index fixes it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an interesting one, I tried to remove tabindex="-1" from children elements of this element as well and didn't notice any difference 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The 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">
Expand Down