Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

correctly blocking onClick for all element types #353

Merged
merged 16 commits into from
Feb 28, 2018
Merged
44 changes: 21 additions & 23 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,12 @@ point-events: none;

**Styles applied using the `data-react-beautiful-dnd-draggable` attribute**

We apply a cursor while dragging to give user feedback that a drag is occurring. You are welcome to override this by applying your own cursor to the element with a greater specificity.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to remove these new comments (they are no longer needed)


```css
cursor: grabbing;
```

This is what we use to control `Draggable`s that need to move out of the way of a dragging `Draggable`.

```css
Expand All @@ -378,7 +384,7 @@ This is described by the type [`DraggableStyle`](https://github.com/atlassian/re

#### (Phase: dragging): body element

We apply a cursor while dragging to give user feedback that a drag is occurring. You are welcome to override this. A good point to do this is the `onDragStart` event.
We apply a cursor while dragging to give user feedback that a drag is occurring. You are welcome to override this. A good point to do this is the `onDragStart` event. This style is applied on both the `drag handle` as well as the `body`. We apply it to the `body` in case the user pointer *slips* off the `drag handle` during a drag. This is possible as we throttle movements into request animation frames.

```css
cursor: grabbing;
Expand Down Expand Up @@ -959,7 +965,6 @@ export type DraggableProps = {|

type DraggableStyle = DraggingStyle | NotDraggingStyle
type DraggingStyle = {|
pointerEvents: 'none',
position: 'fixed',
width: number,
height: number,
Expand Down Expand Up @@ -1091,7 +1096,6 @@ It is an assumption that `Draggable`s are *visible siblings* of one another. The
type DragHandleProps = {|
onMouseDown: (event: MouseEvent) => void,
onKeyDown: (event: KeyboardEvent) => void,
onClick: (event: MouseEvent) => void,
tabIndex: number,
'aria-grabbed': boolean,
draggable: boolean,
Expand Down Expand Up @@ -1142,23 +1146,20 @@ Controlling a whole draggable by just a part of it
You can override some of the `dragHandleProps` props with your own behavior if you need to.

```js
const myOnClick = event => console.log('clicked on', event.target);
const myOnMouseDown = event => console.log('mouse down on', event.target);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no longer using 'click' as part of the example


<Draggable draggableId="draggable-1" index={0}>
{(provided, snapshot) => {
const onClick = (() => {
const onMouseDown = (() => {
// dragHandleProps might be null
if (!provided.dragHandleProps) {
return myOnClick;
return onMouseDown;
}

// creating a new onClick function that calls my onClick
// event as well as the provided one.
return event => {
provided.dragHandleProps.onClick(event);
// You may want to check if event.defaultPrevented
// is true and optionally fire your handler
myOnClick(event);
// creating a new onMouseDown function that calls myOnMouseDown as well as the drag handle one.
return (event) => {
provided.dragHandleProps.onMouseDown(event);
myOnMouseDown(event);
};
})();

Expand All @@ -1168,7 +1169,7 @@ const myOnClick = event => console.log('clicked on', event.target);
ref={provided.innerRef}
{...provided.draggableProps}
{...provided.dragHandleProps}
onClick={onClick}
onMouseDown={onMouseDown}
>
Drag me!
</div>
Expand Down Expand Up @@ -1335,7 +1336,6 @@ export type DraggableProps = {|
|}
type DraggableStyle = DraggingStyle | NotDraggingStyle
type DraggingStyle = {|
pointerEvents: 'none',
position: 'fixed',
width: number,
height: number,
Expand All @@ -1352,18 +1352,16 @@ type NotDraggingStyle = {|
transition: ?string,
transition: null | 'none',
|}
type DragHandleProvided = {|

type DragHandleProps = {|
onMouseDown: (event: MouseEvent) => void,
onKeyDown: (event: KeyboardEvent) => void,
onClick: (event: MouseEvent) => void,
onTouchStart: (event: TouchEvent) => void,
onTouchMove: (event: TouchEvent) => void,
tabIndex: number,
'aria-roledescription': string,
'aria-grabbed': boolean,
draggable: boolean,
onDragStart: () => boolean,
onDrop: () => boolean
|}
onDragStart: () => void,
onDrop: () => void,
|};
```

### Using the flow types
Expand Down
3 changes: 0 additions & 3 deletions src/view/drag-handle/drag-handle-types.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,6 @@ export type DragHandleProps = {|
onTouchStart: (event: TouchEvent) => void,
onTouchMove: (event: TouchEvent) => void,

// Conditionally block clicks
onClick: (event: MouseEvent) => void,

// Control styling from style marshal
'data-react-beautiful-dnd-drag-handle': string,

Expand Down
10 changes: 2 additions & 8 deletions src/view/drag-handle/drag-handle.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import type {
DragHandleProps,
} from './drag-handle-types';
import type {
Sensor,
MouseSensor,
KeyboardSensor,
TouchSensor,
Expand All @@ -24,6 +23,8 @@ import createTouchSensor from './sensor/create-touch-sensor';

const getFalse: () => boolean = () => false;

type Sensor = MouseSensor | KeyboardSensor | TouchSensor;

export default class DragHandle extends Component<Props> {
/* eslint-disable react/sort-comp */
mouseSensor: MouseSensor;
Expand Down Expand Up @@ -155,12 +156,6 @@ export default class DragHandle extends Component<Props> {
this.touchSensor.onTouchMove(event);
}

onClick = (event: MouseEvent) => {
// The mouse or touch sensor may want to block the click
this.mouseSensor.onClick(event);
this.touchSensor.onClick(event);
}

canStartCapturing = (event: Event) => {
// this might be before a drag has started - isolated to this element
if (this.isAnySensorCapturing()) {
Expand Down Expand Up @@ -189,7 +184,6 @@ export default class DragHandle extends Component<Props> {
onKeyDown: this.onKeyDown,
onTouchStart: this.onTouchStart,
onTouchMove: this.onTouchMove,
onClick: this.onClick,
tabIndex: 0,
'data-react-beautiful-dnd-drag-handle': this.styleContext,
// English default. Consumers are welcome to add their own start instruction
Expand Down
41 changes: 11 additions & 30 deletions src/view/drag-handle/sensor/create-mouse-sensor.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import isSloppyClickThresholdExceeded from '../util/is-sloppy-click-threshold-ex
import getWindowFromRef from '../../get-window-from-ref';
import * as keyCodes from '../../key-codes';
import blockStandardKeyEvents from '../util/block-standard-key-events';
import createClickBlocker, { type ClickBlocker } from '../util/create-click-blocker';
import type {
Position,
} from '../../../types';
Expand All @@ -16,11 +17,10 @@ type MouseForceChangedEvent = MouseEvent & {
webkitForce?: number,
}

type State = {
type State = {|
isDragging: boolean,
preventClick: boolean,
pending: ?Position,
}
|}

// https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/button
const primaryButton = 0;
Expand All @@ -34,45 +34,41 @@ export default ({
let state: State = {
isDragging: false,
pending: null,
preventClick: false,
};
const setState = (partial: Object): void => {
const newState: State = {
...state,
...partial,
};
const setState = (newState: State): void => {
state = newState;
};
const isDragging = (): boolean => state.isDragging;
const isCapturing = (): boolean => Boolean(state.pending || state.isDragging);
const schedule = createScheduler(callbacks);
const clickBlocker: ClickBlocker = createClickBlocker();

const startDragging = (fn?: Function = noop) => {
setState({
pending: null,
isDragging: true,
preventClick: true,
});
fn();
};
const stopDragging = (fn?: Function = noop) => {
const stopDragging = (fn?: Function = noop, shouldBlockClick?: boolean = true) => {
schedule.cancel();
unbindWindowEvents();
if (shouldBlockClick) {
clickBlocker.blockNext();
}
setState({
isDragging: false,
pending: null,
});
fn();
};
const startPendingDrag = (point: Position) => {
clickBlocker.reset();
setState({ pending: point, isDragging: false });
bindWindowEvents();
};
const stopPendingDrag = () => {
setState({
preventClick: false,
});
stopDragging();
stopDragging(noop, false);
};

const kill = (fn?: Function = noop) => {
Expand Down Expand Up @@ -223,23 +219,8 @@ export default ({
startPendingDrag(point);
};

const onClick = (event: MouseEvent): void => {
if (!state.preventClick) {
return;
}

// preventing click

// only want to prevent the first click
setState({
preventClick: false,
});
stopEvent(event);
};

const sensor: MouseSensor = {
onMouseDown,
onClick,
kill,
isCapturing,
isDragging,
Expand Down
21 changes: 5 additions & 16 deletions src/view/drag-handle/sensor/create-touch-sensor.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import stopEvent from '../util/stop-event';
import createScheduler from '../util/create-scheduler';
import getWindowFromRef from '../../get-window-from-ref';
import createClickBlocker, { type ClickBlocker } from '../util/create-click-blocker';
import type {
Position,
} from '../../../types';
Expand All @@ -11,7 +12,6 @@ import type { TouchSensor, CreateSensorArgs } from './sensor-types';
type State = {
isDragging: boolean,
hasMoved: boolean,
preventClick: boolean,
longPressTimerId: ?number,
pending: ?Position,
}
Expand All @@ -29,7 +29,6 @@ const initial: State = {
isDragging: false,
pending: null,
hasMoved: false,
preventClick: false,
longPressTimerId: null,
};

Expand All @@ -50,6 +49,7 @@ export default ({
const isCapturing = (): boolean =>
Boolean(state.pending || state.isDragging || state.longPressTimerId);
const schedule = createScheduler(callbacks);
const clickBlocker: ClickBlocker = createClickBlocker();

const startDragging = () => {
const pending: ?Position = state.pending;
Expand Down Expand Up @@ -77,10 +77,8 @@ export default ({
const stopDragging = (fn?: Function = noop) => {
schedule.cancel();
unbindWindowEvents();
setState({
...initial,
preventClick: true,
});
clickBlocker.blockNext();
setState(initial);
fn();
};

Expand All @@ -100,6 +98,7 @@ export default ({
isDragging: false,
hasMoved: false,
});
clickBlocker.reset();
bindWindowEvents();
};

Expand Down Expand Up @@ -272,19 +271,9 @@ export default ({
}
};

const onClick = (event: MouseEvent) => {
if (!state.preventClick) {
return;
}

stopEvent(event);
setState(initial);
};

const sensor: TouchSensor = {
onTouchStart,
onTouchMove,
onClick,
kill,
isCapturing,
isDragging,
Expand Down
21 changes: 11 additions & 10 deletions src/view/drag-handle/sensor/sensor-types.js
Original file line number Diff line number Diff line change
@@ -1,33 +1,34 @@
// @flow
import type { Props, Callbacks } from '../drag-handle-types';

export type Sensor = {
type SensorBase = {|
// force stop and do not fire any events
kill: () => void,
// Is the sensor currently recording a drag
isDragging: () => boolean,
// Is the sensor listening for events.
// This can happen before a drag starts
isCapturing: () => boolean,
}
|}

export type CreateSensorArgs = {|
callbacks: Callbacks,
getDraggableRef: () => ?HTMLElement,
canStartCapturing: (event: Event) => boolean,
|}

export type MouseSensor = Sensor & {
export type MouseSensor = {|
...SensorBase,
onMouseDown: (event: MouseEvent) => void,
onClick: (event: MouseEvent) => void,
}
|}

export type KeyboardSensor = Sensor & {
export type KeyboardSensor = {|
...SensorBase,
onKeyDown: (event: KeyboardEvent, props: Props) => void,
}
|}

export type TouchSensor = Sensor & {
export type TouchSensor = {|
...SensorBase,
onTouchStart: (event: TouchEvent) => void,
onTouchMove: (event: TouchEvent) => void,
onClick: (event: MouseEvent) => void,
}
|}
Loading