From 68ef72a273973fafb31028d2ccaf2e1e0b687627 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Wed, 11 Jul 2018 09:39:37 +1000 Subject: [PATCH] Accounting for cross axis scrollbars in auto scrolling. Fixes #628 (#632) --- src/state/auto-scroller/can-scroll.js | 64 +- src/state/auto-scroller/fluid-scroller.js | 14 +- src/state/auto-scroller/jump-scroller.js | 2 +- .../droppable-dimension-publisher.jsx | 6 + stories/20-super-simple-story.js | 7 +- stories/src/simple/simple-scrollable.jsx | 17 +- .../unit/state/auto-scroll/can-scroll.spec.js | 829 +++++++++--------- .../state/auto-scroll/fluid-scroller.spec.js | 74 +- 8 files changed, 572 insertions(+), 441 deletions(-) diff --git a/src/state/auto-scroller/can-scroll.js b/src/state/auto-scroller/can-scroll.js index 655ded194e..0dff017b8d 100644 --- a/src/state/auto-scroller/can-scroll.js +++ b/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, }); }; diff --git a/src/state/auto-scroller/fluid-scroller.js b/src/state/auto-scroller/fluid-scroller.js index e2d646ae20..af948edc26 100644 --- a/src/state/auto-scroller/fluid-scroller.js +++ b/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, }); diff --git a/src/state/auto-scroller/jump-scroller.js b/src/state/auto-scroller/jump-scroller.js index 51699f92ba..0abc4e36c9 100644 --- a/src/state/auto-scroller/jump-scroller.js +++ b/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'; diff --git a/src/view/droppable-dimension-publisher/droppable-dimension-publisher.jsx b/src/view/droppable-dimension-publisher/droppable-dimension-publisher.jsx index 30887b95c4..998a9f3f8c 100644 --- a/src/view/droppable-dimension-publisher/droppable-dimension-publisher.jsx +++ b/src/view/droppable-dimension-publisher/droppable-dimension-publisher.jsx @@ -348,6 +348,12 @@ export default class DroppableDimensionPublisher extends Component { 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, diff --git a/stories/20-super-simple-story.js b/stories/20-super-simple-story.js index dea19d4d81..d9126b7577 100644 --- a/stories/20-super-simple-story.js +++ b/stories/20-super-simple-story.js @@ -6,4 +6,9 @@ import SimpleWithScroll from './src/simple/simple-scrollable'; storiesOf('Super simple', module) .add('vertical list', () => ) - .add('vertical list with scroll', () => ); + .add('vertical list with scroll (overflow: auto)', () => ( + + )) + .add('vertical list with scroll (overflow: scroll)', () => ( + + )); diff --git a/stories/src/simple/simple-scrollable.jsx b/stories/src/simple/simple-scrollable.jsx index f6b1b478eb..7f3daa70dd 100644 --- a/stories/src/simple/simple-scrollable.jsx +++ b/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) => (
{this.state.items.map((item, index) => ( diff --git a/test/unit/state/auto-scroll/can-scroll.spec.js b/test/unit/state/auto-scroll/can-scroll.spec.js index 5c7aa9aa85..88a8459604 100644 --- a/test/unit/state/auto-scroll/can-scroll.spec.js +++ b/test/unit/state/auto-scroll/can-scroll.spec.js @@ -6,8 +6,8 @@ import { getOverlap, getWindowOverlap, getDroppableOverlap, - canScrollDroppable, canScrollWindow, + canScrollDroppable, } from '../../../../src/state/auto-scroller/can-scroll'; import { add, subtract } from '../../../../src/state/position'; import { getPreset, getDroppableDimension } from '../../../utils/dimension'; @@ -61,505 +61,534 @@ const customViewport: Viewport = createViewport({ scrollWidth: 100, }); -describe('can scroll', () => { - describe('can partially scroll', () => { - it('should return true if not scrolling anywhere', () => { +describe('can partially scroll', () => { + it('should return true if not scrolling anywhere', () => { + const result: boolean = canPartiallyScroll({ + max: { x: 100, y: 100 }, + current: { x: 0, y: 0 }, + // not + change: origin, + }); + + expect(result).toBe(true); + }); + + it('should return true if scrolling to a boundary', () => { + const current: Position = origin; + const max: Position = { x: 100, y: 200 }; + + const corners: Position[] = [ + // top left + { x: 0, y: 0 }, + // top right + { x: max.x, y: 0 }, + // bottom right + { x: max.x, y: max.y }, + // bottom left + { x: 0, y: max.y }, + ]; + + corners.forEach((corner: Position) => { const result: boolean = canPartiallyScroll({ - max: { x: 100, y: 100 }, - current: { x: 0, y: 0 }, - // not - change: origin, + max, + current, + change: corner, }); expect(result).toBe(true); }); + }); - it('should return true if scrolling to a boundary', () => { - const current: Position = origin; - const max: Position = { x: 100, y: 200 }; - - const corners: Position[] = [ - // top left - { x: 0, y: 0 }, - // top right - { x: max.x, y: 0 }, - // bottom right - { x: max.x, y: max.y }, - // bottom left - { x: 0, y: max.y }, - ]; + it('should return true if moving in any direction within the allowable scroll region', () => { + const max: Position = { x: 100, y: 100 }; + const current: Position = { x: 50, y: 50 }; + + // all of these movements are totally possible + const changes: Position[] = [ + // top left + { x: -10, y: 10 }, + // top right + { x: 10, y: 10 }, + // bottom right + { x: 10, y: -10 }, + // bottom left + { x: -10, y: -10 }, + ]; + + changes.forEach((point: Position) => { + const result: boolean = canPartiallyScroll({ + max, + current, + change: point, + }); - corners.forEach((corner: Position) => { - const result: boolean = canPartiallyScroll({ - max, - current, - change: corner, - }); + expect(result).toBe(true); + }); + }); - expect(result).toBe(true); + it('should return true if able to partially move in both directions', () => { + const max: Position = { x: 100, y: 100 }; + const current: Position = { x: 50, y: 50 }; + + // all of these movements are partially possible + const changes: Position[] = [ + // top left + { x: -200, y: 200 }, + // top right + { x: 200, y: 200 }, + // bottom right + { x: 200, y: -200 }, + // bottom left + { x: -200, y: -200 }, + ]; + + changes.forEach((point: Position) => { + const result: boolean = canPartiallyScroll({ + max, + current, + change: point, }); + + expect(result).toBe(true); }); + }); - it('should return true if moving in any direction within the allowable scroll region', () => { - const max: Position = { x: 100, y: 100 }; - const current: Position = { x: 50, y: 50 }; - - // all of these movements are totally possible - const changes: Position[] = [ - // top left - { x: -10, y: 10 }, - // top right - { x: 10, y: 10 }, - // bottom right - { x: 10, y: -10 }, - // bottom left - { x: -10, y: -10 }, - ]; + type Item = {| + current: Position, + change: Position, + |}; + + it('should return true if can only partially move in one direction', () => { + const max: Position = { x: 100, y: 200 }; + + const changes: Item[] = [ + // Can move back in the y direction, but not back in the x direction + { + current: { x: 0, y: 1 }, + change: { x: -1, y: -1 }, + }, + // Can move back in the x direction, but not back in the y direction + { + current: { x: 1, y: 0 }, + change: { x: -1, y: -1 }, + }, + // Can move forward in the y direction, but not forward in the x direction + { + current: subtract(max, { x: 0, y: 1 }), + change: { x: 1, y: 1 }, + }, + // Can move forward in the x direction, but not forward in the y direction + { + current: subtract(max, { x: 1, y: 0 }), + change: { x: 1, y: 1 }, + }, + ]; + + changes.forEach((item: Item) => { + const result: boolean = canPartiallyScroll({ + max, + current: item.current, + change: item.change, + }); - changes.forEach((point: Position) => { - const result: boolean = canPartiallyScroll({ - max, - current, - change: point, - }); + expect(result).toBe(true); + }); + }); - expect(result).toBe(true); + it('should return false if on the min point and move backward in any direction', () => { + const current: Position = origin; + const max: Position = { x: 100, y: 200 }; + const tooFarBack: Position[] = [{ x: 0, y: -1 }, { x: -1, y: 0 }]; + + tooFarBack.forEach((point: Position) => { + const result: boolean = canPartiallyScroll({ + max, + current, + change: point, }); + + expect(result).toBe(false); }); + }); - it('should return true if able to partially move in both directions', () => { - const max: Position = { x: 100, y: 100 }; - const current: Position = { x: 50, y: 50 }; - - // all of these movements are partially possible - const changes: Position[] = [ - // top left - { x: -200, y: 200 }, - // top right - { x: 200, y: 200 }, - // bottom right - { x: 200, y: -200 }, - // bottom left - { x: -200, y: -200 }, - ]; + it('should return false if on the max point and move forward in any direction', () => { + const max: Position = { x: 100, y: 200 }; + const current: Position = max; + const tooFarForward: Position[] = [ + add(max, { x: 0, y: 1 }), + add(max, { x: 1, y: 0 }), + ]; - changes.forEach((point: Position) => { - const result: boolean = canPartiallyScroll({ - max, - current, - change: point, - }); + tooFarForward.forEach((point: Position) => { + const result: boolean = canPartiallyScroll({ + max, + current, + change: point, + }); + + expect(result).toBe(false); + }); + }); + + // It is possible in certain situations for the max scroll to exceed the current scroll. + // In this case we allow the movement backwards even though it is still above the max scroll point + it('should return true if moving backwards and the current scroll is greater than the max scroll', () => { + const max: Position = { x: 100, y: 200 }; + const current: Position = { x: 110, y: 220 }; + // Small changes that would still result in a current scroll greater than the max scroll + const backwards: Position[] = [{ x: -1, y: 0 }, { x: 0, y: -1 }]; - expect(result).toBe(true); + backwards.forEach((change: Position) => { + const result: boolean = canPartiallyScroll({ + current, + max, + change, }); + + expect(result).toBe(true); }); + }); + + it('should return false if moving forwards and the current scroll is greater than the max scroll', () => { + const max: Position = { x: 100, y: 200 }; + const current: Position = { x: 110, y: 220 }; + const forwards: Position[] = [{ x: 1, y: 0 }, { x: 0, y: 1 }]; + + forwards.forEach((change: Position) => { + const result: boolean = canPartiallyScroll({ + current, + max, + change, + }); + + expect(result).toBe(false); + }); + }); +}); + +describe('get overlap', () => { + describe('returning the remainder', () => { + const max: Position = { x: 100, y: 100 }; + const current: Position = { x: 50, y: 50 }; type Item = {| - current: Position, change: Position, + expected: Position, |}; - it('should return true if can only partially move in one direction', () => { - const max: Position = { x: 100, y: 200 }; - - const changes: Item[] = [ - // Can move back in the y direction, but not back in the x direction + it('should return overlap on a single axis', () => { + const items: Item[] = [ + // too far back: top { - current: { x: 0, y: 1 }, - change: { x: -1, y: -1 }, + change: { x: 0, y: -70 }, + expected: { x: 0, y: -20 }, }, - // Can move back in the x direction, but not back in the y direction + // too far back: left { - current: { x: 1, y: 0 }, - change: { x: -1, y: -1 }, + change: { x: -70, y: 0 }, + expected: { x: -20, y: 0 }, }, - // Can move forward in the y direction, but not forward in the x direction + // too far forward: right { - current: subtract(max, { x: 0, y: 1 }), - change: { x: 1, y: 1 }, + change: { x: 70, y: 0 }, + expected: { x: 20, y: 0 }, }, - // Can move forward in the x direction, but not forward in the y direction + // too far forward: bottom { - current: subtract(max, { x: 1, y: 0 }), - change: { x: 1, y: 1 }, + change: { x: 0, y: 70 }, + expected: { x: 0, y: 20 }, }, ]; - changes.forEach((item: Item) => { - const result: boolean = canPartiallyScroll({ + items.forEach((item: Item) => { + const result: ?Position = getOverlap({ + current, max, - current: item.current, change: item.change, }); - expect(result).toBe(true); + expect(result).toEqual(item.expected); }); }); - it('should return false if on the min point and move backward in any direction', () => { - const current: Position = origin; - const max: Position = { x: 100, y: 200 }; - const tooFarBack: Position[] = [{ x: 0, y: -1 }, { x: -1, y: 0 }]; + it('should return overlap on two axis in the same direction', () => { + const items: Item[] = [ + // too far back: top + { + change: { x: -80, y: -70 }, + expected: { x: -30, y: -20 }, + }, + // too far back: left + { + change: { x: -70, y: -80 }, + expected: { x: -20, y: -30 }, + }, + // too far forward: right + { + change: { x: 70, y: 0 }, + expected: { x: 20, y: 0 }, + }, + // too far forward: bottom + { + change: { x: 80, y: 70 }, + expected: { x: 30, y: 20 }, + }, + ]; - tooFarBack.forEach((point: Position) => { - const result: boolean = canPartiallyScroll({ - max, + items.forEach((item: Item) => { + const result: ?Position = getOverlap({ current, - change: point, + max, + change: item.change, }); - expect(result).toBe(false); + expect(result).toEqual(item.expected); }); }); - it('should return false if on the max point and move forward in any direction', () => { - const max: Position = { x: 100, y: 200 }; - const current: Position = max; - const tooFarForward: Position[] = [ - add(max, { x: 0, y: 1 }), - add(max, { x: 1, y: 0 }), + it('should return overlap on two axis in different directions', () => { + const items: Item[] = [ + // too far back: vertical + // too far forward: horizontal + { + change: { x: 80, y: -70 }, + expected: { x: 30, y: -20 }, + }, + // too far back: horizontal + // too far forward: vertical + { + change: { x: -70, y: 80 }, + expected: { x: -20, y: 30 }, + }, ]; - tooFarForward.forEach((point: Position) => { - const result: boolean = canPartiallyScroll({ - max, + items.forEach((item: Item) => { + const result: ?Position = getOverlap({ current, - change: point, + max, + change: item.change, }); - expect(result).toBe(false); + expect(result).toEqual(item.expected); }); }); - }); - describe('get overlap', () => { - describe('returning the remainder', () => { - const max: Position = { x: 100, y: 100 }; - const current: Position = { x: 50, y: 50 }; - - type Item = {| - change: Position, - expected: Position, - |}; - - it('should return overlap on a single axis', () => { - const items: Item[] = [ - // too far back: top - { - change: { x: 0, y: -70 }, - expected: { x: 0, y: -20 }, - }, - // too far back: left - { - change: { x: -70, y: 0 }, - expected: { x: -20, y: 0 }, - }, - // too far forward: right - { - change: { x: 70, y: 0 }, - expected: { x: 20, y: 0 }, - }, - // too far forward: bottom - { - change: { x: 0, y: 70 }, - expected: { x: 0, y: 20 }, - }, - ]; - - items.forEach((item: Item) => { - const result: ?Position = getOverlap({ - current, - max, - change: item.change, - }); - - expect(result).toEqual(item.expected); - }); - }); - - it('should return overlap on two axis in the same direction', () => { - const items: Item[] = [ - // too far back: top - { - change: { x: -80, y: -70 }, - expected: { x: -30, y: -20 }, - }, - // too far back: left - { - change: { x: -70, y: -80 }, - expected: { x: -20, y: -30 }, - }, - // too far forward: right - { - change: { x: 70, y: 0 }, - expected: { x: 20, y: 0 }, - }, - // too far forward: bottom - { - change: { x: 80, y: 70 }, - expected: { x: 30, y: 20 }, - }, - ]; - - items.forEach((item: Item) => { - const result: ?Position = getOverlap({ - current, - max, - change: item.change, - }); - - expect(result).toEqual(item.expected); - }); - }); + it('should trim values that can be scrolled', () => { + const items: Item[] = [ + // too far back: top + { + // x can be scrolled entirely + // y can be partially scrolled + change: { x: -20, y: -70 }, + expected: { x: 0, y: -20 }, + }, + // too far back: left + { + // x can be partially scrolled + // y can be scrolled entirely + change: { x: -70, y: -40 }, + expected: { x: -20, y: 0 }, + }, + // too far forward: right + { + // x can be partially scrolled + // y can be scrolled entirely + change: { x: 70, y: 40 }, + expected: { x: 20, y: 0 }, + }, + // too far forward: bottom + { + // x can be scrolled entirely + // y can be partially scrolled + change: { x: 20, y: 70 }, + expected: { x: 0, y: 20 }, + }, + ]; - it('should return overlap on two axis in different directions', () => { - const items: Item[] = [ - // too far back: vertical - // too far forward: horizontal - { - change: { x: 80, y: -70 }, - expected: { x: 30, y: -20 }, - }, - // too far back: horizontal - // too far forward: vertical - { - change: { x: -70, y: 80 }, - expected: { x: -20, y: 30 }, - }, - ]; - - items.forEach((item: Item) => { - const result: ?Position = getOverlap({ - current, - max, - change: item.change, - }); - - expect(result).toEqual(item.expected); + items.forEach((item: Item) => { + const result: ?Position = getOverlap({ + current, + max, + change: item.change, }); - }); - it('should trim values that can be scrolled', () => { - const items: Item[] = [ - // too far back: top - { - // x can be scrolled entirely - // y can be partially scrolled - change: { x: -20, y: -70 }, - expected: { x: 0, y: -20 }, - }, - // too far back: left - { - // x can be partially scrolled - // y can be scrolled entirely - change: { x: -70, y: -40 }, - expected: { x: -20, y: 0 }, - }, - // too far forward: right - { - // x can be partially scrolled - // y can be scrolled entirely - change: { x: 70, y: 40 }, - expected: { x: 20, y: 0 }, - }, - // too far forward: bottom - { - // x can be scrolled entirely - // y can be partially scrolled - change: { x: 20, y: 70 }, - expected: { x: 0, y: 20 }, - }, - ]; - - items.forEach((item: Item) => { - const result: ?Position = getOverlap({ - current, - max, - change: item.change, - }); - - expect(result).toEqual(item.expected); - }); + expect(result).toEqual(item.expected); }); }); }); +}); - describe('can scroll droppable', () => { - it('should return false if the droppable is not scrollable', () => { - const result: boolean = canScrollDroppable(preset.home, { x: 1, y: 1 }); - - expect(result).toBe(false); - }); - - it('should return true if the droppable is able to be scrolled', () => { - const result: boolean = canScrollDroppable(scrollable, { x: 0, y: 20 }); +describe('can scroll droppable', () => { + it('should return false if the droppable is not scrollable', () => { + const result: boolean = canScrollDroppable(preset.home, { x: 1, y: 1 }); - expect(result).toBe(true); - }); + expect(result).toBe(false); + }); - it('should return false if the droppable is not able to be scrolled', () => { - const result: boolean = canScrollDroppable(scrollable, { x: -1, y: 0 }); + it('should return true if the droppable is able to be scrolled', () => { + const result: boolean = canScrollDroppable(scrollable, { x: 0, y: 20 }); - expect(result).toBe(false); - }); + expect(result).toBe(true); }); - describe('can scroll window', () => { - it('should return true if the window is able to be scrolled', () => { - const viewport: Viewport = createViewport({ - frame: customViewport.frame, - scrollHeight: 200, - scrollWidth: 100, - scroll: origin, - }); + it('should return false if the droppable is not able to be scrolled', () => { + const result: boolean = canScrollDroppable(scrollable, { x: -1, y: 0 }); - const result: boolean = canScrollWindow(viewport, { x: 0, y: 50 }); + expect(result).toBe(false); + }); +}); - expect(result).toBe(true); +describe('can scroll window', () => { + it('should return true if the window is able to be scrolled', () => { + const viewport: Viewport = createViewport({ + frame: customViewport.frame, + scrollHeight: 200, + scrollWidth: 100, + scroll: origin, }); - it('should return false if the window is not able to be scrolled', () => { - const viewport: Viewport = createViewport({ - frame: customViewport.frame, - scrollHeight: 200, - scrollWidth: 100, - // already at the max scroll - scroll: { - x: 0, - y: 200, - }, - }); - - const result: boolean = canScrollWindow(viewport, { x: 0, y: 1 }); + const result: boolean = canScrollWindow(viewport, { x: 0, y: 50 }); - expect(result).toBe(false); - }); + expect(result).toBe(true); }); - describe('get droppable overlap', () => { - it('should return null if there is no scroll container', () => { - const result: ?Position = getDroppableOverlap(preset.home, { - x: 1, - y: 1, - }); - - expect(result).toBe(null); - }); - - it('should return null if the droppable cannot be scrolled', () => { - // end of the scrollable area - const scroll: Position = { + it('should return false if the window is not able to be scrolled', () => { + const viewport: Viewport = createViewport({ + frame: customViewport.frame, + scrollHeight: 200, + scrollWidth: 100, + // already at the max scroll + scroll: { x: 0, y: 200, - }; - const scrolled: DroppableDimension = scrollDroppable(scrollable, scroll); - const result: ?Position = getDroppableOverlap(scrolled, { x: 0, y: 1 }); - - expect(result).toBe(null); + }, }); - // tested in get remainder - it('should return the overlap', () => { - // how far the droppable has already - const scroll: Position = { - x: 10, - y: 20, - }; - const scrolled: DroppableDimension = scrollDroppable(scrollable, scroll); - // $ExpectError - not checking for null - const max: Position = scrolled.viewport.closestScrollable.scroll.max; - const totalSpace: Position = { - x: scrollableScrollSize.scrollWidth - max.x, - y: scrollableScrollSize.scrollHeight - max.y, - }; - const remainingSpace = subtract(totalSpace, scroll); - const change: Position = { x: 300, y: 300 }; - const expectedOverlap: Position = subtract(change, remainingSpace); - - const result: ?Position = getDroppableOverlap(scrolled, change); - - expect(result).toEqual(expectedOverlap); - }); + const result: boolean = canScrollWindow(viewport, { x: 0, y: 1 }); - it('should return null if there is no overlap', () => { - const change: Position = { x: 0, y: 1 }; + expect(result).toBe(false); + }); +}); - const result: ?Position = getDroppableOverlap(scrollable, change); +describe('get droppable overlap', () => { + it('should return null if there is no scroll container', () => { + const result: ?Position = getDroppableOverlap(preset.home, { + x: 1, + y: 1, + }); - expect(result).toEqual(null); + expect(result).toBe(null); + }); - // verifying correctness of test + it('should return null if the droppable cannot be scrolled', () => { + // end of the scrollable area + const scroll: Position = { + x: 0, + y: 200, + }; + const scrolled: DroppableDimension = scrollDroppable(scrollable, scroll); + const result: ?Position = getDroppableOverlap(scrolled, { x: 0, y: 1 }); - expect(canScrollDroppable(scrollable, change)).toBe(true); - }); + expect(result).toBe(null); }); - describe('get window overlap', () => { - it('should return null if the window cannot be scrolled', () => { - const viewport: Viewport = createViewport({ - frame: customViewport.frame, - scrollHeight: 200, - scrollWidth: 100, - // already at the max scroll - scroll: { - x: 0, - y: 200, - }, - }); + // tested in get remainder + it('should return the overlap', () => { + // how far the droppable has already + const scroll: Position = { + x: 10, + y: 20, + }; + const scrolled: DroppableDimension = scrollDroppable(scrollable, scroll); + // $ExpectError - not checking for null + const max: Position = scrolled.viewport.closestScrollable.scroll.max; + const totalSpace: Position = { + x: scrollableScrollSize.scrollWidth - max.x, + y: scrollableScrollSize.scrollHeight - max.y, + }; + const remainingSpace = subtract(totalSpace, scroll); + const change: Position = { x: 300, y: 300 }; + const expectedOverlap: Position = subtract(change, remainingSpace); + + const result: ?Position = getDroppableOverlap(scrolled, change); + + expect(result).toEqual(expectedOverlap); + }); - const result: ?Position = getWindowOverlap(viewport, { x: 0, y: 1 }); + it('should return null if there is no overlap', () => { + const change: Position = { x: 0, y: 1 }; - expect(result).toBe(null); + const result: ?Position = getDroppableOverlap(scrollable, change); + + expect(result).toEqual(null); + + // verifying correctness of test + expect(canScrollDroppable(scrollable, change)).toBe(true); + }); +}); + +describe('get window overlap', () => { + it('should return null if the window cannot be scrolled', () => { + const viewport: Viewport = createViewport({ + frame: customViewport.frame, + scrollHeight: 200, + scrollWidth: 100, + // already at the max scroll + scroll: { + x: 0, + y: 200, + }, }); - // tested in get remainder - it('should return the overlap', () => { - const viewport: Viewport = createViewport({ - frame: customViewport.frame, - scrollHeight: 200, - scrollWidth: 200, - scroll: { - x: 50, - y: 50, - }, - }); + const result: ?Position = getWindowOverlap(viewport, { x: 0, y: 1 }); - // little validation - const maxScroll: Position = getMaxScroll({ - scrollHeight: 200, - scrollWidth: 200, - height: viewport.frame.height, - width: viewport.frame.width, - }); - expect(maxScroll).toEqual({ x: 100, y: 100 }); + expect(result).toBe(null); + }); - const availableScrollSpace: Position = { + // tested in get remainder + it('should return the overlap', () => { + const viewport: Viewport = createViewport({ + frame: customViewport.frame, + scrollHeight: 200, + scrollWidth: 200, + scroll: { x: 50, y: 50, - }; - // cannot be absorbed in the current scroll plane - const bigChange: Position = { x: 300, y: 300 }; - const expectedOverlap: Position = subtract( - bigChange, - availableScrollSpace, - ); - - const result: ?Position = getWindowOverlap(viewport, bigChange); + }, + }); - expect(result).toEqual(expectedOverlap); + // little validation + const maxScroll: Position = getMaxScroll({ + scrollHeight: 200, + scrollWidth: 200, + height: viewport.frame.height, + width: viewport.frame.width, }); + expect(maxScroll).toEqual({ x: 100, y: 100 }); - it('should return null if there is no overlap', () => { - const viewport: Viewport = createViewport({ - frame: customViewport.frame, - scrollHeight: 200, - scrollWidth: 200, - scroll: origin, - }); + const availableScrollSpace: Position = { + x: 50, + y: 50, + }; + // cannot be absorbed in the current scroll plane + const bigChange: Position = { x: 300, y: 300 }; + const expectedOverlap: Position = subtract(bigChange, availableScrollSpace); - const result: ?Position = getWindowOverlap(viewport, { x: 10, y: 10 }); + const result: ?Position = getWindowOverlap(viewport, bigChange); - expect(result).toBe(null); + expect(result).toEqual(expectedOverlap); + }); + + it('should return null if there is no overlap', () => { + const viewport: Viewport = createViewport({ + frame: customViewport.frame, + scrollHeight: 200, + scrollWidth: 200, + scroll: origin, }); + + const result: ?Position = getWindowOverlap(viewport, { x: 10, y: 10 }); + + expect(result).toBe(null); }); }); diff --git a/test/unit/state/auto-scroll/fluid-scroller.spec.js b/test/unit/state/auto-scroll/fluid-scroller.spec.js index 9c901c9408..40373b661d 100644 --- a/test/unit/state/auto-scroll/fluid-scroller.spec.js +++ b/test/unit/state/auto-scroll/fluid-scroller.spec.js @@ -137,12 +137,10 @@ describe('fluid auto scrolling', () => { selection, viewport, ); - if (!impact) { - return base; - } + return { ...base, - impact, + impact: impact || base.impact, }; }; @@ -304,7 +302,11 @@ describe('fluid auto scrolling', () => { }); const selection: Position = onMaxBoundary; const custom: DraggingState = addDraggable( - state.dragging(preset.inHome1.descriptor.id, selection), + state.dragging( + preset.inHome1.descriptor.id, + selection, + scrollableViewport, + ), tooBig, ); @@ -507,7 +509,11 @@ describe('fluid auto scrolling', () => { }); const selection: Position = onMaxBoundary; const custom: DraggingState = addDraggable( - state.dragging(preset.inHome1.descriptor.id, selection), + state.dragging( + preset.inHome1.descriptor.id, + selection, + scrollableViewport, + ), tooBig, ); @@ -1629,6 +1635,62 @@ describe('fluid auto scrolling', () => { expect(mocks.scrollDroppable).not.toHaveBeenCalled(); }); }); + + // This can happen when there is a scrollbar on the cross axis + describe('moving backwards when current scroll is greater than max', () => { + const droppableScroll: Position = add( + getClosestScrollable(scrollable).scroll.max, + patch(axis.line, 10), + ); + const scrolled: DroppableDimension = scrollDroppable( + scrollable, + droppableScroll, + ); + const onStartBoundary: Position = patch( + axis.line, + // to the boundary is not enough to start + frameClient.borderBox[axis.start] + thresholds.startFrom, + frameClient.borderBox.center[axis.crossAxisLine], + ); + + it('should have a current scroll greater than the current scroll (validation)', () => { + expect( + getClosestScrollable(scrolled).scroll.max[axis.line], + ).toBeLessThan( + getClosestScrollable(scrolled).scroll.current[axis.line], + ); + }); + + it('should allow scrolling backwards - even if still above the max scroll', () => { + // going backwards + const target: Position = subtract( + onStartBoundary, + // scrolling less than the excess so it is still above the max + patch(axis.line, 1), + ); + + fluidScroll( + addDroppable( + dragTo({ + selection: target, + viewport: unscrollableViewport, + }), + scrolled, + ), + ); + + expect(mocks.scrollDroppable).not.toHaveBeenCalled(); + // only called after a frame + requestAnimationFrame.step(); + expect(mocks.scrollDroppable).toHaveBeenCalled(); + const [id, offset] = mocks.scrollDroppable.mock.calls[0]; + + expect(id).toBe(scrollable.descriptor.id); + // moving backwards + expect(offset[axis.line]).toBeLessThan(0); + expect(offset[axis.crossAxisLine]).toBe(0); + }); + }); }); describe('window scrolling before droppable scrolling', () => {