From b7226033e6fced7cf7e8248b2b55c288bbc95d94 Mon Sep 17 00:00:00 2001 From: Jared Crowe Date: Tue, 17 Oct 2017 14:03:38 +1100 Subject: [PATCH] Keyboard drag dimension clipping fix (#137) * cross-axis keyboard movement now clips droppable dimensions when calculating best droppable * move getCorners into spacing.js. add tests for untested spacing functions. add comment. --- .../is-within-visible-bounds-of-droppable.js | 2 +- .../get-best-cross-axis-droppable.js | 108 +++++++++--------- src/state/spacing.js | 7 ++ stories/5-multiple-vertical-lists-story.js | 2 +- .../get-best-cross-axis-droppable.spec.js | 26 +++++ test/unit/state/spacing.spec.js | 42 +++++++ 6 files changed, 133 insertions(+), 54 deletions(-) diff --git a/src/state/is-within-visible-bounds-of-droppable.js b/src/state/is-within-visible-bounds-of-droppable.js index f7b978a1e2..7474590ea7 100644 --- a/src/state/is-within-visible-bounds-of-droppable.js +++ b/src/state/is-within-visible-bounds-of-droppable.js @@ -10,7 +10,7 @@ import type { Spacing, } from '../types'; -const getVisibleBounds = (droppable: DroppableDimension): Spacing => { +export const getVisibleBounds = (droppable: DroppableDimension): Spacing => { const { scroll, bounds: containerBounds } = droppable.container; // Calculate the mid-drag scroll ∆ of the scroll container diff --git a/src/state/move-cross-axis/get-best-cross-axis-droppable.js b/src/state/move-cross-axis/get-best-cross-axis-droppable.js index a0f4aaeb62..4dd8b9e748 100644 --- a/src/state/move-cross-axis/get-best-cross-axis-droppable.js +++ b/src/state/move-cross-axis/get-best-cross-axis-droppable.js @@ -1,26 +1,17 @@ // @flow import { closest } from '../position'; import isWithin from '../is-within'; +import { getCorners } from '../spacing'; +import { getVisibleBounds } from '../is-within-visible-bounds-of-droppable'; import type { Axis, - Position, - DroppableId, - DimensionFragment, DroppableDimension, DroppableDimensionMap, + DroppableId, + Position, + Spacing, } from '../../types'; -const getCorners = (droppable: DroppableDimension): Position[] => { - const fragment: DimensionFragment = droppable.page.withMargin; - - return [ - { x: fragment.left, y: fragment.top }, - { x: fragment.right, y: fragment.top }, - { x: fragment.left, y: fragment.bottom }, - { x: fragment.right, y: fragment.bottom }, - ]; -}; - type GetBestDroppableArgs = {| isMovingForward: boolean, // the current position of the dragging item @@ -31,6 +22,13 @@ type GetBestDroppableArgs = {| droppables: DroppableDimensionMap, |} +// This is the shape of an object which we use in this module for +// passing a droppable's clipped bounds along with its dimension +type Candidate = {| + bounds: Spacing, + droppable: DroppableDimension, +|}; + export default ({ isMovingForward, pageCenter, @@ -38,53 +36,59 @@ export default ({ droppables, }: GetBestDroppableArgs): ?DroppableDimension => { const axis: Axis = source.axis; + const sourceBounds: Spacing = getVisibleBounds(source); - const candidates: DroppableDimension[] = Object.keys(droppables) - .map((id: DroppableId) => droppables[id]) + const candidates: Candidate[] = Object.keys(droppables) + .map((id: DroppableId): DroppableDimension => droppables[id]) // Remove the source droppable from the list .filter((droppable: DroppableDimension): boolean => droppable !== source) // Remove any options that are not enabled .filter((droppable: DroppableDimension): boolean => droppable.isEnabled) + // Get the true visible bounds of the droppables. + // We calculate it once here and pass it on in an + // object along with the original dimension. + .map((droppable: DroppableDimension): Candidate => ({ + bounds: getVisibleBounds(droppable), + droppable, + })) // Get only droppables that are on the desired side - .filter((droppable: DroppableDimension): boolean => { + .filter(({ bounds }: Candidate): boolean => { if (isMovingForward) { // is the droppable in front of the source on the cross axis? - return source.page.withMargin[axis.crossAxisEnd] <= - droppable.page.withMargin[axis.crossAxisStart]; + return sourceBounds[axis.crossAxisEnd] <= + bounds[axis.crossAxisStart]; } // is the droppable behind the source on the cross axis? - return droppable.page.withMargin[axis.crossAxisEnd] <= - source.page.withMargin[axis.crossAxisStart]; + return bounds[axis.crossAxisEnd] <= + sourceBounds[axis.crossAxisStart]; }) // Must have some overlap on the main axis - .filter((droppable: DroppableDimension): boolean => { - const sourceFragment: DimensionFragment = source.page.withMargin; - const destinationFragment: DimensionFragment = droppable.page.withMargin; - + .filter(({ bounds }: Candidate): boolean => { const isBetweenSourceBounds = isWithin( - sourceFragment[axis.start], - sourceFragment[axis.end] + sourceBounds[axis.start], + sourceBounds[axis.end] ); const isBetweenDestinationBounds = isWithin( - destinationFragment[axis.start], - destinationFragment[axis.end] + bounds[axis.start], + bounds[axis.end] ); - return isBetweenSourceBounds(destinationFragment[axis.start]) || - isBetweenSourceBounds(destinationFragment[axis.end]) || - isBetweenDestinationBounds(sourceFragment[axis.start]) || - isBetweenDestinationBounds(sourceFragment[axis.end]); + return isBetweenSourceBounds(bounds[axis.start]) || + isBetweenSourceBounds(bounds[axis.end]) || + isBetweenDestinationBounds(sourceBounds[axis.start]) || + isBetweenDestinationBounds(sourceBounds[axis.end]); }) - .filter((droppable: DroppableDimension) => ( + // Filter any droppables which are obscured by their container + .filter(({ droppable }: Candidate) => ( (droppable.page.withoutMargin[axis.crossAxisStart] >= droppable.container.bounds[axis.crossAxisStart]) && (droppable.page.withoutMargin[axis.crossAxisEnd] <= droppable.container.bounds[axis.crossAxisEnd]) )) // Sort on the cross axis - .sort((a: DroppableDimension, b: DroppableDimension) => { - const first: number = a.page.withMargin[axis.crossAxisStart]; - const second: number = b.page.withMargin[axis.crossAxisStart]; + .sort(({ bounds: a }: Candidate, { bounds: b }: Candidate) => { + const first: number = a[axis.crossAxisStart]; + const second: number = b[axis.crossAxisStart]; if (isMovingForward) { return first - second; @@ -92,9 +96,9 @@ export default ({ return second - first; }) // Find the droppables that have the same cross axis value as the first item - .filter((droppable: DroppableDimension, index: number, array: DroppableDimension[]): boolean => - droppable.page.withMargin[axis.crossAxisStart] === - array[0].page.withMargin[axis.crossAxisStart] + .filter(({ bounds }: Candidate, index: number, array: Candidate[]): boolean => + bounds[axis.crossAxisStart] === + array[0].bounds[axis.crossAxisStart] ); // no possible candidates @@ -104,38 +108,38 @@ export default ({ // only one result - all done! if (candidates.length === 1) { - return candidates[0]; + return candidates[0].droppable; } // At this point we have a number of candidates that // all have the same axis.crossAxisStart value. // Check to see if the center position is within the size of a Droppable on the main axis - const contains: DroppableDimension[] = candidates - .filter((droppable: DroppableDimension) => { + const contains: Candidate[] = candidates + .filter(({ bounds }: Candidate) => { const isWithinDroppable = isWithin( - droppable.page.withMargin[axis.start], - droppable.page.withMargin[axis.end] + bounds[axis.start], + bounds[axis.end] ); return isWithinDroppable(pageCenter[axis.line]); }); if (contains.length === 1) { - return contains[0]; + return contains[0].droppable; } // The center point of the draggable falls on the boundary between two droppables if (contains.length > 1) { // sort on the main axis and choose the first - return contains.sort((a: DroppableDimension, b: DroppableDimension) => ( - a.page.withMargin[axis.start] - b.page.withMargin[axis.start] - ))[0]; + return contains.sort(({ bounds: a }: Candidate, { bounds: b }: Candidate) => ( + a[axis.start] - b[axis.start] + ))[0].droppable; } // The center is not contained within any droppable // 1. Find the candidate that has the closest corner // 2. If there is a tie - choose the one that is first on the main axis - return candidates.sort((a: DroppableDimension, b: DroppableDimension) => { + return candidates.sort(({ bounds: a }: Candidate, { bounds: b }: Candidate) => { const first = closest(pageCenter, getCorners(a)); const second = closest(pageCenter, getCorners(b)); @@ -146,6 +150,6 @@ export default ({ // They both have the same distance - // choose the one that is first on the main axis - return a.page.withMargin[axis.start] - b.page.withMargin[axis.start]; - })[0]; + return a[axis.start] - b[axis.start]; + })[0].droppable; }; diff --git a/src/state/spacing.js b/src/state/spacing.js index b317512abb..b879bce798 100644 --- a/src/state/spacing.js +++ b/src/state/spacing.js @@ -26,3 +26,10 @@ export const offset = (spacing: Spacing, position: Position): Spacing => ({ bottom: spacing.bottom + position.y, left: spacing.left + position.x, }); + +export const getCorners = (spacing: Spacing): Position[] => [ + { x: spacing.left, y: spacing.top }, + { x: spacing.right, y: spacing.top }, + { x: spacing.left, y: spacing.bottom }, + { x: spacing.right, y: spacing.bottom }, +]; diff --git a/stories/5-multiple-vertical-lists-story.js b/stories/5-multiple-vertical-lists-story.js index e6d50538a1..85926ef427 100644 --- a/stories/5-multiple-vertical-lists-story.js +++ b/stories/5-multiple-vertical-lists-story.js @@ -21,7 +21,7 @@ const quoteMap: QuoteMap = { [beta]: getQuotes(3), [gamma]: getQuotes(7), [delta]: getQuotes(2), - [epsilon]: getQuotes(5), + [epsilon]: getQuotes(10), [zeta]: getQuotes(5), [eta]: getQuotes(5), [theta]: getQuotes(5), diff --git a/test/unit/state/move-cross-axis/get-best-cross-axis-droppable.spec.js b/test/unit/state/move-cross-axis/get-best-cross-axis-droppable.spec.js index 732079e809..cf4873d457 100644 --- a/test/unit/state/move-cross-axis/get-best-cross-axis-droppable.spec.js +++ b/test/unit/state/move-cross-axis/get-best-cross-axis-droppable.spec.js @@ -223,6 +223,13 @@ describe('get best cross axis droppable', () => { id: 'sibling1', direction: axis.direction, clientRect: getClientRect({ + top: 20, + left: 20, + right: 40, + // long droppable inside a shorter container - this should be clipped + bottom: 80, + }), + containerRect: getClientRect({ // not the same top value as source top: 20, // shares the left edge with the source @@ -266,6 +273,25 @@ describe('get best cross axis droppable', () => { expect(result).toBe(sibling2); }); + it('should account for container clipping', () => { + // inside sibling1's droppable bounds, but outside its clipped bounds + const center: Position = { + y: 50, + x: 10, + }; + + // if we're clipping dimensions correctly we should land in sibling2 + // if not, we'll land in sibling1 + const result: ?DroppableDimension = getBestCrossAxisDroppable({ + isMovingForward: true, + pageCenter: center, + source, + droppables, + }); + + expect(result).toBe(sibling2); + }); + describe('center point is not contained within a droppable', () => { it('should return the droppable that has the closest corner', () => { // Choosing a point that is above the first sibling diff --git a/test/unit/state/spacing.spec.js b/test/unit/state/spacing.spec.js index 3f83827751..5855660ab0 100644 --- a/test/unit/state/spacing.spec.js +++ b/test/unit/state/spacing.spec.js @@ -1,8 +1,10 @@ // @flow import { add, + addPosition, isEqual, offset, + getCorners, } from '../../../src/state/spacing'; import type { Position, Spacing } from '../../../src/types'; @@ -33,6 +35,28 @@ describe('spacing', () => { }); }); + describe('addPosition', () => { + it('should add a position to the right and bottom bounds of a spacing box', () => { + const spacing = { + top: 0, + right: 10, + bottom: 10, + left: 0, + }; + const position = { + x: 5, + y: 5, + }; + const expected = { + top: 0, + right: 15, + bottom: 15, + left: 0, + }; + expect(addPosition(spacing, position)).toEqual(expected); + }); + }); + describe('isEqual', () => { it('should return true when two spacings are the same', () => { expect(isEqual(spacing1, spacing1)).toBe(true); @@ -63,4 +87,22 @@ describe('spacing', () => { expect(offset(spacing1, offsetPosition)).toEqual(expected); }); }); + + describe('getCorners', () => { + it('should return the corners of a spacing box in the order TL, TR, BL, BR', () => { + const spacing: Spacing = { + top: 1, + right: 2, + bottom: 3, + left: 4, + }; + const expected = [ + { x: 4, y: 1 }, + { x: 2, y: 1 }, + { x: 4, y: 3 }, + { x: 2, y: 3 }, + ]; + expect(getCorners(spacing)).toEqual(expected); + }); + }); });