Mobile navigation focus issue #787

ludoboludo · 2021-10-18T19:24:22Z

Why are these changes introduced?

Fixes #76

What approach did you take?

The code in placed looked good. What I found is that the issue was related to the transition of the submenu appearing. So I had to move the trapFocus() into a setTimeout() function as well.

It seemed to work with 50ms but I used 100ms as I thought it gave us a safe gap. I tried with no value mentioned but it wasn't working sill. I tried on slow 3g and it worked fine.

Other considerations

Open to other suggestions.

Demo links

Store
Editor

Checklist

ludoboludo · 2021-10-18T19:27:06Z

Another issue I noticed though while looking into this one is the fact that focus isn't trapped properly in submenus: video

The first part is fine, it's trapped properly but submenu isn't working as it should I believe.

svinkle · 2021-10-18T19:45:44Z

focus isn't trapped properly in submenus

So this one's interesting. Since the navigation is functionally implemented as a disclosure pattern (and not a modal dialog pattern), trapping keyboard focus is not required.

However, since we visually hide content by pushing the submenu content "over top" of the parent content, items "below" should not be focusable. Details in #769.

tyleralsbury · 2021-10-19T14:24:40Z

If the issue is the transition's timing, did you try using the transitionend event?

https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/transitionend_event

assets/global.js

svinkle · 2021-10-19T16:26:41Z

It seems to work some of the time. How can we avoid using timers? Also per my comment above, trapping focus is not required here, if that helps at all.

ludoboludo · 2021-10-19T16:40:17Z

It seems to work some of the time. How can we avoid using timers? Also per my comment above, trapping focus is not required here, if that helps at all.

With the latest change it's not using a timer but a transitionend event listener. So it waits for the drawer of the submenu to slide in then applies the focus to the right element.

When I tested it worked every time (chrome on my laptop and on my android device), when did it fail for you @svinkle ?

svinkle · 2021-10-19T16:46:13Z

when did it fail for you

Just tried the linked demo store when I made the above comment.

Tab to nav control
Space to open
Tab to subnav control
Space to open – works well so far
Esc to try again
Repeat 2-4, no visual indicator or focus management.

ludoboludo · 2021-10-19T21:31:52Z

assets/global.js

@@ -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;


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.

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

assets/global.js

ludoboludo · 2021-10-20T13:58:16Z

Should work properly now @svinkle if you want to give it another shot 🙂

svinkle · 2021-10-21T13:18:48Z

@ludoboludo Seems to work well everywhere except Chrome + Talkback. Sometimes it doesn't shift focus as expected. Testing on my Pixel 5, so it shouldn't be "underpowered." 🤔

Video

Notice Talkback green boarder sometimes does not shift in focus as expected.

screen-20211020-153825_2.mp4

ludoboludo · 2021-10-21T14:01:06Z

Hmm yeah from what I notice, talkback doesn't trigger the animation of the next menu happen to appear. And since we're waiting for the transitionend event, then trapFocus() doesn't end up running. I wonder why in VoiceOver it does trigger things properly but not in TalkBack 🤔
I will try and see what I can find.

ludoboludo · 2021-10-21T16:20:58Z

From what I can tell, the issue is that double tapping to open the submenu does, most times, not trigger the transition that we have set in CSS when applying the .menu-opening class. Not sure why 🤔

That made me think that if there isn't a transition, then our transitionend event listener isn't going to work. So I added a check for the prefer-reduce-motion media query. From what I tested on my mac it works well when I check the reduce motion option in the accessibility settings. 👍

ludoboludo · 2021-10-21T18:56:52Z

assets/global.js

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


The only thing I found so far is that adding 100ms to this setTimeout() seem to make it work consistently with TalkBack...

When I check on /main the same issue is happening specifically with TalkBack. The transition isn't always triggered but when you're navigating by touch it's fine. Not sure how else to tackle this one. cc: @svinkle

Co-authored-by: Tyler Alsbury <60230011+tyleralsbury@users.noreply.github.com>

svinkle · 2021-11-01T17:45:05Z

@ludoboludo Let's get this one merged and revisit if it comes up again during usability testing.

assets/global.js

ludoboludo · 2021-11-01T21:19:03Z

Should be good for approval and merge now I believe . cc: @tyleralsbury @svinkle

tyleralsbury

Re-tested and code looks good.

ludoboludo requested a review from svinkle October 18, 2021 19:24

tyleralsbury previously approved these changes Oct 19, 2021

View reviewed changes

assets/global.js Outdated Show resolved Hide resolved

ludoboludo dismissed tyleralsbury’s stale review via 7057d5f October 19, 2021 15:52

ludoboludo requested a review from tyleralsbury October 19, 2021 15:53

tyleralsbury previously approved these changes Oct 19, 2021

View reviewed changes

ludoboludo dismissed tyleralsbury’s stale review via 0b64378 October 19, 2021 20:22

ludoboludo commented Oct 19, 2021

View reviewed changes

assets/global.js Show resolved Hide resolved

ludoboludo commented Oct 21, 2021

View reviewed changes

ludoboludo and others added 7 commits October 25, 2021 16:10

Mobile navigation focus issue

ab9e9dc

rebase 1

c00eb10

Update assets/global.js

bec9cea

Co-authored-by: Tyler Alsbury <60230011+tyleralsbury@users.noreply.github.com>

adjust the summary keyup listener

4b5b3c8

add event in function call

83566c3

rebase 2

32c2f05

add timing to timeout function

8a8c016

ludoboludo force-pushed the mobile-sub-nav branch from 9dbac4e to 8a8c016 Compare October 25, 2021 20:13

ludoboludo requested a review from tyleralsbury October 28, 2021 13:41

svinkle previously approved these changes Nov 1, 2021

View reviewed changes

tyleralsbury reviewed Nov 1, 2021

View reviewed changes

assets/global.js Outdated Show resolved Hide resolved

add event in function call

5c8daf5

ludoboludo dismissed svinkle’s stale review via 5c8daf5 November 1, 2021 21:18

ludoboludo requested review from tyleralsbury and svinkle November 1, 2021 21:19

svinkle approved these changes Nov 2, 2021

View reviewed changes

tyleralsbury approved these changes Nov 3, 2021

View reviewed changes

ludoboludo merged commit a890eed into main Nov 3, 2021

ludoboludo deleted the mobile-sub-nav branch November 3, 2021 19:42

svinkle mentioned this pull request Nov 12, 2021

[Faceted Filters] Mobile sub-navigation control does not manage focus #120

Closed

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

Mobile navigation focus issue (Shopify#787)

948c7e9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mobile navigation focus issue #787

Mobile navigation focus issue #787

ludoboludo commented Oct 18, 2021

ludoboludo commented Oct 18, 2021

svinkle commented Oct 18, 2021

tyleralsbury commented Oct 19, 2021

svinkle commented Oct 19, 2021

ludoboludo commented Oct 19, 2021

svinkle commented Oct 19, 2021

ludoboludo Oct 19, 2021

ludoboludo Oct 20, 2021

ludoboludo commented Oct 20, 2021

svinkle commented Oct 21, 2021

ludoboludo commented Oct 21, 2021

ludoboludo commented Oct 21, 2021 •

edited

Loading

ludoboludo Oct 21, 2021 •

edited

Loading

ludoboludo Oct 25, 2021 •

edited

Loading

svinkle commented Nov 1, 2021

ludoboludo commented Nov 1, 2021

tyleralsbury left a comment

Mobile navigation focus issue #787

Mobile navigation focus issue #787

Conversation

ludoboludo commented Oct 18, 2021

ludoboludo commented Oct 18, 2021

svinkle commented Oct 18, 2021

tyleralsbury commented Oct 19, 2021

svinkle commented Oct 19, 2021

ludoboludo commented Oct 19, 2021

svinkle commented Oct 19, 2021

ludoboludo Oct 19, 2021

Choose a reason for hiding this comment

ludoboludo Oct 20, 2021

Choose a reason for hiding this comment

ludoboludo commented Oct 20, 2021

svinkle commented Oct 21, 2021

ludoboludo commented Oct 21, 2021

ludoboludo commented Oct 21, 2021 • edited Loading

ludoboludo Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

ludoboludo Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

svinkle commented Nov 1, 2021

ludoboludo commented Nov 1, 2021

tyleralsbury left a comment

Choose a reason for hiding this comment

ludoboludo commented Oct 21, 2021 •

edited

Loading

ludoboludo Oct 21, 2021 •

edited

Loading

ludoboludo Oct 25, 2021 •

edited

Loading