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

Preserve focus #23

Merged
merged 2 commits into from
Jul 31, 2015
Merged

Preserve focus #23

merged 2 commits into from
Jul 31, 2015

Conversation

adamesque
Copy link
Contributor

Currently, if you have a component that attempts to shift focus on didInsertElement, and you wrap that component in a wormhole, it's possible for focus to be lost because the inner component's didInsertElement fires before ember-wormhole's, and focus is lost when reparenting DOM elements.

This PR ensures that the element that has focus prior to appendToDestination retains focus once the append operation is complete. Should resolve #22.

@krisselden
Copy link
Contributor

@lukemelia I'm +1 on this, what do you think?

@lukemelia
Copy link
Contributor

Seems OK to me. Could this behavior be problematic in some circumstances? Do we need an opt out?

@adamesque
Copy link
Contributor Author

Could this behavior be problematic in some circumstances? Do we need an opt out?

It seems unlikely, but I'm not certain. I initially considered only refocusing if the previously-focused element was part of the wormhole'd content, which is a little more expensive to calculate but feels slightly safer.

BUT — I'd consider the current loss of focus to be an undefined/undocumented behavior (if not an outright bug), so perhaps this PR could include some documentation defining the new behavior and leave it at that.

Thoughts?

@lukemelia
Copy link
Contributor

Let's start with docs on this and we'll see if we get any reports of issues. I'm concerned that people who have events set on focus may be surprised to have them running twice. Then again, to the point of this PR, they may be surprised to have lost focus too.

@sethkinast
Copy link

We're working around this in our Ember app by storing the focus state and re-focusing manually if the wormhole activates. Would like to see focus preserved :)

@krisselden
Copy link
Contributor

@lukemelia I think the latter is more surprising and this should just be merged.

lukemelia added a commit that referenced this pull request Jul 31, 2015
@lukemelia lukemelia merged commit 0dfc76e into yapplabs:master Jul 31, 2015
@lukemelia
Copy link
Contributor

I'm sold. Merged.

@lukemelia
Copy link
Contributor

Thanks @adamesque!

@adamesque adamesque deleted the preserve-focus branch July 31, 2015 19:10
@adamesque
Copy link
Contributor Author

Thank you!

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.

"wormholing" elements destroys focus, selection
4 participants