Fix for Issue 4329: Updated dropzone provider to check hovered zones on dragover. #4604

chriskmnds · 2018-01-19T15:24:41Z

Description

This is a fix for issue 4329.

To my understanding, there is an issue with the top/bottom dropzones. Initially the dropzones were checked and activated on the dragenter and dragleave events - which fire once each, when we enter an area while dragging and when we leave that area. So when we are already dragging somewhere in the bottom part of the top dropzone and drag the item towards the top of it - the positioning is never updated (since nothing keeps track of the cursor position), and once we drag passed the dropzone then we are not within the zone bounds for the update logic to fire - hence we need to come back into the dropzone area to re-activate the top/first dropzone.

With dragover (the fix), we keep track of the cursor position and do our checks constantly + as this was written in a way that only the dropzones that need to be updated will change their state, then the overhead is not severe. This way we have the update logic fire when we drag from the bottom half to the top half of the first dropzone (and vice versa for the bottom one), hence the activation of the topmost and bottommost goes through as we are on our way to leave the dropzone area.

Another Approach

Another possibility to investigate for a different solution may be to pad the top part of the blocks list with an additional dropzone in order to have the dragenter handler fire when leaving the topmost dropzone (so we never really leave the dropzone area, even though we won't be hovering over one of the visible blocks). This may not require changes to the provider.

How Has This Been Tested?

Manually. Dragging an image from the desktop into a block and image gallery dropzone - confirming insert.

Screenshots (jpeg or gifs if applicable):

A gif has been added to the issue referenced above.

Types of changes

Updated handler to check on dragover instead of once on dragenter/leave. Reasoning: we need to tell when the cursor goes from the bottom to the top of the same block, otherwise we can't activate the top/bottom dropzones in the block list unless we drag something out and into the top/bottom dropzones respectively.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code has proper inline documentation.

boundaries on dragover instead of dragenter/leave.

nb · 2018-01-19T17:08:01Z

components/drop-zone/provider.js

@@ -92,21 +83,29 @@ class DropZoneProvider extends Component {
 		// Computing state
 		const { onVerifyValidTransfer } = this.props;
 		const isValidDrag = ! onVerifyValidTransfer || onVerifyValidTransfer( detail.dataTransfer );
+
+		// Always true (onVerifyValidTransfer is not defined).


Can we remove it if it's always true?

We can, although I can't tell what the original intention was to have this property in the first place. For the moment the property is not defined. Maybe @youknowriad can give us a bit more insight.

I'm fine removing this, this is a leftover since this component has been adapted from Calypso

chriskmnds · 2018-01-25T17:48:14Z

components/drop-zone/provider.js

 		this.setState( {
 			isDraggingOverDocument,
 			hoveredDropZone,
 			position,
 		} );
+
+		event.preventDefault();
+		event.stopPropagation();


Reminder to check again and probably remove this last line. No need to stop propagation here.

Any decision here?

nb · 2018-01-26T20:28:07Z

components/drop-zone/provider.js

@@ -162,14 +165,18 @@ class DropZoneProvider extends Component {
 		// This seemingly useless line has been shown to resolve a Safari issue
 		// where files dragged directly from the dock are not recognized
 		event.dataTransfer && event.dataTransfer.files.length; // eslint-disable-line no-unused-expressions
+


Do we need those empty lines?

jk! In general I prefer to keep declarations (const/let/var/etc) and conditionals separate from other statements. That's just personal preference though - I am more than happy to remove them. Do we have any general approach/convention that I can adopt for such details?

nb · 2018-01-26T20:30:08Z

Good catch and works well for me. Added @youknowriad as a review to confirm whether we need isDraggingOverDocument and for another sanity check.

…dragover callback.

chriskmnds · 2018-01-27T15:52:05Z

Thanks @nb! I also pinged @youknowriad in the comments to shed some light on that missing onVerifyValidTransfer property.

youknowriad · 2018-01-29T08:46:32Z

components/drop-zone/provider.js

-	onDragOver( event ) {
-		event.preventDefault();
-	}
-
 	toggleDraggingOverDocument( event ) {


This function is called so many times when dragging right now, do you think we should throttle it?

Indeed. Great idea Riad. I am looking into this, currently trying something out with lodash. Sorry for the delayed response, I was just away since yesterday.

Hey Riad, I've finally added throttling at 300ms using lodash. It feels ok to me at this value, but please have a go and let me know what you think. Also, this is the best I could come up with code-wise to get the event.preventDefault call working. Tested it in Chrome/Firefox/Safari/IE11. :-)

youknowriad

So this change can create performance issues when dragging files in theory but I'm not seeing the performance issues we had when we first tried this.

I think we optimized the blocks to avoid rerendering without changes which makes this strategy possible now.

cc @jasmussen for sanity check (performance wise) but in general, this is looking good.

jasmussen · 2018-01-29T09:56:10Z

In my quick tests here, dragging and dropping both single and multiple images into a long document caused no performance issues for me and was simply a delight.

chriskmnds · 2018-01-29T14:12:26Z

I think we optimized the blocks to avoid rerendering without changes which makes this strategy possible now.

Indeed. I noticed some smart checks in there to only update the dropzones that need to be updated. Last I remember it was I believe 2 state updates as we drag something over a block top to bottom. I will run a few more checks to be sure and clean up that onVerifyValidTransfer property.

…property onVerifyValidTransfer.

- The top/bottom negative offsets of the block dropzones allow for the top/bottom borders shown when on hover to overlap, hence visualy appearing as one (or not changing). This was off by 1px, which this commit fixes.

chriskmnds · 2018-01-29T16:46:05Z

@jasmussen when/if you get a chance, can you please take a look at my very last commit here? There is a brief description there - not sure if you noticed it, but if you look closer there is a 1px difference in overlap of the top/bottom blue borders of stacked dropzones. These are supposed to overlap exactly when you drag something from one block to the next. This seemingly simple change fixes it, but I can't determine what the cause was. Any ideas? shall we just call it fixed and not worry?

Thanks! 😄

jasmussen · 2018-01-29T17:18:00Z

Sure thing, I'll take a look first thing tomorrow morning.

chriskmnds · 2018-01-29T19:47:31Z

Thanks :-)

jasmussen · 2018-01-30T08:36:50Z

Checked out your branch, tried to drag and drop some images in, tried to create some reusable blocks, tried dragging and dropping again. I can't seem to find any visual regressions in this change. If you see the same, then let's call it fixed for now, and we can revisit if any issues pop up.

youknowriad · 2018-01-30T08:39:06Z

This comment #4604 (comment) doesn't seem resolved yet. Any thoughts about this?

chriskmnds · 2018-01-31T14:55:32Z

Thanks @jasmussen ! I was just puzzled why I had to reduce the bottom offset by 1px to get the blue borders aligned when you drag across blocks -but yeah, it's such a minor issue, don't worry about it if it's looking fine. Thanks for checking 😄

… dragover listener.

youknowriad · 2018-01-31T18:41:31Z

components/drop-zone/provider.js

@@ -17,6 +17,13 @@ class DropZoneProvider extends Component {
 		this.isWithinZoneBounds = this.isWithinZoneBounds.bind( this );
 		this.onDrop = this.onDrop.bind( this );

+		const throttledAction = throttle( this.toggleDraggingOverDocument, 500 );
+
+		this.dragOverListener = ( event ) => {


Minor: I guess it's just a preference of mine but what do you think about something like this:

this.toggleDraggingOverDocument = throttle( this.toggleDraggingOverDocument.bind( this ), 500 ); this.dragOverListener = this.dragOverListener.bind( this );

and move the dragOverListener declaration as a regular method (not defined in the constructor).

Also wondering if 500 is not too high :) maybe 200?

Indeed, 200 is a lot better.

Minor: I guess it's just a preference of mine but what do you think about something like this:...

Definitely. That is a lot cleaner/better. Just updated now. :-)

Also, you'll notice I make the call to the throttled method as this.toggleDraggingOverDocument.call( this, event ); - it's not necessary - we can just this.toggleDraggingOverDocument( event ) - lodash uses the last this value on each call. I don't know why but I feel safer to have it this way instead? Should I just revert to the normal invocation?

Sorry this is going back/forth so many times... :-)

No worries, thanks for handling all these details :)

I'd be in favor of a regular this.toggleDraggingOverDocument( event )

I'll give it a last spin tomorrow morning and approve

Awesome! Updated. Let me know if you spot anything else :-)

…d throttle interval to 200.

youknowriad

Nice work here 👍 Thanks for addressing all the concerns

youknowriad · 2018-02-01T15:52:22Z

Hey @chriskmnds I'm seeing a small bug on master where there's a lingering placeholder at the end of the post when drag and dropping a file.

I wonder if it's related to this PR

chriskmnds · 2018-02-01T17:09:43Z

Hey @youknowriad I notice that too. It doesn't look like it's related to this PR. I've checked the commit before it (d8e13bf) and I can still see the lingering placeholder. This is what you are referring to right:

I sense this is related to the latest changes with the sibling inserter. I can look into this first thing tomorrow in any case. Shall I create a ticket for it? :-)

chriskmnds added 3 commits January 19, 2018 17:12

Fix for Issue 4329: Updated dropzone provider to check hovered zone

db732c5

boundaries on dragover instead of dragenter/leave.

Fix for Issue 4329: Linting errors.

e09051d

Fix for Issue 4329: Cleanup.

97bc6f5

nb reviewed Jan 19, 2018

View reviewed changes

chriskmnds commented Jan 25, 2018

View reviewed changes

Merge branch 'master' into fix/dropzone-provider

22013ae

nb reviewed Jan 26, 2018

View reviewed changes

nb requested a review from youknowriad January 26, 2018 20:29

chriskmnds added 2 commits January 27, 2018 17:34

Merge branch 'master' into fix/dropzone-provider

93ad7ad

Fix for Issue 4329: Removed inclusion of stopPropagation call in the …

64de0b6

…dragover callback.

youknowriad reviewed Jan 29, 2018

View reviewed changes

chriskmnds added 2 commits January 29, 2018 17:44

Fix for Issue 4329: Cleanup - removed redundant check with undefined …

c33fcfd

…property onVerifyValidTransfer.

Fix for Issue 4329: Improved border overlap between dropzones.

eed854f

- The top/bottom negative offsets of the block dropzones allow for the top/bottom borders shown when on hover to overlap, hence visualy appearing as one (or not changing). This was off by 1px, which this commit fixes.

chriskmnds added 4 commits January 31, 2018 19:53

Fix for Issue 4329: Added throttling of 500 ms to dropzone provider's…

df5f586

… dragover listener.

Fix for Issue 4329: Reduced throttling to 300 ms.

25738a0

Fix for Issue 4329: Cleanup - linting errors.

aa6f3b3

Merge branch 'master' into fix/dropzone-provider

c4c784e

youknowriad reviewed Jan 31, 2018

View reviewed changes

Fix for Issue 4329: Cleanup/refactored after Riad's comments + lowere…

6f5ac93

…d throttle interval to 200.

Fix for Issue 4329: Cleanup/refactored after review.

f7cc69e

youknowriad approved these changes Feb 1, 2018

View reviewed changes

youknowriad merged commit 321f5c3 into WordPress:master Feb 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for Issue 4329: Updated dropzone provider to check hovered zones on dragover. #4604

Fix for Issue 4329: Updated dropzone provider to check hovered zones on dragover. #4604

chriskmnds commented Jan 19, 2018 •

edited

Loading

nb Jan 19, 2018

chriskmnds Jan 19, 2018

youknowriad Jan 29, 2018

chriskmnds Jan 25, 2018

nb Jan 26, 2018

nb Jan 26, 2018

chriskmnds Jan 27, 2018 •

edited

Loading

chriskmnds Jan 27, 2018 •

edited

Loading

nb commented Jan 26, 2018

chriskmnds commented Jan 27, 2018 •

edited

Loading

youknowriad Jan 29, 2018

chriskmnds Jan 31, 2018

chriskmnds Jan 31, 2018

youknowriad left a comment

jasmussen commented Jan 29, 2018

chriskmnds commented Jan 29, 2018

chriskmnds commented Jan 29, 2018 •

edited

Loading

jasmussen commented Jan 29, 2018

chriskmnds commented Jan 29, 2018

jasmussen commented Jan 30, 2018

youknowriad commented Jan 30, 2018

chriskmnds commented Jan 31, 2018

youknowriad Jan 31, 2018

youknowriad Jan 31, 2018

chriskmnds Jan 31, 2018

chriskmnds Jan 31, 2018

youknowriad Jan 31, 2018

chriskmnds Jan 31, 2018

youknowriad left a comment

youknowriad commented Feb 1, 2018

chriskmnds commented Feb 1, 2018 •

edited

Loading

Fix for Issue 4329: Updated dropzone provider to check hovered zones on dragover. #4604

Fix for Issue 4329: Updated dropzone provider to check hovered zones on dragover. #4604

Conversation

chriskmnds commented Jan 19, 2018 • edited Loading

Description

Another Approach

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

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

chriskmnds Jan 27, 2018 • edited Loading

Choose a reason for hiding this comment

chriskmnds Jan 27, 2018 • edited Loading

Choose a reason for hiding this comment

nb commented Jan 26, 2018

chriskmnds commented Jan 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

jasmussen commented Jan 29, 2018

chriskmnds commented Jan 29, 2018

chriskmnds commented Jan 29, 2018 • edited Loading

jasmussen commented Jan 29, 2018

chriskmnds commented Jan 29, 2018

jasmussen commented Jan 30, 2018

youknowriad commented Jan 30, 2018

chriskmnds commented Jan 31, 2018

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

youknowriad left a comment

Choose a reason for hiding this comment

youknowriad commented Feb 1, 2018

chriskmnds commented Feb 1, 2018 • edited Loading

chriskmnds commented Jan 19, 2018 •

edited

Loading

chriskmnds Jan 27, 2018 •

edited

Loading

chriskmnds Jan 27, 2018 •

edited

Loading

chriskmnds commented Jan 27, 2018 •

edited

Loading

chriskmnds commented Jan 29, 2018 •

edited

Loading

chriskmnds commented Feb 1, 2018 •

edited

Loading