Merge pull request #251 from /issues/244

Fix elastic bounce issues and RTL issues

src/__tests__/FixedSizeGrid.js

@@ -27,6 +27,31 @@ describe('FixedSizeGrid', () => {
   beforeEach(() => {
     jest.useFakeTimers();
 
+    // JSdom does not do actual layout and so doesn't return meaningful values here.
+    // For the purposes of our tests though, we can mock out semi-meaningful values.
+    Object.defineProperties(HTMLElement.prototype, {
+      clientWidth: {
+        configurable: true,
+        get: function() {
+          return parseInt(this.style.width, 10) || 0;
+        },
+      },
+      clientHeight: {
+        configurable: true,
+        get: function() {
+          return parseInt(this.style.height, 10) || 0;
+        },
+      },
+      scrollHeight: {
+        configurable: true,
+        get: () => Number.MAX_SAFE_INTEGER,
+      },
+      scrollWidth: {
+        configurable: true,
+        get: () => Number.MAX_SAFE_INTEGER,
+      },
+    });
+
     // Mock the DOM helper util for testing purposes.
     getScrollbarSize = domHelpers.getScrollbarSize = jest.fn(() => 0);
 
@@ -99,6 +124,7 @@ describe('FixedSizeGrid', () => {
       // Scroll, then capture the rendered style for item 1,
       // Then let the debounce timer clear the cached styles.
       simulateScroll(instance, { scrollLeft: 100, scrollTop: 25 });
+      expect(itemRenderer).toHaveBeenCalled();
       const itemOneArgsA = itemRenderer.mock.calls.find(
         ([params]) => params.columnIndex === 1 && params.rowIndex === 1
       );
@@ -107,6 +133,7 @@ describe('FixedSizeGrid', () => {
       // Scroll again, then capture the rendered style for item 1,
       // And confirm that the style was recreated.
       simulateScroll(instance, { scrollLeft: 0, scrollTop: 0 });
+      expect(itemRenderer).toHaveBeenCalled();
       const itemOneArgsB = itemRenderer.mock.calls.find(
         ([params]) => params.columnIndex === 1 && params.rowIndex === 1
       );

src/__tests__/FixedSizeList.js

@@ -29,6 +29,31 @@ describe('FixedSizeList', () => {
   beforeEach(() => {
     jest.useFakeTimers();
 
+    // JSdom does not do actual layout and so doesn't return meaningful values here.
+    // For the purposes of our tests though, we can mock out semi-meaningful values.
+    Object.defineProperties(HTMLElement.prototype, {
+      clientWidth: {
+        configurable: true,
+        get: function() {
+          return parseInt(this.style.width, 10) || 0;
+        },
+      },
+      clientHeight: {
+        configurable: true,
+        get: function() {
+          return parseInt(this.style.height, 10) || 0;
+        },
+      },
+      scrollHeight: {
+        configurable: true,
+        get: () => Number.MAX_SAFE_INTEGER,
+      },
+      scrollWidth: {
+        configurable: true,
+        get: () => Number.MAX_SAFE_INTEGER,
+      },
+    });
+
     onItemsRendered = jest.fn();
 
     itemRenderer = jest.fn(({ style, ...rest }) => (

src/createGridComponent.js

@@ -3,7 +3,7 @@
 import memoizeOne from 'memoize-one';
 import { createElement, PureComponent } from 'react';
 import { cancelTimeout, requestTimeout } from './timer';
-import { getScrollbarSize } from './domHelpers';
+import { getScrollbarSize, isRTLOffsetNegative } from './domHelpers';
 
 import type { TimeoutID } from './timer';
 
@@ -324,21 +324,42 @@ export default function createGridComponent({
 
     componentDidMount() {
       const { initialScrollLeft, initialScrollTop } = this.props;
-      if (typeof initialScrollLeft === 'number' && this._outerRef != null) {
-        ((this._outerRef: any): HTMLDivElement).scrollLeft = initialScrollLeft;
-      }
-      if (typeof initialScrollTop === 'number' && this._outerRef != null) {
-        ((this._outerRef: any): HTMLDivElement).scrollTop = initialScrollTop;
+
+      if (this._outerRef != null) {
+        const outerRef = ((this._outerRef: any): HTMLElement);
+        if (typeof initialScrollLeft === 'number') {
+          outerRef.scrollLeft = initialScrollLeft;
+        }
+        if (typeof initialScrollTop === 'number') {
+          outerRef.scrollTop = initialScrollTop;
+        }
       }
 
       this._callPropsCallbacks();
     }
 
     componentDidUpdate() {
+      const { direction } = this.props;
       const { scrollLeft, scrollTop, scrollUpdateWasRequested } = this.state;
-      if (scrollUpdateWasRequested && this._outerRef !== null) {
-        ((this._outerRef: any): HTMLDivElement).scrollLeft = scrollLeft;
-        ((this._outerRef: any): HTMLDivElement).scrollTop = scrollTop;
+
+      if (scrollUpdateWasRequested && this._outerRef != null) {
+        // TRICKY According to the spec, scrollLeft should be negative for RTL aligned elements.
+        // This is not the case for all browsers though (e.g. Chrome reports values as positive, measured relative to the left).
+        // So we need to determine which browser behavior we're dealing with, and mimic it.
+        const outerRef = ((this._outerRef: any): HTMLElement);
+        if (direction === 'rtl') {
+          const isNegative = isRTLOffsetNegative();
+          if (isNegative) {
+            outerRef.scrollLeft = -scrollLeft;
+          } else {
+            const { clientWidth, scrollWidth } = outerRef;
+            outerRef.scrollLeft = scrollWidth - clientWidth - scrollLeft;
+          }
+        } else {
+          outerRef.scrollLeft = Math.max(0, scrollLeft);
+        }
+
+        outerRef.scrollTop = Math.max(0, scrollTop);
       }
 
       this._callPropsCallbacks();
@@ -685,9 +706,11 @@ export default function createGridComponent({
 
     _onScroll = (event: ScrollEvent): void => {
       const {
+        clientHeight,
         clientWidth,
         scrollLeft,
         scrollTop,
+        scrollHeight,
         scrollWidth,
       } = event.currentTarget;
       this.setState(prevState => {
@@ -703,24 +726,37 @@ export default function createGridComponent({
 
         const { direction } = this.props;
 
-        // HACK According to the spec, scrollLeft should be negative for RTL aligned elements.
-        // Chrome does not seem to adhere; its scrollLeft values are positive (measured relative to the left).
-        // See https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollLeft
         let calculatedScrollLeft = scrollLeft;
         if (direction === 'rtl') {
-          if (scrollLeft <= 0) {
+          const isNegative = isRTLOffsetNegative();
+
+          // TRICKY According to the spec, scrollLeft should be negative for RTL aligned elements.
+          // This is not the case for all browsers though (e.g. Chrome reports values as positive, measured relative to the left).
+          // It's also easier for this component if we convert offsets to the same format as they would be in for ltr.
+          // So the simplest solution is to determine which browser behavior we're dealing with, and convert based on it.
+          if (isNegative) {
             calculatedScrollLeft = -scrollLeft;
           } else {
             calculatedScrollLeft = scrollWidth - clientWidth - scrollLeft;
           }
         }
 
+        // Prevent Safari's elastic scrolling from causing visual shaking when scrolling past bounds.
+        calculatedScrollLeft = Math.max(
+          0,
+          Math.min(calculatedScrollLeft, scrollWidth - clientWidth)
+        );
+        const calculatedScrollTop = Math.max(
+          0,
+          Math.min(scrollTop, scrollHeight - clientHeight)
+        );
+
         return {
           isScrolling: true,
           horizontalScrollDirection:
             prevState.scrollLeft < scrollLeft ? 'forward' : 'backward',
           scrollLeft: calculatedScrollLeft,
-          scrollTop,
+          scrollTop: calculatedScrollTop,
           verticalScrollDirection:
             prevState.scrollTop < scrollTop ? 'forward' : 'backward',
           scrollUpdateWasRequested: false,

src/createListComponent.js

@@ -3,6 +3,7 @@
 import memoizeOne from 'memoize-one';
 import { createElement, PureComponent } from 'react';
 import { cancelTimeout, requestTimeout } from './timer';
+import { isRTLOffsetNegative } from './domHelpers';
 
 import type { TimeoutID } from './timer';
 
@@ -215,14 +216,13 @@ export default function createListComponent({
     componentDidMount() {
       const { direction, initialScrollOffset, layout } = this.props;
 
-      if (typeof initialScrollOffset === 'number' && this._outerRef !== null) {
+      if (typeof initialScrollOffset === 'number' && this._outerRef != null) {
+        const outerRef = ((this._outerRef: any): HTMLElement);
         // TODO Deprecate direction "horizontal"
         if (direction === 'horizontal' || layout === 'horizontal') {
-          ((this
-            ._outerRef: any): HTMLDivElement).scrollLeft = initialScrollOffset;
+          outerRef.scrollLeft = initialScrollOffset;
         } else {
-          ((this
-            ._outerRef: any): HTMLDivElement).scrollTop = initialScrollOffset;
+          outerRef.scrollTop = initialScrollOffset;
         }
       }
 
@@ -233,12 +233,26 @@ export default function createListComponent({
       const { direction, layout } = this.props;
       const { scrollOffset, scrollUpdateWasRequested } = this.state;
 
-      if (scrollUpdateWasRequested && this._outerRef !== null) {
+      if (scrollUpdateWasRequested && this._outerRef != null) {
+        const outerRef = ((this._outerRef: any): HTMLElement);
         // TODO Deprecate direction "horizontal"
         if (direction === 'horizontal' || layout === 'horizontal') {
-          ((this._outerRef: any): HTMLDivElement).scrollLeft = scrollOffset;
+          if (direction === 'rtl') {
+            // TRICKY According to the spec, scrollLeft should be negative for RTL aligned elements.
+            // This is not the case for all browsers though (e.g. Chrome reports values as positive, measured relative to the left).
+            // So we need to determine which browser behavior we're dealing with, and mimic it.
+            const isNegative = isRTLOffsetNegative();
+            if (isNegative) {
+              outerRef.scrollLeft = -scrollOffset;
+            } else {
+              const { clientWidth, scrollWidth } = outerRef;
+              outerRef.scrollLeft = scrollWidth - clientWidth - scrollOffset;
+            }
+          } else {
+            outerRef.scrollLeft = scrollOffset;
+          }
         } else {
-          ((this._outerRef: any): HTMLDivElement).scrollTop = scrollOffset;
+          outerRef.scrollTop = scrollOffset;
         }
       }
 
@@ -496,18 +510,27 @@ export default function createListComponent({
 
         const { direction } = this.props;
 
-        // HACK According to the spec, scrollLeft should be negative for RTL aligned elements.
-        // Chrome does not seem to adhere; its scrolLeft values are positive (measured relative to the left).
-        // See https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollLeft
         let scrollOffset = scrollLeft;
         if (direction === 'rtl') {
-          if (scrollLeft <= 0) {
-            scrollOffset = -scrollOffset;
+          const isNegative = isRTLOffsetNegative();
+
+          // TRICKY According to the spec, scrollLeft should be negative for RTL aligned elements.
+          // This is not the case for all browsers though (e.g. Chrome reports values as positive, measured relative to the left).
+          // It's also easier for this component if we convert offsets to the same format as they would be in for ltr.
+          // So the simplest solution is to determine which browser behavior we're dealing with, and convert based on it.
+          if (isNegative) {
+            scrollOffset = -scrollLeft;
           } else {
             scrollOffset = scrollWidth - clientWidth - scrollLeft;
           }
         }
 
+        // Prevent Safari's elastic scrolling from causing visual shaking when scrolling past bounds.
+        scrollOffset = Math.max(
+          0,
+          Math.min(scrollOffset, scrollWidth - clientWidth)
+        );
+
         return {
           isScrolling: true,
           scrollDirection:
@@ -519,7 +542,7 @@ export default function createListComponent({
     };
 
     _onScrollVertical = (event: ScrollEvent): void => {
-      const { scrollTop } = event.currentTarget;
+      const { clientHeight, scrollHeight, scrollTop } = event.currentTarget;
       this.setState(prevState => {
         if (prevState.scrollOffset === scrollTop) {
           // Scroll position may have been updated by cDM/cDU,
@@ -528,11 +551,17 @@ export default function createListComponent({
           return null;
         }
 
+        // Prevent Safari's elastic scrolling from causing visual shaking when scrolling past bounds.
+        const scrollOffset = Math.max(
+          0,
+          Math.min(scrollTop, scrollHeight - clientHeight)
+        );
+
         return {
           isScrolling: true,
           scrollDirection:
-            prevState.scrollOffset < scrollTop ? 'forward' : 'backward',
-          scrollOffset: scrollTop,
+            prevState.scrollOffset < scrollOffset ? 'forward' : 'backward',
+          scrollOffset,
           scrollUpdateWasRequested: false,
         };
       }, this._resetIsScrollingDebounced);

src/domHelpers.js

@@ -20,3 +20,40 @@ export function getScrollbarSize(recalculate?: boolean = false): number {
 
   return size;
 }
+
+let cachedRTLResult: boolean | null = null;
+
+// TRICKY According to the spec, scrollLeft should be negative for RTL aligned elements.
+// Chrome does not seem to adhere; its scrollLeft values are positive (measured relative to the left).
+// Safari's elastic bounce makes detecting this even more complicated wrt potential false positives.
+// The safest way to check this is to intentionally set a negative offset,
+// and then verify that the subsequent "scroll" event matches the negative offset.
+// If it does not match, then we can assume a non-standard RTL scroll implementation.
+export function isRTLOffsetNegative(recalculate?: boolean = false): boolean {
+  if (cachedRTLResult === null || recalculate) {
+    const outerDiv = document.createElement('div');
+    const outerStyle = outerDiv.style;
+    outerStyle.width = '50px';
+    outerStyle.height = '50px';
+    outerStyle.overflow = 'scroll';
+    outerStyle.direction = 'rtl';
+
+    const innerDiv = document.createElement('div');
+    const innerStyle = innerDiv.style;
+    innerStyle.width = '100px';
+    innerStyle.height = '100px';
+
+    outerDiv.appendChild(innerDiv);
+
+    ((document.body: any): HTMLBodyElement).appendChild(outerDiv);
+
+    outerDiv.scrollLeft = -10;
+    cachedRTLResult = outerDiv.scrollLeft === -10;
+
+    ((document.body: any): HTMLBodyElement).removeChild(outerDiv);
+
+    return cachedRTLResult;
+  }
+
+  return cachedRTLResult;
+}