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.

Sign up for GitHub

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

Jump to bottom

Set scroll velocity from drag distance #23082

Merged
merged 8 commits into from
Jun 25, 2020
Merged

Set scroll velocity from drag distance #23082

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f9ba478
Set scroll velocity from drag distance
gravityrail Jun 10, 2020
5c05196
Fix linting
gravityrail Jun 10, 2020
7af31e1
Add a 50px non-scrolling region above and below the drag point
gravityrail Jun 15, 2020
8a91a7b
Linting
gravityrail Jun 15, 2020
dd2a314
Remove erroneous comment
gravityrail Jun 15, 2020
1fcbd72
Update comment to explain component key change
gravityrail Jun 22, 2020
297c04a
Use getScrollContainer() to get the scroll container
gravityrail Jun 22, 2020
b45996a
Make draggable callbacks consistent and document event param
talldan Jun 24, 2020
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
View file
Open in desktop
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={ ( clientX, clientY ) => {
startDraggingBlocks();
isDragging.current = true;
dragStartY.current = 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 •
edited
Loading

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
5 changes: 4 additions & 1 deletion packages/block-editor/src/components/block-list/block-contextual-toolbar.js
View file
Open in desktop
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
View file
Open in desktop
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
11 changes: 10 additions & 1 deletion packages/components/src/draggable/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ class Draggable extends Component {
// Update cursor coordinates.
this.cursorLeft = event.clientX;
this.cursorTop = event.clientY;

const { onDragOver = noop } = this.props;

this.props.setTimeout( onDragOver.bind( this, event ) );
}

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

this.props.setTimeout( onDragStart );
this.props.setTimeout(
onDragStart.bind( this, event.clientX, event.clientY )
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be good to pass the event object rather than the clientX and Y.

Maybe also the same for onDragEnd so that the interface is consistent for each of the callbacks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately I couldn't get that to work - something happens to the event object when I pass it, and it ends up with null clientX and clientY props. I don't know enough to about React, javascript or how browsers work to precisely know why. I would love for someone else to fix it because I agree - that API shape ain't the cleanest.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gravityrail It's possibly related to React's event pooling behavior given the event is being passed to a timeout:
https://reactjs.org/docs/events.html#event-pooling

I'll do some debugging.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, it was related to the event pooling.

I pushed a commit that makes the change. If you want to git fetch from this repo and then git cherry-pick db82900 will bring it onto your branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Thanks for the patch 👍

);
}

/**
Expand All @@ -165,6 +171,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
View file
Open in desktop
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