kiwix / kiwix-js

Fully portable & lightweight ZIM reader in Javascript
https://www.kiwix.org/
GNU General Public License v3.0
310 stars 135 forks source link

Drag of images causes to disconnect from the loaded zim file #1037

Closed FractalU closed 6 months ago

FractalU commented 1 year ago

In the kiwix browser extension 3.9.0 of both firefox and chrome, whenever I drag an image by mouse the loaded zim file gets disconnected and I get sent back to the configuration page. This also happens with the progressive web app. I tested the progressive web app on https://pwa.kiwix.org/www/index.html. What I expect is that the dragging of images does nothing as it's the default behavior of any web page. The kiwix browser extension and the progressive web app are both based on kiwix-js. I guess there is a bug in kiwix-js. That's why I'm writing here.

Jaifroid commented 1 year ago

@FractalU Thank you for the bug report. Did you try the option in Configuration to disable drag-and-drop under Troubleshooting and Development (in both apps)? I introduced that option precisely because another user reported a similar issue. Let me know if it works for you.

FractalU commented 1 year ago

@Jaifroid Thank you for this option. It fixed the issue on the kiwix browser extension on both firefox and chrome as well as on the progressive web app on those browsers. I guess the drag and drop mechanism on the configuration page is still active after the zim file has been loaded and the pages from that zim file are being displayed. When I accidentally drag an image while the pages from the zim file are being displayed, the drag mechanism comes in and thinks there might be another zim file to load. It sends me back to the configuration page and shows the dotted red lines to drop the dragged item. My suggestion is to have the drag and drop mechanism active only on the configuration page. That would solve the issue. There would also be no need to have the disable drag-and-drop option anymore.

Jaifroid commented 1 year ago

@FractalU I'm keen to make opening ZIM files as frictionless as possible, hence the idea that the whole app is a target for drag-and-drop, because it means that a user doesn't need to click any buttons to open a new ZIM file. They can simply drag a zim anywhere into the app, even if another ZIM is open.

Having said that, it would be perfectly possible to make it an option so that only the Config page receives drag-and-drop if a ZIM is already open. The control could be a dropdown with the options:

I think that would probably address the issue fully.

FractalU commented 1 year ago

@Jaifroid I see. You really want to keep the current behavior where the new zim file could always be opened by drag-and-drop. The problem I have with adding options for this issue is that it doesn't solve this issue at all. It doesn't improve the current situation. If you add an option for drag-and-drop to be only active at the configuration page or an option to disable drag-and-drop entirely, it's obvious that the currently buggy behavior remains. The problem is almost none of the end users who experiences this issue will figure out on their own that there is an option to fix this issue while sacrificing the drag-and-drop convenience. It would be so much better if there is an implementation to make this problem go away and the program just works with the default settings. In my opinion this is how this issue gets fully addressed.

I would suggest instead to check whether the dragged item is a file or not. If it's a file then open the configuration page and show the red-dotted lines to drop the dragged item as it's the current behavior. If it's not a file then do nothing. This behavior would suit because the dragged items that matters must be a file. Everything else such as dragging of images or selected text is irrelevant. That way a new zim file could always be opened by drag-and-drop while dragging of images does nothing. In vanilla javascript this could be achieved with the event.dataTransfer.types property. It return a array of data formats used in the drag operation represented as strings. If the Files string is in an element of that array then the dragged item is a file, otherwise it's not. I tested it on both firefox and chrome. There it works. I don't know what framework kiwix-js is using and whether it works in that framework. So it's up to you to figure out.

This is a non-trivial issue, but I'm sure it can be properly solved.

For more detail on event.dataTransfer.types property, see https://developer.mozilla.org/en-US/docs/Web/API/DataTransfer/types

Jaifroid commented 1 year ago

@FractalU Thanks for testing the event.dataTransfer.types property -- it certainly sounds like it could be used as part of a more intelligent solution so that the app can decide on its own what the user's intention with the drag-and-drop is. I think we might not be able to inspect the contents until after the drop, so there might need to be some change of the current behaviour (which is to open Config as soon as a user initiates a drag). The reason for that had to do with the danger of dropping something into the iframe with unintended consequences, but I agree that the current situation is clunky, and the current "solution" is a workaround.

Jaifroid commented 1 year ago

Looking in more detail at the MDN info, it does look like we may be able to distinguish dragged (rather than dropped) files from other data types.

FractalU commented 1 year ago

I tested the event.dataTransfer.types property with the ondrag, ondragover, ondragenter events. It works on firefox and chrome. Though the event.dataTransfer.types property has different results on different browser. However, one consistency with that property I noticed is the Files type. If the returned array from event.dataTransfer.types contains a Files string element, then the dragged item has at least a file or directory, otherwise it doesn't. That property can contain other elements. As an example on firefox, if the types property contains Files then it also contains application/x-moz-file. Detecting whether the dragged item is an image or a selected text might be more complicated or impossible with the types property. Maybe some other dataTransfer properties such as items should be used instead. Honestly, I find the dataTransfer API rather complicated.

I don't know anything about the iframe issue. I guess a lot of testing with different dragged items other than files has to be done. If I turn off drag-and-drop then the dragging of images and selected texts works fine for me.

skushagra9 commented 9 months ago

this issue is solved ?

Jaifroid commented 9 months ago

@skushagra9 It's not clear if there's anything we can do. Essentially, we already have a workaround, which is to disable drag-and-drop completely.. If you have an intelligent solution for inspecting file types when dragging, then please do make your suggestion here.

D3V-D commented 6 months ago

Can we simply implement a check on the dragenter event to see if a file is being dragged over the area? (Like in this example I made quickly: https://codepen.io/D3V3ND/full/eYoXLGj. You can see the logs in the console when you drag something over the grey square).

Text and images are not considered files, and neither are other HTML elements as far as I know. I don't think you are able to see the file extensions on all browsers, but that's fine; you can just show the popup saying that one or more files are not ZIM files after the file has been dropped (if its not a ZIM file).

Also, is it intentional that dragging something over the area returns it to the configuration page right away? Why even return them at all? Is it not possible to just outline the current page and allow the drop, without closing the current page?

Let me know if I'm missing something here @Jaifroid

Jaifroid commented 6 months ago

@D3V-D Thank you for making that test. I can certainly see "files are being dragged" in console. However, when I dragged a PNG file over the area it did say "files are being dragged" (in Chromium). However this may be different if the user is "accidentally" dragging an image or some text that they've selected in the iframe (i.e. from the article). It's quite likely this is seen as an HTML element rather than a file.

It certainly sounds like it could be a solution. It should be easy enough to do the file check on dragover (there's an event or two defined in app.js) and only switch to Config if it is a file. There is already a check if a user drops a file that is not a ZIM file, so I don't think that would need to be implemented.

If you'd like to try to implement this solution, @D3V-D, please do make a PR. Or let me know if you need any guidance. If so, be sure to read CONTRIBUTING.md carefully in this Repo to learn how to set up.

Jaifroid commented 6 months ago

Also let me know if you'd like to be assigned, of course.

D3V-D commented 6 months ago

Yeah, please do assign me.

I'm thinking we shouldn't switch to config at all until the dragover item has been dropped, to optimize the UX. After all, if they for example accidentally drag a file over the page, then we don't want to reset them. I think it's easiest to wait until after the drop to reset to config; maybe don't even reset to config unless it's a ZIM file, just show the error and stay on the current page.

Also, about the image, yeah I meant mainly for if they accidentally drag (or even purposely, maybe to drop elsewhere) from the same page, then it wouldn't throw them out of the page.

D3V-D commented 6 months ago

I'll probably make the PR later today or maybe tomorrow, once I get some time. I've already read CONTRIBUTING.md but I'll make sure to refer to it when working on it. Let me know if there's anything else @Jaifroid

Jaifroid commented 6 months ago

I'm thinking we shouldn't switch to config at all until the dragover item has been dropped, to optimize the UX

Possibly, but I think there's an issue with dropping into the iframe. I may be wrong, it was a long time ago I coded it! I think (but not sure) that there was a reason for switching to Config on dragover rather than on drop. But feel free to experiment and see what works and what makes good UX!

D3V-D commented 6 months ago

Sounds good. I'll try and see what I can do, maybe I'll do some experimentation in a sandbox first to see how exactly iFrames interact with drag events before I test it in the kiwix environment.