From dd1910d589833d2f7af4a3db02415e2baf9284e0 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Thu, 21 Jun 2018 10:22:06 +1000 Subject: [PATCH] fixing drag handle --- src/view/drag-handle/drag-handle.jsx | 4 +- .../sensor/create-keyboard-sensor.js | 6 +- .../drag-handle/sensor/create-mouse-sensor.js | 13 +- .../drag-handle/sensor/create-touch-sensor.js | 10 +- .../unit/view/drag-handle/drag-handle.spec.js | 327 ++++++------------ 5 files changed, 115 insertions(+), 245 deletions(-) diff --git a/src/view/drag-handle/drag-handle.jsx b/src/view/drag-handle/drag-handle.jsx index 380b31739c..c00eabe2ba 100644 --- a/src/view/drag-handle/drag-handle.jsx +++ b/src/view/drag-handle/drag-handle.jsx @@ -204,8 +204,8 @@ export default class DragHandle extends Component { } onKeyDown = (event: KeyboardEvent) => { - // let the mouse sensor deal with it - if (this.mouseSensor.isCapturing()) { + // let the other sensors deal with it + if (this.mouseSensor.isCapturing() || this.touchSensor.isCapturing()) { return; } diff --git a/src/view/drag-handle/sensor/create-keyboard-sensor.js b/src/view/drag-handle/sensor/create-keyboard-sensor.js index ad3deef83f..e8174364aa 100644 --- a/src/view/drag-handle/sensor/create-keyboard-sensor.js +++ b/src/view/drag-handle/sensor/create-keyboard-sensor.js @@ -1,5 +1,6 @@ // @flow /* eslint-disable no-use-before-define */ +import invariant from 'tiny-invariant'; import { type Position } from 'css-box-model'; import createScheduler from '../util/create-scheduler'; import preventStandardKeyEvents from '../util/prevent-standard-key-events'; @@ -82,10 +83,7 @@ export default ({ const ref: ?HTMLElement = getDraggableRef(); - if (!ref) { - console.error('cannot start a keyboard drag without a draggable ref'); - return; - } + invariant(ref, 'Cannot start a keyboard drag without a draggable ref'); // using center position as selection const center: Position = getBorderBoxCenterPosition(ref); diff --git a/src/view/drag-handle/sensor/create-mouse-sensor.js b/src/view/drag-handle/sensor/create-mouse-sensor.js index cbdc45bb83..7f08d75986 100644 --- a/src/view/drag-handle/sensor/create-mouse-sensor.js +++ b/src/view/drag-handle/sensor/create-mouse-sensor.js @@ -1,5 +1,6 @@ // @flow /* eslint-disable no-use-before-define */ +import invariant from 'tiny-invariant'; import { type Position } from 'css-box-model'; import createScheduler from '../util/create-scheduler'; import isSloppyClickThresholdExceeded from '../util/is-sloppy-click-threshold-exceeded'; @@ -247,13 +248,13 @@ export default ({ return; } - if (!canStartCapturing(event)) { - return; - } + invariant(!isCapturing(), 'Should not be able to perform a mouse down while a drag or pending drag is occurring'); - if (isCapturing()) { - console.error('should not be able to perform a mouse down while a drag or pending drag is occurring'); - cancel(); + if (!canStartCapturing(event)) { + // blocking the event as we want to opt out of the standard browser behaviour + // this *could* occur during a drop - although it should not thanks to pointer-events: none + // this is just being really safe + event.preventDefault(); return; } diff --git a/src/view/drag-handle/sensor/create-touch-sensor.js b/src/view/drag-handle/sensor/create-touch-sensor.js index dc007eb4dd..65e6d5c3df 100644 --- a/src/view/drag-handle/sensor/create-touch-sensor.js +++ b/src/view/drag-handle/sensor/create-touch-sensor.js @@ -1,5 +1,6 @@ // @flow /* eslint-disable no-use-before-define */ +import invariant from 'tiny-invariant'; import { type Position } from 'css-box-model'; import createScheduler from '../util/create-scheduler'; import createPostDragEventPreventer, { type EventPreventer } from '../util/create-post-drag-event-preventer'; @@ -368,14 +369,9 @@ export default ({ return; } - // TODO: call prevent default here? - if (!canStartCapturing(event)) { - return; - } + invariant(!isCapturing(), 'Should not be able to perform a touch start while a drag or pending drag is occurring'); - if (isCapturing()) { - console.error('should not be able to perform a touch start while a drag or pending drag is occurring'); - cancel(); + if (!canStartCapturing(event)) { return; } diff --git a/test/unit/view/drag-handle/drag-handle.spec.js b/test/unit/view/drag-handle/drag-handle.spec.js index 9e525ac754..2cd4c92551 100644 --- a/test/unit/view/drag-handle/drag-handle.spec.js +++ b/test/unit/view/drag-handle/drag-handle.spec.js @@ -30,10 +30,10 @@ const auxiliaryButton: number = 1; const getStubCallbacks = (): Callbacks => ({ onLift: jest.fn(), onMove: jest.fn(), - onMoveForward: jest.fn(), - onMoveBackward: jest.fn(), - onCrossAxisMoveForward: jest.fn(), - onCrossAxisMoveBackward: jest.fn(), + onMoveUp: jest.fn(), + onMoveDown: jest.fn(), + onMoveRight: jest.fn(), + onMoveLeft: jest.fn(), onDrop: jest.fn(), onCancel: jest.fn(), onWindowScroll: jest.fn(), @@ -48,10 +48,10 @@ const resetCallbacks = (callbacks: Callbacks) => { type CallBacksCalledFn = {| onLift?: number, onMove?: number, - onMoveForward?: number, - onMoveBackward?: number, - onCrossAxisMoveForward ?: number, - onCrossAxisMoveBackward?: number, + onMoveUp?: number, + onMoveDown?: number, + onMoveRight ?: number, + onMoveLeft?: number, onDrop?: number, onCancel ?: number, onWindowScroll ?: number, @@ -60,21 +60,21 @@ type CallBacksCalledFn = {| const callbacksCalled = (callbacks: Callbacks) => ({ onLift = 0, onMove = 0, - onMoveForward = 0, - onMoveBackward = 0, - onCrossAxisMoveForward = 0, - onCrossAxisMoveBackward = 0, + onMoveUp = 0, + onMoveDown = 0, + onMoveRight = 0, + onMoveLeft = 0, onDrop = 0, onCancel = 0, }: CallBacksCalledFn = {}) => callbacks.onLift.mock.calls.length === onLift && callbacks.onMove.mock.calls.length === onMove && - callbacks.onMoveForward.mock.calls.length === onMoveForward && - callbacks.onMoveBackward.mock.calls.length === onMoveBackward && + callbacks.onMoveUp.mock.calls.length === onMoveUp && + callbacks.onMoveDown.mock.calls.length === onMoveDown && callbacks.onDrop.mock.calls.length === onDrop && callbacks.onCancel.mock.calls.length === onCancel && - callbacks.onCrossAxisMoveForward.mock.calls.length === onCrossAxisMoveForward && - callbacks.onCrossAxisMoveBackward.mock.calls.length === onCrossAxisMoveBackward; + callbacks.onMoveRight.mock.calls.length === onMoveRight && + callbacks.onMoveLeft.mock.calls.length === onMoveLeft; const whereAnyCallbacksCalled = (callbacks: Callbacks) => !callbacksCalled(callbacks)(); @@ -178,7 +178,6 @@ const getNestedWrapper = (parentCallbacks: Callbacks, childCallbacks: Callbacks) +const getWrapper = (callbacks: Callbacks, context: ?Object = basicContext): ReactWrapper => mount( )} , - { context: basicContext } + { context } ); describe('drag handle', () => { @@ -268,7 +265,6 @@ describe('drag handle', () => { isEnabled isDragging={false} isDropAnimating={false} - direction={null} getDraggableRef={() => singleRef} canDragInteractiveElements={false} > @@ -294,7 +290,6 @@ describe('drag handle', () => { isEnabled isDragging={false} isDropAnimating={false} - direction={null} getDraggableRef={() => singleRef} canDragInteractiveElements={false} > @@ -329,7 +324,6 @@ describe('drag handle', () => { isDragging={false} isDropAnimating={false} isEnabled - direction={null} getDraggableRef={() => singleRef} canDragInteractiveElements={false} > @@ -344,7 +338,7 @@ describe('drag handle', () => { windowMouseMove(point); expect(customCallbacks.onLift) - .toHaveBeenCalledWith({ client: point, autoScrollMode: 'FLUID' }); + .toHaveBeenCalledWith({ clientSelection: point, autoScrollMode: 'FLUID' }); customWrapper.unmount(); }); @@ -443,6 +437,26 @@ describe('drag handle', () => { })).toBe(true); }); + it('should not start a drag if the state says that a drag cannot start', () => { + const customCallbacks: Callbacks = getStubCallbacks(); + const customContext = { + [styleContextKey]: 'hello', + [canLiftContextKey]: () => false, + }; + const customWrapper = getWrapper(customCallbacks, customContext); + const mock: MockEvent = createMockEvent(); + + // prevent default called on mousedown + mouseDown(customWrapper, origin, primaryButton, mock); + expect(mock.preventDefault).toHaveBeenCalled(); + + // a normal lift will not occur + windowMouseMove({ x: 0, y: sloppyClickThreshold }); + expect(callbacksCalled(callbacks)({ + onLift: 0, + })).toBe(true); + }); + describe('cancelled before moved enough', () => { describe('cancelled with any keydown', () => { Object.keys(keyCodes).forEach((key: string) => { @@ -633,8 +647,6 @@ describe('drag handle', () => { expect(callbacksCalled(callbacks)({ onLift: 1, - onMoveForward: 0, - onMoveBackward: 0, })).toBe(true); }); @@ -1413,7 +1425,7 @@ describe('drag handle', () => { pressSpacebar(wrapper, event); expect(callbacks.onLift).toHaveBeenCalledWith({ - client: fakeCenter, + clientSelection: fakeCenter, autoScrollMode: 'JUMP', }); // default action is prevented @@ -1457,6 +1469,24 @@ describe('drag handle', () => { })).toBe(true); expect(mock.preventDefault).not.toHaveBeenCalled(); }); + + it('should not lift and prevent the default action if the state does not currently allow lifting', () => { + const customCallbacks: Callbacks = getStubCallbacks(); + const customContext = { + [styleContextKey]: 'hello', + [canLiftContextKey]: () => false, + }; + const customWrapper = getWrapper(customCallbacks, customContext); + const mock: MockEvent = createMockEvent(); + + pressSpacebar(customWrapper, mock); + + expect(callbacksCalled(callbacks)({ + onLift: 0, + })).toBe(true); + // preventing the event to stop the default browser action + expect(mock.preventDefault).toHaveBeenCalled(); + }); }); describe('progress', () => { @@ -1488,39 +1518,10 @@ describe('drag handle', () => { expect(callbacksCalled(callbacks)({ onLift: 1, onMove: 0, - onMoveForward: 0, - onMoveBackward: 0, })).toBe(true); expect(event.defaultPrevented).toBe(false); }); - it('should be able to lift without a direction provided', () => { - const customCallbacks = getStubCallbacks(); - const customWrapper = mount( - singleRef} - canDragInteractiveElements={false} - > - {(dragHandleProps: ?DragHandleProps) => ( - - )} - , - { context: basicContext } - ); - - pressSpacebar(customWrapper); - - expect(callbacksCalled(customCallbacks)({ - onLift: 1, - })).toBe(true); - }); - it('should instantly fire a scroll action when the window scrolls', () => { // lift pressSpacebar(wrapper); @@ -1557,43 +1558,7 @@ describe('drag handle', () => { }); }); - it('should stop dragging if the keyboard is used after a lift and a direction is not provided', () => { - const customCallbacks = getStubCallbacks(); - const mockEvent: MockEvent = createMockEvent(); - const customWrapper = mount( - singleRef} - canDragInteractiveElements={false} - > - {(dragHandleProps: ?DragHandleProps) => ( - - )} - , - { context: basicContext } - ); - - // lift - all good - pressSpacebar(customWrapper); - - // boom - pressArrowDown(customWrapper, mockEvent); - - expect(console.error).toHaveBeenCalled(); - expect(callbacksCalled(customCallbacks)({ - onLift: 1, - onCancel: 1, - onMoveForward: 0, - })).toBe(true); - expect(mockEvent.preventDefault).toHaveBeenCalled(); - }); - - describe('dragging in a vertical list', () => { + describe('directional movement', () => { it('should move backward when the user presses ArrowUp', () => { const mockEvent: MockEvent = createMockEvent(); @@ -1604,7 +1569,7 @@ describe('drag handle', () => { expect(callbacksCalled(callbacks)({ onLift: 1, - onMoveBackward: 1, + onMoveUp: 1, })).toBe(true); // we are using the event as a part of the drag expect(mockEvent.preventDefault).toHaveBeenCalled(); @@ -1620,7 +1585,7 @@ describe('drag handle', () => { expect(callbacksCalled(callbacks)({ onLift: 1, - onMoveForward: 1, + onMoveDown: 1, })).toBe(true); // we are using the event as a part of the drag expect(mockEvent.preventDefault).toHaveBeenCalled(); @@ -1635,7 +1600,7 @@ describe('drag handle', () => { expect(callbacksCalled(callbacks)({ onLift: 1, - onCrossAxisMoveBackward: 1, + onMoveLeft: 1, })).toBe(true); // we are using the event as a part of the drag expect(mockEvent.preventDefault).toHaveBeenCalled(); @@ -1650,97 +1615,7 @@ describe('drag handle', () => { expect(callbacksCalled(callbacks)({ onLift: 1, - onCrossAxisMoveForward: 1, - })).toBe(true); - // we are using the event as a part of the drag - expect(mockEvent.preventDefault).toHaveBeenCalled(); - }); - }); - - describe('dragging in a horizontal list', () => { - let customWrapper: ReactWrapper; - let customCallbacks: Callbacks; - - beforeEach(() => { - customCallbacks = getStubCallbacks(); - customWrapper = mount( - singleRef} - canDragInteractiveElements={false} - > - {(dragHandleProps: ?DragHandleProps) => ( - - )} - , - { context: basicContext } - ); - }); - - afterEach(() => { - customWrapper.unmount(); - }); - - it('should move backward when the user presses LeftArrow', () => { - const mockEvent: MockEvent = createMockEvent(); - - pressSpacebar(customWrapper); - pressArrowLeft(customWrapper, mockEvent); - requestAnimationFrame.step(); - - expect(callbacksCalled(customCallbacks)({ - onLift: 1, - onMoveBackward: 1, - })).toBe(true); - // we are using the event as a part of the drag - expect(mockEvent.preventDefault).toHaveBeenCalled(); - }); - - it('should move forward when the user presses RightArrow', () => { - const mockEvent: MockEvent = createMockEvent(); - - pressSpacebar(customWrapper); - pressArrowRight(customWrapper, mockEvent); - requestAnimationFrame.step(); - - expect(callbacksCalled(customCallbacks)({ - onLift: 1, - onMoveForward: 1, - })).toBe(true); - // we are using the event as a part of the drag - expect(mockEvent.preventDefault).toHaveBeenCalled(); - }); - - it('should request a backward cross axis move when the user presses ArrowUp', () => { - const mockEvent: MockEvent = createMockEvent(); - - pressSpacebar(customWrapper); - pressArrowUp(customWrapper, mockEvent); - requestAnimationFrame.step(); - - expect(callbacksCalled(customCallbacks)({ - onLift: 1, - onCrossAxisMoveBackward: 1, - })).toBe(true); - // we are using the event as a part of the drag - expect(mockEvent.preventDefault).toHaveBeenCalled(); - }); - - it('should request a forward cross axis move when the user presses ArrowDown', () => { - const mockEvent: MockEvent = createMockEvent(); - - pressSpacebar(customWrapper); - pressArrowDown(customWrapper, mockEvent); - requestAnimationFrame.step(); - - expect(callbacksCalled(customCallbacks)({ - onLift: 1, - onCrossAxisMoveForward: 1, + onMoveRight: 1, })).toBe(true); // we are using the event as a part of the drag expect(mockEvent.preventDefault).toHaveBeenCalled(); @@ -1759,8 +1634,7 @@ describe('drag handle', () => { expect(callbacksCalled(callbacks)({ onLift: 1, onMove: 0, - onMoveForward: 1, - onMoveBackward: 0, + onMoveDown: 1, })).toBe(true); // being super safe and ensuring nothing firers later @@ -1769,8 +1643,7 @@ describe('drag handle', () => { expect(callbacksCalled(callbacks)({ onLift: 1, onMove: 0, - onMoveForward: 1, - onMoveBackward: 0, + onMoveDown: 1, })).toBe(true); }); @@ -1785,8 +1658,8 @@ describe('drag handle', () => { expect(callbacksCalled(callbacks)({ onLift: 1, onMove: 0, - onMoveForward: 0, - onMoveBackward: 1, + onMoveDown: 0, + onMoveUp: 1, })).toBe(true); // being super safe and ensuring nothing firers later @@ -1795,8 +1668,8 @@ describe('drag handle', () => { expect(callbacksCalled(callbacks)({ onLift: 1, onMove: 0, - onMoveForward: 0, - onMoveBackward: 1, + onMoveDown: 0, + onMoveUp: 1, })).toBe(true); }); @@ -1810,7 +1683,7 @@ describe('drag handle', () => { requestAnimationFrame.flush(); expect(callbacksCalled(callbacks)({ - onMoveForward: 0, + onMoveDown: 0, onLift: 1, onDrop: 1, })).toBe(true); @@ -1826,7 +1699,7 @@ describe('drag handle', () => { requestAnimationFrame.flush(); expect(callbacksCalled(callbacks)({ - onMoveBackward: 0, + onMoveUp: 0, onLift: 1, onDrop: 1, })).toBe(true); @@ -1903,7 +1776,7 @@ describe('drag handle', () => { expect(event.defaultPrevented).toBe(false); }); - it('should not prevent any subsequent click actions', () => { + it('should not prevent any subsequent window click actions', () => { // lift pressSpacebar(wrapper); // drop @@ -1916,13 +1789,10 @@ describe('drag handle', () => { expect(isAWindowClickPrevented()).toBe(false); }); - }); - describe('post drag click', () => { - it('should not prevent any clicks after a drag', () => { + it('should not prevent any subsequent on element click actions', () => { const mockEvent: MockEvent = createMockEvent(); pressSpacebar(wrapper); - pressArrowDown(wrapper); pressSpacebar(wrapper); mouseClick(wrapper, origin, primaryButton, mockEvent); @@ -2007,18 +1877,15 @@ describe('drag handle', () => { }); }); - describe('unmounted mid drag', () => { - beforeEach(() => { - pressSpacebar(wrapper); - wrapper.unmount(); - }); + it('should call the onCancel prop if unmounted mid drag', () => { + pressSpacebar(wrapper); - it('should call the onCancel prop', () => { - expect(callbacksCalled(callbacks)({ - onLift: 1, - onCancel: 1, - })).toBe(true); - }); + wrapper.unmount(); + + expect(callbacksCalled(callbacks)({ + onLift: 1, + onCancel: 1, + })).toBe(true); }); describe('subsequent drags', () => { @@ -2034,7 +1901,7 @@ describe('drag handle', () => { expect(callbacksCalled(callbacks)({ onLift: val + 1, - onMoveForward: val + 1, + onMoveDown: val + 1, onDrop: val + 1, })).toBe(true); }); @@ -2076,16 +1943,16 @@ describe('drag handle', () => { describe('initiation', () => { it('should start a drag on long press', () => { - const client: Position = { + const clientSelection: Position = { x: 50, y: 100, }; - touchStart(wrapper, client); + touchStart(wrapper, clientSelection); jest.runTimersToTime(timeForLongPress); expect(callbacks.onLift).toHaveBeenCalledWith({ - client, + clientSelection, autoScrollMode: 'FLUID', }); }); @@ -2097,6 +1964,22 @@ describe('drag handle', () => { expect(mockEvent.preventDefault).not.toHaveBeenCalled(); }); + + it('should not start a drag if the application state does not allow it', () => { + const customCallbacks: Callbacks = getStubCallbacks(); + const customContext = { + [styleContextKey]: 'hello', + [canLiftContextKey]: () => false, + }; + const customWrapper = getWrapper(customCallbacks, customContext); + const mock: MockEvent = createMockEvent(); + + touchStart(wrapper, origin, 0, mock); + jest.runTimersToTime(timeForLongPress); + + expect(mock.preventDefault).not.toHaveBeenCalled(); + expect(callbacks.onLift).not.toHaveBeenCalled(); + }); }); describe('drag ending before it started', () => { @@ -2690,7 +2573,6 @@ describe('drag handle', () => { isEnabled={false} isDragging={false} isDropAnimating={false} - direction={null} getDraggableRef={() => singleRef} canDragInteractiveElements={false} > @@ -3075,7 +2957,6 @@ describe('drag handle', () => { isDragging={false} isDropAnimating={false} isEnabled - direction={null} getDraggableRef={() => singleRef} canDragInteractiveElements={false} > @@ -3111,7 +2992,6 @@ describe('drag handle', () => { isDragging={false} isDropAnimating={false} isEnabled - direction={null} getDraggableRef={() => singleRef} canDragInteractiveElements={false} > @@ -3150,7 +3030,6 @@ describe('drag handle', () => { isDragging={false} isDropAnimating={false} isEnabled - direction={null} getDraggableRef={() => singleRef} canDragInteractiveElements={false} > @@ -3194,7 +3073,6 @@ describe('drag handle', () => { isDragging={false} isDropAnimating={false} isEnabled - direction={null} getDraggableRef={() => singleRef} canDragInteractiveElements={false} > @@ -3241,7 +3119,6 @@ describe('drag handle', () => { isDragging={false} isDropAnimating={false} isEnabled - direction={null} getDraggableRef={() => singleRef} // stating that we can drag canDragInteractiveElements @@ -3284,7 +3161,6 @@ describe('drag handle', () => { isDragging={false} isDropAnimating={false} isEnabled - direction={null} getDraggableRef={() => singleRef} // stating that we can drag canDragInteractiveElements @@ -3339,7 +3215,6 @@ describe('drag handle', () => { isDragging={false} isDropAnimating={false} isEnabled - direction={null} getDraggableRef={() => singleRef} canDragInteractiveElements={false} >