$modal does not stop tabbing on the background

[chriswu14]

This can be reproduced on the sample page on http://angular-ui.github.io/bootstrap/

• in the Dialog section click 'Open Dialog' button
• keep hitting tab key

result: tab going through background page and data can also be entered.

Hope someone can advise on this problem. Thanks.

[pkozlowski-opensource]

Yeh, this is a valid point. We should fix this.

[chriswu14]

I can see the last Bootstrap version modal dialog was setting the tabindex = -1 to prevent this. But I tried it on the markup it does not fix it.

[hallister]

Well this is going to be a tough one. I played with it a bit today and here's what I've found:

The tab-index trick Bootstrap is using is quite dependent on careful layout of the page (in fact, all items that can be tabbed to on a page must also have a tab-index).

The only other way to fix this is to bind to the tab keypress event, determine if the event.target is a child of modal and event.preventDefault() if it's not. Unless there's a "better way", I'm not sure how this is fixed.

[jmwolfe]

👍 I would really like to see this one fixed soon! @hall5714 - i wonder if there is a way to wrap it around to the first element on the modal if the target isn't a child of the modal. having it halt on the Cancel button or whatever would be sub-optimal. How long to get this on the roadmap? Shall I have a go and submit a pull?

[robvdl]

They have a workaround for this problem in Bootbox (http://bootboxjs.com/#examples), which also uses Bootstrap dialogs, except they somehow managed to disable tabbing on the page behind the dialog. Can we maybe look how they got around this issue?

[robvdl]

I also did my own workaround for this problem, but by completely overriding the tab behaviour and taking over from the browser (manually moving focus between fields), and only when a dialog was open. I don't think this is a good solution however, and think it would be better to see how they got around this problem in Bootbox.js first.

[jmwolfe]

I think the fix is in bootstrap itself, as I don't see any tab handling code in bootbox.js.

I would fork this and work on a fix for this, but I'm having GIT difficulties. Seems I already forked master and made some changes for another defect that were submitted as a pull but weren't needed. Now when I fork master it just goes back to my old fork and it has junk that I don't want. I tried reverting changes, but I just don't know if my current source really reflects the bootstrap master. How can do a completely clean fork? Can I delete the old one? It's not jumping out at me.

[jmwolfe]

figured out how to delete my old repo and have reforked it. Will take a go at this issue.

[jmwolfe]

yup... bootstrap 3 has this to keep the focus in the modal:

  Modal.prototype.enforceFocus = function () {
    $(document)
      .off('focusin.bs.modal') // guard against infinite focus loop
      .on('focusin.bs.modal', $.proxy(function (e) {
        if (this.$element[0] !== e.target && !this.$element.has(e.target).length) {
          this.$element.focus()
        }
      }, this))
  }

unfortunately, this is using a lot of heavy jquery, like $.proxy to provide context, event namespacing, the .has() function. Let me see what I can do with @hall5714's suggestion above.

[jmwolfe]

I have a branch commit in my repo that implements a fix for this issue (jmwolfe@759cddb). I would appreciate feedback before bothering with a pull request. This uses the same basic approach that bootstrap 3's modals use (without bootstrap of course). I do assume you want to move your focus after the last item to the first INPUT or TEXTAREA tag. We can discuss that as needed.

[pkozlowski-opensource]

@jmwolfe left some comments in jmwolfe@759cddb

[jmwolfe]

I have incorporated Pawel's feedback in the most recent revision in my tab-fix branch, here: https://github.com/jmwolfe/bootstrap/compare/modal_bg_tab_disable. Hope we can get close and I can propose a PR.

[kelseyishmael]

Hey, I was wondering if there is any eta on when this will be fixed.

[etsuo]

Hi @jmwolfe, any feedback on a PR?

[rattermeyer]

+1 on having this fixed. @jmwolfe: Have actually propsed a PR yet?

[tlierfeld]

+1
Please submit this changes by sending a Pull Request

[nwbvt]

Its been 8 months since that fork was made, can it be assumed that the effort to get them PR'ed into the main repo is pretty much dead?

[chrisirhc]

Perhaps someone else (you) can create a PR containing those changes and credit the original contributors who weren't able to make a PR.

[jmwolfe]

Hey Guys... Sorry I have been delinquent with the PR!

I forked this a while back. I imagine this code base has changed quite a bit since September.

Suggestions on the fastest way to get a useful PR out for you guys? Should I do a new fork from some version (please specify) and re-implement the changes? What about Boostrap 3? I think I did start backing off when I heard 3.0 was coming.. I was not sure a fix based on an earlier version would be useful.

Unfortunately, I took on a more demanding role at my workplace and my time is more limited. I'll try to get a PR together in the next few weeks. If I could get a bit of summary from someone who's had time to follow all the goings on so that the PR would be acceptable, that would be a great contribution.

[chrisirhc]

@jmwolfe , no worries, I think that happens to every one of us (or most of us anyway 😄 ), I think the fastest way would be to create your fix on top the current master branch. We're unlikely to maintain older versions of UI Bootstrap due to our lack of resources. Thank you! 👍

[jmwolfe]

Update to the below... I just did as I suggested and wiped the old repo. Unfortunately I was not smart enough to have the current branch set to mine before I did so, so I lost those changes. :/ Fortunately, I had the changes in my other project (that i need this functionality for) so I got the mods and now have a branch off my master fork working well.

OK. I have my old repository that was forked from angular-ui/boostrap in
September. I can't seem to create a new fork from the current master. When
I do, it just puts me back into my old (forked) repo. Even when I renamed
the old repo. Just keeps going back to it.

I guess I could move the local cloned files elsewhere for reference and
then delete the repo from GitHub and start a new fork... but it seems
GitHub should support something like this without so much pain.

Tips?

On Thu, Jun 12, 2014 at 11:40 PM, Chris Chua notifications@github.com
wrote:

@jmwolfe https://github.com/jmwolfe , no worries, I think that happens
to every one of us (or most of us anyway [image: 😄] ), I think the
fastest way would be to create your fix on top the current master branch.
We're unlikely to maintain older versions of UI Bootstrap due to our lack
of resources. Thank you! [image: 👍]

—
Reply to this email directly or view it on GitHub
#738 (comment)
.

[jmwolfe]

OK! I think I have what appears to be a fix,. However.... in writing the automated tests, I'm having a very difficult time simulating tab presses... looks like FF has security issues that prevent "full user input emulation". In Chrome, the triggerkeyDown() method supplied in the existing modal spec is not changing the activeElement on the document, which is I think the most straightforward way to test whether the focus changed. However, I'm about ready to abandon activeElement probing and try another approach to validate focus... but honestly, after a few hours of googling, we may have to forgo automated testing of this part unless someone has some working examples.

Also, not sure I can do this, but do we need to exclude the move to the address bar when tabbing? that happens at this point in my solution.

Tx

[iandotkelly]

@jmwolfe - its normal with a WCAG compliant accessible modal that focus is entirely contained within the modal - both tab and shift-tab, no moving to the address bar.

[jmwolfe]

Has AngularJS or Angular-UI made any assertions of being WCAG compliant? I
don't think it's even close.

On Sat, Jul 5, 2014 at 8:00 PM, Ian Kelly notifications@github.com wrote:

@jmwolfe https://github.com/jmwolfe - its normal with a WCAG compliant
accessible modal that focus is entirely contained within the modal - both
tab and shift-tab, no moving to the address bar.

—
Reply to this email directly or view it on GitHub
#738 (comment)
.

[jmwolfe]

tabindex is doing interesting things. All works well with no tabindex setting on any element... just tabs through all valid elements in the order of creation (I guess). If you add tabindex ="-1" it will take them out of the tab sequence. Which is good. But if you use any other value of tabindex, the focus will leave the modal unexpectedly. I can capture that and put it back into the first element in the modal.. but not sure that's what we want.

But even stranger, the browser is having a hard time honoring the tabindex order... not sure if that's because i'm using the demo page and there are so many other elements or what.

E.g., I have five tabbable elements, and I put tabindex attrs on the last two. When I open the modal and hit tab, the focus goes to the first of the five elements, not the fourth as I would have expected. Any ideas why?

[jmwolfe]

Sorry all, it appears the PR was considered substandard (it re-implemented tabbing rather than using a bettter approach of catching focusin events). So I am back to the drawing board.

[michelbetancourt]

Have we considered the following?

• Get the first tabbable element and override it's shift-tab behavior
• Get the last tabbable element and override it's tab (regular) behavior

I can POC this and show that this can work with jQuery. What I'm not too sure of is whether there's a jQlite / angular way of getting all tabbable elements which is required for an approach like this to work.

Thoughts?

*** Slight update

See WAI-ARIA 1.0 Authoring Practices, 3.3.1 Trapping Focus - http://www.w3.org/TR/wai-aria-practices/

[jmwolfe]

Your approach does minimize how much of the tabbing behavior we override,
so it's better.

The issue you get here though, is what really is the last tabbable element?
The last one in the DOM is not necessarily where you want to wrap around
to. For instance, on our modals, the last element is the "close" icon in
the upper right hand corner. I really don't want it to tab to that when
shift tabbing. I want it on the Cancel button.

This was the reason I ended up overriding tab behavior in general, so that
tabindex would continue to be supported properly. But as its been pointed
out, this makes it a little brittle, should tabbing behavior standards
change.

We could just use the standard DOM selector to find all the focusable
elements (would that make it too brittle for folks?) and go with the first
and last elements here.

Would it be a bad idea to perhaps add an attribute to the last element (and
maybe the first as well), and $modal could use that as the focal point if
you tab to a non-modal element? If such a decorator attribute isn't found,
it just goes with the first/last focusable element in the modal's DOM

On Wed, Sep 10, 2014 at 1:29 PM, michelbetancourt notifications@github.com
wrote:

Have we considered the following?

• Get the first tabbable element and override it's shift-tab behavior
• Get the last tabbable element and override it's tab (regular)
  behavior

I can POC this and show that this can work with jQuery. What I'm not too
sure of is whether there's a jQlite / angular way of getting all tabbable
elements which is required an approach like this to work.

Thoughts?

—
Reply to this email directly or view it on GitHub
#738 (comment)
.

[iandotkelly]

@jmwolfe

Your suggestion seems pragmatic. Support first/last focusable element by default. Use an attribute to decorate another element if you're using some tab-index behavior.

I'm curious about your example of the close-icon as to why you could not just use DOM order. If its not the first or the last item in your tab order, where does it gain focus?

[iandotkelly]

@jmwolfe - to emphasise my question about your modals. They seem like they would be similar to the example here:

http://accessibility.oit.ncsu.edu/training/aria/modal-window/version-2/

This has a close icon and a cancel button. The keyboard navigation behavior seems completely natural

[kevinoid]

@michelbetancourt that is a good suggestion and good find on the WAI-ARIA guidelines doc. I'd have to assume that the WAI-ARIA guidelines are well-researched when they came to that conclusion, but my concern is that you'd still be determining the tab order in the JavaScript (to determine the first/last tabbable element), which is fragile/difficult.

Although spec change is a concern, I think the bigger issue would be handling dynamic changes to the tab order. If the last element in the tab order is an Accept/Apply button which becomes disabled based on the validity of form elements in the modal, it will be added and removed from the tabbing order as the validity changes. You'd have to detect this and recalculate the tab ordering to find the new first/last element. Same with changes to tabindex, contenteditable, visibility, and element addition/removal.

I suppose you could declare dynamic changes to tab order as unsupported, but that seems likely to bite users.

[jmwolfe]

Its not difficult to recalculate the tabbable elements with each focus
event. My last PR for this checked it once but there was a comment pinting
to a simple change to recalculate each iteration. I was caching them just
to keep things quick, but not caching is no big deal.

If we incorporate a default DOM order, with an option to override with an
additional directive (ng-first-tab, ng-last-tab) to force focus, I think we
should be in good shape.

If there are any JQuery mavens out there that would care to help
deconstruct what is going on with the bootstrap solution to this problem
and how we could implement without JQ (or in JQLite) that would be
helpful. The Jquery-fu employed there is outside my experience scope.

On Wed, Sep 10, 2014 at 3:25 PM, Kevin Locke notifications@github.com
wrote:

@michelbetancourt https://github.com/michelbetancourt that is a good
suggestion and good find on the WAI-ARIA guidelines doc. I'd have to assume
that the WAI-ARIA guidelines are well-researched when they came to that
conclusion, but my concern is that you'd still be determining the tab order
in the JavaScript (to determine the first/last tabbable element), which is
fragile/difficult.

Although spec change is a concern, I think the bigger issue would be
handling dynamic changes to the tab order. If the last element in the tab
order is an Accept/Apply button which becomes disabled based on the
validity of form elements in the modal, it will be added and removed from
the tabbing order as the validity changes. You'd have to detect this and
recalculate the tab ordering to find the new first/last element. Same with
changes to tabindex, contenteditable, visibility, and element
addition/removal.

I suppose you could declare dynamic changes to tab order as unsupported,
but that seems likely to bite users.

—
Reply to this email directly or view it on GitHub
#738 (comment)
.

[jmwolfe]

@iandotkelly , regarding your question about the close... the close "x" icon is actually the third or fourth element in the DOM list (it's at the top) but I would want to tab there only after the last element on the modal. So I really want it to be last, not 3rd. So I would want my shift=tab to go back to the "X", not to the Cancel button. I can handle this with tabindex, and in this next go-round, I'll handle it in an ng-last-tab directive (well, technically attribute with the dashes).

Hope that clarifies.

[chrisirhc]

Trying to help out with jQuery-fu as you requested. If you're trying to rewrite the focusin handler mentioned above without jQuery proxy etc. Here's one way (assuming $modal refers to the modal element wrapped in angular.element):

.on('focusin', function (e) {
    if ($modal[0] !== e.target && !$modal[0].contains(e.target)) {
      $modal.focus()
    }
  })

.contains is in the DOM4 specification and should be supported in all modern browsers (see reference here). It's a good way to do a .has call.

[michelbetancourt]

And in case it helps, here is a POC based on WAI-ARIA 1.0 Authoring Practices, 3.3.1 Trapping Focus - http://www.w3.org/TR/wai-aria-practices/

http://jsfiddle.net/michelbetancourt/ugkwn1bj/25/

POC uses jQuery and jQuery UI to simplify interaction in this example concept.

[michelbetancourt]

I see what you guys were pointing out earlier. I expanded my POC a bit so see if the WAI-ARIA 1.0 Authoring Practices, 3.3.1 Trapping Focus was still viable in some way. It got a bit complex due to tab-index sorting and the need to update the tabbing index to reflect the intended order after sorting.

Anyway in case it helps or inspires here is where I left off with my POC. This POC like the last one includes the cycling feature (going from first to last and last to first):
http://jsfiddle.net/michelbetancourt/56bckz12/

[jmwolfe]

Sincere thanks @michelbetancourt and @chrisirhc and everyone for chiming in with the information and examples! This should all be a helpful boost in moving forward with a robust "trapping focus" implementation. If I need to deviate from the approaches we discuss here, I'll bring back any issues to this defect and we'll sort them out, so we don't have any PR surprises later on.

Unfortunately, like all of us, I have to squeeze in development on this during weekends and free time, so it may take some time to get a working solution ready. If anyone wants to take this on, I am not going to be offended in the least, and would be happy to help out or review, etc.

Just let me know if you do take this on so we don't end up both working at the same time. I think we could also consider collaborating on my cloned repo... i have it clean and ready to work off the latest master. I would just need to figure out how to grant access to others.

[Tobino]

Bonsoir,

Pull Request #2920.
This solution support Tab and Shift+Tab so the focus loop inside the modal. Focus is also returned to the launching element after close. But this solution doesn’t work with tabindex inside despite of my efforts.
Jasmine was quite a nightmare so I didn’t try to simulating tab presses, instead I change focus to a background element.

Manual testing works with Chrome 38/MacOs and Safari 8/MacOs. It doesn’t work with Firefox because of focusin not support. Test with IE will come.

For Safari/MacOs and FF/MacOs don’t forget to turn on full keyboard access.

Any suggestion/improvement are welcome.

[icfantv]

Hey all, it's been nearly seven months since the last update and nearly two years since this issue was first opened. Are we any closer to an acceptable solution? Thanks.

[keyurpatel]

Really very nicely crafted and explained. @michelbetancourt, @chriswu14 , @jmwolfe

[Luukash]

There is an error in the keypress event binding, if you disable the keyboard (in order to disallow the esc key to close the modal) this event never runs, i think this isnt correct (As i want my modals not to be closed, but i dont want tab to go to the background), here is my fix (in my case i never care about esc key)
It could be solved easily.

$document.bind('keydown', function (evt) {
        if (evt.isDefaultPrevented()) {
          return evt;
        }

        var modal = openedWindows.top();
        if (modal /*&& modal.value.keyboard*/) {
          switch (evt.which) {
            //case 27: {
            //  evt.preventDefault();
            //  $rootScope.$apply(function() {
            //    $modalStack.dismiss(modal.key, 'escape key press');
            //  });
            //  break;
            //}
            case 9: {
              $modalStack.loadFocusElementList(modal);
              var focusChanged = false;
              if (evt.shiftKey) {
                if ($modalStack.isFocusInFirstItem(evt)) {
                  focusChanged = $modalStack.focusLastFocusableElement();
                }
              } else {
                if ($modalStack.isFocusInLastItem(evt)) {
                  focusChanged = $modalStack.focusFirstFocusableElement();
                }
              }

              if (focusChanged) {
                evt.preventDefault();
                evt.stopPropagation();
              }
              break;
            }
          }
        }
      });

Ps. Sorry for my bad english

[icfantv]

@Luukash: Hi there! It sounds like you think you've found a bug. The horror! If so, please follow these instructions so we don't lose this since you're commenting on an already closed (and quite old) issue. Thanks!

Unfortunately, if you think you've found a bug and do not follow the above instructions when opening a new issue, we will close the issue due to lack of activity after a reasonable amount of time.