Merge pull request atom#1923 from atom/ns-fix-scrollbar-overlap

Refine scrollbar interactions

spec/display-buffer-spec.coffee

@@ -979,6 +979,7 @@ describe "DisplayBuffer", ->
   describe "::setScrollLeft", ->
     beforeEach ->
       displayBuffer.manageScrollPosition = true
+      displayBuffer.setLineHeight(10)
       displayBuffer.setDefaultCharWidth(10)
 
     it "disallows negative values", ->
@@ -1001,6 +1002,7 @@ describe "DisplayBuffer", ->
       displayBuffer.manageScrollPosition = true
       displayBuffer.setLineHeight(10)
       displayBuffer.setDefaultCharWidth(10)
+      displayBuffer.setHorizontalScrollbarHeight(0)
 
       displayBuffer.setHeight(50)
       displayBuffer.setWidth(50)

spec/editor-component-spec.coffee

@@ -1,9 +1,9 @@
-{extend, flatten, toArray} = require 'underscore-plus'
+{extend, flatten, toArray, last} = require 'underscore-plus'
 ReactEditorView = require '../src/react-editor-view'
 nbsp = String.fromCharCode(160)
 
 describe "EditorComponent", ->
-  [editor, wrapperView, component, node, verticalScrollbarNode, horizontalScrollbarNode] = []
+  [contentNode, editor, wrapperView, component, node, verticalScrollbarNode, horizontalScrollbarNode] = []
   [lineHeightInPixels, charWidth, delayAnimationFrames, nextAnimationFrame] = []
 
   beforeEach ->
@@ -26,6 +26,9 @@ describe "EditorComponent", ->
       atom.project.open('sample.js').then (o) -> editor = o
 
     runs ->
+      contentNode = document.querySelector('#jasmine-content')
+      contentNode.style.width = '1000px'
+
       wrapperView = new ReactEditorView(editor)
       wrapperView.attachToDom()
       {component} = wrapperView
@@ -38,6 +41,9 @@ describe "EditorComponent", ->
       verticalScrollbarNode = node.querySelector('.vertical-scrollbar')
       horizontalScrollbarNode = node.querySelector('.horizontal-scrollbar')
 
+  afterEach ->
+    contentNode.style.width = ''
+
   describe "line rendering", ->
     it "renders only the currently-visible lines", ->
       node.style.height = 4.5 * lineHeightInPixels + 'px'
@@ -188,6 +194,20 @@ describe "EditorComponent", ->
       expect(lines[4].textContent).toBe "#{nbsp}3"
       expect(lines[5].textContent).toBe "#{nbsp}•"
 
+    it "pads line numbers to be right justified based on the maximum number of line number digits", ->
+      editor.getBuffer().setText([1..10].join('\n'))
+      lineNumberNodes = toArray(node.querySelectorAll('.line-number'))
+
+      for node, i in lineNumberNodes[0..8]
+        expect(node.textContent).toBe "#{nbsp}#{i + 1}"
+      expect(lineNumberNodes[9].textContent).toBe '10'
+
+      # Removes padding when the max number of digits goes down
+      editor.getBuffer().delete([[1, 0], [2, 0]])
+      lineNumberNodes = toArray(node.querySelectorAll('.line-number'))
+      for node, i in lineNumberNodes
+        expect(node.textContent).toBe "#{i + 1}"
+
   describe "cursor rendering", ->
     it "renders the currently visible cursors", ->
       cursor1 = editor.getCursor()
@@ -529,6 +549,110 @@ describe "EditorComponent", ->
 
       expect(editor.getScrollLeft()).toBe 100
 
+    it "does not obscure the last line with the horizontal scrollbar", ->
+      node.style.height = 4.5 * lineHeightInPixels + 'px'
+      node.style.width = 10 * charWidth + 'px'
+      component.measureHeightAndWidth()
+      editor.setScrollBottom(editor.getScrollHeight())
+      lastLineNode = last(node.querySelectorAll('.line'))
+      bottomOfLastLine = lastLineNode.getBoundingClientRect().bottom
+      topOfHorizontalScrollbar = horizontalScrollbarNode.getBoundingClientRect().top
+      expect(bottomOfLastLine).toBe topOfHorizontalScrollbar
+
+      # Scroll so there's no space below the last line when the horizontal scrollbar disappears
+      node.style.width = 100 * charWidth + 'px'
+      component.measureHeightAndWidth()
+      lastLineNode = last(node.querySelectorAll('.line'))
+      bottomOfLastLine = lastLineNode.getBoundingClientRect().bottom
+      bottomOfEditor = node.getBoundingClientRect().bottom
+      expect(bottomOfLastLine).toBe bottomOfEditor
+
+    it "does not obscure the last character of the longest line with the vertical scrollbar", ->
+      node.style.height = 7 * lineHeightInPixels + 'px'
+      node.style.width = 10 * charWidth + 'px'
+      component.measureHeightAndWidth()
+
+      editor.setScrollLeft(Infinity)
+
+      lineNodes = node.querySelectorAll('.line')
+      rightOfLongestLine = lineNodes[6].getBoundingClientRect().right
+      leftOfVerticalScrollbar = verticalScrollbarNode.getBoundingClientRect().left
+
+      expect(rightOfLongestLine).toBe leftOfVerticalScrollbar - 1 # Leave 1 px so the cursor is visible on the end of the line
+
+    it "only displays dummy scrollbars when scrollable in that direction", ->
+      expect(verticalScrollbarNode.style.display).toBe 'none'
+      expect(horizontalScrollbarNode.style.display).toBe 'none'
+
+      node.style.height = 4.5 * lineHeightInPixels + 'px'
+      node.style.width = '1000px'
+      component.measureHeightAndWidth()
+
+      expect(verticalScrollbarNode.style.display).toBe ''
+      expect(horizontalScrollbarNode.style.display).toBe 'none'
+
+      node.style.width = 10 * charWidth + 'px'
+      component.measureHeightAndWidth()
+
+      expect(verticalScrollbarNode.style.display).toBe ''
+      expect(horizontalScrollbarNode.style.display).toBe ''
+
+      node.style.height = 20 * lineHeightInPixels + 'px'
+      component.measureHeightAndWidth()
+
+      expect(verticalScrollbarNode.style.display).toBe 'none'
+      expect(horizontalScrollbarNode.style.display).toBe ''
+
+    it "makes the dummy scrollbar divs only as tall/wide as the actual scrollbars", ->
+      node.style.height = 4 * lineHeightInPixels + 'px'
+      node.style.width = 10 * charWidth + 'px'
+      component.measureHeightAndWidth()
+
+      atom.themes.applyStylesheet "test", """
+        ::-webkit-scrollbar {
+          width: 8px;
+          height: 8px;
+        }
+      """
+
+      scrollbarCornerNode = node.querySelector('.scrollbar-corner')
+      expect(verticalScrollbarNode.offsetWidth).toBe 8
+      expect(horizontalScrollbarNode.offsetHeight).toBe 8
+      expect(scrollbarCornerNode.offsetWidth).toBe 8
+      expect(scrollbarCornerNode.offsetHeight).toBe 8
+
+    it "assigns the bottom/right of the scrollbars to the width of the opposite scrollbar if it is visible", ->
+      scrollbarCornerNode = node.querySelector('.scrollbar-corner')
+
+      expect(verticalScrollbarNode.style.bottom).toBe ''
+      expect(horizontalScrollbarNode.style.right).toBe ''
+
+      node.style.height = 4.5 * lineHeightInPixels + 'px'
+      node.style.width = '1000px'
+      component.measureHeightAndWidth()
+      expect(verticalScrollbarNode.style.bottom).toBe ''
+      expect(horizontalScrollbarNode.style.right).toBe verticalScrollbarNode.offsetWidth + 'px'
+      expect(scrollbarCornerNode.style.display).toBe 'none'
+
+      node.style.width = 10 * charWidth + 'px'
+      component.measureHeightAndWidth()
+      expect(verticalScrollbarNode.style.bottom).toBe horizontalScrollbarNode.offsetHeight + 'px'
+      expect(horizontalScrollbarNode.style.right).toBe verticalScrollbarNode.offsetWidth + 'px'
+      expect(scrollbarCornerNode.style.display).toBe ''
+
+      node.style.height = 20 * lineHeightInPixels + 'px'
+      component.measureHeightAndWidth()
+      expect(verticalScrollbarNode.style.bottom).toBe horizontalScrollbarNode.offsetHeight + 'px'
+      expect(horizontalScrollbarNode.style.right).toBe ''
+      expect(scrollbarCornerNode.style.display).toBe 'none'
+
+    it "accounts for the width of the gutter in the scrollWidth of the horizontal scrollbar", ->
+      gutterNode = node.querySelector('.gutter')
+      node.style.width = 10 * charWidth + 'px'
+      component.measureHeightAndWidth()
+
+      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)", ->
         node.style.height = 4.5 * lineHeightInPixels + 'px'

spec/editor-spec.coffee

@@ -698,6 +698,7 @@ describe "Editor", ->
         editor.setHorizontalScrollMargin(2)
         editor.setLineHeight(10)
         editor.setDefaultCharWidth(10)
+        editor.setHorizontalScrollbarHeight(0)
         editor.setHeight(5.5 * 10)
         editor.setWidth(5.5 * 10)
 
@@ -1138,6 +1139,7 @@ describe "Editor", ->
           editor.setDefaultCharWidth(10)
           editor.setHeight(50)
           editor.setWidth(50)
+          editor.setHorizontalScrollbarHeight(0)
           expect(editor.getScrollTop()).toBe 0
 
           editor.setSelectedBufferRange([[5, 6], [6, 8]], autoscroll: true)
@@ -3098,6 +3100,7 @@ describe "Editor", ->
       editor.setDefaultCharWidth(10)
       editor.setHeight(50)
       editor.setWidth(50)
+      editor.setHorizontalScrollbarHeight(0)
       expect(editor.getScrollTop()).toBe 0
       expect(editor.getScrollLeft()).toBe 0
 

spec/theme-manager-spec.coffee

@@ -131,15 +131,21 @@ describe "ThemeManager", ->
 
   describe "requireStylesheet(path)", ->
     it "synchronously loads css at the given path and installs a style tag for it in the head", ->
+      themeManager.on 'stylesheets-changed', stylesheetsChangedHandler = jasmine.createSpy("stylesheetsChangedHandler")
+      themeManager.on 'stylesheet-added', stylesheetAddedHandler = jasmine.createSpy("stylesheetAddedHandler")
       cssPath = atom.project.resolve('css.css')
       lengthBefore = $('head style').length
 
       themeManager.requireStylesheet(cssPath)
       expect($('head style').length).toBe lengthBefore + 1
 
+      expect(stylesheetAddedHandler).toHaveBeenCalled()
+      expect(stylesheetsChangedHandler).toHaveBeenCalled()
+
       element = $('head style[id*="css.css"]')
       expect(element.attr('id')).toBe themeManager.stringToId(cssPath)
       expect(element.text()).toBe fs.readFileSync(cssPath, 'utf8')
+      expect(element[0].sheet).toBe stylesheetAddedHandler.argsForCall[0][0]
 
       # doesn't append twice
       themeManager.requireStylesheet(cssPath)
@@ -187,9 +193,18 @@ describe "ThemeManager", ->
       themeManager.requireStylesheet(cssPath)
       expect($(document.body).css('font-weight')).toBe("bold")
 
+      themeManager.on 'stylesheet-removed', stylesheetRemovedHandler = jasmine.createSpy("stylesheetRemovedHandler")
       themeManager.on 'stylesheets-changed', stylesheetsChangedHandler = jasmine.createSpy("stylesheetsChangedHandler")
+
       themeManager.removeStylesheet(cssPath)
+
       expect($(document.body).css('font-weight')).not.toBe("bold")
+
+      expect(stylesheetRemovedHandler).toHaveBeenCalled()
+      stylesheet = stylesheetRemovedHandler.argsForCall[0][0]
+      expect(stylesheet instanceof CSSStyleSheet).toBe true
+      expect(stylesheet.cssRules[0].selectorText).toBe 'body'
+
       expect(stylesheetsChangedHandler).toHaveBeenCalled()
 
   describe "base stylesheet loading", ->
@@ -219,35 +234,47 @@ describe "ThemeManager", ->
 
   describe "when the user stylesheet changes", ->
     it "reloads it", ->
+      [stylesheetRemovedHandler, stylesheetAddedHandler, stylesheetsChangedHandler] = []
       userStylesheetPath = path.join(temp.mkdirSync("atom"), 'styles.less')
       fs.writeFileSync(userStylesheetPath, 'body {border-style: dotted !important;}')
-
       spyOn(themeManager, 'getUserStylesheetPath').andReturn userStylesheetPath
-      themeManager.on 'stylesheets-changed', stylesheetsChangedHandler = jasmine.createSpy("stylesheetsChangedHandler")
 
       waitsForPromise ->
         themeManager.activateThemes()
 
       runs ->
-        expect($(document.body).css('border-style')).toBe 'dotted'
-        expect(stylesheetsChangedHandler).toHaveBeenCalled()
-        stylesheetsChangedHandler.reset()
+        themeManager.on 'stylesheets-changed', stylesheetsChangedHandler = jasmine.createSpy("stylesheetsChangedHandler")
+        themeManager.on 'stylesheet-removed', stylesheetRemovedHandler = jasmine.createSpy("stylesheetRemovedHandler")
+        themeManager.on 'stylesheet-added', stylesheetAddedHandler = jasmine.createSpy("stylesheetAddedHandler")
         spyOn(themeManager, 'loadUserStylesheet').andCallThrough()
+
+        expect($(document.body).css('border-style')).toBe 'dotted'
         fs.writeFileSync(userStylesheetPath, 'body {border-style: dashed}')
 
       waitsFor ->
         themeManager.loadUserStylesheet.callCount is 1
 
       runs ->
         expect($(document.body).css('border-style')).toBe 'dashed'
+
+        expect(stylesheetRemovedHandler).toHaveBeenCalled()
+        expect(stylesheetRemovedHandler.argsForCall[0][0].cssRules[0].style.border).toBe 'dotted'
+
+        expect(stylesheetAddedHandler).toHaveBeenCalled()
+        expect(stylesheetAddedHandler.argsForCall[0][0].cssRules[0].style.border).toBe 'dashed'
+
         expect(stylesheetsChangedHandler).toHaveBeenCalled()
+
+        stylesheetRemovedHandler.reset()
         stylesheetsChangedHandler.reset()
         fs.removeSync(userStylesheetPath)
 
       waitsFor ->
         themeManager.loadUserStylesheet.callCount is 2
 
       runs ->
+        expect(stylesheetRemovedHandler).toHaveBeenCalled()
+        expect(stylesheetRemovedHandler.argsForCall[0][0].cssRules[0].style.border).toBe 'dashed'
         expect($(document.body).css('border-style')).toBe 'none'
         expect(stylesheetsChangedHandler).toHaveBeenCalled()