[fixed] Handle case when no tabbable element exists #120
Conversation
|
@evoyy This looks like a great change. Can you add a spec that tests the functionality? I generally like specs to accompany code changes. |
|
@claydiffrient Spec added. |
| @@ -92,6 +92,12 @@ describe('Modal', function () { | |||
| }); | |||
| }); | |||
|
|
|||
| it('handles case when child has no tabbable elements', function() { | |||
There was a problem hiding this comment.
It appears that you forgot to make an assertion in the spec. It just opens the modal, presses tab then closes it.
There was a problem hiding this comment.
@claydiffrient I have updated the spec to use assert.doesNotThrow().
|
Okay, I checked out the code and took a look at the actual functionality this makes happen. I'm slightly concerned because this fix allows focus to escape from the modal, which from an accessibility standpoint isn't ideal. Granted I think most use cases of the modal will include a close button of some nature, which avoids this problem. This is nice in that it avoids the errors being thrown when there is nothing to tab to when the user tabs like is currently on master. I think this is good for now. I want to think about maybe adding a way to switch between modal and non-modal in the future probably though. |
Fixes #44