Merge pull request atom#15580 from atom/as-update-scrollbars-on-detach-reattach

Antonio Scandurra · web-flow · commit 4b3fa34d4e26 · 2017-09-07T16:39:16.000+02:00
Flush scroll position to dummy scrollbar components on re-attach
diff --git a/spec/text-editor-component-spec.js b/spec/text-editor-component-spec.js
@@ -378,35 +378,50 @@ describe('TextEditorComponent', () => {
       expect(horizontalScrollbar.style.visibility).toBe('')
     })
 
-    it('updates the bottom/right of dummy scrollbars and client height/width measurements without forgetting the previous scroll top/left when scrollbar styles change', async () => {
-      const {component, element, editor} = buildComponent({height: 100, width: 100})
-      expect(getHorizontalScrollbarHeight(component)).toBeGreaterThan(10)
-      expect(getVerticalScrollbarWidth(component)).toBeGreaterThan(10)
-      setScrollTop(component, 20)
-      setScrollLeft(component, 10)
-      await component.getNextUpdatePromise()
-
-      const style = document.createElement('style')
-      style.textContent = '::-webkit-scrollbar { height: 10px; width: 10px; }'
-      jasmine.attachToDOM(style)
+    describe('when scrollbar styles change or the editor element is detached and then reattached', () => {
+      it('updates the bottom/right of dummy scrollbars and client height/width measurements', async () => {
+        const {component, element, editor} = buildComponent({height: 100, width: 100})
+        expect(getHorizontalScrollbarHeight(component)).toBeGreaterThan(10)
+        expect(getVerticalScrollbarWidth(component)).toBeGreaterThan(10)
+        setScrollTop(component, 20)
+        setScrollLeft(component, 10)
+        await component.getNextUpdatePromise()
 
-      TextEditor.didUpdateScrollbarStyles()
-      await component.getNextUpdatePromise()
+        // Updating scrollbar styles.
+        const style = document.createElement('style')
+        style.textContent = '::-webkit-scrollbar { height: 10px; width: 10px; }'
+        jasmine.attachToDOM(style)
+        TextEditor.didUpdateScrollbarStyles()
+        await component.getNextUpdatePromise()
 
-      expect(getHorizontalScrollbarHeight(component)).toBe(10)
-      expect(getVerticalScrollbarWidth(component)).toBe(10)
-      expect(component.refs.horizontalScrollbar.element.style.right).toBe('10px')
-      expect(component.refs.verticalScrollbar.element.style.bottom).toBe('10px')
-      expect(component.refs.horizontalScrollbar.element.scrollLeft).toBe(10)
-      expect(component.refs.verticalScrollbar.element.scrollTop).toBe(20)
-      expect(component.getScrollContainerClientHeight()).toBe(100 - 10)
-      expect(component.getScrollContainerClientWidth()).toBe(100 - component.getGutterContainerWidth() - 10)
+        expect(getHorizontalScrollbarHeight(component)).toBe(10)
+        expect(getVerticalScrollbarWidth(component)).toBe(10)
+        expect(component.refs.horizontalScrollbar.element.style.right).toBe('10px')
+        expect(component.refs.verticalScrollbar.element.style.bottom).toBe('10px')
+        expect(component.refs.horizontalScrollbar.element.scrollLeft).toBe(10)
+        expect(component.refs.verticalScrollbar.element.scrollTop).toBe(20)
+        expect(component.getScrollContainerClientHeight()).toBe(100 - 10)
+        expect(component.getScrollContainerClientWidth()).toBe(100 - component.getGutterContainerWidth() - 10)
+
+        // Detaching and re-attaching the editor element.
+        element.remove()
+        jasmine.attachToDOM(element)
 
-      // Ensure we don't throw an error trying to remeasure non-existent scrollbars for mini editors.
-      await editor.update({mini: true})
-      TextEditor.didUpdateScrollbarStyles()
-      component.scheduleUpdate()
-      await component.getNextUpdatePromise()
+        expect(getHorizontalScrollbarHeight(component)).toBe(10)
+        expect(getVerticalScrollbarWidth(component)).toBe(10)
+        expect(component.refs.horizontalScrollbar.element.style.right).toBe('10px')
+        expect(component.refs.verticalScrollbar.element.style.bottom).toBe('10px')
+        expect(component.refs.horizontalScrollbar.element.scrollLeft).toBe(10)
+        expect(component.refs.verticalScrollbar.element.scrollTop).toBe(20)
+        expect(component.getScrollContainerClientHeight()).toBe(100 - 10)
+        expect(component.getScrollContainerClientWidth()).toBe(100 - component.getGutterContainerWidth() - 10)
+
+        // Ensure we don't throw an error trying to remeasure non-existent scrollbars for mini editors.
+        await editor.update({mini: true})
+        TextEditor.didUpdateScrollbarStyles()
+        component.scheduleUpdate()
+        await component.getNextUpdatePromise()
+      })
     })
 
     it('renders cursors within the visible row range', async () => {
@@ -1142,7 +1157,7 @@ describe('TextEditorComponent', () => {
       expect(component.getScrollTopRow()).toBe(4)
       expect(component.getScrollTop()).toBe(Math.round(4 * component.getLineHeight()))
 
-      // Preserves the scrollTopRow when sdetached
+      // Preserves the scrollTopRow when detached
       element.remove()
       expect(component.getScrollTopRow()).toBe(4)
       expect(component.getScrollTop()).toBe(Math.round(4 * component.getLineHeight()))
diff --git a/src/text-editor-component.js b/src/text-editor-component.js
@@ -1403,6 +1403,9 @@ class TextEditorComponent {
 
       if (this.isVisible()) {
         this.didShow()
+
+        if (this.refs.verticalScrollbar) this.refs.verticalScrollbar.flushScrollPosition()
+        if (this.refs.horizontalScrollbar) this.refs.horizontalScrollbar.flushScrollPosition()
       } else {
         this.didHide()
       }

Original file line number	Diff line number	Diff line change
`@@ -1403,6 +1403,9 @@ class TextEditorComponent {`
`1403`	`1403`
`1404`	`1404`	`if (this.isVisible()) {`
`1405`	`1405`	`this.didShow()`
	`1406`	`+`
	`1407`	`+ if (this.refs.verticalScrollbar) this.refs.verticalScrollbar.flushScrollPosition()`
	`1408`	`+ if (this.refs.horizontalScrollbar) this.refs.horizontalScrollbar.flushScrollPosition()`
`1406`	`1409`	`} else {`
`1407`	`1410`	`this.didHide()`
`1408`	`1411`	`}`