Added accessibility improvements (keyboard focus management / ARIA attribute)

[richardszalay]

I've added some improvements on the accessibility front, specifically around focus management and aria attributes.

The keyboard focus works across the examples (incuding the nested dialogs) and has been tested in IE 11/Chrome 40/Firefox 37 for browsers and Windows Narrator and NVDA for screen readers.

The updates to the readme (and lesser extent, the code) should describe the changes sufficiently, but I thought I'd discuss some details that could be deemed controversial.

Firstly, the keyboard focus support (specifically trapFocus, which I've enabled by default) forces the first content element (eg. p tag) to be focusable if there aren't any actual-focusable elements. This causes a focus rect to appear around it, which might cause some complains about styling (it can be disabled via CSS) This has been resolved in the latest commit, but it forces .css('outline','css') on the element which might be controversial in its own right.

Secondly, I've defaulted ariaAuto to true, which will automatically apply aria attributes based on the available content. It's possible this could conflict with users that have applied aria attributes in their dialog content.

In both cases, my justification is that the vast majority of developers won't look at the accessibility side of things. I'd rather force the minority of accessibility-friendly developers to disable the features and have improved accessibility for the majority.

Happy to discuss further.

[richardszalay]

Since I'm integrating the accessible version with a full app, I've discovered a bug with focusable elements that are display: none and added a fix that filters out invisible elements. It uses the same technique that jQuery uses, so it should be fine down to IE8.

[octavioamu]

Excellent!! +1 for be merged ASAP.

[voronianski]

@richardszalay this looks like a big change, is it ready to be merged?

P.S. Please test it thoroughly in order to not break any existing functionality. Thanks!

[richardszalay]

Having resolved one final niggle (focus outline added to dialogs with no focusable elements), I'm pretty happy with it now.

I've run through the example page numerous times with both mouse and keyboard and it "feels" good from a focus perspective. I'm also using this build in a "mostly complete" site and have had no issues with the dialog functionality.

[octavioamu]

The only bug I could find already was before this PR and is about Enter key to open a dialog.
Don't know why if I press enter once the same dialog is open twice, if I press spacebar works ok and only one dialog is open.
I'm talking about #164, I think the issue was confused because a comment talking about a focus issue (already fixed with this PR) happened when a element clicks focus continue in the original element so with enter it was opening the same dialog.

[richardszalay]

@octavioamu Have you been able to reproduce the Enter-double-open bug with this PR?

[octavioamu]

@richardszalay yes I'm using your PR, This was happening before your PR too.

[richardszalay]

@octavioamu Can you see if you can reproduce it in the "example" page that is part of the repo? I can't reproduce it myself so it's possible something in your app is causing the event to fire twice.

[octavioamu]

@richardszalay You are right, looks like I'm doing something wrong, have not ideia what is producing that behavior.

[octavioamu]

@richardszalay Ok, I'm using ngAria looks like this create some duplicate behavior only in the not happening when the element is a button.

If cache is set to false both div created with enter have the same id, is cache is not set (true ) also create 2 dialogs but with different id

[richardszalay]

@octavioamu It looks like it's actually a bug in ngAria, rather than with nDialog. See angular/angular.js#10388

Hmmmm. Looks like this introduces a bug? Looks like this is a problem in Chrome, Firefox, and Safari. Tab to the button (turns red) and hit "Enter" and the alert pops up twice

[octavioamu]

@richardszalay Good to know. Thanks for the info.
@voronianski for me the PR is ready to be merged, have tested since you post it in a complex webapp with nested dialogs, dialog data between them, also in some parts of te app I have making some modifications and integrated with a slide to get a kind of lightbox in fullsize and with this PR solve the focus problems.

[vsumner]

I'd love to see this merged. How can I help?

[richardszalay]

@voronianski I'd say I'm definitely confident enough in the quality levels for it to be merged. Several people seem to be using it in large apps (myself included) and there's been no issues reported.

[voronianski]

@richardszalay ok, then could you rebase your PR with latest master and I'll merge it.

[richardszalay]

@voronianski Done!

[voronianski]

@richardszalay I don't like such comments 😄

[richardszalay]

Did you want me to remove it?

[ms88privat]

pls merge it, thank you

[voronianski]

@richardszalay ok, merging it. I would like to merge another cool enhancement PR and release new major version of ngDialog today or tomorrow.

[Ravindra042]

Facing the same problem with version v0.4.0. Can anybody explain me in simple steps, How to fix this.