maybeScrollWindow is wrong for overflow elements

[xeenon]

It looks like maybeScrollWindow was written for a world where scrolling only happens at a document level. This not really an accurate assumption for a world where many designs or single page web apps use overflow: scroll elements.

In the Safari Web Inspector we are seeing this function go down the path that it thinks a scroll needs to happen, when it does not. It ends up making a scrollNode that gets scrollIntoView() called on it, which does correctly handle overflow scrolling. Unfortunately scrollIntoView() forces a scroll to the top or bottom of the overflow area (which is why you are only calling it if you think it is needed). However, getBoundingClientRect() does not give you a true sense of overflow scroll — so the preflight check is wrong in many cases. This forces our CodeMirror editor to jump around inside its overflow: scroll parent element.

CodeMirror should really try to take advantage of WebKit/Blink's scrollIntoViewIfNeeded() instead, which does exactly what it sounds like. It won't scroll unless the element is really out of view. That way CodeMirror does not need to calculate anything. And ideally it would use an existing element instead of making and throwing away a scrollNode (like cursorDiv).

I tested this code, and it works great for us:

  // If an editor sits on the top or bottom of the window, partially
  // scrolled out of view, this ensures that the cursor is visible.
  function maybeScrollWindow(cm, coords) {
    var cursorDiv = cm.display.cursorDiv.getElementsByClassName("CodeMirror-cursor")[0];
    if (cursorDiv && cursorDiv.scrollIntoViewIfNeeded)
      cursorDiv.scrollIntoViewIfNeeded(false);
  }

You could polyfill scrollIntoViewIfNeeded as well. Here is one: https://gist.github.com/doxxx/8987233 However, that pollyfill only handles one level deep of overflow: scroll elements — unlike the native version.

[marijnh]

This is a duplicate of #1104.

Calling scrollIntoViewIfNeeded when it is available might be a good idea, but the issue there is that it would require creating the extra node every time, which creates another relayout, and thus would cause a significant performance regression. (This can probably be worked around by reordering things so that the dummy node is created at the same time the selection and cursors are drawn.)

The polyfill you link to seems to only look at the node's parent node, and thus won't solve the bug you are reporting here. It would be possible to create a version that walks all the way up the DOM tree, but I'm a bit worried about performance there.

[xeenon]

Yes, that pollyfill needs work.

In my snippet above, I used the cursor inside cursorDiv. Why isn't that an option? Maybe it isn't always present? It works well for scrolling into view the line being typed, at least in our case.

However, it sounds like from #1104, that you won't consider a large pollyfill. Would you consider a hook to replace this CodeMirror function from the outside? Our only choice to fix this right now is to modify CodeMirror, which we would rather not do long term without merging it into master.

[marijnh]

There isn't always a cursor, and there are some kludges applied (an extra 30px at the bottom) in the scrolly div to work around idiosyncracies of CodeMirror's DOM structure.

Would firing a "scrollCursorIntoView" event, that client code can then react to, work for you?

[xeenon]

That 30px would be wrong for a CM instance that has no horizontal scrollbars, as in our case.

An event would work for us, if it prevents the CM implementation.

[marijnh]

That 30px would be wrong for a CM instance that has no horizontal scrollbars, as in our case.

No, it isn't. Inspect the .CodeMirror-scroll element in devtools. It has, by design, an invisible 30px sticking out at the bottom.

Given attached patch, you can do something like the below to handle scrolling the cursor into view:

editor.on("scrollCursorIntoView", function(cm, e) {
  e.preventDefault();
  /* your code here */
});

[marijnh]

(I'm holding off on documenting/finalizing this until I get your feedback. Please let me know whether this works for you.)

[xeenon]

I guess what I meant to say was, the cursor div is the right position already by accounting for that 30px. If it was always there it would work.

Thanks for the patch, I'll give it a try in an hour when I get to my computer.

[xeenon]

But I'm likely not testing the right cases that require that 30px padding. Thanks again, will let you know shortly if it works for us.

[xeenon]

Yes, your fix works for us. Thanks!

[marijnh]

Great. I'll keep this event in the code then. Documented in fa0294e