Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

fix(modal): Captures focus in both directions - using tab & shift-tab #2920

Closed
wants to merge 2 commits into from
Closed

fix(modal): Captures focus in both directions - using tab & shift-tab #2920

wants to merge 2 commits into from

Conversation

Tobino
Copy link
Contributor

@Tobino Tobino commented Nov 2, 2014

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.
#738

    When dismissed focus is returned to the launching element
@chrisirhc
Copy link
Contributor

As per my comment in #2461 (comment) , I don't think we should be re-implementing the tab behavior.

The upstream Bootstrap behavior of a simple focusin handler should work well as described in #2461 (comment) .

@chrisirhc
Copy link
Contributor

On further reading of your code, I see that it's you're trying to preserve the focus on the previous active element. I think we shouldn't attempt to do that here. That makes it seem as if tabbing is broken. Making it focus on the modal element should be adequate.

@Tobino
Copy link
Contributor Author

Tobino commented Nov 2, 2014

The focus is cycling correctly through the modal as expected by http://www.w3.org/TR/wai-aria-practices/#dialog_modal
Tabbing after the last element and it will go to the first.
Shift tabbing before the first element and it will go to the last.

@Tobino
Copy link
Contributor Author

Tobino commented Nov 2, 2014

plunker preview:
http://plnkr.co/edit/yAbHrAYQ3AkjtlU8dGQy?p=preview

Works with Chrome and Safari. For Safari don’t forget to turn on full keyboard access.

I'm currently working on support for Firefox.

@chrisirhc
Copy link
Contributor

I'm looking at it in detail, the parts that bugs me is the tabbable selector and the trapDomElm.

The firstFocusable/lastFocusable element logic seems unnecessary as that seems to just break tabindex. Without it, wouldn't tabindex work if we always shifted focus to the modal?

Is the trapDomElm required only because we're appending to the body? I'm thinking that'll be removed once we have append to. It seems that you only need to use such an element to trap focus if the modal is inserted at the end of the body.

Also, for Firefox support, it maybe helpful to look at how jQuery does it.

Great work on the tests.

@Tobino
Copy link
Contributor Author

Tobino commented Nov 3, 2014

Modal with tabindex need more investigation it don't seem to be easy to handle.
Example with current version of bootstrap http://plnkr.co/edit/RkuHo9zpsPMvrvVEYUKn?p=preview
or with current version of angular-ui/bootstrap http://plnkr.co/edit/VdQyTI1a5qYp28BnXFZf?p=preview

The trapDomElm is require because we're appending to the body. It also help me to know witch side the user was tabbing (I don't use event key).
There is 3 main cases:

  • The user Shift-Tabbing and focus fall outside the modal => go back to last element.
  • The user Tabbing and focus fall inside trapDomElm => go back to first element.
  • The user Tabbing from address bar and focus fall outside the modal without relatedTarget => go back to first element.

For example, if the modal is not appending to the body, it would be very hard to know which side the focus fall without trapDomElm.

Let me know if I have to support tabindex. Because modal can split tab order page into two different parts : #738 (comment)

@Tobino
Copy link
Contributor Author

Tobino commented Nov 14, 2014

Plunker updated with Firefox support
http://plnkr.co/edit/yAbHrAYQ3AkjtlU8dGQy?p=preview

I'm not a big fan of tabbable selector and the trapDomElm. If we drop this part, the focus will not cycling correctly, but we could open a new issue and close #738

@Tobino Tobino closed this Mar 30, 2015
@karianna karianna added this to the 0.13.0 milestone Mar 30, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants