Prevent the default browser behaviour for dropped files from running #43493
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
glendaviesnz
added
[Type] Bug
|
Size Change: +125 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
ramonjd
approved these changes
Aug 23, 2022
There was a problem hiding this comment.
In the post editor:
✅ can drag/update/add on image, media and text, gallery, cover and video blocks
✅ can drag/add on the inserter in a post (to add an image between blocks for example)
✅ can drag/add in media library
✅ cannot drag onto the document anywhere else
Do we need a follow up for the site editor frames?
Good point, have pushed a change for this to this PR, might as well cover it off in one PR. @talldan with your experience with drag and drop can you see any issues with preventing the browser default at the top level of editors this way? |
actually that change only works in top level site editor, not the likes of the focused editing of header, etc. so don't bother testing until I have taken another look. |
I'm not sure. I don't think so. It'd be interesting to know whether the post editor previously had a way to prevent this happening. My feeling is that it probably did, but perhaps it was removed somewhere down the line. |
|
I had a quick look and I think this would have been prevented previously by the editor being wrapped in gutenberg/packages/edit-post/src/editor.js Lines 92 to 112 in d50e309 That called gutenberg/packages/components/src/drop-zone/provider.js Lines 192 to 222 in d50e309 At some point in the past |
|
thanks for the extra detail @talldan - in my memory it seemed like it had always done that, so handy to know it is a regression. |
|
... just trying to work out where these event listeners can be added in way that is not going to end up with duplicate event listeners, particularly in site editor |
|
This is going to make a big difference, I've been running into this often. |
glendaviesnz
commented
Aug 24, 2022
| @@ -195,9 +195,15 @@ function Iframe( | |||
| const clearerRef = useBlockSelectionClearer(); | |||
| const [ before, writingFlowRef, after ] = useWritingFlow(); | |||
| const setRef = useRefEffect( ( node ) => { | |||
| let iFrameDocument; | |||
There was a problem hiding this comment.
This part is a bit messy, but the event listeners didn't seem to work if just attached to node, so had to add to the iframes contentDocument once loaded, but then need access to that doc outside the onload method in order to remove the event listeners as this code is instantiated numerous times while using the site editor ... probably the event handlers are disposed of when the iframe doc is unmounted anyway - not sure 🤔
glendaviesnz
commented
Aug 24, 2022
| @@ -177,6 +177,10 @@ export function initializeEditor( | |||
| } ); | |||
| } | |||
|
|
|||
| // Prevent the default browser action for files dropped outside of dropzones. | |||
There was a problem hiding this comment.
this code, and the same in the edit site file below in my testing is only ever executed once on editor load, so I don't think we need to try and tidy up these event listeners.
|
@ramonjd I think I have it working in all the editor instances in the site editor now also. To test the iframed version try editing the header for footer in isolation in the site editor |
|
Tested in the site editor on page and individual templates. Things are working as expected. I can drag on dropzones but nowhere else. Thanks @glendaviesnz ! |
glendaviesnz
force-pushed
the
fix/default-drop-behaviour
branch
from
59e68b1 to
bf6ad6f
Compare
…if files dropped outside of dropzones
glendaviesnz
force-pushed
the
fix/default-drop-behaviour
branch
from
bf6ad6f to
005add4
Compare
|
@ramonjd I had to rebase this due to some minor conflicts, they were pretty straight forward to resolve, but another quick test probably wouldn't hurt when you have a minute. |
|
Retested LGTM! 🎉 |
|
This is great, but we should consider opening an issue to improve what could happen when you drop a file and it's not handled by a dropzone. For example, we can ask the user in a modal, "where do you want to add the image?"
|
|
Another thing we could try (in addition to @mtias suggestion) is to highlight the drop area with a border, reinforcing the fact that it accepts items: gray.movblue.mov |
38 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Prevents the default browse actions from running (usually opens file in a new window) if a file is dropped outside of editor dropzones.
As a follow-on to this it would be good to extend the existing dropzones within the canvas so dropping creates blocks in a wider range of areas, and this prevention of the behaviour only kicks in when dropped in obvious error spots like the top toolbar.
Why?
It is unlikely that a user will ever be expecting a file dropped in the editor window to open that file in a new tab, and doing so is disruptive to the editing workflow.
How?
Adds drop and dragover event listeners to the window to prevent the default action on these events that bubble up past valid drop zones.
Testing Instructions
Screenshots or screencast
Before:
drag-drop-before.mp4
After:
drag-after.mp4