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

Fix activeElement check #193 #194

Merged
merged 2 commits into from
Sep 4, 2017
Merged

Fix activeElement check #193 #194

merged 2 commits into from
Sep 4, 2017

Conversation

michaelwnelson
Copy link
Contributor

Closes #193

@danez
Copy link
Collaborator

danez commented Aug 12, 2017

checking for typeof unknown seems weird to me as this is not really somewhere in the spec. I would think it is more clear what the intend is if we do the same fix that vue did: vuejs/vue@fc3d7cd

Do you think that would work? Also please add a comment to the code so we know why we do this.

@michaelwnelson
Copy link
Contributor Author

Sure thing, I'll reimplement similar to vuejs/vue@fc3d7cd and add a comment for context.

@michaelwnelson
Copy link
Contributor Author

michaelwnelson commented Aug 14, 2017

The most recent PR failed because the catch block is empty (caught in eslint rules). Is there something you'd prefer be done in the catch block?

Or, I could update the try to instead evaluate if typeof document.activeElement resolves to an 'object'.

var activeElementAvailable = typeof document.activeElement === 'object';

@danez danez merged commit f6f0bdf into reactjs:master Sep 4, 2017
danez pushed a commit that referenced this pull request Sep 5, 2017
* Fix activeElement check #193

* Add test and fix check
danez pushed a commit that referenced this pull request Sep 5, 2017
* Fix activeElement check #193

* Add test and fix check
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants