Adding save/restore of scrollbar state when hiding/showing editor. #118
Conversation
…dresses issue 112
|
I'm a little worried that by fixing the symptom here we're hiding some underlying problem that could bite us in other ways. Editors don't normally lose their scroll position when resized, and they don't normally lose their scroll position when merely hidden & re-shown. It's only doing the two at the same time that mucks things up -- and it's hard to say what other state might be getting lost/broken at the same time... Fwiw, I did a quick test and commenting out the refresh() call in EditorManager._showEditor() makes the bug go away. I don't understand what that call does or why it's needed during resize, though (although it seems pretty clear that it IS needed). |
|
@peterflynn I don't believe commenting out the 'refresh()' call in _showEditor makes the bug go away. If you comment that out, it does initially appears to work. However, as soon as you do a tiny bit of scrolling (at least in Lion) in the editor, you're moved back to the top. I agree, however, that this may be masking the symptom of a larger problem. This problem was introduced in our modifications of CodeMirror. You can test this by opening the "demo/theme.html" file on the codemirror website and in our repo. If you add a bunch of code to the editor (so that there is scrolling available), and then do the following three javascript calls: // important to let the browser redraw between each step
editor.getWrapperElement().setAttribute('style', 'display: none');
editor.getWrapperElement().removeAttribute('style');
editor.refresh()You'll see that our CodeMirror code has the bug, and the old code does not. |
|
Looking into this now. |
| // this terrible hack to get the scrollbar element is taken directly | ||
| // from codemirror.js lines 40-47 (which contains the comment "I've | ||
| // never seen more elegant code in my life.") | ||
| var scrollbar = this._editor.getWrapperElement().firstChild.nextSibling; |
There was a problem hiding this comment.
Is this code safe? What if any of the intermediate object references are null? Will Brackets crash, or just fail silently?
Do we need to check references for null like this:
if (this._editor &&
this._editor.getWrapperElement() &&
this._editor.getWrapperElement().firstChild &&
this._editor.getWrapperElement().firstChild.nextSibling) {
var scrollbar = this._editor.getWrapperElement().firstChild.nextSibling;
this._scrollPosition = scrollbar.scrollTop;
}
|
The main issue is that when you set a container to display: none, any children that are scrolled seem to lose their scroll position. CodeMirror recently added code that saves off the scroll position on each scroll and restores it on refresh, which is why the problem doesn't occur in their version--this is essentially the same as Joel's fix, but in CodeMirror itself, which is where I think it belongs. Our fix needs to be slightly different, because (1) we need to save/restore the scrollbar's position, and (2) in order to avoid a noticeable flicker when switching between documents, we need to restore the scroll position before updating the display rather than after. The latter will need to be reconciled with Marijn's code when we send him our scrolling changes. I've made this fix in our CodeMirror repo as pull request 16: I'm going to go ahead and close this pull request since that fix supersedes this one. |
|
This pull request has been superseded by https://github.com/adobe/CodeMirror2/pull/16. Closing. |
@njx @peterflynn
This addresses issue 112 -- preserving scrolling state when switching editors.
The hack is pretty gross to get access to the scrollbar. But, it is exactly what is done in the codemirror.js code.