diff --git a/src/state/action-creators.js b/src/state/action-creators.js index 95b2e6a7c6..c947e7c056 100644 --- a/src/state/action-creators.js +++ b/src/state/action-creators.js @@ -278,7 +278,6 @@ export const completeDrop = (result: DropResult): DropCompleteAction => ({ export const drop = () => (dispatch: Dispatch, getState: () => State): void => { const state: State = getState(); - // This can occur if the user ends a drag before // the collecting phase is finished. // This will not trigger a hook as the hook waits diff --git a/src/view/drag-handle/drag-handle.jsx b/src/view/drag-handle/drag-handle.jsx index edd5430bd4..0f997a3c3f 100644 --- a/src/view/drag-handle/drag-handle.jsx +++ b/src/view/drag-handle/drag-handle.jsx @@ -17,6 +17,12 @@ import type { const noop = (): void => { }; const getFalse: () => boolean = () => false; +// If we are controlling an event +const stop = (event: Event) => { + event.preventDefault(); + event.stopPropagation(); +}; + // https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/button const primaryButton = 0; @@ -239,8 +245,7 @@ export default class DragHandle extends Component { return; } - event.stopPropagation(); - event.preventDefault(); + stop(event); const point: Position = { x: clientX, @@ -263,12 +268,12 @@ export default class DragHandle extends Component { // window keyboard events are bound during a keyboard drag // or after the user presses the mouse down - onWindowKeydown = (event: KeyboardEvent): void => { + onWindowKeyDown = (event: KeyboardEvent): void => { const isMouseDragPending: boolean = Boolean(this.state.pending); if (isMouseDragPending) { if (event.keyCode === keyCodes.escape) { - event.preventDefault(); + stop(event); this.stopPendingMouseDrag(); } return; @@ -300,30 +305,67 @@ export default class DragHandle extends Component { return; } - if (this.state.draggingWith === 'MOUSE') { + // Handled through the keydown handler as the element has focus + // while dragging with a keyboard + if (this.state.draggingWith === 'KEYBOARD') { + return; + } + // Want to block scrolling the page with the space bar + if (event.keyCode === keyCodes.space) { + event.preventDefault(); + } + + // at this point we know what the direction is + } + + // When dragging with a mouse - the element may not have focus + // When dragging with a keyboard - the element will have focus + onKeyDown = (event: KeyboardEvent): void => { + if (!this.props.isEnabled) { + return; + } + + // Handled in the window key down function as it may not have focus + if (this.state.pending || this.state.draggingWith === 'MOUSE') { + return; + } + + const canStartKeyboardDrag: boolean = this.props.canLift && !this.state.draggingWith; + const isKeyboardDragging: boolean = this.state.draggingWith === 'KEYBOARD'; + + if (!canStartKeyboardDrag && !isKeyboardDragging) { + return; + } + + if (canStartKeyboardDrag) { + // Lifting with a keyboard if (event.keyCode === keyCodes.space) { - event.preventDefault(); + stop(event); + this.startDragging('KEYBOARD', () => this.props.callbacks.onKeyLift()); } return; } - // Only keyboard dragging if (!this.props.direction) { console.error('cannot handle keyboard event if direction is not provided'); + stop(event); this.stopDragging(() => this.props.callbacks.onCancel()); return; } - // at this point we do not know what the direction is + // Dropping + if (event.keyCode === keyCodes.space) { - event.preventDefault(); + // need to stop parent Draggable's thinking this is a lift + stop(event); this.stopDragging(() => this.props.callbacks.onDrop()); - return; } + // Movement + if (event.keyCode === keyCodes.arrowDown) { - event.preventDefault(); + stop(event); this.executeBasedOnDirection({ vertical: this.scheduleMoveForward, horizontal: this.scheduleCrossAxisMoveForward, @@ -332,7 +374,7 @@ export default class DragHandle extends Component { } if (event.keyCode === keyCodes.arrowUp) { - event.preventDefault(); + stop(event); this.executeBasedOnDirection({ vertical: this.scheduleMoveBackward, horizontal: this.scheduleCrossAxisMoveBackward, @@ -341,7 +383,7 @@ export default class DragHandle extends Component { } if (event.keyCode === keyCodes.arrowRight) { - event.preventDefault(); + stop(event); this.executeBasedOnDirection({ vertical: this.scheduleCrossAxisMoveForward, horizontal: this.scheduleMoveForward, @@ -350,7 +392,7 @@ export default class DragHandle extends Component { } if (event.keyCode === keyCodes.arrowLeft) { - event.preventDefault(); + stop(event); this.executeBasedOnDirection({ vertical: this.scheduleCrossAxisMoveBackward, horizontal: this.scheduleMoveBackward, @@ -358,24 +400,6 @@ export default class DragHandle extends Component { } } - // the on element keydown is only for lifting - otherwise using the window keydown - onKeyDown = (event: KeyboardEvent): void => { - if (!this.props.isEnabled || - this.state.pending || - this.state.draggingWith || - !this.props.canLift) { - return; - } - - // At this point we will not have a direction provided - if (event.keyCode === keyCodes.space) { - event.preventDefault(); - // stopping the event from bubbling up to the window event handler - event.stopPropagation(); - this.startDragging('KEYBOARD', () => this.props.callbacks.onKeyLift()); - } - } - onClick = (event: MouseEvent): void => { if (!this.preventClick) { return; @@ -497,7 +521,7 @@ export default class DragHandle extends Component { window.removeEventListener('mousemove', this.onWindowMouseMove); window.removeEventListener('mouseup', this.onWindowMouseUp); window.removeEventListener('mousedown', this.onWindowMouseDown); - window.removeEventListener('keydown', this.onWindowKeydown); + window.removeEventListener('keydown', this.onWindowKeyDown); window.removeEventListener('resize', this.onWindowResize); window.removeEventListener('scroll', this.onWindowScroll); window.removeEventListener('webkitmouseforcechanged', this.mouseForceChanged); @@ -507,7 +531,7 @@ export default class DragHandle extends Component { window.addEventListener('mousemove', this.onWindowMouseMove); window.addEventListener('mouseup', this.onWindowMouseUp); window.addEventListener('mousedown', this.onWindowMouseDown); - window.addEventListener('keydown', this.onWindowKeydown); + window.addEventListener('keydown', this.onWindowKeyDown); window.addEventListener('resize', this.onWindowResize); window.addEventListener('scroll', this.onWindowScroll, { passive: true }); window.addEventListener('webkitmouseforcechanged', this.mouseForceChanged); diff --git a/test/unit/view/drag-handle.spec.js b/test/unit/view/drag-handle.spec.js index d34f534e0c..b3f7f70a27 100644 --- a/test/unit/view/drag-handle.spec.js +++ b/test/unit/view/drag-handle.spec.js @@ -92,11 +92,11 @@ const mouseDown = mouseEvent.bind(null, 'mousedown'); const click = mouseEvent.bind(null, 'click'); const pressSpacebar = withKeyboard(keyCodes.space); const windowSpacebar = dispatchWindowKeyDownEvent.bind(null, keyCodes.space); +const pressArrowDown = withKeyboard(keyCodes.arrowDown); +const pressArrowUp = withKeyboard(keyCodes.arrowUp); +const pressArrowRight = withKeyboard(keyCodes.arrowRight); +const pressArrowLeft = withKeyboard(keyCodes.arrowLeft); const windowEscape = dispatchWindowKeyDownEvent.bind(null, keyCodes.escape); -const windowArrowUp = dispatchWindowKeyDownEvent.bind(null, keyCodes.arrowUp); -const windowArrowDown = dispatchWindowKeyDownEvent.bind(null, keyCodes.arrowDown); -const windowArrowLeft = dispatchWindowKeyDownEvent.bind(null, keyCodes.arrowLeft); -const windowArrowRight = dispatchWindowKeyDownEvent.bind(null, keyCodes.arrowRight); const windowTab = dispatchWindowKeyDownEvent.bind(null, keyCodes.tab); const windowEnter = dispatchWindowKeyDownEvent.bind(null, keyCodes.enter); @@ -313,7 +313,7 @@ describe('drag handle', () => { mouseDown(wrapper); windowMouseMove(0, sloppyClickThreshold); - windowSpacebar(wrapper); + pressSpacebar(wrapper); expect(callbacksCalled(callbacks)({ onLift: 1, @@ -334,8 +334,8 @@ describe('drag handle', () => { mouseDown(wrapper); windowMouseMove(0, sloppyClickThreshold); - windowArrowDown(); - windowArrowUp(); + pressArrowDown(wrapper); + pressArrowUp(wrapper); expect(callbacksCalled(callbacks)({ onLift: 1, @@ -1005,6 +1005,18 @@ describe('drag handle', () => { onKeyLift: 0, })).toBe(true); }); + + it('should not lift if disabled', () => { + wrapper.setProps({ + isEnabled: false, + }); + + pressSpacebar(wrapper); + + expect(callbacksCalled(callbacks)({ + onKeyLift: 0, + })).toBe(true); + }); }); describe('progress', () => { @@ -1076,8 +1088,8 @@ describe('drag handle', () => { // lift - all good pressSpacebar(customWrapper); - // boom. - windowArrowDown(); + // boom + pressArrowDown(customWrapper); expect(console.error).toHaveBeenCalled(); expect(callbacksCalled(customCallbacks)({ @@ -1091,7 +1103,7 @@ describe('drag handle', () => { it('should move backward when the user presses ArrowUp', () => { pressSpacebar(wrapper); // move backward - windowArrowUp(); + pressArrowUp(wrapper); requestAnimationFrame.step(); expect(callbacksCalled(callbacks)({ @@ -1103,7 +1115,7 @@ describe('drag handle', () => { it('should move forward when the user presses ArrowDown', () => { pressSpacebar(wrapper); // move forward - windowArrowDown(); + pressArrowDown(wrapper); requestAnimationFrame.step(); expect(callbacksCalled(callbacks)({ @@ -1114,7 +1126,7 @@ describe('drag handle', () => { it('should request to move to a droppable on the left when the user presses LeftArrow', () => { pressSpacebar(wrapper); - windowArrowLeft(); + pressArrowLeft(wrapper); requestAnimationFrame.step(); expect(callbacksCalled(callbacks)({ @@ -1125,7 +1137,7 @@ describe('drag handle', () => { it('should request to move to a droppable on the right when the user presses RightArrow', () => { pressSpacebar(wrapper); - windowArrowRight(); + pressArrowRight(wrapper); requestAnimationFrame.step(); expect(callbacksCalled(callbacks)({ @@ -1162,7 +1174,7 @@ describe('drag handle', () => { it('should move backward when the user presses LeftArrow', () => { pressSpacebar(customWrapper); - windowArrowLeft(); + pressArrowLeft(customWrapper); requestAnimationFrame.step(); expect(callbacksCalled(customCallbacks)({ @@ -1173,7 +1185,7 @@ describe('drag handle', () => { it('should move forward when the user presses RightArrow', () => { pressSpacebar(customWrapper); - windowArrowRight(); + pressArrowRight(customWrapper); requestAnimationFrame.step(); expect(callbacksCalled(customCallbacks)({ @@ -1184,7 +1196,7 @@ describe('drag handle', () => { it('should request a backward cross axis move when the user presses ArrowUp', () => { pressSpacebar(customWrapper); - windowArrowUp(); + pressArrowUp(customWrapper); requestAnimationFrame.step(); expect(callbacksCalled(customCallbacks)({ @@ -1195,7 +1207,7 @@ describe('drag handle', () => { it('should request a forward cross axis move when the user presses ArrowDown', () => { pressSpacebar(customWrapper); - windowArrowDown(); + pressArrowDown(customWrapper); requestAnimationFrame.step(); expect(callbacksCalled(customCallbacks)({ @@ -1209,9 +1221,9 @@ describe('drag handle', () => { it('should collapse multiple forward movements into a single animation frame', () => { pressSpacebar(wrapper); - windowArrowDown(); - windowArrowDown(); - windowArrowDown(); + pressArrowDown(wrapper); + pressArrowDown(wrapper); + pressArrowDown(wrapper); requestAnimationFrame.step(); expect(callbacksCalled(callbacks)({ @@ -1235,9 +1247,9 @@ describe('drag handle', () => { it('should collapse multiple backward movements into a single animation frame', () => { pressSpacebar(wrapper); - windowArrowUp(); - windowArrowUp(); - windowArrowUp(); + pressArrowUp(wrapper); + pressArrowUp(wrapper); + pressArrowUp(wrapper); requestAnimationFrame.step(); expect(callbacksCalled(callbacks)({ @@ -1260,9 +1272,9 @@ describe('drag handle', () => { it('should not fire a scheduled forward movement if no longer dragging', () => { pressSpacebar(wrapper); - windowArrowDown(); + pressArrowDown(wrapper); // finishing drag before animation frame - windowSpacebar(); + pressSpacebar(wrapper); // flushing any animation frames requestAnimationFrame.flush(); @@ -1276,9 +1288,9 @@ describe('drag handle', () => { it('should not fire a scheduled backward movement if no longer dragging', () => { pressSpacebar(wrapper); - windowArrowUp(); + pressArrowUp(wrapper); // finishing drag before animation frame - windowSpacebar(); + pressSpacebar(wrapper); // flushing any animation frames requestAnimationFrame.flush(); @@ -1295,13 +1307,24 @@ describe('drag handle', () => { describe('finish', () => { it('should drop when the user presses spacebar', () => { pressSpacebar(wrapper); - windowSpacebar(); + pressSpacebar(wrapper); expect(callbacksCalled(callbacks)({ onKeyLift: 1, onDrop: 1, })).toBe(true); }); + + it('should stop the event before it can be listened to', () => { + const preventDefault = jest.fn(); + const stopPropagation = jest.fn(); + + pressSpacebar(wrapper); + pressSpacebar(wrapper, { preventDefault, stopPropagation }); + + expect(preventDefault).toHaveBeenCalled(); + expect(stopPropagation).toHaveBeenCalled(); + }); }); describe('cancel', () => { @@ -1322,7 +1345,7 @@ describe('drag handle', () => { pressSpacebar(wrapper); mouseDown(wrapper, 0, 0, button); // should now do nothing - windowArrowUp(wrapper); + pressArrowUp(wrapper); expect(callbacksCalled(callbacks)({ onKeyLift: index + 1, @@ -1365,8 +1388,8 @@ describe('drag handle', () => { it('should not prevent any clicks after a drag', () => { const mock = jest.fn(); pressSpacebar(wrapper); - windowArrowDown(wrapper); - windowSpacebar(); + pressArrowDown(wrapper); + pressSpacebar(wrapper); click(wrapper, 0, 0, primaryButton, { preventDefault: mock }); @@ -1408,9 +1431,9 @@ describe('drag handle', () => { })).toBe(true); // should have no impact - windowArrowDown(); + pressArrowDown(wrapper); requestAnimationFrame.step(); - windowArrowUp(); + pressArrowUp(wrapper); requestAnimationFrame.step(); windowEscape(); requestAnimationFrame.step(); @@ -1445,10 +1468,10 @@ describe('drag handle', () => { // lift pressSpacebar(wrapper); // move forward - windowArrowDown(wrapper); + pressArrowDown(wrapper); requestAnimationFrame.step(); // drop - windowSpacebar(); + pressSpacebar(wrapper); expect(callbacksCalled(callbacks)({ onKeyLift: val + 1, @@ -1470,7 +1493,7 @@ describe('drag handle', () => { // lift and drop pressSpacebar(wrapper); - windowSpacebar(wrapper); + pressSpacebar(wrapper); expect(callbacksCalled(callbacks)({ onCancel: 1,