Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
FractalU opened this issue Jul 31, 2023 · 16 comments · Fixed by #1245
Closed

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

FractalU opened this issue Jul 31, 2023 · 16 comments · Fixed by #1245
Assignees
Labels
accessibility bug-non-critical For bugs that it would be nice to fix rather than critical to fix user interface
Milestone

Comments

@FractalU
Copy link

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
Copy link
Member

@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
Copy link
Author

@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 Jaifroid self-assigned this Aug 1, 2023
@Jaifroid Jaifroid removed their assignment Aug 1, 2023
@Jaifroid Jaifroid added this to the v4.0 milestone Aug 1, 2023
@Jaifroid
Copy link
Member

Jaifroid commented Aug 1, 2023

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

  • Default (drag-and-drop always active)
  • Config only (only the Config page can receive drag-and-drop)
  • Disable

I think that would probably address the issue fully.

@FractalU
Copy link
Author

FractalU commented Aug 1, 2023

@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
Copy link
Member

Jaifroid commented Aug 1, 2023

@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
Copy link
Member

Jaifroid commented Aug 1, 2023

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
Copy link
Author

FractalU commented Aug 1, 2023

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
Copy link

this issue is solved ?

@Jaifroid
Copy link
Member

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

@Jaifroid Jaifroid modified the milestones: v4.0, v4.1 Feb 21, 2024
@D3V-D
Copy link
Contributor

D3V-D commented Apr 26, 2024

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
Copy link
Member

@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
Copy link
Member

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

@D3V-D
Copy link
Contributor

D3V-D commented Apr 27, 2024

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
Copy link
Contributor

D3V-D commented Apr 27, 2024

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
Copy link
Member

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
Copy link
Contributor

D3V-D commented Apr 27, 2024

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.

@Jaifroid Jaifroid linked a pull request May 3, 2024 that will close this issue
@Jaifroid Jaifroid added user interface accessibility bug-non-critical For bugs that it would be nice to fix rather than critical to fix and removed good first issue labels May 3, 2024
Jaifroid pushed a commit that referenced this issue May 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility bug-non-critical For bugs that it would be nice to fix rather than critical to fix user interface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants