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 issue 104: Only call setNativeRange() in response to a mutation when the editor has focus #1407

Closed
wants to merge 1 commit into from

Conversation

kloots
Copy link

@kloots kloots commented Apr 13, 2017

This is a fix for issue #1404.

@jhchen
Copy link
Member

jhchen commented Apr 14, 2017

Need to investigate a bit more but two current thoughts:

  • I generally prefer to avoid setTimeout as it makes code non-deterministic and hides the real stack trace when issues occur. But some tasks cannot be done without it but this warrants exploring alternatives.

  • There is a TODO above this code that iirc was because of a Firefox issue but it could be the case that it would be best not to have this SCROLL_UPDATE handler at all

@kloots
Copy link
Author

kloots commented Apr 14, 2017

@jhchen thanks for the reply.

I saw that TODO in the comment above, but lacked the context as to why the SCROLL_UPDATE handler existed in the first place. Can you recall? If it no longer needs to exist, then that would be the best solution.

If it needs to remain, I can't think of a better solution than leveraging setTimeout(). I'm with you on the downsides. But for this specific problem (ensuring document.activeElement is accurate) I'm not aware of another solution.

If there's anything I can do to move this forward, please let me know.

@jhchen
Copy link
Member

jhchen commented Apr 17, 2017

I opted for another way to fix the issue but thank you for the detailed report in #1404 -- it made the investigation a lot easier.

The code was originally added to deal with if you select-all (cmd-a) and typed a character, Firefox would lose the selection. There was a logical inconsistency with how the fix as it should only care about the case where it has focus but getNativeRange() returns the range regardless.

@jhchen jhchen closed this Apr 17, 2017
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