Merge pull request #2463 from atom/ns-react-fix-duplicate-lines

Fix line duplication in React editor

spec/editor-component-spec.coffee

@@ -747,47 +747,92 @@ describe "EditorComponent", ->
       expect(horizontalScrollbarNode.scrollWidth).toBe gutterNode.offsetWidth + editor.getScrollWidth()
 
     describe "when a mousewheel event occurs on the editor", ->
-      it "updates the horizontal or vertical scrollbar depending on which delta is greater (x or y)", ->
+
+  describe "mousewheel events", ->
+    it "updates the scrollLeft or scrollTop on mousewheel events depending on which delta is greater (x or y)", ->
+      node.style.height = 4.5 * lineHeightInPixels + 'px'
+      node.style.width = 20 * charWidth + 'px'
+      component.measureHeightAndWidth()
+
+      expect(verticalScrollbarNode.scrollTop).toBe 0
+      expect(horizontalScrollbarNode.scrollLeft).toBe 0
+
+      node.dispatchEvent(new WheelEvent('mousewheel', wheelDeltaX: -5, wheelDeltaY: -10))
+      expect(verticalScrollbarNode.scrollTop).toBe 10
+      expect(horizontalScrollbarNode.scrollLeft).toBe 0
+
+      node.dispatchEvent(new WheelEvent('mousewheel', wheelDeltaX: -15, wheelDeltaY: -5))
+      expect(verticalScrollbarNode.scrollTop).toBe 10
+      expect(horizontalScrollbarNode.scrollLeft).toBe 15
+
+    describe "when the mousewheel event's target is a line", ->
+      it "keeps the line on the DOM if it is scrolled off-screen", ->
         node.style.height = 4.5 * lineHeightInPixels + 'px'
         node.style.width = 20 * charWidth + 'px'
         component.measureHeightAndWidth()
 
-        expect(verticalScrollbarNode.scrollTop).toBe 0
-        expect(horizontalScrollbarNode.scrollLeft).toBe 0
+        lineNode = node.querySelector('.line')
+        wheelEvent = new WheelEvent('mousewheel', wheelDeltaX: 0, wheelDeltaY: -500)
+        Object.defineProperty(wheelEvent, 'target', get: -> lineNode)
+        node.dispatchEvent(wheelEvent)
 
-        node.dispatchEvent(new WheelEvent('mousewheel', wheelDeltaX: -5, wheelDeltaY: -10))
-        expect(verticalScrollbarNode.scrollTop).toBe 10
-        expect(horizontalScrollbarNode.scrollLeft).toBe 0
+        expect(node.contains(lineNode)).toBe true
 
-        node.dispatchEvent(new WheelEvent('mousewheel', wheelDeltaX: -15, wheelDeltaY: -5))
-        expect(verticalScrollbarNode.scrollTop).toBe 10
-        expect(horizontalScrollbarNode.scrollLeft).toBe 15
+      it "does not set the mouseWheelScreenRow if scrolling horizontally", ->
+        node.style.height = 4.5 * lineHeightInPixels + 'px'
+        node.style.width = 20 * charWidth + 'px'
+        component.measureHeightAndWidth()
 
-      describe "when the mousewheel event's target is a line", ->
-        it "keeps the line on the DOM if it is scrolled off-screen", ->
-          node.style.height = 4.5 * lineHeightInPixels + 'px'
-          node.style.width = 20 * charWidth + 'px'
-          component.measureHeightAndWidth()
+        lineNode = node.querySelector('.line')
+        wheelEvent = new WheelEvent('mousewheel', wheelDeltaX: 10, wheelDeltaY: 0)
+        Object.defineProperty(wheelEvent, 'target', get: -> lineNode)
+        node.dispatchEvent(wheelEvent)
 
-          lineNode = node.querySelector('.line')
-          wheelEvent = new WheelEvent('mousewheel', wheelDeltaX: 0, wheelDeltaY: -500)
-          Object.defineProperty(wheelEvent, 'target', get: -> lineNode)
-          node.dispatchEvent(wheelEvent)
+        expect(component.mouseWheelScreenRow).toBe null
 
-          expect(node.contains(lineNode)).toBe true
+      it "clears the mouseWheelScreenRow after a delay even if the event does not cause scrolling", ->
+        spyOn(_._, 'now').andCallFake -> window.now # Ensure _.debounce is based on our fake spec timeline
 
-      describe "when the mousewheel event's target is a line number", ->
-        it "keeps the line number on the DOM if it is scrolled off-screen", ->
-          node.style.height = 4.5 * lineHeightInPixels + 'px'
-          node.style.width = 20 * charWidth + 'px'
-          component.measureHeightAndWidth()
+        expect(editor.getScrollTop()).toBe 0
 
-          lineNumberNode = node.querySelectorAll('.line-number')[1]
-          wheelEvent = new WheelEvent('mousewheel', wheelDeltaX: 0, wheelDeltaY: -500)
-          Object.defineProperty(wheelEvent, 'target', get: -> lineNumberNode)
-          node.dispatchEvent(wheelEvent)
+        lineNode = node.querySelector('.line')
+        wheelEvent = new WheelEvent('mousewheel', wheelDeltaX: 0, wheelDeltaY: 10)
+        Object.defineProperty(wheelEvent, 'target', get: -> lineNode)
+        node.dispatchEvent(wheelEvent)
 
-          expect(node.contains(lineNumberNode)).toBe true
+        expect(editor.getScrollTop()).toBe 0
+
+        expect(component.mouseWheelScreenRow).toBe 0
+        advanceClock(component.mouseWheelScreenRowClearDelay)
+        expect(component.mouseWheelScreenRow).toBe null
+
+      it "does not preserve the line if it is on screen", ->
+        expect(node.querySelectorAll('.line-number').length).toBe 14 # dummy line
+        lineNodes = node.querySelectorAll('.line')
+        expect(lineNodes.length).toBe 13
+        lineNode = lineNodes[0]
+
+        wheelEvent = new WheelEvent('mousewheel', wheelDeltaX: 0, wheelDeltaY: 100) # goes nowhere, we're already at scrollTop 0
+        Object.defineProperty(wheelEvent, 'target', get: -> lineNode)
+        node.dispatchEvent(wheelEvent)
+
+        expect(component.mouseWheelScreenRow).toBe 0
+        editor.insertText("hello")
+        expect(node.querySelectorAll('.line-number').length).toBe 14 # dummy line
+        expect(node.querySelectorAll('.line').length).toBe 13
+
+    describe "when the mousewheel event's target is a line number", ->
+      it "keeps the line number on the DOM if it is scrolled off-screen", ->
+        node.style.height = 4.5 * lineHeightInPixels + 'px'
+        node.style.width = 20 * charWidth + 'px'
+        component.measureHeightAndWidth()
+
+        lineNumberNode = node.querySelectorAll('.line-number')[1]
+        wheelEvent = new WheelEvent('mousewheel', wheelDeltaX: 0, wheelDeltaY: -500)
+        Object.defineProperty(wheelEvent, 'target', get: -> lineNumberNode)
+        node.dispatchEvent(wheelEvent)
+
+        expect(node.contains(lineNumberNode)).toBe true
 
   describe "input events", ->
     inputNode = null

src/editor-component.coffee

@@ -29,6 +29,7 @@ EditorComponent = React.createClass
   pendingVerticalScrollDelta: 0
   pendingHorizontalScrollDelta: 0
   mouseWheelScreenRow: null
+  mouseWheelScreenRowClearDelay: 150
 
   render: ->
     {focused, fontSize, lineHeight, fontFamily, showIndentGuide, showInvisibles, visible} = @state
@@ -38,6 +39,7 @@ EditorComponent = React.createClass
 
     if @isMounted()
       renderedRowRange = @getRenderedRowRange()
+      [renderedStartRow, renderedEndRow] = renderedRowRange
       scrollHeight = editor.getScrollHeight()
       scrollWidth = editor.getScrollWidth()
       scrollTop = editor.getScrollTop()
@@ -48,6 +50,8 @@ EditorComponent = React.createClass
       verticalScrollbarWidth = editor.getVerticalScrollbarWidth()
       verticallyScrollable = editor.verticallyScrollable()
       horizontallyScrollable = editor.horizontallyScrollable()
+      if @mouseWheelScreenRow? and not (renderedStartRow <= @mouseWheelScreenRow < renderedEndRow)
+        mouseWheelScreenRow = @mouseWheelScreenRow
 
     className = 'editor editor-colors react'
     className += ' is-focused' if focused
@@ -56,7 +60,7 @@ EditorComponent = React.createClass
       GutterComponent {
         ref: 'gutter', editor, renderedRowRange, maxLineNumberDigits,
         scrollTop, scrollHeight, lineHeight, lineHeightInPixels, fontSize, fontFamily,
-        @pendingChanges, onWidthChanged: @onGutterWidthChanged, @mouseWheelScreenRow
+        @pendingChanges, onWidthChanged: @onGutterWidthChanged, mouseWheelScreenRow
       }
 
       EditorScrollViewComponent {
@@ -64,7 +68,7 @@ EditorComponent = React.createClass
         lineHeight, lineHeightInPixels, renderedRowRange, @pendingChanges,
         scrollTop, scrollLeft, scrollHeight, scrollWidth, @scrollingVertically,
         @cursorsMoved, @selectionChanged, @selectionAdded, cursorBlinkPeriod,
-        cursorBlinkResumeDelay, @onInputFocused, @onInputBlurred, @mouseWheelScreenRow,
+        cursorBlinkResumeDelay, @onInputFocused, @onInputBlurred, mouseWheelScreenRow,
         invisibles, visible, scrollViewHeight, focused
       }
 
@@ -356,16 +360,19 @@ EditorComponent = React.createClass
 
   onMouseWheel: (event) ->
     event.preventDefault()
-    screenRow = @screenRowForNode(event.target)
-    @mouseWheelScreenRow = screenRow if screenRow?
     animationFramePending = @pendingHorizontalScrollDelta isnt 0 or @pendingVerticalScrollDelta isnt 0
 
     # Only scroll in one direction at a time
     {wheelDeltaX, wheelDeltaY} = event
     if Math.abs(wheelDeltaX) > Math.abs(wheelDeltaY)
+      # Scrolling horizontally
       @pendingHorizontalScrollDelta -= wheelDeltaX
     else
+      # Scrolling vertically
       @pendingVerticalScrollDelta -= wheelDeltaY
+      @mouseWheelScreenRow = @screenRowForNode(event.target)
+      @clearMouseWheelScreenRowAfterDelay ?= debounce(@clearMouseWheelScreenRow, @mouseWheelScreenRowClearDelay)
+      @clearMouseWheelScreenRowAfterDelay()
 
     unless animationFramePending
       requestAnimationFrame =>
@@ -375,6 +382,13 @@ EditorComponent = React.createClass
         @pendingVerticalScrollDelta = 0
         @pendingHorizontalScrollDelta = 0
 
+  clearMouseWheelScreenRow: ->
+    if @mouseWheelScreenRow?
+      @mouseWheelScreenRow = null
+      @requestUpdate()
+
+  clearMouseWheelScreenRowAfterDelay: null # created lazily
+
   screenRowForNode: (node) ->
     while node isnt document
       if screenRow = node.dataset.screenRow

src/gutter-component.coffee

@@ -33,7 +33,10 @@ GutterComponent = React.createClass
   # non-zero-delta change to the screen lines has occurred within the current
   # visible row range.
   shouldComponentUpdate: (newProps) ->
-    return true unless isEqualForProperties(newProps, @props, 'renderedRowRange', 'scrollTop', 'lineHeightInPixels', 'fontSize')
+    return true unless isEqualForProperties(newProps, @props,
+      'renderedRowRange', 'scrollTop', 'lineHeightInPixels', 'fontSize',
+      'mouseWheelScreenRow'
+    )
 
     {renderedRowRange, pendingChanges} = newProps
     for change in pendingChanges when Math.abs(change.screenDelta) > 0 or Math.abs(change.bufferDelta) > 0

src/lines-component.coffee

@@ -40,7 +40,7 @@ LinesComponent = React.createClass
     return true unless isEqualForProperties(newProps, @props,
       'renderedRowRange', 'fontSize', 'fontFamily', 'lineHeight', 'lineHeightInPixels',
       'scrollTop', 'scrollLeft', 'showIndentGuide', 'scrollingVertically', 'invisibles',
-      'visible', 'scrollViewHeight'
+      'visible', 'scrollViewHeight', 'mouseWheelScreenRow'
     )
 
     {renderedRowRange, pendingChanges} = newProps