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

Mobile navigation focus issue #787

Merged
merged 8 commits into from
Nov 3, 2021
Merged

Mobile navigation focus issue #787

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ab9e9dc
Mobile navigation focus issue
ludoboludo Oct 18, 2021
c00eb10
rebase 1
ludoboludo Oct 25, 2021
bec9cea
Update assets/global.js
ludoboludo Oct 19, 2021
4b5b3c8
adjust the summary keyup listener
ludoboludo Oct 19, 2021
83566c3
add event in function call
ludoboludo Oct 20, 2021
32c2f05
rebase 2
ludoboludo Oct 25, 2021
8a8c016
add timing to timeout function
ludoboludo Oct 25, 2021
5c8daf5
add event in function call
ludoboludo Nov 1, 2021
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
17 changes: 11 additions & 6 deletions assets/global.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm adding a check here so it doesn't add the event listener on the summary elements from the menu drawer. As it has it's own behaviour that is tackled in the class declaration (onKeyUp(event)).

But is it good idea to add a check for a specific element within a function that is general like this one.

I could also remove the IDs on the <details elements from the menu drawer and re add an accessibility function in the menu drawer class that would basically do the same as what's happening in this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tyleralsbury if you have a chance to look at the code again. Curious to hear your thoughts on adding a check here.

summary.parentElement.addEventListener('keyup', onKeyUpEscape);
});

Expand Down Expand Up @@ -306,24 +307,29 @@ class MenuDrawer extends HTMLElement {
const openDetailsElement = event.target.closest('details[open]');
if(!openDetailsElement) return;

openDetailsElement === this.mainDetailsToggle ? this.closeMenuDrawer(this.mainDetailsToggle.querySelector('summary')) : this.closeSubmenu(openDetailsElement);
openDetailsElement === this.mainDetailsToggle ? this.closeMenuDrawer(event, this.mainDetailsToggle.querySelector('summary')) : this.closeSubmenu(openDetailsElement);
}

onSummaryClick(event) {
const summaryElement = event.currentTarget;
const detailsElement = summaryElement.parentNode;
const isOpen = detailsElement.hasAttribute('open');
const reducedMotion = window.matchMedia("(prefers-reduced-motion: reduce)");

function addTrapFocus() {
trapFocus(summaryElement.nextElementSibling, detailsElement.querySelector('button'));
summaryElement.nextElementSibling.removeEventListener('transitionend', addTrapFocus);
}

if (detailsElement === this.mainDetailsToggle) {
if(isOpen) event.preventDefault();
isOpen ? this.closeMenuDrawer(summaryElement) : this.openMenuDrawer(summaryElement);
isOpen ? this.closeMenuDrawer(event, summaryElement) : this.openMenuDrawer(summaryElement);
} else {
trapFocus(summaryElement.nextElementSibling, detailsElement.querySelector('button'));

setTimeout(() => {
detailsElement.classList.add('menu-opening');
summaryElement.setAttribute('aria-expanded', true);
});
!reducedMotion || reducedMotion.matches ? addTrapFocus() : summaryElement.nextElementSibling.addEventListener('transitionend', addTrapFocus);
}, 100);
}
}

Expand All @@ -343,7 +349,6 @@ class MenuDrawer extends HTMLElement {
details.removeAttribute('open');
details.classList.remove('menu-opening');
});
this.mainDetailsToggle.querySelector('summary').setAttribute('aria-expanded', false);
document.body.classList.remove(`overflow-hidden-${this.dataset.breakpoint}`);
removeTrapFocus(elementToFocus);
this.closeAnimation(this.mainDetailsToggle);
Expand Down