From 50088b16c9a4e6fda78ea98430bb0705229883e8 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 30 Aug 2017 16:27:02 +0200 Subject: [PATCH] Honor macOS "Show scrollbars only when scrolling" setting Previously, we were hiding scrollbars when their height/width was 0. On macOS, however, users can decide to only show scrollbars while scrolling, which causes Atom to detect scrollbars as being invisible during measurements. As a result, we were mistakenly setting the visibility property to `hidden` when this setting was on, thus preventing users from seeing the scrollbar on scroll. With this commit we are changing the dummy scrollbar components to only become invisible when the content is not scrollable rather than when the scrollbars have zero width or height. As part of this, we have also renamed the `is{Horizontal,Vertical}ScrollbarVisible` functions to `canScroll{Horizontally,Vertically}`, to better express their intent. --- spec/text-editor-component-spec.js | 12 ++++---- src/text-editor-component.js | 47 +++++++++++++++++------------- 2 files changed, 33 insertions(+), 26 deletions(-) diff --git a/spec/text-editor-component-spec.js b/spec/text-editor-component-spec.js index 84d964b06..d4c14dc63 100644 --- a/spec/text-editor-component-spec.js +++ b/spec/text-editor-component-spec.js @@ -688,8 +688,8 @@ describe('TextEditorComponent', () => { await component.getNextUpdatePromise() element.style.width = component.getGutterContainerWidth() + component.getContentHeight() - 20 + 'px' await component.getNextUpdatePromise() - expect(component.isHorizontalScrollbarVisible()).toBe(true) - expect(component.isVerticalScrollbarVisible()).toBe(false) + expect(component.canScrollHorizontally()).toBe(true) + expect(component.canScrollVertically()).toBe(false) expect(element.offsetHeight).toBe(component.getContentHeight() + component.getHorizontalScrollbarHeight() + 2 * editorPadding) // When a vertical scrollbar is visible, autoWidth accounts for it @@ -697,8 +697,8 @@ describe('TextEditorComponent', () => { await component.getNextUpdatePromise() element.style.height = component.getContentHeight() - 20 await component.getNextUpdatePromise() - expect(component.isHorizontalScrollbarVisible()).toBe(false) - expect(component.isVerticalScrollbarVisible()).toBe(true) + expect(component.canScrollHorizontally()).toBe(false) + expect(component.canScrollVertically()).toBe(true) expect(element.offsetWidth).toBe( component.getGutterContainerWidth() + component.getContentWidth() + @@ -898,8 +898,8 @@ describe('TextEditorComponent', () => { editor.setText('x'.repeat(20) + 'y'.repeat(20)) await component.getNextUpdatePromise() - expect(component.isHorizontalScrollbarVisible()).toBe(false) - expect(component.isVerticalScrollbarVisible()).toBe(false) + expect(component.canScrollVertically()).toBe(false) + expect(component.canScrollHorizontally()).toBe(false) expect(component.refs.horizontalScrollbar).toBeUndefined() expect(component.refs.verticalScrollbar).toBeUndefined() }) diff --git a/src/text-editor-component.js b/src/text-editor-component.js index e22b53db8..0329eee91 100644 --- a/src/text-editor-component.js +++ b/src/text-editor-component.js @@ -381,7 +381,10 @@ class TextEditorComponent { this.measureGutterDimensions() this.remeasureGutterDimensions = false } - const wasHorizontalScrollbarVisible = this.isHorizontalScrollbarVisible() + const wasHorizontalScrollbarVisible = ( + this.canScrollHorizontally() && + this.getHorizontalScrollbarHeight() > 0 + ) this.measureLongestLineWidth() this.measureHorizontalPositions() @@ -391,7 +394,12 @@ class TextEditorComponent { this.derivedDimensionsCache = {} const {screenRange, options} = this.pendingAutoscroll this.autoscrollHorizontally(screenRange, options) - if (!wasHorizontalScrollbarVisible && this.isHorizontalScrollbarVisible()) { + + const isHorizontalScrollbarVisible = ( + this.canScrollHorizontally() && + this.getHorizontalScrollbarHeight() > 0 + ) + if (!wasHorizontalScrollbarVisible && isHorizontalScrollbarVisible) { this.autoscrollVertically(screenRange, options) } this.pendingAutoscroll = null @@ -439,13 +447,13 @@ class TextEditorComponent { if (this.hasInitialMeasurements) { if (model.getAutoHeight()) { clientContainerHeight = this.getContentHeight() - if (this.isHorizontalScrollbarVisible()) clientContainerHeight += this.getHorizontalScrollbarHeight() + if (this.canScrollHorizontally()) clientContainerHeight += this.getHorizontalScrollbarHeight() clientContainerHeight += 'px' } if (model.getAutoWidth()) { style.width = 'min-content' clientContainerWidth = this.getGutterContainerWidth() + this.getContentWidth() - if (this.isVerticalScrollbarVisible()) clientContainerWidth += this.getVerticalScrollbarWidth() + if (this.canScrollVertically()) clientContainerWidth += this.getVerticalScrollbarWidth() clientContainerWidth += 'px' } else { style.width = this.element.style.width @@ -716,18 +724,21 @@ class TextEditorComponent { if (this.shouldRenderDummyScrollbars && !this.props.model.isMini()) { let scrollHeight, scrollTop, horizontalScrollbarHeight let scrollWidth, scrollLeft, verticalScrollbarWidth, forceScrollbarVisible + let canScrollHorizontally, canScrollVertically if (this.hasInitialMeasurements) { scrollHeight = this.getScrollHeight() scrollWidth = this.getScrollWidth() scrollTop = this.getScrollTop() scrollLeft = this.getScrollLeft() + canScrollHorizontally = this.canScrollHorizontally() + canScrollVertically = this.canScrollVertically() horizontalScrollbarHeight = - this.isHorizontalScrollbarVisible() + canScrollHorizontally ? this.getHorizontalScrollbarHeight() : 0 verticalScrollbarWidth = - this.isVerticalScrollbarVisible() + canScrollVertically ? this.getVerticalScrollbarWidth() : 0 forceScrollbarVisible = this.remeasureScrollbars @@ -741,10 +752,10 @@ class TextEditorComponent { orientation: 'vertical', didScroll: this.didScrollDummyScrollbar, didMouseDown: this.didMouseDownOnContent, + canScroll: canScrollVertically, scrollHeight, scrollTop, horizontalScrollbarHeight, - verticalScrollbarWidth, forceScrollbarVisible }), $(DummyScrollbarComponent, { @@ -752,9 +763,9 @@ class TextEditorComponent { orientation: 'horizontal', didScroll: this.didScrollDummyScrollbar, didMouseDown: this.didMouseDownOnContent, + canScroll: canScrollHorizontally, scrollWidth, scrollLeft, - horizontalScrollbarHeight, verticalScrollbarWidth, forceScrollbarVisible }) @@ -2568,7 +2579,7 @@ class TextEditorComponent { } getScrollContainerClientWidth () { - if (this.isVerticalScrollbarVisible()) { + if (this.canScrollVertically()) { return this.getScrollContainerWidth() - this.getVerticalScrollbarWidth() } else { return this.getScrollContainerWidth() @@ -2576,14 +2587,14 @@ class TextEditorComponent { } getScrollContainerClientHeight () { - if (this.isHorizontalScrollbarVisible()) { + if (this.canScrollHorizontally()) { return this.getScrollContainerHeight() - this.getHorizontalScrollbarHeight() } else { return this.getScrollContainerHeight() } } - isVerticalScrollbarVisible () { + canScrollVertically () { const {model} = this.props if (model.isMini()) return false if (model.getAutoHeight()) return false @@ -2594,7 +2605,7 @@ class TextEditorComponent { ) } - isHorizontalScrollbarVisible () { + canScrollHorizontally () { const {model} = this.props if (model.isMini()) return false if (model.getAutoWidth()) return false @@ -2943,8 +2954,8 @@ class DummyScrollbarComponent { render () { const { orientation, scrollWidth, scrollHeight, - verticalScrollbarWidth, horizontalScrollbarHeight, forceScrollbarVisible, - didScroll, didMouseDown + verticalScrollbarWidth, horizontalScrollbarHeight, + canScroll, forceScrollbarVisible, didScroll, didMouseDown } = this.props const outerStyle = { @@ -2953,6 +2964,8 @@ class DummyScrollbarComponent { zIndex: 1, willChange: 'transform' } + if (!canScroll) outerStyle.visibility = 'hidden' + const innerStyle = {} if (orientation === 'horizontal') { let right = (verticalScrollbarWidth || 0) @@ -2963,9 +2976,6 @@ class DummyScrollbarComponent { outerStyle.overflowY = 'hidden' outerStyle.overflowX = forceScrollbarVisible ? 'scroll' : 'auto' outerStyle.cursor = 'default' - if (horizontalScrollbarHeight === 0) { - outerStyle.visibility = 'hidden' - } innerStyle.height = '15px' innerStyle.width = (scrollWidth || 0) + 'px' } else { @@ -2977,9 +2987,6 @@ class DummyScrollbarComponent { outerStyle.overflowX = 'hidden' outerStyle.overflowY = forceScrollbarVisible ? 'scroll' : 'auto' outerStyle.cursor = 'default' - if (verticalScrollbarWidth === 0) { - outerStyle.visibility = 'hidden' - } innerStyle.width = '15px' innerStyle.height = (scrollHeight || 0) + 'px' }