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.

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

Make dialogs more accessible #133

Closed
wants to merge 1 commit into from

Conversation

huoxito
Copy link

@huoxito huoxito commented Aug 14, 2015

Via aria attributes and trapping the tab key inside the dialogs

It introduces three options:

    - ariaMainContent e.g. '#main-content'
    - trapTabKey e.g. true
    - focusFirstInput e.g. true

Hi all,

We're using this code in one of our projects and I wanted to get some feedback here as well. Pls let me know if you think this is not a proper approach or if there's any other tweaks you'd like to address? We're looking forward to have this as a built in feature.

I left out the focus "toggle" before opening and after closing the dialogs. Mostly because I'm not sure yet we should do it. Currently it looks something like this:

// before open
focusedElementBeforeModal = jQuery(':focus')

// bind on vexClose with
focusedElementBeforeModal.focus()

Let me know if you think it's a good fit and I can add it here too. The implementation here is pretty much the same as the incredible accessible modal article which was based on some w3c pseudo code.

cheers

@TrevorBurnham
Copy link

I like it! Having to specify the "main content" to hide from screen readers while the modal is open feels like a bit of a chore, though. Perhaps ariaMainContent should default to 'body > :not(.vex)'? That is, by default, everything in the document body outside of the .vex would be receive aria-hidden while the modal is open.

@huoxito
Copy link
Author

huoxito commented Aug 14, 2015

nice yeah I'll update the ariaMainContent default to that thanks!

Would you also accept a fourth config to keep track of the focus outside the vex dialog? Exactly like I mentioned above, before opening the dialog grab the current focused element then give it a focus() back when the dialog closes. I'd name it like giveBackFocus: true. It has been a requirement for us and it seems to be a requirement in general to make the dialog 100% accessible.

@TrevorBurnham
Copy link

Sure, that seems like a sensible option. 👍

@adamschwartz
Copy link
Contributor

👍

@cnecochea
Copy link

👍 💯 thank you so much, @huoxito

Via aria attributes and trapping the tab key inside the dialogs

It introduces some new options:

        - ariaMainContent e.g. '#main-content'
        - trapTabKey e.g. true
        - focusFirstInput e.g. true
        - giveBackFocus e.g. true
@huoxito
Copy link
Author

huoxito commented Aug 14, 2015

thanks all I've updated the commit, pls give a try when you have a chance

@modean
Copy link

modean commented Feb 21, 2016

Any plans to get this PR into master? I implemented the same on my own and it would be a lot easier if VEX would support accessibility stuff out of the box. AFAIK there are a couple of other smaller accessibility related PRs like '#134' that could be accumulated into this PR.

Also consider to default focus the 'NO' button on 'YESNO' dialogs and the 'YES' button on 'YES' dialogs.

@bbatliner
Copy link
Collaborator

With the release of vex 3, we've ditched CoffeeScript for vanilla JavaScript, but I love the accessibility upgrades that were made in this PR. @huoxito do you think you could adapt your code for the vanilla JS code in vex 3? We can plan for accessibility upgrades in, say, a future minor version release.

It sounds like there was some support from @adamschwartz in the past. I say we move forward with this, if possible.

@huoxito
Copy link
Author

huoxito commented Aug 17, 2016

@bbatliner yeah absolutely 👍 great to hear it moved to just javascript, I'll try to update this PR or submit a new one later this week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants