Should not try to scroll cursor into view while hidden

[njx]

In endOperation(), if the selection has changed, we call calculateScrollPos() to figure out how to scroll the selection into view. However, if the editor is hidden, calculateScrollPos() returns a bogus value, because display.scroller.clientHeight is 0. This value can end up getting saved off in doc.scrollTop, which then messes up future refresh() operations that try to use the cached value. (In our case, where we have a small hidden inline editor, the bad scroll value actually causes the content to become invisible, because it essentially scrolls the entire content out of view.)

We currently have an interim fix that simply skips the block containing the newScrollPos = calculateScrollPos(...) if display.scroller.clientHeight is 0. I can submit that as a pull request, but I suspect it's not the fix you'd want long term, because you probably do eventually want to scroll the cursor into view once the editor becomes visible again. (In our case, we don't happen to need that right now.) So perhaps you'd want to keep a flag indicating that the scroll should happen on the next refresh() (if the editor is visible at that point).

[marijnh]

Good point. As for the 'proper' solution -- I'm not sure that'd be worthwhile. Properly making sure the scroll position of a hidden editor ends up at the place where it would be if it wasn't hidden would require somehow tracking all actions that might influence scroll position, and playing them back when the editor is un-hidden. Seems like that'd be a lot of code for a very obscure use case.

If you submit your simple patch, I'll gladly merge it.

[njx]

Submitted as #1256.

Regarding the "proper" solution, I was thinking of something fairly simple--basically, add a flag that would be set in endOperation() when a selection change would require a scroll-into-view while the editor was invisible, and then make it so on the next refresh(), if that flag was set and the editor is now visible, then scroll the current selection into view.

Alternatively, maybe it would be good enough to just expose a parameter to refresh() that would force the cursor to scroll into view if it's not already visible, so that clients could pass it when they know they're re-showing an invisible editor.

[marijnh]

The problem is that if someone calls, for example, scrollTo when in a hidden state, that should, if we want to accurately simulate persistent scroll positions, also be somehow saved, and clear the flag for scrolling to the cursor, etc. I feel this'll take more code than justified.

Instead of adding a boolean flag to refresh, I think the fact that people can simply call cm.scrollIntoView(cm.getCursor()) before refreshing should be sufficient.

I've merged your patch.

[njx]

Makes sense. Thanks.