[BUG] Canvas Drop Event not firing

d4yvie commented 4 months ago

CheckList

[X] I agree to follow this project's Code of Conduct
[X] I have read and followed the Contributing Guide
[X] I have read and followed the Issue Tracker Guide
[X] I have searched and referenced existing issues and discussions
[X] I am filing a BUG report.
[X] I have managed to reproduce the bug after upgrading to the latest version
[X] I have created an accurate and minimal reproduction

Version

6.0.1

In What environments are you experiencing the problem?

Chrome

Node Version (if applicable)

It happens in the browser, no node version

Link To Reproduction

https://github.com/fabricjs/fabricjs.github.io/tree/main/src/content/demo/events-inspector

Steps To Reproduce

Instantiate a Canvas
Drop an Object to it

It also doesnt seem to work in in the new Events inspector Demo (https://fabricjs.github.io/demos/events-inspector/) (but works in the old one http://fabricjs.com/events). Hence you can probably reproduce this with:

https://github.com/fabricjs/fabricjs.github.io/tree/main/src/content/demo/events-inspector

This also happens for me locally with fabricjs 6.0.2.

Expected Behavior

The event fires like in Fabric 5.3

Actual Behavior

The event does not fire.

Error Message & Stack Trace

No response

gloriousjob commented 2 months ago

I concur that I'm facing a similar issue when I'm testing out Fabric 6 updates (coming from 4.3). The event inspector demo seems to as well. I did try a dragleave and that throws an event but also throws if you enter and leave the canvas and drop outside the browser so I wouldn't want to key off of that.

Oddly enough, if you drop on top of the textbox, the textbox in the demo seems to pickup drop events but not the canvas itself. Even in my software, if I drop on the canvas, nothing happens. But if I drop on the textbox, it acts like it used to but on top of the textbox. I'm wondering if the textbox is somehow swallowing the drop event (or maybe more attention was given to that and not noticed that the canvas itself wasn't dropped on).

Codewise, the backend to setting up dragleave and drop is identical so not sure what's special here. I actually went into https://github.com/fabricjs/fabric.js/blob/490f57d80ced66b6a81f221898edf509010c8203/src/canvas/Canvas.ts#L183 and changed 'drop' to 'dragleave' and drop events started printing out (although missing some data drop would have) so there seems to be something off with the addeventlistener call.

gloriousjob commented 2 months ago

btw, if it helps, I made a vanilla template using this for index.ts:

import * as fabric from 'fabric';
import './styles.css';

const el = document.getElementById('canvas');
const canvas = (window.canvas = new fabric.Canvas(el, {
  backgroundColor: 'blue',
}));

//  edit from here
canvas.setDimensions({
  width: 200,
  height: 200,
});
canvas!.on('drop', (e) => console.log('got drop', e));
canvas!.on('dragleave', (e) => console.log('got dragleave', e));
const text = new fabric.Textbox('hello');
canvas.add(text);

and this for index.html:

<!DOCTYPE html>
<html>

<head>
  <title>Parcel Sandbox</title>
  <meta charset="UTF-8" />
</head>

<body>
  <canvas id="canvas"></canvas>
  <script src="./src/index.ts" type="module"></script>
  <div draggable="true">
    <svg id="svg" viewBox="0 0 256 256" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
      <path id="heart" d="M10,30 A20,20,0,0,1,50,30 A20,20,0,0,1,90,30 Q90,60,50,90 Q10,60,10,30 Z"
        style="stroke:#000000;fill: #ff0000" />
    </svg>
  </div>
</body>

</html>

I dragged the heart into the square (not on the text) and noted that the console printed 'got dragleave' but not 'got drop'. If you drag onto the text, the 'got drop' prints but not 'got dragleave'.

asturur commented 2 months ago

During the transition to fabric 5 to 6 a number of drag feature were added. One of those was to make the drop event 'more similar to the browser and how a dev would expect them' i have to be honest i work so little with drag and drop on the browser that i just trusted the developer that made the change.

I will have to dig in the code and write updated documentation but i think every instance now has a method 'shouldStartDragging' and something may have changed the way we interact with onDrop logic.

Can you check if the 'drop:before' event fires for you on canvas?

I'll prioritize this issue this weekend

gloriousjob commented 2 months ago

I actually did some tests where it printed out a console log in the onDrop and onDragleave methods in canvas.ts. They both printed out in the same fashion as when calling from my template so the drop:before isn’t firing in canvas from what I can tell.

iddl commented 2 months ago

I think an e.preventDefault() got dropped somewhere from v5 to v6. It used to be here: https://github.com/fabricjs/fabric.js/blob/v5.3.1/src/mixins/canvas_events.mixin.js#L208.

As a very temporary workaround from the userspace, this allowed the drop events to start working again

canvas.on('dragover', (event) => {
  event.e.preventDefault();
});

where event is the fabricjs event and e is the original browser event.

asturur commented 2 months ago

I think that was done for a reason, i ll have to dig the prs out and re-read

gloriousjob commented 1 month ago

This appears to be the PR which introduced it: https://github.com/fabricjs/fabric.js/pull/7802

I agree with @iddl's comment that if the preventDefault is put back in the top of onDragOver(), things work. From what I can tell, the PR was focusing on redoing the drag events to support allowing drag-and-drop into textbox's, which is neat. Because of that focus, the normal drop might have been missed unintentionally. From what I can tell, drag and drop of text still works with that present.

If you look at this example from mdn, it has a preventDefault in dragover to allow drop and in drop to prevent links from opening: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/drag_event

This article also states that dragover and dragenter should have preventDefault's to support drop: https://developer.mozilla.org/en-US/docs/Web/API/HTML_Drag_and_Drop_API/Drag_operations#specifying_drop_targets

@asturur I'm not sure what you'd recommend looking at to see if there's something else to be concerned about.

asturur commented 1 month ago

If you add a on('dragover', function(opt) { opt.e.preventDefault() }), can you make drop work again?

That would be the preferred solution. I'm not in love with the change, nor with text dragging ( that is cumbersome to disable ) but in general i want to avoid a breaking change now.

In the pr is not written but i remember there was something that has been done to make the api more html compatible. Again i m not sure in which sense, it could be in the sense you have to specify manually that your canvas is ready to accept the drop.

gloriousjob commented 1 month ago

I’d love to see an update in v7 if possible but I get the risk in v6. That workaround works for me though. It’d be good to document, though I don’t know where in the myriads of other doc updates needed.

asturur commented 1 month ago

I think we are going to document it and that's it. I think there was the desire to keep the api similar to html drop eventually becuase is more natural for devs. I m a dev and i really dislike how hard is to do drag and drop on html tho.

fabricjs / fabric.js