Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hopefully fixes trigger focus restoration on modal close #13627

Merged
merged 1 commit into from
Jun 6, 2014
Merged

Hopefully fixes trigger focus restoration on modal close #13627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions js/modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,10 +268,13 @@

if ($this.is('a')) e.preventDefault()

Plugin.call($target, option, this)
$target.one('hide.bs.modal', function () {
$this.is(':visible') && $this.trigger('focus')
$target.one('show.bs.modal', function (showEvent) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont quite understand why this is inside the show… maybe im missing something obvious though :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing the setup after the Plugin.call is too late and leaves the tests failing. FWICT, the modal can get shown and dismissed before the code gets a chance to setup the hidden event handler. So we've gotta do it before making the call.
But we need to avoid setting up the handler if the showing of the modal gets cancelled, so we have to wait for show or shown.
IIRC, shown led to test failures, which leaves us with show as the event to wait for.

if (showEvent.isDefaultPrevented()) return // only register focus restorer if modal will actually get shown
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't hidden never get called if shown was prevented?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean show or shown?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problematic scenario is:

  1. I click a button to trigger a modal
  2. The show.bs.modal event gets prevented by some code, so the modal doesn't get shown.
  3. I click a different button that triggers the same modal.
  4. The modal gets successfully shown.

Now when I close the modal, we want the latter button to get refocused (and it'd be best not to have the focus jump around multiple times unnecessarily). Hence, this if statement.

$target.one('hidden.bs.modal', function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hidden change def makes sense 👍

$this.is(':visible') && $this.trigger('focus')
})
})
Plugin.call($target, option, this)
})

}(jQuery);
51 changes: 51 additions & 0 deletions js/tests/unit/modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,4 +201,55 @@ $(function () {

div.remove()
})

test('should restore focus to toggling element when modal is hidden after having been opened via data-api', function () {
stop()
$.support.transition = false
var toggleBtn = $('<button data-toggle="modal" data-target="#modal-test">Launch modal</button>').appendTo('#qunit-fixture')
var div = $('<div id="modal-test"><div class="contents"><div id="close" data-dismiss="modal"></div></div></div>')
div
.on('hidden.bs.modal', function () {
window.setTimeout(function () { // give the focus restoration callback a chance to run
equal(document.activeElement, toggleBtn[0], 'toggling element is once again focused')
div.remove()
toggleBtn.remove()
start()
}, 0)
})
.on('shown.bs.modal', function () {
$('#close').click()
})
.appendTo('#qunit-fixture')
toggleBtn.click()
})

test('should not restore focus to toggling element if the associated show event gets prevented', function () {
stop()
$.support.transition = false
var toggleBtn = $('<button data-toggle="modal" data-target="#modal-test">Launch modal</button>').appendTo('#qunit-fixture')
var otherBtn = $('<button id="other-btn">Golden boy</button>').appendTo('#qunit-fixture')
var div = $('<div id="modal-test"><div class="contents"><div id="close" data-dismiss="modal"></div></div></div>')
div
.one('show.bs.modal', function (e) {
e.preventDefault()
otherBtn.focus()
window.setTimeout(function () { // give the focus event from the previous line a chance to run
div.bootstrapModal('show')
}, 0)
})
.on('hidden.bs.modal', function () {
window.setTimeout(function () { // give the focus restoration callback a chance to run (except it shouldn't run in this case)
equal(document.activeElement, otherBtn[0], 'show was prevented, so focus should not have been restored to toggling element')
div.remove()
toggleBtn.remove()
otherBtn.remove()
start()
}, 0)
})
.on('shown.bs.modal', function () {
$('#close').click()
})
.appendTo('#qunit-fixture')
toggleBtn.click()
})
})