Accounting for cross axis scrollbars in auto scrolling. Fixes #628 (#632

)

src/state/auto-scroller/can-scroll.js

@@ -1,9 +1,9 @@
 // @flow
 import { type Position } from 'css-box-model';
 import { add, apply, isEqual, origin } from '../position';
-import type { Scrollable, DroppableDimension, Viewport } from '../../types';
+import type { DroppableDimension, Viewport, Scrollable } from '../../types';
 
-type CanScrollArgs = {|
+type CanPartiallyScrollArgs = {|
   max: Position,
   current: Position,
   change: Position,
@@ -52,10 +52,19 @@ export const getOverlap = (() => {
 })();
 
 export const canPartiallyScroll = ({
-  max,
+  max: rawMax,
   current,
   change,
-}: CanScrollArgs): boolean => {
+}: CanPartiallyScrollArgs): boolean => {
+  // It is possible for the max scroll to be greater than the current scroll
+  // when there are scrollbars on the cross axis. We adjust for this by
+  // increasing the max scroll point if needed
+  // This will allow movements backwards even if the current scroll is greater than the max scroll
+  const max: Position = {
+    x: Math.max(current.x, rawMax.x),
+    y: Math.max(current.y, rawMax.y),
+  };
+
   // Only need to be able to move the smallest amount in the desired direction
   const smallestChange: Position = smallestSigned(change);
 
@@ -93,23 +102,6 @@ export const canScrollWindow = (
     change,
   });
 
-export const canScrollDroppable = (
-  droppable: DroppableDimension,
-  change: Position,
-): boolean => {
-  const closestScrollable: ?Scrollable = droppable.viewport.closestScrollable;
-  // Cannot scroll when there is no scroll container!
-  if (!closestScrollable) {
-    return false;
-  }
-
-  return canPartiallyScroll({
-    current: closestScrollable.scroll.current,
-    max: closestScrollable.scroll.max,
-    change,
-  });
-};
-
 export const getWindowOverlap = (
   viewport: Viewport,
   change: Position,
@@ -128,23 +120,41 @@ export const getWindowOverlap = (
   });
 };
 
+export const canScrollDroppable = (
+  droppable: DroppableDimension,
+  change: Position,
+): boolean => {
+  const closest: ?Scrollable = droppable.viewport.closestScrollable;
+
+  // Cannot scroll when there is no scrollable
+  if (!closest) {
+    return false;
+  }
+
+  return canPartiallyScroll({
+    current: closest.scroll.current,
+    max: closest.scroll.max,
+    change,
+  });
+};
+
 export const getDroppableOverlap = (
   droppable: DroppableDimension,
   change: Position,
 ): ?Position => {
-  if (!canScrollDroppable(droppable, change)) {
+  const closest: ?Scrollable = droppable.viewport.closestScrollable;
+
+  if (!closest) {
     return null;
   }
 
-  const closestScrollable: ?Scrollable = droppable.viewport.closestScrollable;
-  // Cannot scroll when there is no scroll container!
-  if (!closestScrollable) {
+  if (!canScrollDroppable(droppable, change)) {
     return null;
   }
 
   return getOverlap({
-    current: closestScrollable.scroll.current,
-    max: closestScrollable.scroll.max,
+    current: closest.scroll.current,
+    max: closest.scroll.max,
     change,
   });
 };

src/state/auto-scroller/fluid-scroller.js

@@ -312,11 +312,19 @@ export default ({ scrollWindow, scrollDroppable }: Api): FluidScroller => {
       return;
     }
 
-    // using the can partially scroll function directly as we want to control
-    // the current and max values without modifying the droppable
+    // Cannot use the standard canScrollDroppable function as we have
+    // modified the max and current values
+
+    // Cannot scroll if there is no scrollable
+    const closest: ?Scrollable = droppable.viewport.closestScrollable;
+
+    if (!closest) {
+      return;
+    }
+
     const canScrollDroppable: boolean = canPartiallyScroll({
-      max: result.max,
       current: result.current,
+      max: result.max,
       change: requiredFrameScroll,
     });
 

src/state/auto-scroller/jump-scroller.js

@@ -3,8 +3,8 @@ import invariant from 'tiny-invariant';
 import { type Position } from 'css-box-model';
 import { add, subtract } from '../position';
 import {
-  canScrollDroppable,
   canScrollWindow,
+  canScrollDroppable,
   getWindowOverlap,
   getDroppableOverlap,
 } from './can-scroll';

src/view/droppable-dimension-publisher/droppable-dimension-publisher.jsx

@@ -348,6 +348,12 @@ export default class DroppableDimensionPublisher extends Component<Props> {
         left: paddingBox.left - base.border.left,
       };
 
+      // We are not accounting for scrollbars
+      // Adjusting for scrollbars is hard because:
+      // - they are different between browsers
+      // - scrollbars can be activated and removed during a drag
+      // We instead account for this slightly in our auto scroller
+
       return createBox({
         borderBox,
         margin: base.margin,

stories/20-super-simple-story.js

@@ -6,4 +6,9 @@ import SimpleWithScroll from './src/simple/simple-scrollable';
 
 storiesOf('Super simple', module)
   .add('vertical list', () => <Simple />)
-  .add('vertical list with scroll', () => <SimpleWithScroll />);
+  .add('vertical list with scroll (overflow: auto)', () => (
+    <SimpleWithScroll overflow="auto" />
+  ))
+  .add('vertical list with scroll (overflow: scroll)', () => (
+    <SimpleWithScroll overflow="scroll" />
+  ));

stories/src/simple/simple-scrollable.jsx

@@ -3,6 +3,7 @@
 /* eslint-disable flowtype/require-valid-file-annotation */
 
 import React, { Component } from 'react';
+import PropTypes from 'prop-types';
 import { DragDropContext, Droppable, Draggable } from '../../../src';
 
 // fake data generator
@@ -37,17 +38,24 @@ const getItemStyle = (isDragging, draggableStyle) => ({
   ...draggableStyle,
 });
 
-const getListStyle = isDraggingOver => ({
+const getListStyle = (isDraggingOver, overflow) => ({
   background: isDraggingOver ? 'lightblue' : 'grey',
   padding: grid,
   margin: grid,
   border: '5px solid pink',
   width: 250,
   maxHeight: '50vh',
-  overflow: 'auto',
+  overflow,
 });
 
 export default class App extends Component {
+  static propTypes = {
+    overflow: PropTypes.string,
+  };
+  static defaultProps = {
+    overflow: 'auto',
+  };
+
   constructor(props, context) {
     super(props, context);
     this.state = {
@@ -82,7 +90,10 @@ export default class App extends Component {
           {(droppableProvided, droppableSnapshot) => (
             <div
               ref={droppableProvided.innerRef}
-              style={getListStyle(droppableSnapshot.isDraggingOver)}
+              style={getListStyle(
+                droppableSnapshot.isDraggingOver,
+                this.props.overflow,
+              )}
             >
               {this.state.items.map((item, index) => (
                 <Draggable key={item.id} draggableId={item.id} index={index}>