List of focusable elements in modal includes hidden ones

[melaniesaraj]

Bug description:

$uibModalStack queries for all of the focusable elements in the modal and uses that list to trap focus in the dialog. Every time tab is pressed, it checks whether the currently focused element is the last focusable element in the dialog. If it is, it moves focus to the first element in the dialog, rather than letting it escape into the rest of the page (modal.js:386). (For shift+tab, or tabbing backwards, switch "last" and "first".)

However, the selector used to get this list of focusable elements is incorrect in that it doesn't filter out hidden (display: none) elements (modal.js:272):

var tababbleSelector = 'a[href], area[href], input:not([disabled]), ' +
        'button:not([disabled]),select:not([disabled]), textarea:not([disabled]), ' +
        'iframe, object, embed, *[tabindex], *[contenteditable=true]';

So, in a dialog where the last element is hidden, focus will not properly be trapped, because we'll never end up focused on that hidden element. Moreover, shift+tab won't be able to loop backwards to the last element, since that hidden element is not focusable, so it'll get stuck on the first element. (For the shift+tab case, we have the same problems if the first element is hidden.)

Version: Current on master (commit linked above)

Example:

Plunker: http://plnkr.co/edit/hL3G8FUUiMnYKJcRkrFN?p=preview

Here, we have a dialog that shows a 'Help' button as the last button in the dialog IFF there's a non-empty help link, using ng-show (which uses display: hidden to hide elements). (Of course, this is a contrived example; ng-if would work fine in a situation like this.) First, open the modal without generating a help link. Tab through the modal. Focus will escape from the modal and enter the address bar and the rest of the page, for the reason described above. Shift+tab through the modal. Focus will get trapped on the first element. Then, generate a help link and open it again. Tabbing and shift+tabbing will properly keep focus in the model, because the last focusable element is... actually focusable.

Proposed fix

Minimal solution: The selector should make sure the items are visible. I suggest generating the list using the :visible pseudoselector, replacing modal.js:272:

var tabbableList = (['a[href]', 'area[href]', 'input:not([disabled])', 'button:not([disabled])',
    'select:not([disabled])', 'textarea:not([disabled])', 'iframe', 'object', 'embed',
    '*[tabindex]', '*[contenteditable=true]']).map(function (sel, i) {
        return sel + ':visible';
    });
var tababbleSelector = tabbableList.join(', ');

(I also suggest renaming 'tababble' to 'tabbable' or better yet, 'focusable' 😄)

Since querySelectorAll() does not support pseudoselectors, we would have to make the following replacement in modal.js:586:

focusableElementList = modalDomE1[0].querySelectorAll(tababbleSelector);
==>
focusableElementList = $(modalDomE1[0]).find(tababbleSelector);

More robust solution: It might also be a good idea to expose a setTabbableSelector() method in $uibModalStack, since different selectors might make sense for different dialogs.

I would be happy to submit a PR for either of those solutions, if anyone thinks it's a good idea and doesn't feel like doing it (it would be my first PR ever, but this is a pretty small change, and I'd write good tests, &c.)

Related issues

See also #5421 ('MODAL: Keyboard focus fail with dynamic content') and #5050 ('Cannot tab to previously disabled button in modal') for other issues involving the focusable element list.

[icfantv]

This looks like a good catch. I'll flag as needing investigation by our devs and they'll hopefully green light a PR. If you want to start now, I won't say no. But it may require several iterations. If you're ok with that, then please go ahead. Thanks again.

[icfantv]

Oh, and don't forget tests! (If you do a PR).

[wesleycho]

This sounds like a legitimate bug - PRs welcome.

[wesleycho]

Maybe we should change the implementation under the hood to not store an in-memory cache of tabbable elements - we should probably query fresh on each tab/shift-tab and navigate to the appropriate element based on that.

[melaniesaraj]

@wesleycho: Yeah, I think we need to do that - that's more relevant to the issues #5421 ('MODAL: Keyboard focus fail with dynamic content') and #5050 ('Cannot tab to previously disabled button in modal'). I don't think we need open issues for both of those - maybe one should subsume the other. This one is distinct, IMO, because it concerns which elements are being picked up, rather than when we get them. Of course, these could easily be fixed at the same time.

[wesleycho]

Yup, just noting something that has been on my head for a little while so the thought isn't lost :) . That would solve all potential issues that I can see with our programmatic accessibility implementation here.

[wawyed]

@wesleycho modalStack actually reruns the query every time you tab already, as per line:
https://github.com/angular-ui/bootstrap/blob/master/src/modal/modal.js#L394

I think all we need to do here is ensure that the focusableElementList contains only visible elements.

[wesleycho]

I think I agree with the proposed solution. I would hold off until #5630 is in, to avoid conflicts.