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

[fixed] Don't steal focus from a descendent when rendering #222

Merged
merged 1 commit into from
Sep 14, 2016

Conversation

conlanpatrek
Copy link
Contributor

@conlanpatrek conlanpatrek commented Sep 12, 2016

Fixes #220.

Changes proposed:

  • Before focusing on the content div, check to see that the currently focused element isn't a descendent of the content div, to respect any focusing the application may have already done.

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.

@claydiffrient
Copy link
Contributor

I'm confused by this. The modal contents, before being opened doesn't exist in the DOM, so nothing inside it should be able to be focused prior to the modal opening.

If you need to set focus to something after the modal has opened, you can you use the onAfterOpen function prop to set focus to something other than the content div immediately after opening.

@conlanpatrek
Copy link
Contributor Author

React life cycle events and ref callbacks are called on children before they are called on parents. When componentDidUpdate is called on ModalPortal, it's already been called on all of it's descendants, so there plenty of opportunity for a child to have already given focus to some element in the modal. Right now though, that focus is being reassigned to the content div immediately afterward.

You could set focus in the onAfterOpen hook, but that would require you to traverse down from the component that opened the modal, coupling all of the components you pass through (especially problematic if your focusable element is deeply nested). Devs also already manage focus with componentDidMount/Update and ref callbacks. It seems strange that they'd need to implement a second set of focus logic to account for their component appearing in a modal.

@claydiffrient
Copy link
Contributor

Good points. I don't see any reason that we shouldn't implement this.

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.

2 participants