Set scroll velocity from drag distance #23082

gravityrail · 2020-06-10T23:17:23Z

Description

Current scrolling behaviour is somewhat unintuitive. The user needs to drag the item to the top of the viewport, and only then does scrolling begin. Also, the scroll placeholder is fully opaque. These two effects combine to make it hard to know where you're scrolling to, and to control scroll velocity.

This PR sets the vertical scroll velocity of the nearest suitable parent according to a function F of the vertical distance between the current mouse position and the Y position where the drag started.

It also sets the opacity of the drag placeholder to 0.7.

How has this been tested?

I just messed with it til it worked, sorry.

Screenshots

Video of previous behaviour: https://cloudup.com/cMcikA6icrw

Video of new behaviour: https://cloudup.com/c7UJlVgp5ry

Types of changes

Changes how scroll velocity reacts to cursor position while dragging.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

gravityrail · 2020-06-11T04:44:29Z

btw I have done very little testing and zero cross browser testing, this PR should serve as inspiration rather than a complete solution. I would love for someone to put it through its paces. Happy to chip away at it if nobody else volunteers.

ellatrix · 2020-06-11T09:01:31Z

That's not a bad idea. I was initially hoping to just allow the user to scroll at least, because currently, you cannot scroll at all while you're dragging a block. What you're doing is essentially replacing mouse behaviour with scroll instead of move around the page. One problem I could see with this: if you already have the trackpad button pressed, and you wish to cover a distance greater than the distance between where you pressed and the edge of the trackpad, you cannot simply let go to restart scrolling from the opposite side, because you'd drop the block.

Maybe it's better to start small and at least allow the user to scroll the page while dragging.

folletto · 2020-06-11T10:28:58Z

I'm not sure I get the trackpad issue. The new behaviour seems start moving when the mouse cursor moves from the initial position, and slows down when the cursor goes back to that initial position. So in theory the trackpad movement never reaches the edge. If anything, this scrolling behaviour seems requiring less trackpad space, as a higher velocity is reached in far less space then getting to the edge of the screen, no?

ellatrix · 2020-06-11T11:01:24Z

Ah, you are right, sorry about that! I had it playing out wrong in my head when thinking about possible problems. :)

I do like the interaction. What would happen if the block position is "live" updated in the block list?

folletto · 2020-06-11T11:17:12Z

That's ok! That's why we discuss and cross check ❤️

I do like the interaction. What would happen if the block position is "live" updated in the block list?

I would fear it would jump around too much, and I would worry what happens if one moves a very large block. Maybe still worth a try to see how it feels?

ellatrix · 2020-06-11T11:36:55Z

Yeah, I think I agree. I see this is what the Github project boards do, but these are much smaller items.

mtias · 2020-06-11T18:14:56Z

This looks good! It seems it could benefit from a larger area around the place where you drag where there is no scroll. From the video it seems like going back to the same position is tricky because it immediately starts scrolling the other way.

gravityrail · 2020-06-12T01:19:53Z

@mtias good suggestion. I can add it to this PR if you like

chrisvanpatten · 2020-06-12T01:32:54Z

This looks EXCELLENT. So so excited to see this — this has been a persistent micro-annoyance for some time :)

gravityrail · 2020-06-15T20:18:49Z

@mtias I just added a 50px non-scrolling region around the drag start point. Let me know how this feels for you. I don't have experience with Gutenberg so I don't know the best way to:

get the first scrollable parent
adapt this for horizontal scrolling situations
do testing

Any pointers to people or examples that I could get this info from?

ellatrix · 2020-06-17T14:05:18Z

Let's try to do this for 5.5 :)

talldan

Thanks for working on this 🎉

talldan · 2020-06-18T06:26:12Z

packages/block-editor/src/components/block-list/block-popover.js

@@ -166,7 +178,7 @@ function BlockPopover( {
 			shouldAnchorIncludePadding
 			// Popover calculates the width once. Trigger a reset by remounting
 			// the component.
-			key={ shouldShowContextualToolbar }
+			key={ shouldShowContextualToolbar || isToolbarForced }


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

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.

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

talldan · 2020-06-18T06:54:56Z

packages/block-editor/src/components/block-draggable/index.js

+				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;
+				}


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.

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.

@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?

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

packages/block-editor/src/components/block-draggable/index.js

talldan · 2020-06-18T07:17:18Z

packages/components/src/draggable/index.js

@@ -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 )


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.

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.

@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.

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.

Done! Thanks for the patch 👍

jasmussen · 2020-06-22T07:05:19Z

I'm seeing an issue in the master branch where the draggable clone disappears when you're dragging around the edge of the screen:

Does this PR address or improve that situation?

ellatrix · 2020-06-22T09:35:51Z

I'm fine with merging this sooner rather than later, iterate on it, and get some exposure in the master branch.

gravityrail · 2020-06-22T17:10:18Z

@jasmussen yes, this PR does address the disappearing clone issue.

jasmussen · 2020-06-22T17:11:27Z

Let's get this in 👌

youknowriad

I'm testing on the demo post and when I drop an element being dragged, the editor scrolls to a completely different location.

youknowriad · 2020-06-23T12:19:01Z

packages/block-editor/src/components/block-draggable/index.js

+	const velocityY = useRef( null );
+	const scrollParentY = useRef( null );
+
+	const scrollEditorInterval = useRef( null );


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()

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.

gravityrail · 2020-06-23T19:30:06Z

I'm testing on the demo post and when I drop an element being dragged, the editor scrolls to a completely different location.

@youknowriad do you have a sense of what the cause/solution of this bug is?

mtias · 2020-06-24T08:19:47Z

I'm testing on the demo post and when I drop an element being dragged, the editor scrolls to a completely different location.

This happens in master as well, I've run into it trying to demo some drag and drop features.

talldan

I can't see anything blocking merge here, and the consensus from those testing seems to be good.

Thanks for all your work @gravityrail, it's great to get some much needed drag and drop improvements into WordPress 5.5.

I'm happy to work on a couple of follow-up PRs as @gravityrail mentioned not having much time:

Encapsulate the scrolling behavior in a React Hook
Try some minor adjustments to the scroll velocity

talldan · 2020-06-25T08:42:15Z

I've made a few small follow-up PRs:

Extract block draggable scroll behaviour into React hook #23444 moves some of the BlockDraggable code into a separate React Hook, as mentioned in a comment on this PR (Set scroll velocity from drag distance #23082 (comment))

More experimentally I tried a couple of things with how the scroll speed is calculated:

Try using a percentage based calculation for block drag scrolling velocity. #23448 uses a similar system as in this PR, but instead of pixels uses a percentage based calculation (Set scroll velocity from drag distance #23082 (comment))
Try using the scroll container boundaries to determine scroll velocity #23446 tries something different and only scrolls when dragging towards the scroll container boundaries.

ellatrix · 2020-06-25T10:14:29Z

Thanks @gravityrail and @talldan! 🎉

lelas · 2021-04-22T21:10:35Z

Came here because drag and drop is a complete hit and miss now (Chrome + Firefox). As soon as I start dragging, scrolling begins and it is super difficult to drop a block in the right position. It's pure "Drag and random drop". Headlines landing 4 blocks down, in the wrong columns, etc. etc. Incredible difficult to work with as-is.

gravityrail · 2021-04-22T23:53:55Z

Thanks for the report. Sounds like a regression. Scrolling is only meant to begin once you have dragged towards the top or bottom of the screen, though maybe there's something weird about how your browser computes screen sizes that is throwing off the algorithm What is your browser, operating system, and screen resolution? (With versions)

…

On Thu, Apr 22, 2021, 2:10 PM Lasse Heindorff Kristiansen < ***@***.***> wrote: Came here because drag and drop is a complete hit and miss now (Chrome + Firefox). As soon as I start dragging, scrolling begins and it is super difficult to drop a block in the right position. It's pure "Drag and *random* drop". Headlines landing 4 blocks down, in the wrong columns, etc. etc. Incredible difficult to work with as-is. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23082 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAMVOC5NZODH5PPXVYEO7TTKCGF3ANCNFSM4N237FWQ> .

lelas · 2021-04-23T00:49:45Z

Thanks for the report. Sounds like a regression. Scrolling is only meant to begin once you have dragged towards the top or bottom of the screen, though maybe there's something weird about how your browser computes screen sizes that is throwing off the algorithm What is your browser, operating system, and screen resolution? (With versions)

Thanks for the swift response, Daniel. And apologies for cross posting. I commented on issue 24770 afterwards and was going to delete my comment on this issue.

OS: Windows 10 Pro, version 10.0.19042, Build 19042
Browser: Chrome, version 89.0.4389.128 (Official Build) (64-bit)
Screen resolution: 3440 x 1440 Pixels
Browser window size: 1734 x 1417 (while experiencing these issues earlier)

I only briefly tried Firefox. Experienced issues there too, so continued in Chrome. I'll see if I experience the same problems on my laptop tomorrow -- to rule out issues with my Chrome installation and mouse settings.

gravityrail added 2 commits June 10, 2020 16:12

Set scroll velocity from drag distance

f9ba478

Fix linting

5c05196

gravityrail requested review from ajitbohra, chrisvanpatten, ellatrix and youknowriad as code owners June 10, 2020 23:17

gziolo requested a review from oandregal June 11, 2020 04:32

gziolo added [Feature] Drag and Drop Drag and drop functionality when working with blocks [Type] Enhancement A suggestion for improvement. labels Jun 11, 2020

gravityrail added 2 commits June 15, 2020 13:14

Add a 50px non-scrolling region above and below the drag point

7af31e1

Linting

8a91a7b

Remove erroneous comment

dd2a314

gravityrail added Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code and removed Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code labels Jun 15, 2020

jasmussen mentioned this pull request Jun 17, 2020

Support drag and drop in horizontal block lists and improve drop zone detection #23020

Merged

6 tasks

talldan mentioned this pull request Jun 18, 2020

Cannot drag and scroll #18498

Closed

talldan linked an issue Jun 18, 2020 that may be closed by this pull request

Cannot drag and scroll #18498

Closed

talldan reviewed Jun 18, 2020

View reviewed changes

gravityrail added 2 commits June 21, 2020 20:32

Update comment to explain component key change

1fcbd72

Use getScrollContainer() to get the scroll container

297c04a

jasmussen mentioned this pull request Jun 22, 2020

Try: Don't show a clone when dragging. #23024

Merged

youknowriad reviewed Jun 23, 2020

View reviewed changes

Make draggable callbacks consistent and document event param

b45996a

talldan approved these changes Jun 25, 2020

View reviewed changes

talldan merged commit ba0cdb6 into WordPress:master Jun 25, 2020

github-actions bot added this to the Gutenberg 8.5 milestone Jun 25, 2020

This was referenced Jun 25, 2020

Try using the scroll container boundaries to determine scroll velocity #23446

Closed

Try using a percentage based calculation for block drag scrolling velocity. #23448

Merged

mapk mentioned this pull request Aug 25, 2020

Automatic scrolling while dragging is not intuitive #24770

Open

kevin940726 mentioned this pull request Sep 26, 2022

Don't move the window while dragging a block #36457

Closed

Set scroll velocity from drag distance #23082

Set scroll velocity from drag distance #23082

Conversation

gravityrail commented Jun 10, 2020 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

gravityrail commented Jun 11, 2020

ellatrix commented Jun 11, 2020 • edited Loading

folletto commented Jun 11, 2020

ellatrix commented Jun 11, 2020 • edited Loading

folletto commented Jun 11, 2020

ellatrix commented Jun 11, 2020

mtias commented Jun 11, 2020

gravityrail commented Jun 12, 2020

chrisvanpatten commented Jun 12, 2020

gravityrail commented Jun 15, 2020

ellatrix commented Jun 17, 2020

talldan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talldan Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmussen commented Jun 22, 2020

ellatrix commented Jun 22, 2020

gravityrail commented Jun 22, 2020

jasmussen commented Jun 22, 2020

youknowriad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gravityrail commented Jun 23, 2020

mtias commented Jun 24, 2020

talldan left a comment • edited Loading

Choose a reason for hiding this comment

talldan commented Jun 25, 2020

ellatrix commented Jun 25, 2020

lelas commented Apr 22, 2021

gravityrail commented Apr 22, 2021 via email

lelas commented Apr 23, 2021 • edited Loading

gravityrail commented Jun 10, 2020 •

edited

Loading

ellatrix commented Jun 11, 2020 •

edited

Loading

ellatrix commented Jun 11, 2020 •

edited

Loading

talldan Jun 18, 2020 •

edited

Loading

talldan left a comment •

edited

Loading

lelas commented Apr 23, 2021 •

edited

Loading