fix(modal): Improve ARIA support

[NickHeiner]

I will add more tests after I get initial acceptance on these changes.

I have tested on:

• VoiceOver iOS (latest iOS)
• VoiceOver macOS (latest OS X El Cap)
• JAWS 16 / Windows 7 / IE 11
• Angular 1.5.8

This fixes the following issues:

VoiceOver does not read modal

1. Launch VoiceOver on iOS
2. Go to the demo page
3. Tap the "Open me!" button to open a modal

Expected: Screenreader reads the modal content automatically
Actual: Screenreader does not read the modal automatically; an assistive technology user would be confused as to what's going on.

To fix this, I added an aria-live attribute. This makes the expected result occur above.

Update: Adding aria-live actually causes more problems than it solves. iOS VoiceOver reads the content, but it reads it out of order. JAWS will read the content in a bizarre way, repeating the aria-labelledby element over and over again, and omitting some of the aria-describedby text. macOS VoiceOver reads the modal just fine without aria-live. So I'm going to omit aria-live entirely.

VoiceOver selects elements in background

1. Launch VoiceOver on iOS
2. Go to the demo page
3. Tap the "Open me!" button to open a modal
4. Scroll to the left and right to see different readable elements

Expected: Only elements within the modal are visible to the screen reader
Actual: User can scroll off the modal, reading elements that are supposed to be hidden

To fix this, I added aria-hidden=true to DOM elements to hide everything but the modal. When the modal is closed, I remove aria-hidden. To account for elements that may already be aria-hidden, and the possibility of multiple modals being opened at once, I use a counter on each element to be hidden. That way, when I go to do cleanup, I know if the element was already hidden, instead of doing a blanket el.removeAttribute('aria-hidden') on everything.

Unfortunately, VoiceOver will still read an element with text "August 2016". I don't know why it reads this element. The element is from the datepicker demo:

<table role="grid" aria-labelledby="datepicker-103-1928-title" aria-activedescendant="datepicker-103-1928-25">
<!-- datepicker table contents -->
</table>

I tried removing the aria-activedescendent and aria-live attributes from other elements, but that did not stop this table from being read. After sinking a bit of time into this, I decided to leave it as-is for now, as people encountering this in the wild will be less common.

VoiceOver puts the cursor on the modal

1. Launch VoiceOver on iOS or MacOS
2. Go to the demo page
3. Tap the "Open me!" button to open a modal
4. One finger swipe (on iOS) to the left and right or VO+left/right arrow (on MacOS) to move the VoiceOver cursor

Expected: The VoiceOver cursor is in the modal
Actual: The VoiceOver cursor is stuck on the content behind the modal, so the modal contents cannot be navigated.

Known Issues

Content Read Order

Using aria-live appears to make VoiceOver iOS read the modal content out of order. Related: http://stackoverflow.com/questions/38088439/accessibility-aria-live-reading-order-changes-on-ios. I am not sure that there's anything to be done about this; it pretty clearly seems like a bug. Here are some experiments I've done with different options.

Returning Focus To Opening Button

Focus is not returning to the button that opened the modal; VoiceOver iOS is reading a different button. I believe that this is related to adding aria-hidden to all background elements. I think that VoiceOver has a bug in the way it handles elements that are hidden and then unhidden throughout their lifespan. When I disable the code that adds aria-hidden, I cannot repro this issue. However, then we're back to the original problem of the VoiceOver cursor escaping the modal and traversing background content. I believe that this is the lesser of two evils.

Additionally, VoiceOver macOS handles this case just fine. So by doing this fix, we eliminate a problem for VoiceOver iOS and macOS, and add a smaller problem for VoiceOver iOS.

Misc

I did a few small fixes to improve the developer experience working on this repo. I also fixed a few grammar nits. I can pull those out into a separate PR if you'd prefer.

[wesleycho]

Isn't the CLI meant to be installed as a global? Not so sure this is a great change.

[NickHeiner]

I actually prefer this approach. I like a modal where npm install gets you everything you need to work with a repo, and you can know that convention works consistently across projects, instead of needing to go digging through READMEs and travis files to find what global deps are needed. I also like that this specifies a version of grunt-cli to use. I've worked on projects where people document needing global deps but not which versions of those deps they use, which causes confusion when people are accidentally mismatching.

Anyway, if you don't like this, I can back it out.

[NickHeiner]

@wesleycho is there anything I can do to help get this reviewed and merged? There are a couple of different fixes that I added; those can be split out into separate PRs if that makes it easier.

[wesleycho]

Sorry, I've been super busy lately (been pulling some long work hours) - I can try taking a look later this week (probably weekend), but there are some pressing tasks I have to complete this week.

[NickHeiner]

Ok, no problem! I appreciate your contribution to open source and no one is entitled to your time 😄 . Let me know if there's anything I can do to help.

[wesleycho]

I think the subsequent code would be nicer if you just passed angularDomEl, and access the raw element only when needed, i.e el[0].tagName. This would be more in keeping with usage of jqLite whenever possible.

[NickHeiner]

I didn't know that was a convention, but I'm happy to adhere to it! 😄

[wesleycho]

This implementation comes with a major flaw it seems - if the modals are added to different elements using append-to, this would break it seems like, unless it was designed with that in mind.

Can you explain if this is fine in that scenario and what happens then?

[NickHeiner]

I agree that this would break as-is if there were multiple modals at once. I'm curious what a use case is for having multiple modals up at once, and I'm not sure there's any way that that's going to be good for a11y users. But it's clearly supported by the code as it stands now, so I'll continue to support it.

My basic idea would be to change the algorithm where instead of marking everything but the modal as aria-hidden, it would mark everything that's not a parent of a modal as aria-hidden.

[wesleycho]

I think we're in agreement that multiple modals suck, but it's a feature we support and users want for strange reasons :( .

I think this as is could be fine as far as adding accessibility goes even with the multiple modal situation, but it wouldn't be a singular count as that information will be strewn across the DOM.

I'll think on it, but after the changes are made, I'm inclined to merge the PR then.

[NickHeiner]

I think we're in agreement that multiple modals suck, but it's a feature we support and users want for strange reasons :( .

😸. Sounds good to me.

[NickHeiner]

Amusingly, it looks like Bootstrap itself does not support multiple open modals:

I'm not going to try to revisit the decision for this project to support multiple open modals here, but I just thought I'd pass that along.

[NickHeiner]

If there are multiple modals on the page at once, are they definitely going to be overlapping? It looks like the modal's positioning is not easily configurable, so even if you have multiple modals open at once, only one will be visible to the user at a given time. If this is the case, I should modify the code to mark everything as aria-hidden except the modal at the top of the stack.

Also, what's the point of having a custom appendTo element? Even when I set a custom parent, the modal still visually looks the same. I'd like to understand the goal here so I can ensure that my changes make the most sense.

Once I figure this out, I'll make code changes to reflect our discussion.

[NickHeiner]

I updated the demo for the modals to show the two cases that we are currently discussing.

[wesleycho]

I forget the reason for the custom append-to, I think it's to give users more control over the styling.

[NickHeiner]

Ok. I think my changes should work ok with appendTo as-is.

What about the multiple modals issue? Can we be certain that only one modal will be visible / interactable at a time?

[NickHeiner]

@wesleycho, anything I can do to help this get merged?

[wesleycho]

Sorry for the slowness, all the traveling I've been doing has been getting to me - I've also been swamped as of late.

Merging now, as I released 2.1.4 a minute ago or so.

[NickHeiner]

No problem at all! Thanks.

[RobJacobs]

@wesleycho, @NickHeiner As a result of this change, I'm seeing a data-bootstrap-modal-aria-hidden-count and aria-hidden attribute added to all my script tags which are siblings of the modal content div. Should the getSiblings method here: https://github.com/angular-ui/bootstrap/blob/master/src/modal/modal.js#L568 be filtering on actual elements?

[wesleycho]

Sounds like a bug - maybe this method needs to be altered: https://github.com/angular-ui/bootstrap/blob/master/src/modal/modal.js#L581-L587

[NickHeiner]

I'm fine making that fix. @RobJacobs, is there an actual problem being caused by this? Or is it just confusing to have those extraneous attributes lying around? I don't think that aria-hidden has any impact on a script tag.

[RobJacobs]

@NickHeiner It doesn't break anything, just extra noise in the markup.

[NickHeiner]

Ok. In that case, I would just as soon leave it as-is, since filtering out script tags would add (admittedly minimal) extra complexity. I'll defer to @wesleycho's judgment. And @RobJacobs you're free to send a PR yourself if you feel strongly 😄

[RobJacobs]

@NickHeiner There's more than just the attributes on the script tags I would change. I'm not sold on using the 'data-bootstrap-modal-aria-hidden-count' attribute to keep track of whether there are open modal windows and if the aria attributes need to be removed. We have access to the openWindows.length() that could be used instead:

if (openedWindows.length() <= 1) 

[NickHeiner]

openedWindows.length() will not work because we need to account for elements that already had aria-hidden before any modal was opened. If we simply did the following, then we'd wipe out pre-existing aria-hidden attributes:

if (openedWindows.length() == 0) {
    removeAllAriaHidden();
}

I do agree that having data-bootstrap-modal-aria-hidden-count on a bunch of elements is a bummer. Can you think of a better solution that preserves pre-existing aria-hidden attributes? Or am I missing something about how the openedWindows.length() solution would work?