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

ludoboludo · 2023-03-27T21:58:25Z

PR Summary:

Issue with locale selector in the drawer menu and closing the menu drawer with the ESC key.

Why are these changes introduced?

What approach did you take?

When you click on the locale selector it opens the dropdown. If you click on the scrollbar, because it's not a focusable element (it's part of the ul), and because the #menu-drawer has a tabindex="-1' it's triggering the focusout event listener we have and closes the dropdown.

It works without an issue in the header and footer outside of the menu drawer 🤔

I've gone ahead and removed the tabindex on the #menu-drawer cause I couldn't see where it was necessary. That fixes our issue.
I'm also adding a fix so that when you close the menu drawer via ESC, it removes the overlay accordingly. It wasn't before.
And finally, I've added some logic around the ESC listener for the localization-form, so that when you press ESC it closes only the dropdown. And if the dropdown is closed it closes the menu drawer.

Other considerations

Decision log

#	Decision	Alternatives	Rationale	Downsides
1

Visual impact on existing themes

Shouldn't visually impact anything.

Testing steps/scenarios

Either shrink your window to have the hamburger menu icon or enable the drawer menu type on desktop.
Test the locale selectors and make sure you can scroll using the scrollbar.
Try with keyboard navigation only as well and using ESC to close the dropdown and menu drawer.
Test the filters on the collection page. Set them to be shown as a drawer and make sure it works as expected. It's reusing the same menu-drawer custom element.
Using ESC to close the menu drawer should remove it's overlay.
Then test in Safari/Firefox as well.

Demo links

Editor

Checklist

Added PR summary for release notes
Requested review from UX (Only for changes that are affecting the experience or perceivable visual details)
Created a ticket for the help.shopify.com documentation team about updates to theme settings. (Internal-only task)
Followed theme code principles
Linted with Theme Check
Tested on mobile
Tested on multiple browsers
Tested for accessibility

… fix locale selectors

ludoboludo · 2023-03-30T14:48:33Z

assets/global.js

@@ -398,6 +398,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);


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.

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.

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 👍

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.

ludoboludo · 2023-03-30T14:50:09Z

assets/localization-form.js

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


Here I added some logic so that when pressing the ESC key, it doesn't trigger both the local dropdown being closed and the menu drawer.
Now it should close the dropdown if it's open. Then if you press again it will close the menu drawer.

ludoboludo · 2023-03-30T14:52:06Z

snippets/header-drawer.liquid

-    <div id="menu-drawer" class="gradient menu-drawer motion-reduce" tabindex="-1">
+    <div id="menu-drawer" class="gradient menu-drawer motion-reduce">


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.

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 🤔

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".

eugenekasimov

Works as expected. I couldn't find anything.

kmeleta

Fixes all work great. Left a couple comments and I also opened an issue to fix focus trapping in the header drawer #2496 which I noticed while testing

kmeleta · 2023-04-05T17:59:39Z

snippets/header-drawer.liquid

-    <div id="menu-drawer" class="gradient menu-drawer motion-reduce" tabindex="-1">
+    <div id="menu-drawer" class="gradient menu-drawer motion-reduce">


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".

kmeleta · 2023-04-05T18:34:30Z

assets/global.js

@@ -398,6 +398,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);


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.

assets/localization-form.js

…ubmenu

assets/global.js

…behaviour (Shopify#2472)

ludoboludo added 3 commits March 27, 2023 17:39

fix header locales when trying to close dropdown

1b8c30f

testing solutions

636e99f

fix menu drawer not closing properly when ESC and changed approach to…

f439347

… fix locale selectors

ludoboludo commented Mar 30, 2023

View reviewed changes

ludoboludo changed the title ~~fix header locales when trying to close dropdown~~ [Header drawer] fix header locales closing dropdown too soon and ESC behaviour Mar 30, 2023

ludoboludo marked this pull request as ready for review March 30, 2023 15:21

kmeleta self-requested a review March 30, 2023 18:27

eugenekasimov self-requested a review March 31, 2023 20:35

melissaperreault self-requested a review April 3, 2023 20:56

eugenekasimov approved these changes Apr 3, 2023

View reviewed changes

kmeleta approved these changes Apr 5, 2023

View reviewed changes

ludoboludo added 2 commits April 5, 2023 15:41

Merge branch 'main' into fix-drawer-locales-closing-issues

7c5f04e

Fix ESC on filter drawer, closing the whole thing instead of just a s…

df6fa0c

…ubmenu

kmeleta reviewed Apr 5, 2023

View reviewed changes

assets/global.js Outdated Show resolved Hide resolved

address review comment

bad3342

ludoboludo merged commit 93078bf into main Apr 6, 2023

ludoboludo deleted the fix-drawer-locales-closing-issues branch April 6, 2023 16:36

vfonic mentioned this pull request Apr 14, 2023

General codebase issues with multiple patterns being followed for the same functionality #2536

Open

vfonic mentioned this pull request Apr 22, 2023

Any plans on introducing JavaScript tests? #2557

Open

phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024

[Header drawer] fix header locales closing dropdown too soon and ESC …

b8489f9

…behaviour (Shopify#2472)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ludoboludo commented Mar 27, 2023 •

edited

Loading

ludoboludo Mar 30, 2023

kmeleta Apr 5, 2023

ludoboludo Apr 5, 2023 •

edited

Loading

kmeleta Apr 5, 2023

ludoboludo Mar 30, 2023

ludoboludo Mar 30, 2023

eugenekasimov Apr 3, 2023

kmeleta Apr 5, 2023

eugenekasimov left a comment

kmeleta left a comment

kmeleta Apr 5, 2023

kmeleta Apr 5, 2023

		<div id="menu-drawer" class="gradient menu-drawer motion-reduce" tabindex="-1">
		<div id="menu-drawer" class="gradient menu-drawer motion-reduce">

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

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

Conversation

ludoboludo commented Mar 27, 2023 • edited Loading

PR Summary:

Why are these changes introduced?

What approach did you take?

Other considerations

Decision log

Visual impact on existing themes

Testing steps/scenarios

Demo links

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ludoboludo Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugenekasimov left a comment

Choose a reason for hiding this comment

kmeleta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ludoboludo commented Mar 27, 2023 •

edited

Loading

ludoboludo Apr 5, 2023 •

edited

Loading