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

Don't try to scroll if editor isn't visible #1350

Closed
wants to merge 1 commit into from
Closed

Don't try to scroll if editor isn't visible #1350

wants to merge 1 commit into from

Conversation

njx
Copy link

@njx njx commented Mar 13, 2013

If someone calls scrollTo(), and then hides the editor before the scroll event is processed, CodeMirror will try to process the scroll while it's hidden, which seems to end up scrolling to (0,0). (I didn't dig into why that happens, but it's plausible to me that the scrolling management stuff would get confused if it's called while the editor is invisible.)

This patch makes it so CM doesn't try to process scrolls while hidden. Since the scroll position is saved in the doc state, it appears that the scroll position gets correctly reset to the scrollTo() position when CM becomes visible again anyway (I didn't trace through to fully verify this, but that appears to be the behavior I'm seeing).

This is somewhat similar to, but different from, the issue and fix for #1236--there, the issue was that a bogus value was explicitly calculated during endOperation() while the editor was hidden. Here, the editor is actually still visible in the endOperation() of the scrollTo()--the problem is that it becomes hidden before the resulting scroll event is asynchronously processed.

@marijnh
Copy link
Member

marijnh commented Mar 13, 2013

Fair enough. I've merged this as is. There may be other similar corner cases that lead to weird scrolling resets, but I haven't run into them yet.

@marijnh marijnh closed this Mar 13, 2013
@njx
Copy link
Author

njx commented Mar 13, 2013

Thanks!

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