Hopefully fixes trigger focus restoration on modal close #13627
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* register the callback before invoking the toggling * listen for `hidden` instead of `hide` * account for the possibility of the associated `show` event being preventDefault-ed Adds relevant unit tests.
This was referenced May 19, 2014
| $target.one('hide.bs.modal', function () { | ||
| $this.is(':visible') && $this.trigger('focus') | ||
| $target.one('show.bs.modal', function (showEvent) { | ||
| if (showEvent.isDefaultPrevented()) return // only register focus restorer if modal will actually get shown |
There was a problem hiding this comment.
wouldn't hidden never get called if shown was prevented?
There was a problem hiding this comment.
Do you mean show or shown?
There was a problem hiding this comment.
The problematic scenario is:
- I click a button to trigger a modal
- The
show.bs.modalevent gets prevented by some code, so the modal doesn't get shown. - I click a different button that triggers the same modal.
- 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.
|
unit tests are great, thanks chris 😍 |
|
@fat So, do the explanations I posted make sense? |
|
yep, lgtm - thanks! |
cvrebert
added a commit
that referenced
this pull request
Jun 6, 2014
Hopefully fixes trigger focus restoration on modal close
|
Thanks for reviewing this. |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #12364? I admit this is not my forte, but this explanation for the cause of the bug seems to make sense, and the new unit tests pass, so...
hiddeninstead ofhideshowevent being preventDefault-edAdds relevant unit tests.
To: @fat for review