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

Set scroll velocity from drag distance #23082

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 87 additions & 2 deletions packages/block-editor/src/components/block-draggable/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,42 @@
import { Draggable } from '@wordpress/components';
import { useSelect, useDispatch } from '@wordpress/data';
import { useEffect, useRef } from '@wordpress/element';
import { getScrollContainer } from '@wordpress/dom';

const BlockDraggable = ( { children, clientIds, cloneClassname } ) => {
const SCROLL_INACTIVE_DISTANCE_PX = 50;
const SCROLL_INTERVAL_MS = 25;
const PIXELS_PER_SECOND_PER_DISTANCE = 5;
const VELOCITY_MULTIPLIER =
PIXELS_PER_SECOND_PER_DISTANCE * ( SCROLL_INTERVAL_MS / 1000 );

function startScrollingY( nodeRef, velocityRef ) {
return setInterval( () => {
if ( nodeRef.current && velocityRef.current ) {
const newTop = nodeRef.current.scrollTop + velocityRef.current;

nodeRef.current.scroll( {
top: newTop,
// behavior: 'smooth' // seems to hurt performance, better to use a small scroll interval
} );
}
}, SCROLL_INTERVAL_MS );
}

function getVerticalScrollParent( node ) {
if ( node === null ) {
return null;
}

return getScrollContainer( node );
}

const BlockDraggable = ( {
children,
clientIds,
cloneClassname,
onDragStart,
onDragEnd,
} ) => {
const { srcRootClientId, index, isDraggable } = useSelect(
( select ) => {
const {
Expand All @@ -30,6 +64,14 @@ const BlockDraggable = ( { children, clientIds, cloneClassname } ) => {
[ clientIds ]
);
const isDragging = useRef( false );

// @todo - do this for horizontal scroll
const dragStartY = useRef( null );
const velocityY = useRef( null );
const scrollParentY = useRef( null );

const scrollEditorInterval = useRef( null );
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think there's away to extract the logic into a dedicated hook as it's kind of obscures the code of the component. Would something like that (or close) work?

const [ onDragStart, onDragOver, onDragEnd ] = useScrollOnDrag()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe. I don't have a ton of bandwidth to refine this so you should feel free to mess around with it and propose a better-factored alternative.


const { startDraggingBlocks, stopDraggingBlocks } = useDispatch(
'core/block-editor'
);
Expand All @@ -40,6 +82,11 @@ const BlockDraggable = ( { children, clientIds, cloneClassname } ) => {
if ( isDragging.current ) {
stopDraggingBlocks();
}

if ( scrollEditorInterval.current ) {
clearInterval( scrollEditorInterval.current );
scrollEditorInterval.current = null;
}
};
}, [] );

Expand All @@ -60,13 +107,51 @@ const BlockDraggable = ( { children, clientIds, cloneClassname } ) => {
cloneClassname={ cloneClassname }
elementId={ blockElementId }
transferData={ transferData }
onDragStart={ () => {
onDragStart={ ( event ) => {
startDraggingBlocks();
isDragging.current = true;
dragStartY.current = event.clientY;

// find nearest parent(s) to scroll
scrollParentY.current = getVerticalScrollParent(
document.getElementById( blockElementId )
);
scrollEditorInterval.current = startScrollingY(
scrollParentY,
velocityY
);
if ( onDragStart ) {
onDragStart();
}
} }
onDragOver={ ( event ) => {
const distanceY = event.clientY - dragStartY.current;
if ( distanceY > SCROLL_INACTIVE_DISTANCE_PX ) {
velocityY.current =
VELOCITY_MULTIPLIER *
( distanceY - SCROLL_INACTIVE_DISTANCE_PX );
} else if ( distanceY < -SCROLL_INACTIVE_DISTANCE_PX ) {
velocityY.current =
VELOCITY_MULTIPLIER *
( distanceY + SCROLL_INACTIVE_DISTANCE_PX );
} else {
velocityY.current = 0;
}
Comment on lines +128 to +139
Copy link
Contributor

@talldan talldan Jun 18, 2020

Choose a reason for hiding this comment

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

A usability issue I had is that the feature is less effective if you're dragging from near the top or the bottom of the screen.

When dragging from the top downwards, if I wanted to aim for a block that I didn't need to scroll to but was nearer the lower part of the screen, I found I would scroll past my target very quickly as I moved my cursor down.

The opposite was true when dragging upwards from near the top of the screen, the page wouldn't scroll very quickly because there wasn't much distance I could move the cursor up.

An answer might be to make the velocity calculation based on the percentage the cursor has moved towards the edge of the screen (or probably scroll container), in pseudocode for moving down (I haven't factored the inactive scroll distance for this):

const movableDistance = screenHeight - dragStartY.current;
const dragDistance = event.clientY - dragStartY.current;
const distancePercentage = dragDistance / movableDistance;
// distancePercentage would be a much smaller value, so VELOCITY_MULTIPLIER would have to be adjusted.
velocityY.current = VELOCITY_MULTIPLIER * distancePercentage

An interesting iteration might also be to try easing on the calculation (e.g. https://easings.net/#easeInQuart), so that the speed isn't too dramatic at first, but worth trying one thing at a time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are great suggestions. Given that this patch is already considered to be an improvement, I would suggest we raise those as tickets for future work and get this merged. That way we can compare multiple strategies while not missing the 5.5 window.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gravityrail I do still have concerns about usability, I feel like it's pretty easy to make the page start scrolling, but harder to make it stop in the right place.

Given others have said they're happy with the change, lets push on and address this as a follow-up 👍

You mentioned that you don't have a lot of time to work on this, would it be better if I try out the suggestion in a follow-up PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would love it if you could do a follow up PR with your suggestions. Happy to pitch in and test.

} }
onDragEnd={ () => {
stopDraggingBlocks();
isDragging.current = false;
dragStartY.current = null;
scrollParentY.current = null;

if ( scrollEditorInterval.current ) {
clearInterval( scrollEditorInterval.current );
scrollEditorInterval.current = null;
}

if ( onDragEnd ) {
onDragEnd();
}
} }
>
{ ( { onDraggableStart, onDraggableEnd } ) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ function BlockContextualToolbar( { focusOnMount, ...props } ) {
aria-label={ __( 'Block tools' ) }
{ ...props }
>
<BlockToolbar />
<BlockToolbar
onDragStart={ props.onDragStart }
onDragEnd={ props.onDragEnd }
/>
</NavigableToolbar>
</div>
);
Expand Down
22 changes: 19 additions & 3 deletions packages/block-editor/src/components/block-list/block-popover.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ function selector( select ) {
isCaretWithinFormattedText,
getSettings,
getLastMultiSelectedBlockClientId,
isDraggingBlocks,
} = select( 'core/block-editor' );
return {
isNavigationMode: isNavigationMode(),
Expand All @@ -40,6 +41,7 @@ function selector( select ) {
hasMultiSelection: hasMultiSelection(),
hasFixedToolbar: getSettings().hasFixedToolbar,
lastClientId: getLastMultiSelectedBlockClientId(),
isDragging: isDraggingBlocks(),
};
}

Expand All @@ -60,6 +62,7 @@ function BlockPopover( {
hasMultiSelection,
hasFixedToolbar,
lastClientId,
isDragging,
} = useSelect( selector, [] );
const isLargeViewport = useViewportMatch( 'medium' );
const [ isToolbarForced, setIsToolbarForced ] = useState( false );
Expand Down Expand Up @@ -96,7 +99,8 @@ function BlockPopover( {
! shouldShowBreadcrumb &&
! shouldShowContextualToolbar &&
! isToolbarForced &&
! showEmptyBlockSideInserter
! showEmptyBlockSideInserter &&
! isDragging
) {
return null;
}
Expand Down Expand Up @@ -136,6 +140,14 @@ function BlockPopover( {
setIsInserterShown( false );
}

function onDragStart() {
setIsToolbarForced( true );
}

function onDragEnd() {
setIsToolbarForced( false );
}

// Position above the anchor, pop out towards the right, and position in the
// left corner. For the side inserter, pop out towards the left, and
// position in the right corner.
Expand Down Expand Up @@ -165,8 +177,10 @@ function BlockPopover( {
onBlur={ () => setIsToolbarForced( false ) }
shouldAnchorIncludePadding
// Popover calculates the width once. Trigger a reset by remounting
// the component.
key={ shouldShowContextualToolbar }
// the component. We include both shouldShowContextualToolbar and isToolbarForced
// in the key to prevent the component being unmounted unexpectedly when isToolbarForced = true,
// e.g. during drag and drop
key={ shouldShowContextualToolbar || isToolbarForced }
Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't able to determine what effect this change has, updating the comment would be great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically, if isToolbarForced isn't part of the key, then the component will still be unmounted unexpectedly when shouldShowContextualToolbar changes.

I will push a change that explains this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ellatrix Maybe we don't need this key anymore since now we compute recompute when the width/height changes in Popover?

>
{ ( shouldShowContextualToolbar || isToolbarForced ) && (
<div
Expand Down Expand Up @@ -198,6 +212,8 @@ function BlockPopover( {
// it should focus the toolbar right after the mount.
focusOnMount={ isToolbarForced }
data-align={ align }
onDragStart={ onDragStart }
onDragEnd={ onDragEnd }
/>
) }
{ shouldShowBreadcrumb && (
Expand Down
12 changes: 10 additions & 2 deletions packages/components/src/draggable/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,23 @@ Arbitrary data object attached to the drag and drop event.

### onDragStart

A function to be called when dragging starts.
A function called when dragging starts. This callback receives the `event` object from the `dragstart` event as its first parameter.

- Type: `Function`
- Required: No
- Default: `noop`

### onDragOver

A function called when the element being dragged is dragged over a valid drop target. This callback receives the `event` object from the `dragover` event as its first parameter.

- Type: `Function`
- Required: No
- Default: `noop`

### onDragEnd

A function to be called when dragging ends.
A function called when dragging ends. This callback receives the `event` object from the `dragend` event as its first parameter.

- Type: `Function`
- Required: No
Expand Down
20 changes: 18 additions & 2 deletions packages/components/src/draggable/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,11 @@ class Draggable extends Component {
event.preventDefault();

this.resetDragState();
this.props.setTimeout( onDragEnd );

// Allow the Synthetic Event to be accessed from asynchronous code.
// https://reactjs.org/docs/events.html#event-pooling
event.persist();
this.props.setTimeout( onDragEnd.bind( this, event ) );
}

/**
Expand All @@ -61,6 +65,12 @@ class Draggable extends Component {
// Update cursor coordinates.
this.cursorLeft = event.clientX;
this.cursorTop = event.clientY;

const { onDragOver = noop } = this.props;

// The `event` from `onDragOver` is not a SyntheticEvent
// and so it doesn't require `event.persist()`.
this.props.setTimeout( onDragOver.bind( this, event ) );
}

/**
Expand Down Expand Up @@ -150,7 +160,10 @@ class Draggable extends Component {
document.body.classList.add( 'is-dragging-components-draggable' );
document.addEventListener( 'dragover', this.onDragOver );

this.props.setTimeout( onDragStart );
// Allow the Synthetic Event to be accessed from asynchronous code.
// https://reactjs.org/docs/events.html#event-pooling
event.persist();
this.props.setTimeout( onDragStart.bind( this, event ) );
}

/**
Expand All @@ -165,6 +178,9 @@ class Draggable extends Component {
this.cloneWrapper = null;
}

this.cursorLeft = null;
this.cursorTop = null;

// Reset cursor.
document.body.classList.remove( 'is-dragging-components-draggable' );
}
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/draggable/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ body.is-dragging-components-draggable {
background: transparent;
pointer-events: none;
z-index: z-index(".components-draggable__clone");
opacity: 0.7;

> * {
// This needs specificity as a theme is meant to define these by default.
Expand Down