From f2a08cd178a331549e2239ea4eb3e7cc21e7a7bd Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Thu, 29 May 2014 15:28:41 -0600 Subject: [PATCH 1/5] Update the lines and gutter when the mouseWheelScreenRow changes --- src/gutter-component.coffee | 5 ++++- src/lines-component.coffee | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/gutter-component.coffee b/src/gutter-component.coffee index bef507a6a..fa04c0afc 100644 --- a/src/gutter-component.coffee +++ b/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 diff --git a/src/lines-component.coffee b/src/lines-component.coffee index be75dddc6..98d4cd02d 100644 --- a/src/lines-component.coffee +++ b/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 From 89c57b6d52552b25ffc709367318a7ac6c2b4310 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Thu, 29 May 2014 15:49:34 -0600 Subject: [PATCH 2/5] Only set the mouseWheelScreenRow when scrolling vertically When we handle a mousewheel event targeting a line or line number, we assign the mousewheelScreenRow to prevent the removal of the target node, which interferes with velocity scrolling. However, the ::mousewheelScreenRow is only cleared 100ms after we stop scrolling vertically. This means that if we're only scrolling horizontally, it's never cleared. This causes the line node associated with this screen row to hang around longer until the mousewheel screen row is cleared again, which is not what we want. This commit only assigns the ::mousewheelScreenRow when scrolling vertically, so we can be sure it will be cleared. --- spec/editor-component-spec.coffee | 12 ++++++++++++ src/editor-component.coffee | 5 +++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/spec/editor-component-spec.coffee b/spec/editor-component-spec.coffee index 3362d76a9..feebc299f 100644 --- a/spec/editor-component-spec.coffee +++ b/spec/editor-component-spec.coffee @@ -776,6 +776,18 @@ describe "EditorComponent", -> expect(node.contains(lineNode)).toBe true + it "does not set the mouseWheelScreenRow if scrolling horizontally", -> + 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) + + expect(component.mouseWheelScreenRow).toBe null + 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' diff --git a/src/editor-component.coffee b/src/editor-component.coffee index c93b75eb3..0a1df0a5c 100644 --- a/src/editor-component.coffee +++ b/src/editor-component.coffee @@ -356,16 +356,17 @@ 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) unless animationFramePending requestAnimationFrame => From 0043072ecffa581155ff88f8b25a7360ca60afc5 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Thu, 29 May 2014 20:54:29 -0600 Subject: [PATCH 3/5] Only preserve mouseWheelScreenRow if it's out of the rendered row range Fixes #2429, #2443 Otherwise, it's possible to duplicate lines. If a line is in the rendered row range and it's not in the set of lines returned by the editor, we should remove it no matter what. Line preservation is only intended for lines that are out of view. --- spec/editor-component-spec.coffee | 15 +++++++++++++++ src/editor-component.coffee | 7 +++++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/spec/editor-component-spec.coffee b/spec/editor-component-spec.coffee index feebc299f..8733db9ce 100644 --- a/spec/editor-component-spec.coffee +++ b/spec/editor-component-spec.coffee @@ -788,6 +788,21 @@ describe "EditorComponent", -> 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' diff --git a/src/editor-component.coffee b/src/editor-component.coffee index 0a1df0a5c..de2e40ddb 100644 --- a/src/editor-component.coffee +++ b/src/editor-component.coffee @@ -38,6 +38,7 @@ EditorComponent = React.createClass if @isMounted() renderedRowRange = @getRenderedRowRange() + [renderedStartRow, renderedEndRow] = renderedRowRange scrollHeight = editor.getScrollHeight() scrollWidth = editor.getScrollWidth() scrollTop = editor.getScrollTop() @@ -48,6 +49,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 +59,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 +67,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 } From 115a7e1dfb495e7521cf1261326ba75d1db690d6 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Sat, 31 May 2014 18:20:27 +0900 Subject: [PATCH 4/5] :lipstick: Give mousewheel events their own describe block --- spec/editor-component-spec.coffee | 110 +++++++++++++++--------------- 1 file changed, 56 insertions(+), 54 deletions(-) diff --git a/spec/editor-component-spec.coffee b/spec/editor-component-spec.coffee index 8733db9ce..271ea856a 100644 --- a/spec/editor-component-spec.coffee +++ b/spec/editor-component-spec.coffee @@ -747,74 +747,76 @@ 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 "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] - it "does not set the mouseWheelScreenRow if scrolling horizontally", -> - node.style.height = 4.5 * lineHeightInPixels + 'px' - node.style.width = 20 * charWidth + 'px' - component.measureHeightAndWidth() + 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) - lineNode = node.querySelector('.line') - wheelEvent = new WheelEvent('mousewheel', wheelDeltaX: 10, wheelDeltaY: 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 - expect(component.mouseWheelScreenRow).toBe null + 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() - 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] + lineNumberNode = node.querySelectorAll('.line-number')[1] + wheelEvent = new WheelEvent('mousewheel', wheelDeltaX: 0, wheelDeltaY: -500) + Object.defineProperty(wheelEvent, 'target', get: -> lineNumberNode) + node.dispatchEvent(wheelEvent) - 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 + expect(node.contains(lineNumberNode)).toBe true describe "input events", -> inputNode = null From 2548891b99fdd8ae6a7bfaa6ea34a77fc9793897 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Sat, 31 May 2014 18:36:59 +0900 Subject: [PATCH 5/5] Clear the mousewheelScreenRow even if the event does not cause scrolling If a mousewheel event is triggered when the editor can't be scrolled, we still want to clear the mouseWheelScreenRow. This is typically done when we stop scrolling, but if we never start scrolling it will never happen. This commit adds another timeout to cover that case. --- spec/editor-component-spec.coffee | 16 ++++++++++++++++ src/editor-component.coffee | 10 ++++++++++ 2 files changed, 26 insertions(+) diff --git a/spec/editor-component-spec.coffee b/spec/editor-component-spec.coffee index 271ea856a..e01fbb853 100644 --- a/spec/editor-component-spec.coffee +++ b/spec/editor-component-spec.coffee @@ -790,6 +790,22 @@ describe "EditorComponent", -> expect(component.mouseWheelScreenRow).toBe null + 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 + + expect(editor.getScrollTop()).toBe 0 + + lineNode = node.querySelector('.line') + wheelEvent = new WheelEvent('mousewheel', wheelDeltaX: 0, wheelDeltaY: 10) + Object.defineProperty(wheelEvent, 'target', get: -> lineNode) + node.dispatchEvent(wheelEvent) + + 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') diff --git a/src/editor-component.coffee b/src/editor-component.coffee index de2e40ddb..ede23e203 100644 --- a/src/editor-component.coffee +++ b/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 @@ -370,6 +371,8 @@ EditorComponent = React.createClass # Scrolling vertically @pendingVerticalScrollDelta -= wheelDeltaY @mouseWheelScreenRow = @screenRowForNode(event.target) + @clearMouseWheelScreenRowAfterDelay ?= debounce(@clearMouseWheelScreenRow, @mouseWheelScreenRowClearDelay) + @clearMouseWheelScreenRowAfterDelay() unless animationFramePending requestAnimationFrame => @@ -379,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