railsdevs.com icon indicating copy to clipboard operation
railsdevs.com copied to clipboard

Focus in mobile filters dialog

Open metamoni opened this issue 3 years ago • 2 comments

Describe the bug

In mobile view, the "Filters" button has aria-expanded="false", but this doesn't update to true when the menu is expanded. As this button opens a dialog rather than a simple menu, we don't need to use aria-expanded here. However, we need to make sure to keep the focus inside the modal while this is open.

To reproduce

Steps to reproduce the behavior:

  1. Log in as a business
  2. Go to http://localhost:3000/developers
  3. Inspect the page and switch to mobile view
  4. Turn on VoiceOver: command + F5
  5. Tab to the "Filters" button and notice the screen reader announces "collapsed"
  6. Hit "Enter" and tab through the modal
  7. Keep tabbing through all the elements and notice that the focus moves outside the modal, back to the main page, while the modal is still open
  8. Turn VoiceOver off: command + F5

Expected behavior

  • Focus should be trapped inside modal while modal is open (should not move back to main page)
  • When modal is closed, focus should go back to "Filters" button
  • When "Filters" button focused, screen readers should not announce "collapsed"

Actual behavior

Once you tab through the elements in the modal, the focus goes back to the main page, outside modal When the modal is closed, the focus is lost and does not go back to "Filters"

Screenshots

https://user-images.githubusercontent.com/22390758/177101137-61fc8084-3730-4e6c-b443-ed9f49e005b3.mov

Additional context

metamoni avatar Jul 04 '22 07:07 metamoni

Hi @joemasilotti Can I take up this issue and raise a PR to resolve it?

atiqueakhtar avatar Aug 29 '22 09:08 atiqueakhtar

Yeah, sure thing. Thanks @atiqueakhtar!

joemasilotti avatar Aug 29 '22 14:08 joemasilotti

Please see #658 for some ideas and notes. Most importantly, I think this can be accomplished with HTML markup only, no JavaScript.

joemasilotti avatar Nov 18 '22 21:11 joemasilotti

Hi @joemasilotti can I pick this one up?

DamonClark avatar Dec 21 '22 00:12 DamonClark

Yes, please. Thanks @DamonClark!

joemasilotti avatar Dec 23 '22 15:12 joemasilotti

For the expected behavior: Focus should be trapped inside modal while modal is open (should not move back to main page). I am able to add a tab-index=-1 to the necessary element but then even if the modal is closed those elements aren't being tabbed. I don't know if there is a good way to do that without custom javascript.

DamonClark avatar Dec 23 '22 19:12 DamonClark

Got it, thanks for digging in! What’s the minimal, most generic, JavaScript we could add to get that behavior.

joemasilotti avatar Dec 25 '22 22:12 joemasilotti

@joemasilotti from what I'm researching I'm not really seeing any alternatives much better then what @atiqueakhtar implementation looked like. This is what I was considering adding https://hidde.blog/using-javascript-to-trap-focus-in-an-element/ but it looks very similar to what atiqueakhtar already committed.

DamonClark avatar Jan 11 '23 02:01 DamonClark

Is there any way to set tab-index=-1 when on mobile vs. desktop so the options get skipped? I think that might save the custom JavaScript for listening for tabs and keys and such, right?

joemasilotti avatar Jan 15 '23 00:01 joemasilotti

I think you could add something like this to the controller

  def mobile?
    request.user_agent =~ /android|blackberry|iphone|ipad|ipod|iemobile|mobile|webos/i
  end

and then add a ternary operator that only passes tab-index=-1 on condition user_agent is a mobile device. That's all I'm seeing so far without javascript, was hoping for something a litter nicer.

DamonClark avatar Jan 15 '23 22:01 DamonClark

I’m ok with some JavaScript. But I didn’t want to rebuild the entire tab selection engine.

Can we get viewport in the request? I’d rather chain it off of that since the media breakpoints are what determines if the mobile vs. desktop views are shown.

joemasilotti avatar Jan 15 '23 23:01 joemasilotti

I have some changes I was going to try but I need permission to push. I fixed it so that "collapsed" wouldn't announce when filters button is focused. Then I started trying to create a viewPort condition so that it would set tabindex -1. But its not picking up the javascript for some reason. Once I can push maybe we can have a look.

DamonClark avatar Jan 16 '23 01:01 DamonClark

Feel free to create a fork and push to that. No need for access to the existing PR or main repo.

joemasilotti avatar Jan 16 '23 02:01 joemasilotti

Hey @joemasilotti I created a fork and pushed to it. My change isn't working but I think its the right idea. It uses minimal javascript to try and set the tabindex -1 to the elements outside of the modal when the view port is less then 768. If you or anyone else can have a look it would be much appreciated. Below is the commit changes. Thanks :)

https://github.com/joemasilotti/railsdevs.com/commit/2e22022544d97ff0289aaa4f317062a72aa28b6d

DamonClark avatar Jan 18 '23 20:01 DamonClark

Awesome, thanks Damon! Can you submit a PR to RailssDevs based on that fork so I can take a look?

joemasilotti avatar Jan 19 '23 02:01 joemasilotti

A few folks, myself included, have taken a pass at this with no luck.

I'm going to close this out for now so we can focus on other stuff.

joemasilotti avatar Jan 28 '23 14:01 joemasilotti