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.

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

Check if focusLaterElements array is empty before popping #581

Merged
merged 1 commit into from
Dec 12, 2017

Conversation

indiesquidge
Copy link
Contributor

@indiesquidge indiesquidge commented Dec 12, 2017

Fixes #577.

Changes proposed

Without a check to see if the focusLaterElements array is empty, we end up calling pop() on an empty array—resulting in an undefined value—and inadvertently jump to the catch block when we attempt to call focus() on undefined.

This PR ensures that if there are no existing elements to return focus to once our modal is closed (this happens often in a testing env), we do not throw a TypeError by accident.

Acceptance Checklist

  • All commits have been squashed to one.
  • The commit message follows the guidelines in CONTRIBUTING.md.
  • Documentation (README.md) and examples have been updated as needed.
  • If this is a code change, a spec testing the functionality has been added.
  • If the commit message has [changed] or [removed], there is an upgrade path above.

Without a check to see if the focusLaterElements array is empty, we
end up calling `pop()` on an empty array—resulting in an undefined
value—and inadvertently jump to the `catch` block when we attempt to
call `focus()` on `undefined`.

This ensures that if there are no existing elements to return focus to
once our modal is closed (this happens often in a testing env), we do
not throw a TypeError by accident.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 86.232% when pulling d3b34ba on indiesquidge:fix-return-focus-warning into 33e1fe1 on reactjs:master.

@diasbruno diasbruno merged commit eb5ea07 into reactjs:master Dec 12, 2017
@diasbruno
Copy link
Collaborator

Thanks, @indiesquidge. Would you like to join and help manage the react-modal?

@diasbruno
Copy link
Collaborator

You have provided a lot of great reports and you really dig in to find a solution for issues. 👍

@indiesquidge indiesquidge deleted the fix-return-focus-warning branch December 13, 2017 23:48
@indiesquidge
Copy link
Contributor Author

@diasbruno, yeah I'd love to help manage react-modal! I appreciate your offer! Is there a place we can discuss the logistics of this in more detail?

@diasbruno
Copy link
Collaborator

You can reach me on the email on my GitHub and the gitter channel I've created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'You tried to return focus to but it is not in the DOM anymore' warning.
3 participants