Always check whether to scroll window on .scrollIntoView

Issue codemirror#4491

Author: marijnh

src/display/operations.js

@@ -155,9 +155,9 @@ function endOperation_finish(op) {
   }
   // If we need to scroll a specific position into view, do so.
   if (op.scrollToPos) {
-    let coords = scrollPosIntoView(cm, clipPos(doc, op.scrollToPos.from),
-                                   clipPos(doc, op.scrollToPos.to), op.scrollToPos.margin)
-    if (op.scrollToPos.isCursor && cm.state.focused) maybeScrollWindow(cm, coords)
+    let rect = scrollPosIntoView(cm, clipPos(doc, op.scrollToPos.from),
+                                 clipPos(doc, op.scrollToPos.to), op.scrollToPos.margin)
+    maybeScrollWindow(cm, rect)
   }
 
   // Fire events for markers that are hidden/unidden by editing or

src/display/scrolling.js

@@ -10,17 +10,17 @@ import { setScrollLeft, setScrollTop } from "./scroll_events"
 
 // If an editor sits on the top or bottom of the window, partially
 // scrolled out of view, this ensures that the cursor is visible.
-export function maybeScrollWindow(cm, coords) {
+export function maybeScrollWindow(cm, rect) {
   if (signalDOMEvent(cm, "scrollCursorIntoView")) return
 
   let display = cm.display, box = display.sizer.getBoundingClientRect(), doScroll = null
-  if (coords.top + box.top < 0) doScroll = true
-  else if (coords.bottom + box.top > (window.innerHeight || document.documentElement.clientHeight)) doScroll = false
+  if (rect.top + box.top < 0) doScroll = true
+  else if (rect.bottom + box.top > (window.innerHeight || document.documentElement.clientHeight)) doScroll = false
   if (doScroll != null && !phantom) {
     let scrollNode = elt("div", "\u200b", null, `position: absolute;
-                         top: ${coords.top - display.viewOffset - paddingTop(cm.display)}px;
-                         height: ${coords.bottom - coords.top + scrollGap(cm) + display.barHeight}px;
-                         left: ${coords.left}px; width: 2px;`)
+                         top: ${rect.top - display.viewOffset - paddingTop(cm.display)}px;
+                         height: ${rect.bottom - rect.top + scrollGap(cm) + display.barHeight}px;
+                         left: ${rect.left}px; width: ${Math.max(2, rect.right - rect.left)}px;`)
     cm.display.lineSpace.appendChild(scrollNode)
     scrollNode.scrollIntoView(doScroll)
     cm.display.lineSpace.removeChild(scrollNode)
@@ -32,15 +32,16 @@ export function maybeScrollWindow(cm, coords) {
 // measured, the position of something may 'drift' during drawing).
 export function scrollPosIntoView(cm, pos, end, margin) {
   if (margin == null) margin = 0
-  let coords
+  let rect
   for (let limit = 0; limit < 5; limit++) {
     let changed = false
-    coords = cursorCoords(cm, pos)
+    let coords = cursorCoords(cm, pos)
     let endCoords = !end || end == pos ? coords : cursorCoords(cm, end)
-    let scrollPos = calculateScrollPos(cm, Math.min(coords.left, endCoords.left),
-                                       Math.min(coords.top, endCoords.top) - margin,
-                                       Math.max(coords.left, endCoords.left),
-                                       Math.max(coords.bottom, endCoords.bottom) + margin)
+    rect = {left: Math.min(coords.left, endCoords.left),
+            top: Math.min(coords.top, endCoords.top) - margin,
+            right: Math.max(coords.left, endCoords.left),
+            bottom: Math.max(coords.bottom, endCoords.bottom) + margin}
+    let scrollPos = calculateScrollPos(cm, rect)
     let startTop = cm.doc.scrollTop, startLeft = cm.doc.scrollLeft
     if (scrollPos.scrollTop != null) {
       setScrollTop(cm, scrollPos.scrollTop)
@@ -52,12 +53,12 @@ export function scrollPosIntoView(cm, pos, end, margin) {
     }
     if (!changed) break
   }
-  return coords
+  return rect
 }
 
 // Scroll a given set of coordinates into view (immediately).
-export function scrollIntoView(cm, x1, y1, x2, y2) {
-  let scrollPos = calculateScrollPos(cm, x1, y1, x2, y2)
+export function scrollIntoView(cm, rect) {
+  let scrollPos = calculateScrollPos(cm, rect)
   if (scrollPos.scrollTop != null) setScrollTop(cm, scrollPos.scrollTop)
   if (scrollPos.scrollLeft != null) setScrollLeft(cm, scrollPos.scrollLeft)
 }
@@ -66,31 +67,31 @@ export function scrollIntoView(cm, x1, y1, x2, y2) {
 // rectangle into view. Returns an object with scrollTop and
 // scrollLeft properties. When these are undefined, the
 // vertical/horizontal position does not need to be adjusted.
-export function calculateScrollPos(cm, x1, y1, x2, y2) {
+export function calculateScrollPos(cm, rect) {
   let display = cm.display, snapMargin = textHeight(cm.display)
-  if (y1 < 0) y1 = 0
+  if (rect.top < 0) rect.top = 0
   let screentop = cm.curOp && cm.curOp.scrollTop != null ? cm.curOp.scrollTop : display.scroller.scrollTop
   let screen = displayHeight(cm), result = {}
-  if (y2 - y1 > screen) y2 = y1 + screen
+  if (rect.bottom - rect.top > screen) rect.bottom = rect.top + screen
   let docBottom = cm.doc.height + paddingVert(display)
-  let atTop = y1 < snapMargin, atBottom = y2 > docBottom - snapMargin
-  if (y1 < screentop) {
-    result.scrollTop = atTop ? 0 : y1
-  } else if (y2 > screentop + screen) {
-    let newTop = Math.min(y1, (atBottom ? docBottom : y2) - screen)
+  let atTop = rect.top < snapMargin, atBottom = rect.bottom > docBottom - snapMargin
+  if (rect.top < screentop) {
+    result.scrollTop = atTop ? 0 : rect.top
+  } else if (rect.bottom > screentop + screen) {
+    let newTop = Math.min(rect.top, (atBottom ? docBottom : rect.bottom) - screen)
     if (newTop != screentop) result.scrollTop = newTop
   }
 
   let screenleft = cm.curOp && cm.curOp.scrollLeft != null ? cm.curOp.scrollLeft : display.scroller.scrollLeft
   let screenw = displayWidth(cm) - (cm.options.fixedGutter ? display.gutters.offsetWidth : 0)
-  let tooWide = x2 - x1 > screenw
-  if (tooWide) x2 = x1 + screenw
-  if (x1 < 10)
+  let tooWide = rect.right - rect.left > screenw
+  if (tooWide) rect.right = rect.left + screenw
+  if (rect.left < 10)
     result.scrollLeft = 0
-  else if (x1 < screenleft)
-    result.scrollLeft = Math.max(0, x1 - (tooWide ? 0 : 10))
-  else if (x2 > screenw + screenleft - 3)
-    result.scrollLeft = x2 + (tooWide ? 0 : 10) - screenw
+  else if (rect.left < screenleft)
+    result.scrollLeft = Math.max(0, rect.left - (tooWide ? 0 : 10))
+  else if (rect.right > screenw + screenleft - 3)
+    result.scrollLeft = rect.right + (tooWide ? 0 : 10) - screenw
   return result
 }
 
@@ -113,7 +114,7 @@ export function ensureCursorVisible(cm) {
     from = cur.ch ? Pos(cur.line, cur.ch - 1) : cur
     to = Pos(cur.line, cur.ch + 1)
   }
-  cm.curOp.scrollToPos = {from: from, to: to, margin: cm.options.cursorScrollMargin, isCursor: true}
+  cm.curOp.scrollToPos = {from: from, to: to, margin: cm.options.cursorScrollMargin}
 }
 
 // When an operation has its scrollToPos property set, and another
@@ -125,10 +126,12 @@ export function resolveScrollToPos(cm) {
   if (range) {
     cm.curOp.scrollToPos = null
     let from = estimateCoords(cm, range.from), to = estimateCoords(cm, range.to)
-    let sPos = calculateScrollPos(cm, Math.min(from.left, to.left),
-                                  Math.min(from.top, to.top) - range.margin,
-                                  Math.max(from.right, to.right),
-                                  Math.max(from.bottom, to.bottom) + range.margin)
+    let sPos = calculateScrollPos(cm, {
+      left: Math.min(from.left, to.left),
+      top: Math.min(from.top, to.top) - range.margin,
+      right: Math.max(from.right, to.right),
+      bottom: Math.max(from.bottom, to.bottom) + range.margin
+    })
     cm.scrollTo(sPos.scrollLeft, sPos.scrollTop)
   }
 }

src/edit/methods.js

@@ -252,7 +252,7 @@ export default function(CodeMirror) {
         node.style.left = left + "px"
       }
       if (scroll)
-        scrollIntoView(this, left, top, left + node.offsetWidth, top + node.offsetHeight)
+        scrollIntoView(this, {left, top, right: left + node.offsetWidth, bottom: top + node.offsetHeight})
     },
 
     triggerOnKeyDown: methodOp(onKeyDown),
@@ -388,10 +388,12 @@ export default function(CodeMirror) {
         resolveScrollToPos(this)
         this.curOp.scrollToPos = range
       } else {
-        let sPos = calculateScrollPos(this, Math.min(range.from.left, range.to.left),
-                                      Math.min(range.from.top, range.to.top) - range.margin,
-                                      Math.max(range.from.right, range.to.right),
-                                      Math.max(range.from.bottom, range.to.bottom) + range.margin)
+        let sPos = calculateScrollPos(this, {
+          left: Math.min(range.from.left, range.to.left),
+          top: Math.min(range.from.top, range.to.top) - range.margin,
+          right: Math.max(range.from.right, range.to.right),
+          bottom: Math.max(range.from.bottom, range.to.bottom) + range.margin
+        })
         this.scrollTo(sPos.scrollLeft, sPos.scrollTop)
       }
     }),